From 2a3ba6170304153b46584bb165975ae0772e4b6f Mon Sep 17 00:00:00 2001 From: Chirag Madlani <12962843+chirag-madlani@users.noreply.github.com> Date: Fri, 30 Aug 2024 11:57:37 +0530 Subject: [PATCH] fix(ui): pick email and name based on claim values at time of login (#17626) * fix(ui): pick email and name based on claim values at time of login * Update Error Message * Update code for better message * Typo * fix playwright failures * fix playwright tests * fix tests * fix login spec failing --------- Co-authored-by: mohitdeuex (cherry picked from commit fa3529f0850ed801e11b752b963ab0cafb637e87) --- .../exception/BadRequestException.java | 8 +++ .../service/jdbi3/CollectionDAO.java | 7 ++ .../service/jdbi3/UserRepository.java | 65 +++++++++++++++--- .../service/resources/teams/UserResource.java | 7 +- .../ui/playwright/e2e/Pages/Glossary.spec.ts | 4 +- .../Auth/AuthProviders/AuthProvider.tsx | 20 +++++- .../Auth/AuthProviders/BasicAuthProvider.tsx | 3 +- .../ui/src/utils/AuthProvider.util.ts | 44 +++++++++++++ .../ui/src/utils/AuthProviderUtil.test.ts | 66 +++++++++++++++++++ 9 files changed, 206 insertions(+), 18 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/exception/BadRequestException.java b/openmetadata-service/src/main/java/org/openmetadata/service/exception/BadRequestException.java index 6061e4a93e6..48d5da514cf 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/exception/BadRequestException.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/exception/BadRequestException.java @@ -24,7 +24,15 @@ public final class BadRequestException extends WebServiceException { super(Response.Status.BAD_REQUEST, ERROR_TYPE, DEFAULT_MESSAGE); } + private BadRequestException(String message) { + super(Response.Status.BAD_REQUEST, ERROR_TYPE, message); + } + public static BadRequestException of() { return new BadRequestException(); } + + public static BadRequestException of(String message) { + return new BadRequestException(message); + } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java index 5fb38e7ed65..590deecc931 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java @@ -3747,6 +3747,13 @@ public interface CollectionDAO { @SqlQuery("SELECT COUNT(*) FROM user_entity WHERE LOWER(email) = LOWER(:email)") int checkEmailExists(@Bind("email") String email); + @SqlQuery("SELECT COUNT(*) FROM user_entity WHERE LOWER(name) = LOWER(:name)") + int checkUserNameExists(@Bind("name") String name); + + @SqlQuery( + "SELECT json FROM user_entity WHERE LOWER(name) = LOWER(:name) AND LOWER(email) = LOWER(:email)") + String findUserByNameAndEmail(@Bind("name") String name, @Bind("email") String email); + @SqlQuery("SELECT json FROM user_entity WHERE LOWER(email) = LOWER(:email)") String findUserByEmail(@Bind("email") String email); 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 1ca788b1388..3bdea9b92d6 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 @@ -60,6 +60,7 @@ import org.openmetadata.schema.type.csv.CsvImportResult; import org.openmetadata.schema.utils.EntityInterfaceUtil; import org.openmetadata.service.Entity; import org.openmetadata.service.OpenMetadataApplicationConfig; +import org.openmetadata.service.exception.BadRequestException; import org.openmetadata.service.exception.CatalogExceptionMessage; import org.openmetadata.service.exception.EntityNotFoundException; import org.openmetadata.service.jdbi3.CollectionDAO.EntityRelationshipRecord; @@ -146,7 +147,21 @@ public class UserRepository extends EntityRepository { } public User getByEmail(UriInfo uriInfo, String email, Fields fields) { - String userString = ((CollectionDAO.UserDAO) dao).findUserByEmail(email); + String userString = daoCollection.userDAO().findUserByEmail(email); + if (userString == null) { + throw EntityNotFoundException.byMessage(CatalogExceptionMessage.entityNotFound(USER, email)); + } + User user = JsonUtils.readValue(userString, User.class); + setFieldsInternal(user, fields); + setInheritedFields(user, fields); + // Clone the entity + User entityClone = JsonUtils.deepCopy(user, User.class); + clearFieldsInternal(entityClone, fields); + return withHref(uriInfo, entityClone); + } + + public User getUserByNameAndEmail(UriInfo uriInfo, String name, String email, Fields fields) { + String userString = daoCollection.userDAO().findUserByNameAndEmail(name, email); if (userString == null) { throw EntityNotFoundException.byMessage(CatalogExceptionMessage.entityNotFound(USER, email)); } @@ -314,6 +329,10 @@ public class UserRepository extends EntityRepository { return daoCollection.userDAO().checkEmailExists(emailId) > 0; } + public boolean checkUserNameExists(String username) { + return daoCollection.userDAO().checkUserNameExists(username) > 0; + } + public void initializeUsers(OpenMetadataApplicationConfig config) { AuthProvider authProvider = config.getAuthenticationConfiguration().getProvider(); // Create Admins @@ -360,19 +379,45 @@ public class UserRepository extends EntityRepository { public List getGroupTeams( UriInfo uriInfo, SecurityContext context, String email) { // Cleanup - User user = getByEmail(uriInfo, email, Fields.EMPTY_FIELDS); - validateLoggedInUserNameAndEmailMatches(context.getUserPrincipal().getName(), email, user); + User user = + getLoggedInUserByNameAndEmail( + uriInfo, context.getUserPrincipal().getName(), email, Fields.EMPTY_FIELDS); List teams = getTeams(user); return getGroupTeams(teams); } - public void validateLoggedInUserNameAndEmailMatches( - String username, String email, User storedUser) { - String lowerCasedName = username.toLowerCase(); - String lowerCasedEmail = email.toLowerCase(); - if (!(lowerCasedName.equals(storedUser.getName().toLowerCase()) - && lowerCasedEmail.equals(storedUser.getEmail().toLowerCase()))) { - throw EntityNotFoundException.byMessage(CatalogExceptionMessage.entityNotFound(USER, email)); + public User getLoggedInUserByNameAndEmail( + UriInfo uriInfo, String username, String email, Fields fields) { + try { + return getUserByNameAndEmail(uriInfo, username, email, fields); + } catch (EntityNotFoundException e) { + boolean existByName = checkUserNameExists(username); + boolean existByEmail = checkEmailAlreadyExists(email); + if (existByName && !existByEmail) { + User userByName = getByName(uriInfo, username, Fields.EMPTY_FIELDS); + throw BadRequestException.of( + String.format( + "User with given name exists but is not associated with the provided email. " + + "Matching User Found By Name [username:email] : [%s:%s], Provided User: [%s:%s]", + userByName.getName().toLowerCase(), + userByName.getEmail().toLowerCase(), + username, + email)); + } else if (!existByName && existByEmail) { + User userByEmail = getByEmail(uriInfo, email, Fields.EMPTY_FIELDS); + throw BadRequestException.of( + String.format( + "User with given email exists but is not associated with provider username. " + + "Matching User Found By Email [username:email] : [%s:%s], Provided User: [%s:%s]", + userByEmail.getName().toLowerCase(), + userByEmail.getEmail().toLowerCase(), + username, + email)); + } else { + throw EntityNotFoundException.byMessage( + String.format( + "User with provider name : %s and email : %s not found", username, email)); + } } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java index 4ddf3dba19c..447c7827c9a 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java @@ -450,10 +450,9 @@ public class UserResource extends EntityResource { (CatalogSecurityContext) containerRequestContext.getSecurityContext(); Fields fields = getFields(fieldsParam); String currentEmail = ((CatalogPrincipal) catalogSecurityContext.getUserPrincipal()).getEmail(); - User user = repository.getByEmail(uriInfo, currentEmail, fields); - - repository.validateLoggedInUserNameAndEmailMatches( - securityContext.getUserPrincipal().getName(), currentEmail, user); + User user = + repository.getLoggedInUserByNameAndEmail( + uriInfo, catalogSecurityContext.getUserPrincipal().getName(), currentEmail, fields); // Sync the Roles from token to User if (Boolean.TRUE.equals(authorizerConfiguration.getUseRolesFromProvider()) diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Glossary.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Glossary.spec.ts index 7d90a245d57..57fa6dfc990 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Glossary.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Glossary.spec.ts @@ -66,7 +66,9 @@ test.describe('Glossary tests', () => { const glossary1 = new Glossary(); glossary1.data.owners = [{ name: 'admin', type: 'user' }]; glossary1.data.mutuallyExclusive = true; - glossary1.data.reviewers = [{ name: user1.getUserName(), type: 'user' }]; + glossary1.data.reviewers = [ + { name: `${user1.data.firstName}${user1.data.lastName}`, type: 'user' }, + ]; glossary1.data.terms = [new GlossaryTerm(glossary1)]; await test.step('Create Glossary', async () => { 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 ecb449e2247..c29fd2d9667 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 @@ -67,6 +67,7 @@ import { getUrlPathnameExpiry, getUserManagerConfig, isProtectedRoute, + prepareUserProfileFromClaims, } from '../../../utils/AuthProvider.util'; import { escapeESReservedCharacters } from '../../../utils/StringsUtils'; import { showErrorToast, showInfoToast } from '../../../utils/ToastUtils'; @@ -122,6 +123,9 @@ export const AuthProvider = ({ setAuthConfig, setAuthorizerConfig, setIsSigningUp, + authorizerConfig, + jwtPrincipalClaims, + jwtPrincipalClaimsMapping, setJwtPrincipalClaims, setJwtPrincipalClaimsMapping, removeRefreshToken, @@ -376,10 +380,18 @@ 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 updatedUserData = getUserDataFromOidc(res, user); - if (!matchUserDetails(res, updatedUserData, ['email'])) { + const updatedUserData = getUserDataFromOidc(res, newUser); + if (!matchUserDetails(res, updatedUserData, ['profile', 'email'])) { getUpdatedUser(updatedUserData, res); } else { setCurrentUser(res); @@ -414,6 +426,10 @@ export const AuthProvider = ({ }, [ authConfig?.enableSelfSignup, + clientType, + authorizerConfig?.principalDomain, + jwtPrincipalClaims, + jwtPrincipalClaimsMapping, setIsSigningUp, setIsAuthenticated, setApplicationLoading, diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/BasicAuthProvider.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/BasicAuthProvider.tsx index 2f3dd982ad8..649b37fc269 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/BasicAuthProvider.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/BasicAuthProvider.tsx @@ -39,6 +39,7 @@ import { } from '../../../utils/ToastUtils'; import { resetWebAnalyticSession } from '../../../utils/WebAnalyticsUtils'; +import { toLower } from 'lodash'; import { useApplicationStore } from '../../../hooks/useApplicationStore'; import { OidcUser } from './AuthProvider.interface'; @@ -115,7 +116,7 @@ const BasicAuthProvider = ({ onLoginSuccess({ id_token: response.accessToken, profile: { - email: response.email, + email: toLower(email), name: '', picture: '', sub: '', 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 3947f97e7d9..d6564f64f97 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 @@ -22,6 +22,7 @@ import { first, get, isEmpty, isNil } from 'lodash'; import { WebStorageStateStore } from 'oidc-client'; import { AuthenticationConfigurationWithScope, + OidcUser, UserProfile, } from '../components/Auth/AuthProviders/AuthProvider.interface'; import { REDIRECT_PATHNAME, ROUTES } from '../constants/constants'; @@ -377,3 +378,46 @@ export const setUrlPathnameExpiryAfterRoute = (pathname: string) => { path: '/', }); }; + +/** + * We support Principle claim as: email,preferred_username,sub in any order + * When Users are created from the initialAdmin we want to pick correct user details based on the principle claim + * This method will ensure that name & email are correctly picked from the principle claim + * @param user - User details extracted from Token + * @param jwtPrincipalClaims - List of principle claims coming from auth API response + * @param principalDomain - Principle Domain value coming from + * @param jwtPrincipalClaimsMapping - Mapping of principle claims to user profile + * @param clientType - Client Type Public or Confidential + * @returns OidcUser with Profile info plucked based on the principle claim + */ +export const prepareUserProfileFromClaims = ({ + user, + jwtPrincipalClaims, + principalDomain, + jwtPrincipalClaimsMapping, + clientType, +}: { + user: OidcUser; + jwtPrincipalClaims: string[]; + principalDomain: string; + jwtPrincipalClaimsMapping: string[]; + clientType: ClientType; +}): OidcUser => { + const newUser = { + ...user, + profile: + clientType === ClientType.Public + ? getNameFromUserData( + user.profile, + jwtPrincipalClaims, + principalDomain, + jwtPrincipalClaimsMapping + ) + : { + name: user.profile?.name ?? '', + email: user.profile?.email ?? '', + }, + } as OidcUser; + + return newUser; +}; diff --git a/openmetadata-ui/src/main/resources/ui/src/utils/AuthProviderUtil.test.ts b/openmetadata-ui/src/main/resources/ui/src/utils/AuthProviderUtil.test.ts index 7d54423872a..f35fa90e122 100644 --- a/openmetadata-ui/src/main/resources/ui/src/utils/AuthProviderUtil.test.ts +++ b/openmetadata-ui/src/main/resources/ui/src/utils/AuthProviderUtil.test.ts @@ -100,3 +100,69 @@ describe('Test Auth Provider utils', () => { expect(email).toEqual('i_am_preferred_username@test.com'); }); }); + +import { OidcUser } from '../components/Auth/AuthProviders/AuthProvider.interface'; +import { ClientType } from '../generated/configuration/authenticationConfiguration'; +import { prepareUserProfileFromClaims } from './AuthProvider.util'; + +describe('prepareUserProfileFromClaims', () => { + const mockUser: OidcUser = { + profile: { + name: 'John Doe', + email: 'john.doe@example.com', + }, + } as OidcUser; + + const mockJwtPrincipalClaims = ['email']; + const mockPrincipalDomain = 'example.com'; + const mockJwtPrincipalClaimsMapping = ['username:name', 'email:email']; + + it('should prepare user profile for public client type', () => { + const result = prepareUserProfileFromClaims({ + user: mockUser, + jwtPrincipalClaims: mockJwtPrincipalClaims, + principalDomain: mockPrincipalDomain, + jwtPrincipalClaimsMapping: mockJwtPrincipalClaimsMapping, + clientType: ClientType.Public, + }); + + expect(result.profile).toEqual({ + name: 'John Doe', + email: 'john.doe@example.com', + }); + }); + + it('should prepare user profile for non-public client type', () => { + const result = prepareUserProfileFromClaims({ + user: mockUser, + jwtPrincipalClaims: mockJwtPrincipalClaims, + principalDomain: mockPrincipalDomain, + jwtPrincipalClaimsMapping: mockJwtPrincipalClaimsMapping, + clientType: ClientType.Confidential, + }); + + expect(result.profile).toEqual({ + name: 'John Doe', + email: 'john.doe@example.com', + }); + }); + + it('should handle missing profile fields for non-public client type', () => { + const mockUserWithMissingFields: OidcUser = { + profile: {}, + } as OidcUser; + + const result = prepareUserProfileFromClaims({ + user: mockUserWithMissingFields, + jwtPrincipalClaims: mockJwtPrincipalClaims, + principalDomain: mockPrincipalDomain, + jwtPrincipalClaimsMapping: mockJwtPrincipalClaimsMapping, + clientType: ClientType.Confidential, + }); + + expect(result.profile).toEqual({ + name: '', + email: '', + }); + }); +});