feat: negotiate and persist authorization scope - #28178
Draft
BobbyHo wants to merge 1 commit into
Draft
Conversation
The authorize endpoint parsed the scope parameter and discarded it, so an app's configured allowlist never restricted anything and a client asking for more than it should get was never told no. Phase 1 added the columns that carry a negotiated scope from a code to the token it becomes, but nothing wrote one, so every code was stamped unrestricted. Negotiate the scope at authorization time and persist the result: - Requested names must be in the external scope catalog, and the app's stored allowlist is filtered through that same catalog. Filtering only ever narrows what can be granted. - The allowlist bounds authority, not spelling. A request is granted when every permission it grants is also granted by the allowlist, whether or not the allowlist names it, so an app allowed coder:workspaces.access can approve a client asking only for workspace:ssh. - Omitting scope grants the filtered allowlist, per RFC 6749 section 3.3. - Both handlers negotiate, so a request that cannot succeed fails before the consent page renders rather than after the user clicks Allow. Each reports the failure the way it already reports its own errors: a static error page on the GET side, an OAuth2 error body on the POST side. - Two paths produce an empty result and are deliberately distinct. No allowlist and no request keeps the previous unrestricted grant, written as an explicit sentinel because the column is NOT NULL with a non-empty CHECK. An allowlist that filters to nothing is rejected, since falling back would grant strictly more than the allowlist ever permitted. Dynamic client registration performs no catalog validation, so apps registered with scopes such as openid or admin hold allowlists this server cannot grant from. They now fail authorization in both directions. Grandfathering unknown names would seed the enforcement path with values it cannot evaluate, trading a visible negotiation-time error for a silent enforcement-time hole. The failure names the registered scopes and the remedy. Issued tokens are still unrestricted: the exchange copies the negotiated scope onto the token record, but the API key it mints carries no scope. This changes which authorization requests succeed, not what a token can do.
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
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
1 participant
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.
Split out of #28045 (PLAT-479). Second of three, stacked on #28167. Review that one first; this diff is against it, not against main.
The authorize endpoint parsed scope and threw it away, so an app's allowlist never restricted anything and a client asking for too much was never told no. Phase 1 added the columns that carry a negotiated scope from a code to the token, but nothing wrote one.
What changes
Accepted compatibility break. DCR does no catalog validation, so apps registered with openid, read or admin hold allowlists this server cannot grant from. They now fail authorization in both directions. Grandfathering unknown names would seed the enforcement path with values it cannot evaluate, trading a visible error now for a silent hole later. The rejection names the registered scopes and the remedy. Pinned in TestOAuth2AuthorizeDCRScopeCompatibility.
Interim error delivery, replaced in PR 3. Each handler reports the rejection the way it already reports its own errors: a static error page on GET, an OAuth2 error body on POST. Delivering these to the client's own callback per RFC 6749 §4.1.2.1 is the next PR, which rewrites the requireInvalidScope helper along with it. About 25 lines here are interim on purpose and flagged as such in that helper.
Not changed. Issued tokens are still unrestricted. The exchange copies the negotiated scope onto the token record, but the API key it mints carries no scope. This changes which authorization requests succeed, not what a token can do.
Stack: #28167, this PR, then the consent page and the invalid_scope redirect.