feat(coderd/oauth2provider): report the negotiated scope to the user and the client - #28179
Draft
BobbyHo wants to merge 2 commits into
Draft
feat(coderd/oauth2provider): report the negotiated scope to the user and the client#28179BobbyHo wants to merge 2 commits into
BobbyHo wants to merge 2 commits into
Conversation
The consent page told every user the app was getting full access to their account, which stopped being true once the authorize endpoint began negotiating a narrower scope. A user approving a request has no other place to learn what they are handing over, so the page has to follow the grant rather than a fixed sentence. List the negotiated permissions when the grant is bounded, and keep the original full-access wording when it is not. An unrestricted grant is reported as full access rather than as "coder:all", since the scope name tells a user less than the sentence does. The list collapses to the full-access wording whenever the unrestricted scope is present, not only when it stands alone: an allowlist registered as `coder:all coder:workspaces.access` grants everything, and naming the narrower entry beside it would describe the grant as bounded. role="list" and role="listitem" are explicit because WebKit drops the implicit list semantics from a list styled with list-style: none, which would otherwise leave VoiceOver announcing the permissions as loose text. Also narrow the fragment the tests match for one rejection branch. The GET side renders its description into HTML, which escapes the apostrophe in "this app's allowed scope list", so the fragment stops before it.
…back A rejected authorization request answered on Coder, which reaches only the user's screen. The client's error handling never ran, and the state it sent was dropped, so it could not correlate the failure with the request that caused it. RFC 6749 section 4.1.2.1 requires the error be delivered to the client's redirect URI once the client is known. Redirect to the app's registered callback with error, error_description, and the state exactly as it arrived. Both handlers use this, replacing the static error page on the GET side and the OAuth2 error body on the POST side. This is safe here specifically because of ordering: extractAuthorizeParams exact-matches the redirect URI against the app's registered callback, and it runs before the scope check, so the destination is the app's own no matter what the request carried. Only errors raised after that point may use this helper, which its precondition states. Errors from extractAuthorizeParams itself must not, since the URI is unvalidated there. MismatchedRedirectURINotRedirected pins the ordering: an unregistered redirect_uri fails on Coder with no Location header on either verb, even when the same request also carries a scope the app cannot be granted. The other error paths in this file are unchanged, since several of them are where redirect URI validation fails.
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). Last of three, stacked on #28178. Read #28167 and #28178 first; this diff is against #28178.
#28178 decided what the scope is. This PR is everything that tells someone about it. Two halves, one commit each.
Half A: the consent page states the scope (a3fcdeb, ~190 lines)
coder:all coder:workspaces.accessgrants everything, and naming the narrower entry beside it would describe the grant as bounded.Half B: invalid_scope goes to the client's callback (0b7827e, ~90 lines)
This half wants a security reviewer. It is the only part of #28045 with an open-redirect failure mode, and its safety is entirely an ordering argument:
extractAuthorizeParamsexact-matches redirect_uri against the app's registered callback, and it runs before the scope check, so the destination is always the app's own regardless of what the request carried.MismatchedRedirectURINotRedirectedpins it: an unregistered redirect_uri fails on Coder with no Location header on either verb, even when the same request also carries a scope the app cannot be granted. It also asserts the rejection comes from redirect_uri validation rather than any 400.extractAuthorizeParamsmay use the helper, which its precondition documents. The file's other error paths are unchanged, since several of them are where that validation fails.Retires the interim plumbing #28178 left behind: the GET side's static error page,
requireInvalidScope's 400-and-body assertion, and theLocationassertions inRejectionNamesTheRegisteredScopes.The three PRs together reproduce #28045 byte for byte.
Stack: #28167, #28178, this PR.