feat(coderd/rbac): compare scopes by permission coverage - #28167
Conversation
Add ScopesCover, which reports whether every permission a requested scope grants is also granted by at least one of a set of allowed scopes. It expands both sides and compares the resulting permissions, so coder:workspaces.access covers workspace:read even though it never names it, and coder:all covers everything. The comparison is deliberately asymmetric. Positive permissions on the allowed side that it does not model are dropped, which can only make the answer stricter. Anything unmodelled on the requested side is an error instead, because ignoring it would answer "covered" about authority that was never compared. Negative permissions are the exception and fail closed on both sides, since dropping an anti-grant from the ceiling would widen it rather than narrow it. Add CanonicalScopeName, which maps the backward-compatibility aliases IsExternalScope accepts onto the names the api_key_scope enum stores. IsExternalScope answers whether a name may be requested, not how that name is spelled once persisted, so a caller that stores what it validated has to canonicalize in between. Both functions are added without production callers. The OAuth2 authorize endpoint uses them to negotiate a requested scope against an app's configured allowlist, which follows in a separate change.
BobbyHo
changed the title
State the rule the guards enforce, site-level grants only, instead of describing the asymmetry abstractly. The allow-list case is now covered alongside negative permissions, which the previous wording omitted even though the code treats them identically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 24 findings (1 P2, 5 P3, 13 Nit, 5 Note), COMMENT. Review Finding inventoryPR #28167 finding inventoryFindings
Contested and acknowledged(None on rounds 1 or 2. CRF-2 was addressed partially, with the residual half re-raised by the panel in R2.) Round logRound 1Panel review. Base 5f6eeda..2d39e04. Netero found 1 P3 (CRF-1 partial) and 1 Note (CRF-5 partial); mechanical floor clean, panel spawned. 17 panel reviewers. 15 findings: 1 P2, 4 P3, 6 Nit, 4 Note. Two systemic themes: (a) the alias vocabulary is spelled out in four places with nothing binding them and no direct test of the mapping (CRF-1, CRF-2); (b) invariant guards on ScopesCover promise more than the code delivers, and the promises are unverifiable through the public API (CRF-3, CRF-4, CRF-5). Ging-go and Komugi reported no findings. Round 2Churn guard PROCEED. All 15 R1 findings addressed; CRF-2 partial (rbac side fixed, apikey.go sites deferred to a claimed but unfiled ticket). Netero R2 no findings. Panel: 18 (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Gon, Leorio, Ging-go, Ryosuke, Kurapika, Razor, Meruem, Knov, Komugi, Robin, Chopper, Kite, Zoro) + Melody wildcard. 5 new findings + 1 re-raise. CRF-2 re-raised at P3 by 11 reviewers (Mafuuu, Mafu-san, Pariston, Ryosuke, Razor, Meruem, Robin, Zoro, Knov, Melody, Kite). New evidence: the deferral in the R1 reply comment carries no linked ticket ID (churn guard flagged before panel spawned), and the multi-scope path at Round 3Churn guard PROCEED. All R2 findings addressed (CRF-16, CRF-17, CRF-18, CRF-19, CRF-20) with commits 1678a77, 2fcce8b, aba3c6c, 9a08105. CRF-2 properly deferred with linked ticket #28322 (stacked on this branch, includes regression test for the 500). CI: Flake Check failing (96184391843) on head SHA 2fcce8b. Reviewed against 71e95a3..2fcce8b. R3 panel: 16 (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Gon, Leorio, Ging-go, Ryosuke, Kurapika, Razor, Meruem, Komugi, Chopper, Kite, Zoro). No findings from Hisoka, Ging-go, Kurapika, Ryosuke, Meruem, Chopper. Mafu-san, Pariston, Komugi, and Ryosuke reported no findings with process-quality confirmations. 6 new Nits/Notes: CRF-21 (comment-vs-code drift introduced by the CRF-17 fix), CRF-22 (IsExternalScope docstring same-class as CRF-20), CRF-23 (missing test for docstring claim from CRF-20 fix), CRF-24 (unexplained About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change adds two pure predicates for scope coverage. ScopesCover and permissionCovered take ScopeName rather than Scope, forcing every caller through ExpandScope and pinning the guard set to a known input surface; that single choice makes the asymmetric drop-vs-error rule (unmodelled positives on the allowed side shrink the ceiling, everything else fails closed) actually enforceable. Test density is high (58.7%), the property test asserts what its comment promises, and the doc comment on ScopesCover is the shape every non-trivial exported function in this package should have.
Two themes recur across the panel and are worth resolving before the follow-up wires an OAuth2 consumer.
Alias vocabulary. CanonicalScopeName is the bridge between IsExternalScope's accepted spellings and ExpandScope's required spellings, but the mapping itself is untested: both switch arms sit at 0% coverage, TestScopesCoverEveryExternalScope's canonical := CanonicalScopeName(...) call is a no-op on every iteration, and swapping the two arms tomorrow leaves the whole suite green. The same alias knowledge is spelled out in four places in the tree today (IsExternalScope, CanonicalScopeName, coderd/apikey.go:94-101, coderd/apikey/apikey.go:91-98), and the multi-scope apikey.go:72-84 path does not canonicalize at all. Hisoka's line lands: pull the thread and the whole catalog is glued together with duplicated switch statements no compiler will keep honest.
Invariant guards. ScopesCover's doc contract states "a negative permission or an allow list is never dropped, on either side." The requested side honors this; the allowed side only inspects expanded.Site, so expanded.User and expanded.ByOrgID are silently discarded. No live trigger today (every current ExpandScope path zeroes both slices), but the first scope that grows a User or ByOrgID negate widens the OAuth ceiling instead of failing closed. Separately, the five guards themselves are unreachable via the public API and untested, so a future refactor that garbles one ships green until a scope with the wrong shape arrives.
Severity counts: 1 P2, 4 P3, 6 Nit, 4 Note.
Process notes. The PR title is feat: compare scopes by permission coverage; per AGENTS.md the commit convention type(scope): message requires a scope naming a real filesystem path containing every changed file. The commit gets it right (feat(coderd/rbac): ...); the PR title should match so the merge log tells the same story. The PR description calls TestScopesCoverEveryExternalScope "a property test"; it is an enumeration over ExternalScopeNames() with no randomized input (no gopter, rapid, or quick.Check). The test is valuable as-is; call it a catalog-wide invariant test in the description to keep terminology honest.
coderd/rbac/scopes_catalog.go:94
P3 [CRF-2] The alias table is duplicated across IsExternalScope, CanonicalScopeName, and two coderd/apikey.go sites, with no test binding them; coderd/apikey.go:72-84 (multi-scope path) does not canonicalize at all. (Hisoka P2, Robin P3, Knov Note, Meruem Note, Zoro Note)
Hisoka: "IsExternalScope accepts four names as backward-compat aliases: all, application_connect, coder:all, coder:application_connect (scopes_catalog.go:92-96). CanonicalScopeName only maps two of them (all, application_connect). The stack the PR body describes routes user input through IsExternalScope -> CanonicalScopeName -> ScopesCover. If IsExternalScope ever grows a third alias (a hyphen variant for an SDK, an uppercase form, a new special) and CanonicalScopeName does not, the OAuth2 authorize path silently produces expand requested scope: no scope named \"...\" from ScopesCover and rejects a request the same file said was requestable. No unit test would notice. apikey.go:94-101 proves this drift risk is not hypothetical. That file already carries a fourth inlined copy of the alias mapping and does not call CanonicalScopeName at all."
Robin: "coderd/apikey.go:94-101 and coderd/apikey/apikey.go:91-98 both open-code the same switch: \"all\" maps to coder:all, \"application_connect\" maps to coder:application_connect, anything else passes through. That is the mapping CanonicalScopeName now owns."
Meruem, verified in the worktree: "The multi-scope path in coderd/apikey.go:72-84 does not canonicalize at all, so legacy aliases in createToken.Scopes are persisted verbatim rather than as the enum name; that's a pre-existing gap that CanonicalScopeName is the natural fix for."
Either fix collapses the drift surface:
- Bind the two switches: an invariant test that iterates the acceptance set and asserts
IsExternalScope(n)impliesScopesCover([]{ScopeAll}, CanonicalScopeName(n))succeeds for every accepted alias. - Or collapse
IsExternalScope's alias arm andCanonicalScopeNameinto a sharedmap[ScopeName]ScopeNamewith a_, ok := aliases[n]fallback inIsExternalScope.
Route the two apikey sites through rbac.CanonicalScopeName in the follow-up (or here, since this PR introduces the helper). Marking the multi-scope path bug as adjacent, not part of this PR's diff, but the same fix closes it.
🤖
🤖 This review was automatically generated with Coder Agents.
ScopesCover checked the requested scope for org and user grants but not the allowed scopes, whose User and ByOrgID permissions were discarded unread. A scope granting workspace:* at site level while negating workspace:delete for the user would have covered a request for workspace:delete, because the negative that carves the action back out lives in the half coverage never examined. No catalog scope populates those fields today, so nothing was miscompared in practice. The gap mattered because these guards exist to keep the comparison fail-closed, and this one failed open. Both sides now run the same checkCoverable helper, which refuses a scope carrying org or user grants, a negative permission, or a resource allow list. The helper names the side, so an error reports which half of the comparison was undecidable. The doc comment claimed an unmodeled grant on the allowed side is dropped; nothing is dropped now, so it is gone. ScopesCover builds every Scope it reads from ExpandScope, which cannot produce these shapes, so the guards are unreachable through the public API. scopes_internal_test.go drives synthetic Scope values through checkCoverable instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
permissionCovered skipped negative permissions, but checkCoverable now refuses a scope carrying one on either side, so the branch was dead. It was never defense in depth. Had a negative reached it, skipping the anti-grant would leave any wildcard beside it free to match, and a scope granting workspace:* while negating workspace:delete would report workspace:delete as covered. The skip widened the ceiling while looking like it narrowed it. The precondition moves to the doc comment, which names checkCoverable as what enforces it and says why subsumption cannot answer the question an anti-grant poses. No behavior change: the branch was unreachable. permissionCovered goes from 88.9% to 100% statement coverage.
Five review findings on the coverage tests, all in scopes_test.go. CanonicalScopeName had both alias arms at zero coverage. Its only caller in the tests loops over ExternalScopeNames, which yields canonical names only, so the canonicalizing call returned its input unchanged on every iteration and read as coverage without being any. Swapping the arms, so that `all` persisted application_connect and the reverse, kept the suite green. TestCanonicalScopeName now pins the mapping and the loop appends the aliases, taking the function from 50% to 100%. The appended aliases raise branch coverage and assert a requestable name is comparable once canonicalized, but they cannot detect a swapped mapping, since both aliases resolve to scopes that cover themselves. The comment says so rather than implying the loop guards more than it does. CompositeDoesNotCoverNonMember and CompositeDoesNotCoverWiderActionOnCoveredResource both asked for an ungranted action on a resource coder:workspaces.access does grant, so they tested one branch twice and left "resource not granted at all" untested. They are now split along that line, with names that describe which failure each one is. The three wantErr rows shared a bare require.Error, so any error passed any row and a bug failing every input on the requested side would have left the allowed-side row green. wantErrContains replaces the bool and names the side. Rewording the allowed-side message as the requested-side one now fails three rows that previously all passed. Alias rejection was tested for one alias on one side. Both aliases are now tested on both sides. The allowed-side rows are the ones that earn their place: they are what would catch someone canonicalizing inside the allowed loop and widening the contract without a caller asking.
ScopesCover expanded and compared in a single pass, so the invariant guards only ever ran on scopes ExpandScope had produced. Every such scope satisfies them, which left the guards unverified in the position that matters: the existing test called checkCoverable directly and could not tell whether ScopesCover consulted it on both sides, or at all. Split the comparison into scopesCoverExpanded, which takes already expanded scopes paired with the names they came from. Tests drive synthetic Scope values through it, so dropping the guard from either side now fails, as does an allowed scope that grants every workspace action except delete answering a request for delete. Expanding every allowed scope before any guard runs reorders two error paths against each other: a requested scope that fails a guard alongside an unknown allowed name now reports the expansion failure rather than the guard failure. Both return (false, error), and no ScopeName reaches that combination today.
…ontract The knowledge of which spellings are backward-compatibility aliases lived in two switches, one in IsExternalScope and one in CanonicalScopeName, kept in step by discipline. Drift between them is asymmetric: a name the first accepts and the second does not rewrite is declared public and then fails to expand on every request naming it. Both now read one table, so they agree by construction, and an internal test walks that table asserting each alias is public, resolves to a public name, and resolves to one ExpandScope accepts. A third alias is covered the day it is added. ScopesCover stated "names must be canonical" in prose only, which is wrong for exactly the two inputs IsExternalScope accepts and ExpandScope does not. The parameters are now canonicalAllowed and canonicalRequested, so the requirement shows up in editor hints at every call site rather than only in a doc comment the caller may not have opened. Naming the parameters was chosen over canonicalizing inside ScopesCover. The single downstream caller already canonicalizes both sides in bulk before comparing, so absorbing the step would remove nothing from it while dissolving the distinction between a public spelling and a stored one at the layer that should hold it.
…roken The site-only, wildcard-allow-list, no-negatives invariant was described on ScopesCover and enforced by its guards, but ExpandScope, which is what produces those values, had no doc comment at all. Someone adding a scope reads ExpandScope and its neighbors; nothing there warned that populating User or adding a negative makes the scope uncomparable. State it there, along with the canonicalization requirement, and name the consequence rather than just the rule. Also note on ScopesCover that a wildcard request needs a wildcard grant. Enumerating today's concrete actions genuinely is narrower than `workspace:*`, so the rejection is intended. The OneActionDoesNotCoverResourceWildcard row already pins the behavior; the note stops the next reader of an authorize endpoint from taking it for a bug and closing the gap. Comments only. Checked that the documented invariant actually holds for all three builtin scopes and all seven composites.
BobbyHo
changed the title
|
All 13 findings are addressed across six commits. Mapping, so the threads can be checked against the diff rather than taken on trust:
A few notes where I did something other than what was suggested. CRF-4: testing CRF-14: I took the parameter-rename option rather than canonicalizing inside CRF-2: the shared table and its test landed here. Routing One correction on CRF-2's framing: Process notes both applied: the title is now |
|
/coder-agents-review |
There was a problem hiding this comment.
All fifteen R1 findings landed clean fixes. checkCoverable and scopesCoverExpanded are exactly the seam CRF-4 asked for: TestScopesCoverGuards drives each unmodeled shape through both sides via scopesCoverExpanded, and per-block coverage on the four new symbols reports 100%. TestScopesCoverAllowedNegativeDoesNotWiden disproves the R1 reasoning that dropping allowed-side User/ByOrgID was safe. permissionCovered no longer inspects Negate, and its docstring names checkCoverable as the single owner. The alias vocabulary now lives in one scopeAliases table and TestScopeAliases binds IsExternalScope, CanonicalScopeName, and ExpandScope in one loop. TestCanonicalScopeName pins the mapping direction: a swap goes red at the direct assertion rather than skating through a self-cover. Mafu-san flagged this as verified-before-claiming, unprompted correction of siblings, and reasoned pushback on a proposed option; the process floor is otherwise clean.
One finding does not close: CRF-2 is re-raised. The rbac-side fix landed (shared scopeAliases + binding test) but the three consumers R1 named still open-code the same table (coderd/apikey.go:94-101, coderd/apikey/apikey.go:91-98), and the multi-scope path at coderd/apikey.go:72-84 never canonicalizes at all. That last site is a user-visible bug today: POST /users/{user}/keys/tokens with {"scopes":["all"]} passes IsExternalScope via the new alias table, appends database.APIKeyScope("all") verbatim, fails APIKeyScope.Valid() (which lists coder:all and coder:application_connect but not the raw aliases), and returns HTTP 500 via httpapi.Write(ctx, rw, http.StatusInternalServerError, ...) at coderd/apikey.go:174. Eleven panel reviewers converged on this at P3 with independent end-to-end verification. The tool that closes the gap (CanonicalScopeName) ships in this PR one file over and never gets called. The author's PR-thread reply says these are "tracked, not dropped" in a separate ticket; no linked ID appears in the PR body, the reply comment, the PLAT-479 stack description, or any commit message on this branch. Per the panel rubric, a deferral without a linked ticket is a drop and needs a human decision: either land the fix here (each site is a one-line rewrite through rbac.CanonicalScopeName), or file the follow-up issue and paste the ID into the deferral thread. Pariston's line lands: given that CanonicalScopeName was added specifically because the panel found the vocabulary duplicated at ingress, and the ingress sites are still the duplicators, what is the plan that closes the loop, and where is it tracked so a reader in three months can find it?
Five smaller findings landed in R2: one duplicated test row weaker than the guards suite it copies (CRF-16), a comment-discipline pattern across five new doc/inline blocks that restate content available elsewhere (CRF-17), a misleading site prefix on shared test fixtures used in Role.User too (CRF-18), an asymmetric error wrap on the requested-side vs allowed-side expand * wrappers (CRF-19), and a docstring mismatch on ExternalScopeNames that predates the PR but will mislead the OAuth2 consumer (CRF-20).
Severity counts (new this round): 1 P3 re-raise, 1 P3, 3 Nit, 1 Note.
CRF-6 and CRF-15 from R1 were process observations (PR title, PR description); both addressed.
🤖 This review was automatically generated with Coder Agents.
TestScopesCoverAllowedNegativeDoesNotWiden drove the same scope shape as the NegativeUserPermission row of TestScopesCoverGuards, but asserted only that some error came back. The row asserts the message, the side it names, and that the comparison reports no coverage, and it runs the shape on both sides rather than one. The weaker copy could pass on a regression that returned the wrong error or stopped naming the side. Fold the scenario it documented into the row's comment and drop the copy. Rename the shared permission fixtures after the value they hold. The site prefix read as "belongs in Role.Site", while two of the three are placed in Role.User to build the shapes the guards refuse, and the No suffix gave no hint that it means Negate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The allowed-side wrap printed the scope name and then wrapped an error that prints it again, so the two sides of one comparison read differently: expand allowed scope "foo": no scope named "foo" expand requested scope: no scope named "foo" Drop the redundant verb and let the inner error carry the name on both sides. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The docstring said the list includes the `all` and `application_connect` special scopes. It appends ScopeAll and ScopeApplicationConnect, which are the `coder:` spellings, so the bare aliases are absent. Two callers already compensate by appending them by hand, one of them with a comment stating the mismatch. Describe what the function returns and name the helper that bridges the gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The invariant that expansion populates Site only was stated in full on ExpandScope, checkCoverable and ScopesCover, and the "everything except delete" example appeared on checkCoverable and again on permissionCovered twenty lines below. State it once on ScopesCover, which is the function whose behaviour depends on it, and cross-reference from the other two. Drop framing that ranked implementation choices nobody proposed, and cut the two test comments down to the facts the assertions do not already carry. Kept in full: what each guard in checkCoverable defends, since no other comment says it, and the wildcard rule on ScopesCover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All five R2 findings landed clean fixes at the commits the churn guard cited: 1678a77 (CRF-16 + CRF-18), 2fcce8b (CRF-17), aba3c6c (CRF-19), and 9a08105 (CRF-20). Independent panel verification lines up with the reply thread.
CRF-2's deferral is proper this round: #28322 is stacked on this branch, includes a regression test that reproduces the HTTP 500 on {"scopes":["all"]} and {"scopes":["application_connect"]} against unfixed code, and quotes the exact error string. Mafu-san's read: "deferral with proof, not deferral by promise, which is the trust signal the R2 panel demanded."
Six smaller residuals landed in R3, all Nits/Notes.
One is a comment-vs-code drift the CRF-17 trim introduced (CRF-21). checkCoverable's new comment says a negative site permission "would be skipped (see permissionCovered)," but CRF-5 (bd40270) removed exactly that skip. Two reviewers independently verified: permissionCovered({workspace, delete}, [{workspace, delete, Negate: true}]) returns true today, so a negative would be honored as a grant on a matching resource:action, not skipped. The guard catches this in the guards suite, but the comment now points a maintainer at removed behavior. If a future edit strips the guard "because permissionCovered skips negatives anyway," the exact bug CRF-3/CRF-4/CRF-5 defended against reappears. Small text fix; worth landing now, not in the next stack PR.
One is CRF-20's sibling (CRF-22). IsExternalScope's docstring names only the aliases and low-level scopes, omitting the composite coder:* scopes it also returns true for via externalComposite at line 114. Three reviewers converged. Same class as CRF-20; the CRF-20 fix rewrote ExternalScopeNames five lines below but did not update this sibling. Root-cause-fix invitation: one sentence.
Four smaller items: CRF-23 (Bisky) asks for one line in TestScopeAliases to pin the freshly-added CRF-20 promise that ExternalScopeNames omits bare aliases; CRF-24 (Gon) flags an unexplained *4 capacity hint (Gon passed on this in R1 as a common idiom; raising in R3 under a stricter bar, discretion note in inventory); CRF-25 (Zoro) points out that the alias iteration in TestScopesCoverEveryExternalScope is redundant with the canonical iteration because CanonicalScopeName resolves both aliases back to spellings already in the list, and the alias round-trip is already pinned by TestCanonicalScopeName and TestScopeAliases; CRF-26 (Kite Note) flags empty-allowlist semantics for the OAuth2 consumer's migration path.
Severity counts (new this round): 5 Nit, 1 Note. No P0-P3 findings this round.
CI note. The Flake Check job (96184391843) on head SHA 2fcce8b shows as failed on the PR. Komugi retrieved the job payload: the failure is in the Select changed tests step (whichtests returned exit code 2), and Run targeted Go flake checks never executed. No test in this diff was actually run in that CI job, so the failure is a workflow tooling issue against whichtests, not a defect in this diff. Local reproduction of the workflow's exact regex under -tags=testsmallbatch -short -count=35 -parallel=16 -shuffle=on (five back-to-back), then -count=100 -parallel=32 -shuffle=on GOMAXPROCS=1, then -race -count=50 -parallel=8: all green. Multiple other reviewers ran independent local iterations (Pariston 50x race, Kite 35x, Mafuuu 100x, Ryosuke -race -count=3, Netero 35x) and reproduced no flake. Safe to rerun the CI job; if it fails again at the same step, that is a workflow issue against whichtests, not a signal on this PR.
Dismissing the R2 REQUEST_CHANGES since all R2 findings are addressed and CRF-2 is properly deferred with a linked ticket.
🤖 This review was automatically generated with Coder Agents.
checkCoverable said a negative site permission would be skipped, naming a branch permissionCovered no longer has. A negative reaching it matches on resource type and action like any other grant, so the anti-grant would read as a grant. Name that instead, so the cross-reference lands on a doc that matches the code.
The docstring listed the aliases and the low-level scopes, omitting the curated composites the function also accepts. A caller consulting it to decide whether coder:workspaces.access is public read no from the doc and yes from the code.
ExternalScopeNames promises it offers each scope under one canonical spelling, and no test held it to that. TestScopesCoverEveryExternalScope appended the two aliases, but canonicalized them back into names the list already carries, so it re-ran assertions the list iteration had made and left the promise itself unpinned. Assert on the alias table instead: the list omits the alias and offers its canonical target. Every offered name is already proven coverable, so the aliases inherit coverage, and a third alias inherits both invariants the day it is added rather than needing a third hardcoded pair here.
dylanhuff-at-coder
left a comment
dylanhuff-at-coder
left a comment
There was a problem hiding this comment.
overall lgtm, left two small testing comments
2 participants
Split out of #28045 (PLAT-479) so the coverage rules can be reviewed on their own. First of three. No behavior change: both functions are added without production callers, and the OAuth2 authorize endpoint consumes them in the next PR.
ScopesCover(allowed, requested) reports whether every permission the request grants is also granted by the allowlist. It expands both sides and compares permissions rather than names, so coder:workspaces.access covers workspace:read without ever naming it.
The fail-closed rules are the part worth reviewing:
CanonicalScopeName maps the aliases IsExternalScope accepts, all and application_connect, onto the api_key_scope enum spellings. IsExternalScope answers whether a name may be requested, not how it is spelled once persisted, so anything storing a validated name has to canonicalize in between. Both sides of ScopesCover must already be canonical, which its parameter names restate at every call site.
Covered by 17 coverage cases, guard cases that drive each unmodeled shape through both sides of the comparison, and an exhaustive sweep over the external catalog: coder:all covers all of it, and every catalog name covers itself.
Stack: this PR, then negotiate and persist the scope, then the consent page and invalid_scope redirect.