From 21251a1ecbba758b6f5d713b48eb4201e3c23685 Mon Sep 17 00:00:00 2001 From: mohitdeuex Date: Sat, 17 Oct 2026 03:27:01 +0530 Subject: [PATCH 1/4] fix(auth): sync SSO display name and picture on server-side OIDC logins 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. --- .../it/tests/EmailFirstIdentityIT.java | 39 +++ .../AuthenticationCodeFlowHandler.java | 51 +++- .../security/OidcIdentityResolver.java | 8 +- .../service/security/UserProfileClaims.java | 105 ++++++++ .../AuthenticationCodeFlowHandlerTest.java | 225 +++++++++++++++--- .../security/OidcIdentityResolverTest.java | 28 +++ .../security/UserProfileClaimsTest.java | 159 +++++++++++++ .../Auth/AuthProviders/AuthProvider.test.tsx | 49 +++- .../Auth/AuthProviders/AuthProvider.tsx | 33 ++- .../ui/src/utils/AuthProvider.util.test.ts | 25 ++ .../ui/src/utils/AuthProvider.util.ts | 11 + 11 files changed, 677 insertions(+), 56 deletions(-) create mode 100644 openmetadata-service/src/main/java/org/openmetadata/service/security/UserProfileClaims.java create mode 100644 openmetadata-service/src/test/java/org/openmetadata/service/security/UserProfileClaimsTest.java diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/EmailFirstIdentityIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/EmailFirstIdentityIT.java index e46b079278e8..ccc3c1ea52a9 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/EmailFirstIdentityIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/EmailFirstIdentityIT.java @@ -28,6 +28,7 @@ import org.openmetadata.service.Entity; import org.openmetadata.service.jdbi3.UserRepository; import org.openmetadata.service.security.EmailFirstUserProvisioner; +import org.openmetadata.service.security.UserProfileClaims; import org.openmetadata.service.util.EntityUtil; import org.openmetadata.service.util.UserUtil; @@ -227,6 +228,24 @@ void testLoginDoesNotEraseStoredUserFields(TestNamespace ns) { assertEquals(user.getId(), after.getId()); } + @Test + void testSignInStoresAndRefreshesTheProviderPicture(TestNamespace ns) { + String localPart = ("picture" + ns.uniqueShortId()).toLowerCase(Locale.ROOT); + String email = localPart + "@x.test.om.org"; + URI firstPicture = URI.create("https://idp.test.om.org/" + localPart + "/first.png"); + URI newPicture = URI.create("https://idp.test.om.org/" + localPart + "/new.png"); + + provisionWithPicture(email, firstPicture); + assertEquals(firstPicture, storedPicture(email), "Sign-up must store the provider's picture"); + + provisionWithPicture(email, newPicture); + assertEquals(newPicture, storedPicture(email), "A new provider picture must replace the old"); + + provisionWithPicture(email, null); + assertEquals( + newPicture, storedPicture(email), "A token without a picture must keep the stored one"); + } + @Test void testSynthesizedEmailDomainListingFindsAccountsToRepair(TestNamespace ns) { String suffix = ns.uniqueShortId().toLowerCase(Locale.ROOT); @@ -257,6 +276,26 @@ private User provision(String email, String displayName) { return provision(email, displayName, null); } + /** Applies the picture the way the OIDC sign-in does, both to a new account and an existing one. */ + private User provisionWithPicture(String email, URI picture) { + return EmailFirstUserProvisioner.forProvider( + "TEST", + new AuthorizerConfiguration(), + Entity.getUserRepository(), + user -> UserProfileClaims.syncPicture(user, picture), + user -> UserProfileClaims.syncPicture(user, picture)) + .getOrCreate(email, "Picture User", null, true); + } + + private URI storedPicture(String email) { + Profile profile = + Entity.getUserRepository() + .getByEmail(null, email, new EntityUtil.Fields(Set.of("profile"))) + .getProfile(); + assertNotNull(profile, "Expected a stored profile for " + email); + return profile.getImages().getImage(); + } + private User provision(String email, String displayName, String identityProviderSubject) { List noTeams = List.of(); return EmailFirstUserProvisioner.forProvider( diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/AuthenticationCodeFlowHandler.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/AuthenticationCodeFlowHandler.java index a285b3374a36..ed7e76454e5f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/AuthenticationCodeFlowHandler.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/AuthenticationCodeFlowHandler.java @@ -97,6 +97,7 @@ import org.openmetadata.service.auth.JwtResponse; import org.openmetadata.service.exception.AuthenticationException; import org.openmetadata.service.exception.EntityNotFoundException; +import org.openmetadata.service.jdbi3.UserRepository; import org.openmetadata.service.security.auth.SecurityConfigurationManager; import org.openmetadata.service.security.jwt.JWTTokenGenerator; import org.openmetadata.service.security.policyevaluator.SubjectCache; @@ -646,8 +647,8 @@ public void handleCallback(HttpServletRequest req, HttpServletResponse resp) { OidcIdentityResolver.ResolvedOidcIdentity identity = resolveOidcIdentity(claims); User user = identity.emailFirstFlow() - ? getOrCreateEmailFirstOidcUser(identity.email(), identity.displayName(), claims) - : getOrCreateOidcUser(identity.userName(), identity.email(), claims); + ? getOrCreateEmailFirstOidcUser(identity, claims) + : getOrCreateOidcUser(identity, claims); syncRolesFromProvider(user, claims); Entity.getUserRepository().updateUserLastLoginTime(user, System.currentTimeMillis()); @@ -1166,33 +1167,57 @@ private OidcIdentityResolver.ResolvedOidcIdentity resolveOidcIdentity( } private User getOrCreateEmailFirstOidcUser( - String email, String displayName, Map claims) { + OidcIdentityResolver.ResolvedOidcIdentity identity, Map claims) { List teamsFromClaim = findTeamsFromClaims(teamClaimMapping, claims); return EmailFirstUserProvisioner.forProvider( "OIDC", authorizerConfiguration, Entity.getUserRepository(), - user -> UserUtil.assignTeamsFromClaim(user, teamsFromClaim), - user -> UserUtil.assignTeamsFromClaim(user, teamsFromClaim)) + user -> applyTeamsAndPicture(user, teamsFromClaim, identity.picture()), + user -> applyTeamsAndPicture(user, teamsFromClaim, identity.picture())) .getOrCreate( - email, - displayName, + identity.email(), + identity.displayName(), SecurityUtil.getClaimOrObject(claims.get(JWTTokenGenerator.SUBJECT_CLAIM)), Boolean.TRUE.equals(authenticationConfiguration.getEnableSelfSignup())); } + private static boolean applyTeamsAndPicture(User user, List teams, URI picture) { + boolean teamsAssigned = UserUtil.assignTeamsFromClaim(user, teams); + boolean pictureChanged = UserProfileClaims.syncPicture(user, picture); + return teamsAssigned || pictureChanged; + } + + private static boolean applyDisplayNameAndPicture( + User user, OidcIdentityResolver.ResolvedOidcIdentity identity) { + boolean displayNameFilled = UserProfileClaims.fillDisplayName(user, identity.displayName()); + boolean pictureChanged = UserProfileClaims.syncPicture(user, identity.picture()); + return displayNameFilled || pictureChanged; + } + + /** + * Loads the account fresh and with every field a PUT writes back: {@code createOrUpdate} compares + * the whole entity, so a field the lookup left out would be stored as deleted. + */ + private static User findUserForUpdate(String userName) { + UserRepository userRepository = Entity.getUserRepository(); + return userRepository.getByName( + null, userName, userRepository.getAuthUpdateFields(), Include.NON_DELETED, false); + } + private boolean isUserAdmin(String email, String username) { return UserUtil.isConfiguredAdmin(authorizerConfiguration, email, username); } - private User getOrCreateOidcUser(String userName, String email, Map claims) { + private User getOrCreateOidcUser( + OidcIdentityResolver.ResolvedOidcIdentity identity, Map claims) { + String userName = identity.userName(); + String email = identity.email(); // Extract teams from claims if configured (supports array claims like groups) List teamsFromClaim = findTeamsFromClaims(teamClaimMapping, claims); try { - // Fetch user with teams relationship loaded to preserve existing team memberships - User user = - Entity.getEntityByName(Entity.USER, userName, "id,roles,teams", Include.NON_DELETED); + User user = findUserForUpdate(userName); boolean shouldBeAdmin = isUserAdmin(email, userName); boolean needsUpdate = false; @@ -1212,7 +1237,8 @@ private User getOrCreateOidcUser(String userName, String email, Map claims) { String displayName = extractDisplayNameFromClaim(claims, authenticationConfiguration.getDisplayNameClaim()); LOG.debug("OIDC email-first flow: email={}, displayName={}", email, displayName); - return new ResolvedOidcIdentity(null, email, displayName, true); + return new ResolvedOidcIdentity(null, email, displayName, pictureFrom(claims), true); } } @@ -90,7 +92,7 @@ public ResolvedOidcIdentity resolve(Map claims) { authorizerConfiguration.getAllowedDomains(), authorizerConfiguration.getEnforcePrincipalDomain()); String displayName = extractDisplayNameFromClaims(claims); - return new ResolvedOidcIdentity(userName, email, displayName, false); + return new ResolvedOidcIdentity(userName, email, displayName, pictureFrom(claims), false); } public List resolveTeams(Map claims, String teamClaimMapping) { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/UserProfileClaims.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/UserProfileClaims.java new file mode 100644 index 000000000000..d8f78854b612 --- /dev/null +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/UserProfileClaims.java @@ -0,0 +1,105 @@ +package org.openmetadata.service.security; + +import static org.openmetadata.common.utils.CommonUtil.nullOrEmpty; + +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Locale; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import lombok.extern.slf4j.Slf4j; +import org.openmetadata.schema.entity.teams.User; +import org.openmetadata.schema.type.ImageList; +import org.openmetadata.schema.type.Profile; + +/** + * Keeps how an account is presented, its display name and picture, in step with what the identity + * provider asserts at sign-in. Only values the provider actually sent are applied: a claim the + * token leaves out never clears what the account already has. + */ +@Slf4j +public final class UserProfileClaims { + private static final String PICTURE_CLAIM = "picture"; + private static final Set WEB_SCHEMES = Set.of("http", "https"); + + private UserProfileClaims() {} + + /** + * The standard OIDC {@code picture} claim, when it is an absolute http(s) address. The address + * becomes an image source in the browser, so anything else is dropped rather than stored. + */ + public static URI pictureFrom(Map claims) { + String claim = SecurityUtil.getClaimOrObject(claims.get(PICTURE_CLAIM)); + return nullOrEmpty(claim) ? null : parseWebAddress(claim.trim()); + } + + /** + * Points the account's picture at the provider's. A new picture replaces every stored size, since + * sizes derived from the previous picture would otherwise keep showing it. + * + * @return whether the account changed + */ + public static boolean syncPicture(User user, URI picture) { + boolean changes = picture != null && !picture.equals(currentPicture(user)); + if (changes) { + Profile profile = Optional.ofNullable(user.getProfile()).orElseGet(Profile::new); + user.setProfile(profile.withImages(new ImageList().withImage(picture))); + } + return changes; + } + + /** + * Fills the display name from the provider when the account has none of its own. A name chosen in + * OpenMetadata is kept. + * + * @return whether the account changed + */ + public static boolean fillDisplayName(User user, String displayName) { + boolean fills = + !nullOrEmpty(displayName) + && !displayName.isBlank() + && lacksOwnDisplayName(user) + && !displayName.equals(user.getDisplayName()); + if (fills) { + user.setDisplayName(displayName); + } + return fills; + } + + /** + * A display name that only repeats the login name says nothing the name does not. The browser + * wrote exactly that on sign-in while the provider's name was being dropped. + */ + private static boolean lacksOwnDisplayName(User user) { + String current = user.getDisplayName(); + return nullOrEmpty(current) || current.equals(user.getName()); + } + + private static URI currentPicture(User user) { + return Optional.ofNullable(user.getProfile()) + .map(Profile::getImages) + .map(ImageList::getImage) + .orElse(null); + } + + private static URI parseWebAddress(String claim) { + URI address = toUri(claim); + return address != null && isWebAddress(address) ? address : null; + } + + private static URI toUri(String claim) { + try { + return new URI(claim); + } catch (URISyntaxException e) { + LOG.debug("Ignoring a picture claim that is not a valid URI: {}", e.getReason()); + return null; + } + } + + private static boolean isWebAddress(URI address) { + return address.isAbsolute() + && address.getHost() != null + && WEB_SCHEMES.contains(address.getScheme().toLowerCase(Locale.ROOT)); + } +} diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java index e28428da13b2..00a906c386b0 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java @@ -10,6 +10,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; @@ -81,6 +82,10 @@ import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.EventType; import org.openmetadata.schema.type.Include; +import org.openmetadata.schema.type.Profile; +import org.openmetadata.schema.type.Webhook; +import org.openmetadata.schema.type.profile.SubscriptionConfig; +import org.openmetadata.schema.utils.JsonUtils; import org.openmetadata.service.Entity; import org.openmetadata.service.exception.AuthenticationException; import org.openmetadata.service.exception.EntityNotFoundException; @@ -90,6 +95,7 @@ import org.openmetadata.service.security.session.SessionService; import org.openmetadata.service.security.session.SessionStatus; import org.openmetadata.service.security.session.UserSession; +import org.openmetadata.service.util.EntityUtil; import org.openmetadata.service.util.RestUtil.PutResponse; import org.pac4j.core.exception.TechnicalException; import org.pac4j.oidc.client.OidcClient; @@ -103,6 +109,8 @@ class AuthenticationCodeFlowHandlerTest { private static final String TEST_SERVER_URL = "https://om.test"; private static final String MCP_CALLBACK = "/mcp/callback"; + private static final URI PROVIDER_PICTURE = + URI.create("https://lh3.googleusercontent.com/a/ACg8ocJ=s96-c"); private static final String DISCOVERY_DOCUMENT = """ { @@ -509,6 +517,84 @@ void getOrCreateOidcUser_selfSignup_allowsConfiguredDomainAndPersistsMappedEmail } } + @Test + void getOrCreateOidcUser_selfSignup_takesTheDisplayNameAndPictureTheProviderAsserts() + throws Exception { + AuthenticationCodeFlowHandler handler = createSelfSignupHandler(null); + + try (MockedStatic mockedEntity = mockStatic(Entity.class)) { + stubUserNotFoundAndEchoCreate(mockedEntity); + + User created = + invokeGetOrCreateOidcUser( + handler, legacyIdentity("jdoe", "Jane Doe", PROVIDER_PICTURE), new HashMap<>()); + + assertEquals("Jane Doe", created.getDisplayName()); + assertEquals(PROVIDER_PICTURE, created.getProfile().getImages().getImage()); + } + } + + @Test + void getOrCreateOidcUser_existingUser_fillsTheDisplayNameAndPictureTheProviderAsserts() + throws Exception { + AuthenticationCodeFlowHandler handler = createSelfSignupHandler(null); + User stored = storedUser("jdoe").withDisplayName("jdoe"); + + try (MockedStatic mockedEntity = mockStatic(Entity.class)) { + UserRepository userRepository = stubStoredUserAndEchoUpdate(mockedEntity, stored); + + invokeGetOrCreateOidcUser( + handler, legacyIdentity("jdoe", "Jane Doe", PROVIDER_PICTURE), new HashMap<>()); + + User persisted = capturePersistedUser(userRepository); + assertEquals("Jane Doe", persisted.getDisplayName()); + assertEquals(PROVIDER_PICTURE, persisted.getProfile().getImages().getImage()); + } + } + + @Test + void getOrCreateOidcUser_existingUser_keepsTheStoredProfileWhenTheLoginUpdatesTheAccount() + throws Exception { + AuthenticationCodeFlowHandler handler = createSelfSignupHandler(null); + makeAdminAtLogin(handler, "jdoe@example.com"); + SubscriptionConfig subscription = + new SubscriptionConfig() + .withSlack(new Webhook().withEndpoint(URI.create("https://hooks.slack.test/jdoe"))); + User stored = storedUser("jdoe").withProfile(new Profile().withSubscription(subscription)); + + try (MockedStatic mockedEntity = mockStatic(Entity.class)) { + UserRepository userRepository = stubStoredUserAndEchoUpdate(mockedEntity, stored); + + invokeGetOrCreateOidcUser(handler, legacyIdentity("jdoe", null, null), new HashMap<>()); + + User persisted = capturePersistedUser(userRepository); + assertTrue(persisted.getIsAdmin()); + assertNotNull(persisted.getProfile(), "Saving the login's changes erased the stored profile"); + assertEquals(subscription, persisted.getProfile().getSubscription()); + } + } + + @Test + void getOrCreateEmailFirstOidcUser_existingUser_takesThePictureTheProviderAsserts() + throws Exception { + AuthenticationCodeFlowHandler handler = createEmailFirstHandler(null); + User stored = storedUser("jdoe"); + + try (MockedStatic mockedEntity = mockStatic(Entity.class)) { + UserRepository userRepository = stubStoredUserAndEchoUpdate(mockedEntity, stored); + when(userRepository.getActiveUserByEmailForAuth(eq(stored.getEmail()), any())) + .thenAnswer(invocation -> withRequestedFieldsOnly(stored, invocation.getArgument(1))); + + invokeGetOrCreateEmailFirstOidcUser( + handler, + new OidcIdentityResolver.ResolvedOidcIdentity( + null, stored.getEmail(), null, PROVIDER_PICTURE, true)); + + User persisted = capturePersistedUser(userRepository); + assertEquals(PROVIDER_PICTURE, persisted.getProfile().getImages().getImage()); + } + } + @Test void getOrCreateOidcUser_selfSignup_rejectsDisallowedEmailDomain() throws Exception { AuthenticationCodeFlowHandler handler = createSelfSignupHandler(Set.of("allowed.com")); @@ -703,20 +789,11 @@ private AuthenticationCodeFlowHandler createSelfSignupHandler(Set allowe } private void stubUserNotFoundAndEchoCreate(MockedStatic mockedEntity) { - UserRepository userRepository = mock(UserRepository.class); - CollectionDAO collectionDAO = mock(CollectionDAO.class); - CollectionDAO.ChangeEventDAO changeEventDAO = mock(CollectionDAO.ChangeEventDAO.class); + UserRepository userRepository = stubUserRepository(mockedEntity); - mockedEntity - .when( - () -> - Entity.getEntityByName( - eq(Entity.USER), anyString(), anyString(), any(Include.class))) + when(userRepository.getByName( + any(), anyString(), any(EntityUtil.Fields.class), any(Include.class), anyBoolean())) .thenThrow(new EntityNotFoundException("user not found")); - mockedEntity.when(() -> Entity.getEntityRepository(Entity.USER)).thenReturn(userRepository); - mockedEntity.when(Entity::getCollectionDAO).thenReturn(collectionDAO); - - when(collectionDAO.changeEventDAO()).thenReturn(changeEventDAO); when(userRepository.findByNameOrNull(any(), any())).thenReturn(null); when(userRepository.createOrUpdate(eq(null), any(User.class), any())) .thenAnswer( @@ -727,15 +804,112 @@ private void stubUserNotFoundAndEchoCreate(MockedStatic mockedEntity) { EventType.ENTITY_CREATED)); } + /** + * Answers lookups the way the repository does: a stored field the caller did not ask for comes + * back empty, and saving that copy would erase it. + */ + private UserRepository stubStoredUserAndEchoUpdate( + MockedStatic mockedEntity, User stored) { + UserRepository userRepository = stubUserRepository(mockedEntity); + + when(userRepository.getByName( + any(), + eq(stored.getName()), + any(EntityUtil.Fields.class), + any(Include.class), + anyBoolean())) + .thenAnswer(invocation -> withRequestedFieldsOnly(stored, invocation.getArgument(2))); + when(userRepository.findByNameOrNull(any(), any())).thenReturn(stored); + when(userRepository.createOrUpdate(eq(null), any(User.class), any())) + .thenAnswer( + invocation -> + new PutResponse<>( + Response.Status.OK, + invocation.getArgument(1, User.class), + EventType.ENTITY_UPDATED)); + return userRepository; + } + + private UserRepository stubUserRepository(MockedStatic mockedEntity) { + UserRepository userRepository = mock(UserRepository.class); + CollectionDAO collectionDAO = mock(CollectionDAO.class); + CollectionDAO.ChangeEventDAO changeEventDAO = mock(CollectionDAO.ChangeEventDAO.class); + + mockedEntity.when(Entity::getUserRepository).thenReturn(userRepository); + mockedEntity.when(() -> Entity.getEntityRepository(Entity.USER)).thenReturn(userRepository); + mockedEntity.when(Entity::getCollectionDAO).thenReturn(collectionDAO); + when(collectionDAO.changeEventDAO()).thenReturn(changeEventDAO); + when(userRepository.getFields(anyString())) + .thenAnswer( + invocation -> + new EntityUtil.Fields(Set.of(invocation.getArgument(0, String.class).split(",")))); + when(userRepository.getAuthUpdateFields()) + .thenReturn(new EntityUtil.Fields(Set.of("profile", "roles", "teams"))); + return userRepository; + } + + private static void makeAdminAtLogin(AuthenticationCodeFlowHandler handler, String email) + throws Exception { + AuthorizerConfiguration authzConfig = mock(AuthorizerConfiguration.class); + when(authzConfig.getAdminEmails()).thenReturn(Set.of(email)); + setField(handler, "authorizerConfiguration", authzConfig); + } + + private static User withRequestedFieldsOnly(User stored, EntityUtil.Fields fields) { + User copy = JsonUtils.deepCopy(stored, User.class); + copy.setProfile(fields.contains("profile") ? copy.getProfile() : null); + return copy; + } + + private static User capturePersistedUser(UserRepository userRepository) { + ArgumentCaptor persisted = ArgumentCaptor.forClass(User.class); + verify(userRepository).createOrUpdate(eq(null), persisted.capture(), any()); + return persisted.getValue(); + } + + private static User storedUser(String name) { + return new User() + .withId(UUID.randomUUID()) + .withName(name) + .withFullyQualifiedName(name) + .withEmail(name + "@example.com"); + } + + private static OidcIdentityResolver.ResolvedOidcIdentity legacyIdentity( + String userName, String displayName, URI picture) { + return new OidcIdentityResolver.ResolvedOidcIdentity( + userName, userName + "@example.com", displayName, picture, false); + } + private User invokeGetOrCreateOidcUser( AuthenticationCodeFlowHandler handler, String userName, String email) throws Exception { + return invokeGetOrCreateOidcUser( + handler, + new OidcIdentityResolver.ResolvedOidcIdentity(userName, email, null, null, false), + new HashMap<>()); + } + + private User invokeGetOrCreateOidcUser( + AuthenticationCodeFlowHandler handler, + OidcIdentityResolver.ResolvedOidcIdentity identity, + Map claims) + throws Exception { + return invokeUserProvisioning(handler, "getOrCreateOidcUser", identity, claims); + } + + private User invokeUserProvisioning( + AuthenticationCodeFlowHandler handler, + String methodName, + OidcIdentityResolver.ResolvedOidcIdentity identity, + Map claims) + throws Exception { Method method = AuthenticationCodeFlowHandler.class.getDeclaredMethod( - "getOrCreateOidcUser", String.class, String.class, Map.class); + methodName, OidcIdentityResolver.ResolvedOidcIdentity.class, Map.class); method.setAccessible(true); User result; try { - result = (User) method.invoke(handler, userName, email, new HashMap()); + result = (User) method.invoke(handler, identity, claims); } catch (InvocationTargetException e) { Throwable cause = e.getCause(); if (cause instanceof RuntimeException runtimeException) { @@ -1969,19 +2143,16 @@ private AuthenticationCodeFlowHandler createEmailFirstHandler(Set regist private User invokeGetOrCreateEmailFirstOidcUser( AuthenticationCodeFlowHandler handler, String email, String displayName) throws Exception { - Method method = - AuthenticationCodeFlowHandler.class.getDeclaredMethod( - "getOrCreateEmailFirstOidcUser", String.class, String.class, Map.class); - method.setAccessible(true); - try { - return (User) method.invoke(handler, email, displayName, new HashMap()); - } catch (InvocationTargetException e) { - Throwable cause = e.getCause(); - if (cause instanceof RuntimeException runtimeException) { - throw runtimeException; - } - throw e; - } + return invokeGetOrCreateEmailFirstOidcUser( + handler, + new OidcIdentityResolver.ResolvedOidcIdentity(null, email, displayName, null, true)); + } + + private User invokeGetOrCreateEmailFirstOidcUser( + AuthenticationCodeFlowHandler handler, OidcIdentityResolver.ResolvedOidcIdentity identity) + throws Exception { + return invokeUserProvisioning( + handler, "getOrCreateEmailFirstOidcUser", identity, new HashMap<>()); } private Object invokePrivateMethod( diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/security/OidcIdentityResolverTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/security/OidcIdentityResolverTest.java index e89db5af3a6d..94f3bcc2e87b 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/security/OidcIdentityResolverTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/security/OidcIdentityResolverTest.java @@ -5,6 +5,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.net.URI; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -113,6 +114,33 @@ void testResolveUsesLegacyFlowWhenEmailClaimIsNotConfigured() { assertFalse(identity.emailFirstFlow()); } + @Test + void testResolveCarriesThePictureClaimInBothFlows() { + String picture = "https://lh3.googleusercontent.com/a/ACg8ocJ=s96-c"; + OidcIdentityResolver emailFirst = + new OidcIdentityResolver( + authConfig(true, List.of(), new ArrayList<>(), "email", "name"), + authzConfig(Set.of(), Set.of(), false), + Map.of(), + List.of(), + "openmetadata.org"); + OidcIdentityResolver legacy = + new OidcIdentityResolver( + authConfig(true, List.of("preferred_username"), new ArrayList<>(), null, "name"), + authzConfig(null, Set.of(), false), + Map.of(), + List.of("preferred_username"), + "openmetadata.org"); + + OidcIdentityResolver.ResolvedOidcIdentity emailFirstIdentity = + emailFirst.resolve(Map.of("email", "jdoe@company.com", "picture", picture)); + OidcIdentityResolver.ResolvedOidcIdentity legacyIdentity = + legacy.resolve(Map.of("preferred_username", "jdoe", "picture", picture)); + + assertEquals(URI.create(picture), emailFirstIdentity.picture()); + assertEquals(URI.create(picture), legacyIdentity.picture()); + } + @Test void testResolveValidatesAllowedEmailDomains() { OidcIdentityResolver resolver = diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/security/UserProfileClaimsTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/security/UserProfileClaimsTest.java new file mode 100644 index 000000000000..618905212aa7 --- /dev/null +++ b/openmetadata-service/src/test/java/org/openmetadata/service/security/UserProfileClaimsTest.java @@ -0,0 +1,159 @@ +package org.openmetadata.service.security; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.net.URI; +import java.util.Map; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; +import org.openmetadata.schema.entity.teams.User; +import org.openmetadata.schema.type.ImageList; +import org.openmetadata.schema.type.Profile; +import org.openmetadata.schema.type.Webhook; +import org.openmetadata.schema.type.profile.SubscriptionConfig; + +@Execution(ExecutionMode.CONCURRENT) +class UserProfileClaimsTest { + + private static final URI PICTURE = + URI.create("https://lh3.googleusercontent.com/a/ACg8ocJ=s96-c"); + + @Test + void pictureFrom_readsTheStandardPictureClaim() { + assertEquals(PICTURE, UserProfileClaims.pictureFrom(Map.of("picture", PICTURE.toString()))); + } + + @Test + void pictureFrom_isNullWhenTheTokenCarriesNoPicture() { + assertNull(UserProfileClaims.pictureFrom(Map.of("email", "jdoe@example.com"))); + } + + @ParameterizedTest + @ValueSource( + strings = { + "", + "javascript:alert(1)", + "data:image/png;base64,iVBORw0KGgo=", + "file:///etc/passwd", + "/avatars/jdoe.png", + "not a picture" + }) + void pictureFrom_ignoresAnythingButAnAbsoluteWebAddress(String claim) { + assertNull(UserProfileClaims.pictureFrom(Map.of("picture", claim))); + } + + @Test + void pictureFrom_ignoresAPictureClaimThatIsNotText() { + assertNull(UserProfileClaims.pictureFrom(Map.of("picture", 42))); + } + + @Test + void syncPicture_setsThePictureOnAnAccountWithoutOne() { + User user = new User().withName("jdoe"); + + assertTrue(UserProfileClaims.syncPicture(user, PICTURE)); + assertEquals(PICTURE, user.getProfile().getImages().getImage()); + } + + @Test + void syncPicture_dropsEverySizeOfThePreviousPicture() { + URI previous = URI.create("https://lh3.googleusercontent.com/a/old=s96-c"); + User user = + new User() + .withName("jdoe") + .withProfile( + new Profile() + .withImages( + new ImageList() + .withImage(previous) + .withImage512( + URI.create("https://lh3.googleusercontent.com/a/old=s512-c")))); + + assertTrue(UserProfileClaims.syncPicture(user, PICTURE)); + assertEquals(new ImageList().withImage(PICTURE), user.getProfile().getImages()); + } + + @Test + void syncPicture_keepsTheRestOfTheProfile() { + SubscriptionConfig subscription = + new SubscriptionConfig() + .withSlack(new Webhook().withEndpoint(URI.create("https://hooks.slack.test/jdoe"))); + User user = + new User().withName("jdoe").withProfile(new Profile().withSubscription(subscription)); + + UserProfileClaims.syncPicture(user, PICTURE); + + assertEquals(subscription, user.getProfile().getSubscription()); + } + + @Test + void syncPicture_leavesTheCurrentPictureAndItsSizesAlone() { + ImageList current = + new ImageList() + .withImage(PICTURE) + .withImage512(URI.create("https://lh3.googleusercontent.com/a/ACg8ocJ=s512-c")); + User user = new User().withName("jdoe").withProfile(new Profile().withImages(current)); + + assertFalse(UserProfileClaims.syncPicture(user, PICTURE)); + assertEquals(current, user.getProfile().getImages()); + } + + @Test + void syncPicture_keepsTheStoredPictureWhenTheTokenHasNone() { + User user = + new User() + .withName("jdoe") + .withProfile(new Profile().withImages(new ImageList().withImage(PICTURE))); + + assertFalse(UserProfileClaims.syncPicture(user, null)); + assertEquals(PICTURE, user.getProfile().getImages().getImage()); + } + + @Test + void fillDisplayName_setsTheProviderNameOnAnAccountWithoutOne() { + User user = new User().withName("jdoe"); + + assertTrue(UserProfileClaims.fillDisplayName(user, "Jane Doe")); + assertEquals("Jane Doe", user.getDisplayName()); + } + + @Test + void fillDisplayName_replacesADisplayNameThatOnlyRepeatsTheLoginName() { + User user = new User().withName("jdoe").withDisplayName("jdoe"); + + assertTrue(UserProfileClaims.fillDisplayName(user, "Jane Doe")); + assertEquals("Jane Doe", user.getDisplayName()); + } + + @Test + void fillDisplayName_keepsANameChosenInOpenMetadata() { + User user = new User().withName("jdoe").withDisplayName("JD"); + + assertFalse(UserProfileClaims.fillDisplayName(user, "Jane Doe")); + assertEquals("JD", user.getDisplayName()); + } + + @Test + void fillDisplayName_reportsNoChangeWhenTheProviderNameIsTheLoginName() { + User user = new User().withName("jdoe").withDisplayName("jdoe"); + + assertFalse(UserProfileClaims.fillDisplayName(user, "jdoe")); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" "}) + void fillDisplayName_ignoresAMissingProviderName(String providerName) { + User user = new User().withName("jdoe"); + + assertFalse(UserProfileClaims.fillDisplayName(user, providerName)); + assertNull(user.getDisplayName()); + } +} diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.test.tsx index 0e1320f2e688..428fb54436c4 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.test.tsx @@ -21,7 +21,10 @@ import { DEFAULT_APP_MODE, } from '../../../constants/appMode.constants'; import { REDIRECT_PATHNAME } from '../../../constants/router.constants'; -import { AuthProvider as AuthProviderProps } from '../../../generated/configuration/authenticationConfiguration'; +import { + AuthProvider as AuthProviderProps, + ClientType, +} from '../../../generated/configuration/authenticationConfiguration'; import { Document } from '../../../generated/entity/docStore/document'; import { useAppModeStore } from '../../../hooks/useAppMode'; import axiosClient from '../../../rest/axiosClient'; @@ -42,6 +45,7 @@ import { RefreshFailedError } from '../../../utils/Auth/AuthCoordinator/RefreshQ import type { RefreshFailedPayload } from '../../../utils/Auth/AuthCoordinator/types'; import { isRefreshableAuthError } from '../../../utils/AuthProvider.util'; import { showErrorToast, showInfoToast } from '../../../utils/ToastUtils'; +import { checkIfUpdateRequired } from '../../../utils/UserDataUtils'; import AuthProvider, { useAuthProvider } from './AuthProvider'; import { OidcUser } from './AuthProvider.interface'; @@ -1478,6 +1482,49 @@ describe('Sign-in routing', () => { }) ); }); + + it('leaves the profile to the server after a confidential-client login', async () => { + const defaults = defaultStoreImplementation(); + const confidentialStore = { + ...defaults, + jwtPrincipalClaims: [], + jwtPrincipalClaimsMapping: [], + authConfig: { + ...defaults.authConfig, + provider: AuthProviderProps.Google, + clientType: ClientType.Confidential, + }, + }; + useApplicationStoreMock.mockImplementation(() => confidentialStore); + renderProvider(); + + await act(async () => { + fireEvent.click(screen.getByTestId('login')); + }); + + await waitFor(() => + expect(confidentialStore.setCurrentUser).toHaveBeenCalledWith( + expect.objectContaining({ id: 'user-1', name: 'aaron' }) + ) + ); + + expect(checkIfUpdateRequired).not.toHaveBeenCalled(); + }); + + it('reconciles the profile in the browser after a public-client login', async () => { + renderProvider(); + + await act(async () => { + fireEvent.click(screen.getByTestId('login')); + }); + + await waitFor(() => + expect(checkIfUpdateRequired).toHaveBeenCalledWith( + expect.objectContaining({ id: 'user-1' }), + expect.anything() + ) + ); + }); }); // Hoisted to module scope (rather than declared inside the describe below) diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx index 3b36af62e18f..1cb734bc6835 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx @@ -104,6 +104,7 @@ import { getUrlPathnameExpiry, getUserManagerConfig, isRefreshableAuthError, + isServerDrivenLogin, prepareUserProfileFromClaims, validateAuthFieldsDetailed, } from '../../../utils/AuthProvider.util'; @@ -816,17 +817,25 @@ export const AuthProvider = ({ ? userAPIQueryFields + ',' + isEmailVerifyField : userAPIQueryFields; try { - const newUser = prepareUserProfileFromClaims({ - user, - jwtPrincipalClaims, - principalDomain: authorizerConfig?.principalDomain ?? '', - jwtPrincipalClaimsMapping, - clientType, - }); - const res = await getLoggedInUser({ fields }); if (res) { - const userDetails = await checkIfUpdateRequired(res, newUser); + // A server-driven login has already synced the profile, and the name it handed the + // browser is the username, which must not become the display name. + const userDetails = isServerDrivenLogin( + clientType, + authConfig?.provider + ) + ? res + : await checkIfUpdateRequired( + res, + prepareUserProfileFromClaims({ + user, + jwtPrincipalClaims, + principalDomain: authorizerConfig?.principalDomain ?? '', + jwtPrincipalClaimsMapping, + clientType, + }) + ); setCurrentUser(userDetails); await hydrateAndResolveAppMode(userDetails); @@ -858,6 +867,7 @@ export const AuthProvider = ({ }, [ authConfig?.enableSelfSignup, + authConfig?.provider, clientType, authorizerConfig?.principalDomain, jwtPrincipalClaims, @@ -1175,10 +1185,7 @@ export const AuthProvider = ({ ); // Handling for SAML moved to GenericAuthenticator - if ( - clientType === ClientType.Confidential || - authConfig?.provider === AuthProviderEnum.Saml - ) { + if (isServerDrivenLogin(clientType, authConfig?.provider)) { return ( {childElement} diff --git a/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.test.ts b/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.test.ts index 9f08d20ebec1..dbe463cc21cd 100644 --- a/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.test.ts +++ b/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.test.ts @@ -21,6 +21,7 @@ import { getAuthConfig, getUserManagerConfig, isRefreshableAuthError, + isServerDrivenLogin, } from './AuthProvider.util'; const baseAuthConfig = ( @@ -189,3 +190,27 @@ describe('isRefreshableAuthError — 401 allow-list semantics (auth-coordinator- expect(isRefreshableAuthError(401, '/tables/name/foo', {})).toBe(true); }); }); + +describe('isServerDrivenLogin', () => { + it('is true for a confidential client, whatever the provider', () => { + expect( + isServerDrivenLogin(ClientType.Confidential, AuthProvider.Google) + ).toBe(true); + }); + + it('is true for SAML, which the server always completes', () => { + expect(isServerDrivenLogin(ClientType.Public, AuthProvider.Saml)).toBe( + true + ); + }); + + it('is false for a public OIDC client, where the browser holds the provider claims', () => { + expect(isServerDrivenLogin(ClientType.Public, AuthProvider.Google)).toBe( + false + ); + }); + + it('treats a missing client type as public', () => { + expect(isServerDrivenLogin(undefined, AuthProvider.Okta)).toBe(false); + }); +}); diff --git a/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.ts b/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.ts index 6a270d1af9f2..2c08c863cab6 100644 --- a/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.ts +++ b/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.ts @@ -415,6 +415,17 @@ export const isLoginConfigurationApplicable = ( provider === AuthProvider.LDAP || provider === AuthProvider.Openmetadata; +/** + * Whether the server, rather than the browser, completes sign-in. The server then applies the + * identity provider's display name and picture to the user itself, and the name it hands the + * browser is the OpenMetadata username, not a display name. + */ +export const isServerDrivenLogin = ( + clientType?: ClientType, + provider?: AuthProvider +): boolean => + clientType === ClientType.Confidential || provider === AuthProvider.Saml; + export const getUrlPathnameExpiry = () => { return new Date(Date.now() + 60 * 60 * 1000); }; From 80b57a70002fdd2e30adae8eb0a5b4c07b1aaadf Mon Sep 17 00:00:00 2001 From: mohitdeuex Date: Sat, 17 Oct 2026 09:20:56 +0530 Subject: [PATCH 2/4] docs(auth): record that the server applies the SSO profile on server-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. --- ...the-sso-profile-on-server-driven-logins.md | 44 +++++++++++++++++++ .../service/security/UserProfileClaims.java | 2 + .../ui/src/utils/AuthProvider.util.ts | 1 + 3 files changed, 47 insertions(+) create mode 100644 docs/decisions/2026-10-17-the-server-applies-the-sso-profile-on-server-driven-logins.md diff --git a/docs/decisions/2026-10-17-the-server-applies-the-sso-profile-on-server-driven-logins.md b/docs/decisions/2026-10-17-the-server-applies-the-sso-profile-on-server-driven-logins.md new file mode 100644 index 000000000000..1d9c2d1ee13d --- /dev/null +++ b/docs/decisions/2026-10-17-the-server-applies-the-sso-profile-on-server-driven-logins.md @@ -0,0 +1,44 @@ +# The server applies the SSO profile on server-driven logins + +- **Status:** Accepted +- **Revisions:** v1 2026-10-17 (initial) +- **Deciders:** Mohit Yadav +- **Guard:** `UserProfileClaimsTest`, `AuthenticationCodeFlowHandlerTest`, `EmailFirstIdentityIT.testSignInStoresAndRefreshesTheProviderPicture`, `AuthProvider.test.tsx` ("leaves the profile to the server after a confidential-client login") +- **Related:** #35674, #25623, #25391 + +## Context + +A user's display name and picture come from the identity provider's ID token claims (`name`, or +`given_name` + `family_name`, and `picture`). Under a public OIDC client the browser holds those +claims and writes them to the user with a PATCH. Under a confidential client, and under SAML, the +server completes sign-in. The browser is only handed the OpenMetadata username in a redirect +parameter. + +So nobody applied the provider's profile on those logins. The legacy principal-claims path +resolved the display name and dropped it (from 1.12.4), and no server path read `picture`. The +browser then saved the username it was handed as the display name. + +## Decision + +- **Who writes it.** When the server completes sign-in (a confidential OIDC client, or SAML), the + server applies the provider's display name and picture. The UI does not write the profile after + such a login (`isServerDrivenLogin`). Under a public OIDC client the browser keeps doing it. +- **Picture.** The server reads the standard OIDC `picture` claim and stores it only if it is an + absolute `http`/`https` URI, because it becomes an image source in the browser. A new picture + replaces the whole stored image list, so sizes derived from an older picture stop showing. A + token without a picture leaves the stored one alone. +- **Display name on the legacy principal-claims path.** The provider's name fills the display name + only when the account has none, or has one equal to its login name, which is what the browser + wrote while the provider's name was dropped. Any other display name was chosen in OpenMetadata + and is kept. The email-first path keeps its existing rule: the provider's name replaces the + stored one whenever the provider sends one. + +## Consequences + +- Accounts that the browser stamped with their login name are repaired on their next sign-in, + without a migration. +- Profile pictures depend on the provider sending `picture` for the requested scopes. Azure AD + sends none, so its users still have no picture. +- Moving the display-name rule of the two paths into line, either way, means revisiting this + record: the legacy path would overwrite chosen names, or the email-first path would stop + following provider renames. diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/UserProfileClaims.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/UserProfileClaims.java index d8f78854b612..1726f8acbc5f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/UserProfileClaims.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/UserProfileClaims.java @@ -17,6 +17,8 @@ * Keeps how an account is presented, its display name and picture, in step with what the identity * provider asserts at sign-in. Only values the provider actually sent are applied: a claim the * token leaves out never clears what the account already has. + * + *

ADR:2026-10-17-the-server-applies-the-sso-profile-on-server-driven-logins */ @Slf4j public final class UserProfileClaims { diff --git a/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.ts b/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.ts index 2c08c863cab6..30315639bc70 100644 --- a/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.ts +++ b/openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.ts @@ -419,6 +419,7 @@ export const isLoginConfigurationApplicable = ( * Whether the server, rather than the browser, completes sign-in. The server then applies the * identity provider's display name and picture to the user itself, and the name it hands the * browser is the OpenMetadata username, not a display name. + * ADR:2026-10-17-the-server-applies-the-sso-profile-on-server-driven-logins */ export const isServerDrivenLogin = ( clientType?: ClientType, From c47998df45e6384ffabf648b94f7829f0829d26b Mon Sep 17 00:00:00 2001 From: mohitdeuex Date: Sat, 17 Oct 2026 09:24:53 +0530 Subject: [PATCH 3/4] test(auth): cover the picture on email-first OIDC sign-up --- .../AuthenticationCodeFlowHandlerTest.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java index 00a906c386b0..e343b50587e9 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java @@ -595,6 +595,28 @@ void getOrCreateEmailFirstOidcUser_existingUser_takesThePictureTheProviderAssert } } + @Test + void getOrCreateEmailFirstOidcUser_selfSignup_takesThePictureTheProviderAsserts() + throws Exception { + AuthenticationCodeFlowHandler handler = createEmailFirstHandler(null); + + try (MockedStatic mockedEntity = mockStatic(Entity.class)) { + stubUserNotFoundAndEchoCreate(mockedEntity); + UserRepository userRepository = Entity.getUserRepository(); + when(userRepository.getActiveUserByEmailForAuth(any(), any())) + .thenThrow(new EntityNotFoundException("user not found")); + + User created = + invokeGetOrCreateEmailFirstOidcUser( + handler, + new OidcIdentityResolver.ResolvedOidcIdentity( + null, "jdoe@example.com", "Jane Doe", PROVIDER_PICTURE, true)); + + assertEquals("Jane Doe", created.getDisplayName()); + assertEquals(PROVIDER_PICTURE, created.getProfile().getImages().getImage()); + } + } + @Test void getOrCreateOidcUser_selfSignup_rejectsDisallowedEmailDomain() throws Exception { AuthenticationCodeFlowHandler handler = createSelfSignupHandler(Set.of("allowed.com")); From a7947748250e955c290c6f85966684db5e3961b5 Mon Sep 17 00:00:00 2001 From: mohitdeuex Date: Sat, 17 Oct 2026 09:30:42 +0530 Subject: [PATCH 4/4] refactor(auth): look up principal-claims logins through UserRepository 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. --- .../it/tests/EmailFirstIdentityIT.java | 21 +++++++++++++++++++ .../service/jdbi3/UserRepository.java | 8 +++++++ .../AuthenticationCodeFlowHandler.java | 13 +----------- .../AuthenticationCodeFlowHandlerTest.java | 1 + 4 files changed, 31 insertions(+), 12 deletions(-) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/EmailFirstIdentityIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/EmailFirstIdentityIT.java index ccc3c1ea52a9..ef22397a5adc 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/EmailFirstIdentityIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/EmailFirstIdentityIT.java @@ -228,6 +228,27 @@ void testLoginDoesNotEraseStoredUserFields(TestNamespace ns) { assertEquals(user.getId(), after.getId()); } + /** + * The principal-claims OIDC login finds the account by name and saves it back when the login + * changes it, here by filling the display name. The copy it loads must carry the stored profile. + */ + @Test + void testLoginByNameDoesNotEraseStoredUserFields(TestNamespace ns) { + String localPart = ("byname" + ns.uniqueShortId()).toLowerCase(Locale.ROOT); + URI picture = URI.create("https://idp.test.om.org/" + localPart + ".png"); + User user = provisionWithPicture(localPart + "@x.test.om.org", picture); + UserRepository userRepository = Entity.getUserRepository(); + + User forLogin = userRepository.getUserByNameForAuthUpdate(user.getName()); + assertEquals(picture, forLogin.getProfile().getImages().getImage()); + + forLogin.setDisplayName("Name From The Provider"); + UserUtil.addOrUpdateUser(forLogin); + + assertEquals( + picture, storedPicture(user.getEmail()), "Saving the login's copy erased the profile"); + } + @Test void testSignInStoresAndRefreshesTheProviderPicture(TestNamespace ns) { String localPart = ("picture" + ns.uniqueShortId()).toLowerCase(Locale.ROOT); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/UserRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/UserRepository.java index 43f225fce45e..71e745f2870e 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/UserRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/UserRepository.java @@ -841,6 +841,14 @@ public Fields getAuthUpdateFields() { return new Fields(fields); } + /** + * Name lookup for an authentication flow that updates the account it finds. Reads it fresh, with + * the {@link #getAuthUpdateFields} a PUT of the result writes back. + */ + public User getUserByNameForAuthUpdate(String name) { + return getByName(null, name, getAuthUpdateFields(), NON_DELETED, false); + } + /** * Streams the accounts whose email sits in {@code domain}, ordered by name, in pages. The * {@code change-email} ops flow uses this to find accounts still holding a diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/AuthenticationCodeFlowHandler.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/AuthenticationCodeFlowHandler.java index ed7e76454e5f..02aede2de6c4 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/AuthenticationCodeFlowHandler.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/AuthenticationCodeFlowHandler.java @@ -97,7 +97,6 @@ import org.openmetadata.service.auth.JwtResponse; import org.openmetadata.service.exception.AuthenticationException; import org.openmetadata.service.exception.EntityNotFoundException; -import org.openmetadata.service.jdbi3.UserRepository; import org.openmetadata.service.security.auth.SecurityConfigurationManager; import org.openmetadata.service.security.jwt.JWTTokenGenerator; import org.openmetadata.service.security.policyevaluator.SubjectCache; @@ -1195,16 +1194,6 @@ private static boolean applyDisplayNameAndPicture( return displayNameFilled || pictureChanged; } - /** - * Loads the account fresh and with every field a PUT writes back: {@code createOrUpdate} compares - * the whole entity, so a field the lookup left out would be stored as deleted. - */ - private static User findUserForUpdate(String userName) { - UserRepository userRepository = Entity.getUserRepository(); - return userRepository.getByName( - null, userName, userRepository.getAuthUpdateFields(), Include.NON_DELETED, false); - } - private boolean isUserAdmin(String email, String username) { return UserUtil.isConfiguredAdmin(authorizerConfiguration, email, username); } @@ -1217,7 +1206,7 @@ private User getOrCreateOidcUser( List teamsFromClaim = findTeamsFromClaims(teamClaimMapping, claims); try { - User user = findUserForUpdate(userName); + User user = Entity.getUserRepository().getUserByNameForAuthUpdate(userName); boolean shouldBeAdmin = isUserAdmin(email, userName); boolean needsUpdate = false; diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java index e343b50587e9..2d2aa8d49655 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java @@ -867,6 +867,7 @@ private UserRepository stubUserRepository(MockedStatic mockedEntity) { new EntityUtil.Fields(Set.of(invocation.getArgument(0, String.class).split(",")))); when(userRepository.getAuthUpdateFields()) .thenReturn(new EntityUtil.Fields(Set.of("profile", "roles", "teams"))); + when(userRepository.getUserByNameForAuthUpdate(anyString())).thenCallRealMethod(); return userRepository; }