Make force refresh AT also update the id token #2163
Conversation
tusharpandey13
changed the title
tusharpandey13
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2163 +/- ##
==========================================
- Coverage 82.61% 82.18% -0.43%
==========================================
Files 21 21
Lines 2042 2094 +52
Branches 358 368 +10
==========================================
+ Hits 1687 1721 +34
- Misses 348 364 +16
- Partials 7 9 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (error) { | ||
| return [error, null]; | ||
| } | ||
| return [null, filterClaims(claims)]; |
There was a problem hiding this comment.
Developers may have provided their own custom filtering function for the ID token claims. In this case, we would be overriding it with the DEFAULT_ALLOWED_CLAIMS which would be inconsistent.
We might want to consider offering a hook to allow for the filtering to be customized and consistent in other places we verify and store the IDT claims.
There was a problem hiding this comment.
I am wondering what the reason is to not have this go through the same logic (including id token claims filtering) we already have in the callback and getAccessToken?
| if (!authorizationServerMetadata.issuer) { | ||
| return [new DiscoveryError("Issuer not found in metadata"), null]; | ||
| } |
There was a problem hiding this comment.
The issuer can never be undefined and we don't have this check elsewhere. Is there a reason you've added this explicit check here?
| if (!authorizationServerMetadata.jwks_uri) { | ||
| return [new DiscoveryError("JWKS URI not found in metadata"), null]; | ||
| } |
There was a problem hiding this comment.
Are there cases where we don't publish a jwks_uri for a tenant? Is this check needed?
| new OAuth2Error({ | ||
| code: e.code || "ID_TOKEN_VERIFICATION_FAILED", | ||
| message: e.message || "ID token verification failed." | ||
| }), |
There was a problem hiding this comment.
Let's be consistent with the casing and naming of our error codes. They should be added to src/errors/index.ts.
Also, we may want to consider a custom error object here similar to what we have for AT refresh.
| new URL(authorizationServerMetadata.jwks_uri) | ||
| ); | ||
|
|
||
| const ID_TOKEN_SIGNING_ALG = "RS256"; |
There was a problem hiding this comment.
Why are we predefining the supported IDT algo here?
| const [profileError, newProfile] = | ||
| await this.authClient.getUserFromIdToken(tokenSet.idToken!); |
There was a problem hiding this comment.
Could it be that we don't get an ID token back? I believe we require the openid scope but perhaps we should consider falling back to calling /userinfo here?
| // Mock jose.jwtVerify to prevent actual JWT verification during getAccessToken flow | ||
| vi.mock("jose", async () => { | ||
| const actual = await vi.importActual("jose"); | ||
| return { | ||
| ...actual, | ||
| jwtVerify: vi.fn(), | ||
| createRemoteJWKSet: vi.fn() | ||
| }; | ||
| }); |
There was a problem hiding this comment.
It's more preferable to mint a valid JWT than to mock the dependency.
Make getAccessToken({refresh:true}) also update the id token