feat(auth): gate overleaf behind a service permission - #54
Merged
Conversation
overleaf.jorisjonkers.dev shipped in fleet-infra#216 behind the forward-auth middleware, but the host resolved to no ServicePermission -- and fromHost returning null makes verify() skip the check entirely. The practical gate was therefore "any authenticated user", not "users who were granted it", which is weaker than every other forward-auth host on the cluster. Overleaf Community Edition has no SSO of its own -- SAML, LDAP and OIDC are all Server Pro features -- so unlike NOTES and HERMES, which enforce their grant at the authorization endpoint through their own OIDC client, this entry is the only per-user gate in front of it. This is a revocation, not an addition. No existing user holds a SERVICE_OVERLEAF row, and only ROLE_ADMIN bypasses the check, so everyone except admins loses access the moment it deploys. Grant before anyone relies on it. Also corrects the HERMES comment, which claimed a missing entry meant "every request is denied". It is the reverse -- a missing entry allows every authenticated user -- and that misreading is exactly what made the overleaf gap easy to miss. No migration: user_service_permissions.service is a VARCHAR(50) with no check constraint, so a new name needs no schema change. The published OpenAPI spec does not enumerate the permission names either, so the contract export is unaffected. Verified: 175/175 tests including the two new fromHost cases, ktlintCheck and detekt clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
overleaf.jorisjonkers.devshipped in JorisJonkers-dev/fleet-infra#216 behind theforward-authmiddleware, but the host resolved to noServicePermission— andfromHostreturningnullmakesverify()skip the check entirely:The practical gate was therefore any authenticated user, not users who were granted it — weaker than every other forward-auth host on the cluster.
AuthVerificationControllerTestalready asserts that behaviour outright: "unknown subdomain host passes through without permission check".Why this one has to be enforced here
Overleaf Community Edition has no SSO of its own — SAML, LDAP and OIDC are all Server Pro features.
NOTESandHERMEScan enforce their grant at the authorization endpoint through their own OIDC client; Overleaf runs no such flow, so this entry is the only per-user gate in front of it.This is a revocation, not an addition
No existing user holds a
SERVICE_OVERLEAFrow, and onlyROLE_ADMINbypasses the check. Everyone except admins loses access the moment this deploys. Grant before anyone relies on it:A corrected comment
The
HERMEScomment claimed a missing entry meant "every request is denied". It is the reverse — a missing entry allows every authenticated user — and that misreading is exactly what made the overleaf gap easy to miss. Its routes also carry no forward-auth at all, which the comment did not say.Verification
175/175 tests, including the two new
fromHostcases.ktlintCheckanddetektclean.No migration:
user_service_permissions.serviceisVARCHAR(50)with no check constraint, so a new name needs no schema change. The published OpenAPI spec does not enumerate the permission names, so the contract export is unaffected — confirmed by grepping the spec for existing values rather than assuming.