fix: add scope to Microsoft OAuth callback#1508
Closed
tembo[bot] wants to merge 1 commit into
Closed
Conversation
…hange Co-authored-by: Konsti <n2d4xc@gmail.com>
Contributor
Author
|
Requesting review from @N2D4 who has experience with the following files modified in this PR:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Contributor
Greptile SummaryFixes the
Confidence Score: 4/5Safe to merge; the change is narrowly scoped to Microsoft and leaves all other providers' token exchange behavior unchanged. The implementation is clean and well-isolated. apps/backend/src/oauth/providers/base.test.ts — the no-extra-scope case for Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant CallbackRoute as OAuth Callback Route
participant MicrosoftProvider
participant getOAuthCallbackExtras
participant MSTokenEndpoint as Microsoft Token Endpoint
Browser->>CallbackRoute: "GET /callback/microsoft?code=..."
CallbackRoute->>CallbackRoute: Read providerScope from OAuthOuterInfo
CallbackRoute->>MicrosoftProvider: "getCallback({ extraScope: providerScope, ... })"
MicrosoftProvider->>getOAuthCallbackExtras: "{ baseScope, extraScope, includeScopeInCallbackTokenRequest: true }"
getOAuthCallbackExtras-->>MicrosoftProvider: "{ exchangeBody: { scope: "User.Read openid [extra]" } }"
MicrosoftProvider->>MSTokenEndpoint: "POST /token (code + scope=...)"
MSTokenEndpoint-->>MicrosoftProvider: access_token + id_token
MicrosoftProvider-->>CallbackRoute: "{ userInfo, tokenSet }"
CallbackRoute-->>Browser: Redirect with session
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/backend/src/oauth/providers/base.test.ts:113-124
Missing test for the common case where a Microsoft user has no extra scopes. When `includeScopeInCallbackTokenRequest` is `true` but `extraScope` is absent, the function should still return `exchangeBody.scope` containing just the base scope. Without this test, a future refactor of the `extraScope || ""` fallback could silently drop the scope from the token exchange body in the default (no extra scopes) flow.
```suggestion
it("adds the merged scope to callback token exchange extras when enabled", () => {
expect(getOAuthCallbackExtras({
baseScope: "User.Read openid",
extraScope: "Mail.Read User.Read",
includeScopeInCallbackTokenRequest: true,
})).toEqual({
exchangeBody: {
scope: "User.Read openid Mail.Read",
},
});
});
it("uses only baseScope when no extraScope is provided and includeScopeInCallbackTokenRequest is true", () => {
expect(getOAuthCallbackExtras({
baseScope: "User.Read openid",
includeScopeInCallbackTokenRequest: true,
})).toEqual({
exchangeBody: {
scope: "User.Read openid",
},
});
});
});
```
Reviews (1): Last reviewed commit: "fix(oauth): include scope param in Micro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+113
to
+124
| it("adds the merged scope to callback token exchange extras when enabled", () => { | ||
| expect(getOAuthCallbackExtras({ | ||
| baseScope: "User.Read openid", | ||
| extraScope: "Mail.Read User.Read", | ||
| includeScopeInCallbackTokenRequest: true, | ||
| })).toEqual({ | ||
| exchangeBody: { | ||
| scope: "User.Read openid Mail.Read", | ||
| }, | ||
| }); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
includeScopeInCallbackTokenRequest is true but extraScope is absent, the function should still return exchangeBody.scope containing just the base scope. Without this test, a future refactor of the extraScope || "" fallback could silently drop the scope from the token exchange body in the default (no extra scopes) flow.
Suggested change
| it("adds the merged scope to callback token exchange extras when enabled", () => { | |
| expect(getOAuthCallbackExtras({ | |
| baseScope: "User.Read openid", | |
| extraScope: "Mail.Read User.Read", | |
| includeScopeInCallbackTokenRequest: true, | |
| })).toEqual({ | |
| exchangeBody: { | |
| scope: "User.Read openid Mail.Read", | |
| }, | |
| }); | |
| }); | |
| }); | |
| it("adds the merged scope to callback token exchange extras when enabled", () => { | |
| expect(getOAuthCallbackExtras({ | |
| baseScope: "User.Read openid", | |
| extraScope: "Mail.Read User.Read", | |
| includeScopeInCallbackTokenRequest: true, | |
| })).toEqual({ | |
| exchangeBody: { | |
| scope: "User.Read openid Mail.Read", | |
| }, | |
| }); | |
| }); | |
| it("uses only baseScope when no extraScope is provided and includeScopeInCallbackTokenRequest is true", () => { | |
| expect(getOAuthCallbackExtras({ | |
| baseScope: "User.Read openid", | |
| includeScopeInCallbackTokenRequest: true, | |
| })).toEqual({ | |
| exchangeBody: { | |
| scope: "User.Read openid", | |
| }, | |
| }); | |
| }); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/oauth/providers/base.test.ts
Line: 113-124
Comment:
Missing test for the common case where a Microsoft user has no extra scopes. When `includeScopeInCallbackTokenRequest` is `true` but `extraScope` is absent, the function should still return `exchangeBody.scope` containing just the base scope. Without this test, a future refactor of the `extraScope || ""` fallback could silently drop the scope from the token exchange body in the default (no extra scopes) flow.
```suggestion
it("adds the merged scope to callback token exchange extras when enabled", () => {
expect(getOAuthCallbackExtras({
baseScope: "User.Read openid",
extraScope: "Mail.Read User.Read",
includeScopeInCallbackTokenRequest: true,
})).toEqual({
exchangeBody: {
scope: "User.Read openid Mail.Read",
},
});
});
it("uses only baseScope when no extraScope is provided and includeScopeInCallbackTokenRequest is true", () => {
expect(getOAuthCallbackExtras({
baseScope: "User.Read openid",
includeScopeInCallbackTokenRequest: true,
})).toEqual({
exchangeBody: {
scope: "User.Read openid",
},
});
});
});
```
How can I resolve this? If you propose a fix, please make it concise.
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
2 participants
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.
Summary
Add support for including the 'scope' parameter in Microsoft OAuth callback token exchange to fix invalid_scope error.
providerScopeasextraScopetogetCallbackin OAuth callback handler.getOAuthCallbackExtrasto conditionally add merged scopes to token exchange request.includeScopeInCallbackTokenRequestflag in Microsoft provider to include scope in token exchange.getOAuthCallbackExtrasbehavior.This resolves the AADSTS70011 error caused by missing 'scope' in token exchange request for Microsoft OAuth.
Summary by cubic
Fix Microsoft OAuth invalid_scope (AADSTS70011) by sending the merged scope in the callback token exchange. Scoped to Microsoft only; other providers are unchanged.
providerScopeasextraScopetogetCallbackin the OAuth callback.getOAuthCallbackExtrasto merge scopes and include them in the token exchange when enabled.includeScopeInCallbackTokenRequest: truefor the Microsoft provider.getOAuthCallbackExtras.Written for commit 24f28da. Summary will update on new commits.
Review in cubic