feat: unify OAuth scope discovery between automatic and manual flows #701
+469
−20
Conversation
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
btiernay
force-pushed
the
feat/oauth-scope-discovery-consistency
branch
24 times, most recently
from
9c3daec to
cec765c
Compare
9 tasks
btiernay
force-pushed
the
feat/oauth-scope-discovery-consistency
branch
from
cec765c to
fc4475f
Compare
cliffhall
requested changes
Aug 14, 2025
- Add shared discoverScopes function in auth.ts with resource metadata preference - Update automatic flow (handleAuthError) to use dynamic scope discovery - Update manual flow (OAuth state machine) to use shared scope discovery logic - Add comprehensive test suite for discoverScopes function (11 test cases) - Fix empty array handling to return undefined instead of empty string - Maintain backward compatibility with existing oauthScope parameter - All quality gates pass: TypeScript, linting, formatting, and tests Resolves OAuth scope discovery inconsistency where automatic flow used static scopes while manual flow used metadata scopes. Both flows now use identical shared logic with proper resource metadata preference.
btiernay
force-pushed
the
feat/oauth-scope-discovery-consistency
branch
from
ca6ba87 to
34b3953
Compare
btiernay
commented
Aug 14, 2025
Comment on lines
+80
to
+98
| clientInformation: jest.fn().mockImplementation(async () => { | ||
| const serverUrl = "https://example.com/mcp"; | ||
| const preregisteredKey = `[${serverUrl}] ${SESSION_KEYS.PREREGISTERED_CLIENT_INFORMATION}`; | ||
| const preregisteredData = sessionStorage.getItem(preregisteredKey); | ||
| if (preregisteredData) { | ||
| return JSON.parse(preregisteredData); | ||
| } | ||
| const dynamicKey = `[${serverUrl}] ${SESSION_KEYS.CLIENT_INFORMATION}`; | ||
| const dynamicData = sessionStorage.getItem(dynamicKey); | ||
| if (dynamicData) { | ||
| return JSON.parse(dynamicData); | ||
| } | ||
| return undefined; | ||
| }), | ||
| saveClientInformation: jest.fn().mockImplementation((clientInfo) => { | ||
| const serverUrl = "https://example.com/mcp"; | ||
| const key = `[${serverUrl}] ${SESSION_KEYS.CLIENT_INFORMATION}`; | ||
| sessionStorage.setItem(key, JSON.stringify(clientInfo)); | ||
| }), |
There was a problem hiding this comment.
After the OAuth scope discovery commits, 2 AuthDebugger tests were failing because the test mocks were too simple. The clientInformation mock always returned undefined instead of checking sessionStorage for preregistered client info, and the saveClientInformation mock did nothing instead of actually saving data. The OAuth flow expected these mocks to behave like the real OAuth client provider by checking sessionStorage for existing client information and saving dynamic registration results when needed. Once we fixed the mocks to simulate this real behavior, the tests started passing again.
cliffhall
previously approved these changes
Aug 14, 2025
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.
OAuth Scope Discovery Unification
Fixes: #580, #587
This PR introduces centralized scope discovery logic through a new
discoverScopesfunction and integrates it across both OAuth authentication flows to ensure consistent scope resolution behavior.Also related: #705
Implementation Details
New
discoverScopesFunction:undefinedif no scopes availableIntegration Points:
useConnection): Integrates scope discovery in the 401 error handlerOAuthStateMachine): Replaces hardcoded scope logic in authorization redirect stepBefore/After Behavior
discoverScopes()discoverScopes()Scope Resolution Priority
Both flows now use identical scope resolution logic:
The
discoverScopesfunction implements the priority hierarchyTest Coverage
Added test coverage ensures correct behavior across scenarios including resource metadata preference, OAuth fallback, URL normalization, and error handling.