-
Notifications
You must be signed in to change notification settings - Fork 427
RI-7793 Fix SSO login by adding Sm-Id-Token header #5320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RI-7793 Fix SSO login by adding Sm-Id-Token header #5320
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Token renewal does not update idToken for SSO
The renewTokens method stores the new accessToken and refreshToken from the OAuth response but does not include idToken from data.id_token. The initial callback method was correctly updated to store idToken, but renewTokens was missed. When tokens are renewed, if the OAuth provider returns a new id_token, it gets discarded. Since the Sm-Id-Token header requires the idToken for SSO authentication, this could cause SSO login failures after token renewal when the original idToken expires.
redisinsight/api/src/modules/cloud/auth/cloud-auth.service.ts#L344-L351
RedisInsight/redisinsight/api/src/modules/cloud/auth/cloud-auth.service.ts
Lines 344 to 351 in a55870d
| await this.sessionService.updateSessionData(sessionMetadata.sessionId, { | |
| accessToken: data.access_token, | |
| refreshToken: data.refresh_token, | |
| idpType, | |
| csrf: null, | |
| apiSessionId: null, | |
| }); |
Code Coverage - Integration Tests
|
Code Coverage - Backend unit tests
Test suite run success3009 tests passing in 287 suites. Report generated by 🧪jest coverage report action from ab4eaab |
|
Images automagically compressed by Calibre's image-actions ✨ Compression reduced images by 5.8%, saving 20.0 KB.
|
- Add idToken field to CloudSession model and ICloudApiCredentials interface - Store id_token from OAuth token response in session after authentication - Add Sm-Id-Token header to all API requests when idToken is present - Header is automatically included for all auth types (Google, GitHub, SSO) Fixes #RI-7793
- Add idToken to mock data (cloud-session, cloud-auth, cloud-user) - Add test cases for Sm-Id-Token header in getHeaders method - Add tests to verify idToken is stored in session after OAuth callback - Add test to verify graceful handling of missing idToken - Update mock headers to include Sm-Id-Token header References: #RI-7793
Update renewTokens method to include idToken from renewal response in session data, ensuring Sm-Id-Token header continues to work after token refresh. - Add idToken to session update in renewTokens method - Add tests to verify idToken is stored when present in renewal response - Add test to verify graceful handling when idToken is missing References: #RI-7793
eadf3d8 to
ab4eaab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Token renewal may lose idToken when missing from refresh response
When the OAuth token refresh response doesn't include id_token (common behavior for many OAuth providers), data.id_token evaluates to undefined. Passing idToken: undefined to updateSessionData causes the existing idToken value in the session to be overwritten with undefined due to object spread behavior. This would break SSO functionality after token refresh because the Sm-Id-Token header would no longer be sent. The PR description states that idToken should be "persisted during token renewal to ensure header continues to work after token refresh," but the current implementation doesn't preserve the existing value when the refresh response lacks id_token.
redisinsight/api/src/modules/cloud/auth/cloud-auth.service.ts#L344-L352
RedisInsight/redisinsight/api/src/modules/cloud/auth/cloud-auth.service.ts
Lines 344 to 352 in ab4eaab
| await this.sessionService.updateSessionData(sessionMetadata.sessionId, { | |
| accessToken: data.access_token, | |
| refreshToken: data.refresh_token, | |
| idToken: data.id_token, | |
| idpType, | |
| csrf: null, | |
| apiSessionId: null, | |
| }); |
redisinsight/api/src/modules/cloud/auth/cloud-auth.service.ts#L222-L231
RedisInsight/redisinsight/api/src/modules/cloud/auth/cloud-auth.service.ts
Lines 222 to 231 in ab4eaab
| await this.sessionService.updateSessionData( | |
| authRequest.sessionMetadata.sessionId, | |
| { | |
| accessToken: tokens.access_token, | |
| refreshToken: tokens.refresh_token, | |
| idToken: tokens.id_token, | |
| idpType: authRequest.idpType, | |
| }, | |
| ); |
ArtemHoruzhenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
What
Fix SSO login issue where users cannot log in to Redis Cloud using SSO authentication. The login request was returning a 400 error because the required "Sm-Id-Token" header was missing.
idTokenfield toCloudSessionmodel andICloudApiCredentialsinterfaceid_tokenfrom OAuth/Okta token response in session after authenticationidTokenin session during token renewal to ensure header continues to work after token refreshidTokenis presentidTokenexistsgetHeadersmethod for consistencyTesting
SSO Login Flow:
Token Renewal:
idTokenis persisted in session when token is renewedid_token(should update session)id_token(should handle gracefully)Other Auth Types (Regression):
Edge Cases:
id_tokenin OAuth response (should handle gracefully)idTokenis presentTechnical Details
Implementation:
idTokenis stored in encrypted session after OAuth token exchangeidTokenis persisted during token renewal (overwritten from renewal response if present)CloudApiProvider.getHeaders()methodidTokenFiles Changed:
cloud-session.ts: AddedidTokenfield to modelapi.interface.ts: AddedidTokenfield to interfacecloud-auth.service.ts: Storeid_tokenfrom token response and persist during renewalcloud.api.provider.ts: Add "Sm-Id-Token" header whenidTokenexistsCloses #RI-7793
Note
Adds idToken handling and Sm-Id-Token header to fix SSO login, persisting idToken across token renewals.
id_tokenfrom OAuth callback and during token renewal in session (cloud-auth.service.ts), tolerating missingid_token.idToken(CloudSession,ICloudApiCredentials).Sm-Id-Tokenheader whenidTokenis present (cloud.api.provider.ts).idTokenstorage, renewal, and header generation (cloud-auth.service.spec.ts,cloud.api.provider.spec.ts,__mocks__).Written by Cursor Bugbot for commit ab4eaab. This will update automatically on new commits. Configure here.