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-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..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 @@ -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,45 @@ 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); + 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 +297,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/jdbi3/UserRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/UserRepository.java index 9b811f970aed..231172dcc2fb 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 a285b3374a36..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 @@ -646,8 +646,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 +1166,47 @@ 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; + } + 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 = Entity.getUserRepository().getUserByNameForAuthUpdate(userName); boolean shouldBeAdmin = isUserAdmin(email, userName); boolean needsUpdate = false; @@ -1212,7 +1226,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..1726f8acbc5f --- /dev/null +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/UserProfileClaims.java @@ -0,0 +1,107 @@ +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. + * + *

ADR:2026-10-17-the-server-applies-the-sso-profile-on-server-driven-logins + */ +@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..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 @@ -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,106 @@ 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 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")); @@ -703,20 +811,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 +826,113 @@ 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"))); + when(userRepository.getUserByNameForAuthUpdate(anyString())).thenCallRealMethod(); + 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 +2166,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 9133d92ca1b3..2ec4a5bdb06a 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, validateAuthFieldsDetailed, } from './AuthProvider.util'; @@ -215,3 +216,27 @@ describe('validateAuthFieldsDetailed — providerName is required only where the expect(result.errors.map((error) => error.field)).toEqual(['providerName']); }); }); + +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 64a32f3c406c..5a2b981a8bad 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,18 @@ 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. + * ADR:2026-10-17-the-server-applies-the-sso-profile-on-server-driven-logins + */ +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); };