Repository navigation
Fixes #35674: sync SSO display name and picture on server-side OIDC logins - #35675
mohityadav766 wants to merge 5 commits into
Conversation
On the confidential (server-side) OIDC flow the server signs the user in, so it is the one that has to apply the identity provider's profile claims. It applied neither: - The legacy principal-claims path, which is the default while emailClaim is unset, resolved the provider's display name and then dropped it. #25391 removed the sync that #25623 had added, and every release since 1.12.4 shipped without it. - No path read the standard OIDC picture claim, so avatars only worked for public clients, where the browser saves them. The browser made the name worse: after a server-driven login it saved the username it was handed as the display name. The OIDC resolver now reads the picture claim once, as an absolute http(s) address, and both sign-in paths store it on the profile, replacing every stored size when it changes. A token without a picture keeps the stored one. The legacy path fills the display name when the account has none, or has one that only repeats the username, so accounts the browser already stamped are repaired on their next login; a name chosen in OpenMetadata is kept. The legacy path also loads the user with the auth-update field set. Saving its "id,roles,teams" copy erased the stored profile whenever a login promoted an admin or assigned a team. The UI no longer writes the profile after a confidential-client or SAML login.
…driven logins Who writes the display name and picture after a confidential-client or SAML login, which picture addresses are stored, and when the legacy path may replace a display name.
getUserByNameForAuthUpdate is the name counterpart of getActiveUserByEmailForAuth: it reads the account fresh, with the auth-update fields a PUT of it writes back. With the lookup there, EmailFirstIdentityIT runs it against a real database and shows that the loaded copy carries the stored profile and that saving it keeps it.
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
|
| Count | Rule |
|---|---|
| 8 | react-hooks/exhaustive-deps |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:345:9 |
react-hooks/exhaustive-deps |
The 'onLoginHandler' function makes the dependencies of useMemo Hook (at line 1213) change on every render. Move it inside the useMemo callback. Alternatively, |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:463:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'navigate', 'setApplicationLoading', 'setCurrentUser', and 'setIsAuthenticated'. Either include them or remove |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:549:9 |
react-hooks/exhaustive-deps |
The 'resetUserDetails' function makes the dependencies of useMemo Hook (at line 1213) change on every render. To fix this, wrap the definition of 'resetUserDeta |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:795:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'handleRefreshFailed', 'handleStoreProtectedRedirectPath', 'setIsAuthenticated', and 'signOutAfterRefreshFailure' |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:797:9 |
react-hooks/exhaustive-deps |
The 'handleFailedLogin' function makes the dependencies of useMemo Hook (at line 1213) change on every render. Move it inside the useMemo callback. Alternativel |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:868:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'handledVerifiedUser', 'navigate', and 'resetUserDetails'. Either include them or remove the dependency array. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:919:9 |
react-hooks/exhaustive-deps |
The 'initializeAxiosInterceptors' function makes the dependencies of useMemo Hook (at line 1213) change on every render. To fix this, wrap the definition of 'in |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:1202:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'fetchAuthConfig'. Either include it or remove the dependency array. |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed
|
Local Java test run: PASSED — details
Tests run locally
Per-class counts come from the test reports, which can credit a test to the wrong class when classes run concurrently, and keep only the last run of a nested class that several test classes inherit. The summary's step totals are Maven's own counts. integration · mysql-elasticsearch · parallel: 64 classes
Why these tests
Impacted, not run locally (2)
Commandsmvn -B package -pl openmetadata-mcp,openmetadata-service -am -Dspring-boot.repackage.skip=true -Dtest=AuthCallbackServletTest,AuthenticationCodeFlowHandlerTest,BasicAuthServletHandlerTest,CacheBundleTest,CachedPermissionEvaluationTest,DataContractPipelineTokenRefreshTest,EntityCsvTest,JwtFilterTest,K8sPipelineClientBotTokenBugTest,K8sPipelineClientTest,LdapAuthServletHandlerTest,LdapAuthenticatorTest,McpCallbackServletTest,OidcIdentityResolverTest,OpenMetadataConnectionBuilderTest,PermissionDebugServiceTest,SamlAuthServletHandlerTest,SetApprovalAssigneesImplTest,SsoRedirectBoundaryTest,SubjectCacheTest,SubjectContextTest,TestLoginServiceTest,UserActivityTrackerTest,UserMetricsServletTest,UserProfileClaimsTest,UserRepositoryUnitTest,UserUtilTest -Dsurefire.failIfNoSpecifiedTests=false
mvn -B verify -pl :openmetadata-integration-tests -am -Dspring-boot.repackage.skip=true -Pmysql-elasticsearch -DintegrationTests.lane=parallel '-Dit.test=<63 classes>' -Dfailsafe.failIfNoSpecifiedTests=false -Dtest=NoUnitTestsInThisRun -Dsurefire.failIfNoSpecifiedTests=false
mvn -B verify -pl :openmetadata-integration-tests -am -Dspring-boot.repackage.skip=true -Pmysql-elasticsearch -DintegrationTests.lane=global-state -Dit.test=SessionMultiNodeIT,UserMetricsResourceIT -Dfailsafe.failIfNoSpecifiedTests=false -Dtest=NoUnitTestsInThisRun -Dsurefire.failIfNoSpecifiedTests=false
mvn -B verify -pl :openmetadata-integration-tests -am -Dspring-boot.repackage.skip=true -Pmysql-elasticsearch -DintegrationTests.lane=rdf -Dit.test=RdfFusekiAuthorizationIT,RdfPolicyRevocationIT -Dfailsafe.failIfNoSpecifiedTests=false -Dtest=NoUnitTestsInThisRun -Dsurefire.failIfNoSpecifiedTests=false
mvn -B verify -pl :openmetadata-integration-tests -am -Dspring-boot.repackage.skip=true -Pcache-tests -DintegrationTests.lane=global-state -Dit.test=SessionRedisMultiNodeIT,TokenRevocationRedisMultiNodeIT -Dfailsafe.failIfNoSpecifiedTests=false -Dtest=NoUnitTestsInThisRun -Dsurefire.failIfNoSpecifiedTests=false |
Jest test CoverageUI tests summary
|
Code Review ✅ Approved🟡 Medium risk · Server-driven logins now persist provider-supplied profile claims, so unexpected names or picture URLs could misrepresent users’ displayed identities. Server-driven OIDC logins now persist the provider's display name and picture, fixing SSO users who saw an initial and their login name instead of their profile. The principal-claims lookup also loads the full user so login updates no longer erase stored profile fields, and the browser skips writing the profile after confidential-client and SAML logins. No issues found. VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
|
✅ SSO Login Nightly — PASSED View workflow run · Commit
Totals: 94 passed · 0 failed · 0 flaky · 0 skipped · duration 18m 01s |
This branch was successfully deployed
3 participants
Describe your changes:
Fixes #35674
With SSO set up as a confidential (server-side) OIDC client, signed-in users got neither a profile picture nor a display name from the identity provider: the avatar showed an initial, and the name shown was the login name. On that flow the server completes sign-in, so the server has to apply the provider's profile claims. It applied neither:
emailClaimunset),OidcIdentityResolverresolves the provider's name (name, orgiven_name+family_name) andgetOrCreateOidcUserdropped it. feat(ui): support displayName extraction from user when login #25623 added the sync. Mcp oauth #25391 (MCP OAuth) removed it while merging over it, and its cherry-pick took it out of 1.12.4. 1.11.10–1.12.3 had it; 1.12.4+, 1.13.x, 2.0.x andmaindon't.pictureclaim, and the callback page hands the browserpicture: ''. Only public clients, where the browser PATCHes the user, got pictures.checkIfUpdateRequiredsaved thenameredirect parameter, which is the OpenMetadata username, as the display name when the account had none.id,roles,teamsonly and PUT that copy back when a login promoted an admin or assigned a team from claims, whichcreateOrUpdaterecords as deletingprofileand the other fields the lookup left out (seeUserRepository.getAuthUpdateFields()).This PR:
OidcIdentityResolver.ResolvedOidcIdentitygainspicture: thepictureclaim, parsed once and kept only when it is an absolutehttp/httpsURI, because it ends up as an image source in the browser.UserProfileClaims:syncPicturestores the picture asprofile.images.image. A new picture replaces the whole image list, so sizes derived from an older picture stop showing. A token without a picture never clears the stored one.fillDisplayNamefills the display name when the account has none, or has one that only repeats its login name. A name chosen in OpenMetadata is kept.AuthenticationCodeFlowHandler: both OIDC paths store the picture. The email-first path does it through the provisioner's OIDC mutators, which already assigned teams; it already synced the display name. The principal-claims path applies the display name again, on sign-up and on login.UserRepository.getUserByNameForAuthUpdate(name), the name counterpart ofgetActiveUserByEmailForAuth: the principal-claims path now loads the account fresh withgetAuthUpdateFields(), so saving it can't erase fields.isServerDrivenLogin(clientType, provider): a confidential client or SAML, the same condition that already picksGenericAuthenticator.handleSuccessfulLoginskipscheckIfUpdateRequiredfor those logins. Public OIDC clients are unchanged.docs/decisions/2026-10-17-the-server-applies-the-sso-profile-on-server-driven-logins.md.Existing accounts: a user whose display name was stamped with their login name gets the provider's name on their next sign-in, with no migration.
Type of change:
High-level design:
ResolvedOidcIdentity, and a small pure helper,UserProfileClaims, applies them to aUser, so the rules are unit-tested on realUserobjects.=s512-c) on the server. That's provider-specific, and the UI's image fallback chain already ends atimage.pictureclaim, so Azure users still get no picture.isServerDrivenLogin.Tests:
Use cases covered
javascript:,data:,file:and relative values are ignored.Unit tests
UserProfileClaimsTest(new, 21 cases),OidcIdentityResolverTest(+1),AuthenticationCodeFlowHandlerTest(+5).main's behaviour before the fix:expected: <Jane Doe> but was: <null>on sign-up, nothing persisted for an existing user (principal-claims and email-first), andSaving the login's changes erased the stored profile.AuthProvider.util.test.ts(+4,isServerDrivenLogin),AuthProvider.test.tsx(+2). The confidential-client test fails againstmain'sAuthProvider.tsx:checkIfUpdateRequiredreceived{"profile": {"email": "aaron@example.com", "name": "aaron"}}.AuthProviderUtil.test.ts,UserDataUtils.test.ts,SamlCallback.test.tsx(113 tests across the 5 suites).Backend integration tests
openmetadata-integration-tests/for the changed behaviour.EmailFirstIdentityIT, against the real repository:testSignInStoresAndRefreshesTheProviderPicture: sign-up stores the picture, a new one replaces it, a token without one keeps it;testLoginByNameDoesNotEraseStoredUserFields: the principal-claims lookup loads the stored profile, and saving that copy after the login fills the display name keeps it.Local Java test run: PASSED
eaa97997d226(baseorigin/main)Ingestion integration tests
Playwright (UI) tests
make playwright_affectedselects 43 specs, all of which sign in with basic auth, whereisServerDrivenLoginis false and the login path is unchanged; none reaches a confidential-client or SAML login. Not run locally; the merge queue runs the full suite. The SSO sign-in suites (GoogleSsoSignInUIIT,GoogleSsoBootstrapUIIT) run nightly and assert neither display name nor picture.Manual testing performed
Not run against a live SSO deployment; the integration tests above cover persistence. To reproduce by hand:
nameandpicturefor theprofilescope (Google, Auth0, or a Keycloak user with a picture), withclientType: confidentialandemailClaimunset.GET /api/v1/users/loggedInUser?fields=profilereturnsdisplayNameandprofile.images.image.PATCH /api/v1/users/{id}), sign in again → it becomes the provider's name. Set it to something else, sign in again → it is kept.UI screen recording:
Not applicable — a bug fix, not a UI feature. The UI change only stops the browser from writing the profile after a confidential-client or SAML login; the avatar renders the stored picture through the existing component.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.docs/decisions/2026-10-17-the-server-applies-the-sso-profile-on-server-driven-logins.md.AuthenticationCodeFlowHandlerTest.getOrCreateOidcUser_*TheProviderAsserts,AuthProvider.test.tsx"leaves the profile to the server after a confidential-client login").🤖 Generated with Claude Code
The PR appears safe to merge; no actionable issues were found.Summary
The server now saves OIDC profile pictures and fills missing display names during confidential-client sign-in. The browser leaves server-driven profiles alone.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[OIDC provider claims] --> B[Resolve identity and picture] B --> C[Load or create user] C --> D[Apply name and picture rules] D --> E[Save changed user] E --> F[Browser loads saved user] F --> G[Skip browser profile write] H[Public OIDC login] --> I[Keep browser profile update]Reviews (2) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." · Reviewed by Greptile