From 1cfdd6d7b0f565d3f59ebafa96017df18e3ef00b Mon Sep 17 00:00:00 2001 From: Sachin Chaurasiya Date: Wed, 25 Jan 2023 18:50:39 +0530 Subject: [PATCH] fix(ui): JwtClaim should also check for "sub" claim and add fallback mechanism (#9909) * fix(ui): JwtClaim should also check for "sub" claim and add fallback mechanism * test: add unit tests --- .../auth-provider/AuthProvider.interface.ts | 3 +- .../auth-provider/basic-auth.provider.tsx | 1 + .../auth-provider/okta-auth-provider.tsx | 1 + .../authenticators/MsalAuthenticator.tsx | 1 + .../Auth0Callback/Auth0Callback.test.tsx | 1 + .../callbacks/Auth0Callback/Auth0Callback.tsx | 1 + .../ui/src/pages/signup/index.test.tsx | 7 ++ .../resources/ui/src/pages/signup/index.tsx | 2 +- .../ui/src/utils/AuthProvider.util.ts | 37 +++++--- .../ui/src/utils/AuthProviderUtil.test.ts | 94 +++++++++++++++++++ 10 files changed, 133 insertions(+), 15 deletions(-) create mode 100644 openmetadata-ui/src/main/resources/ui/src/utils/AuthProviderUtil.test.ts diff --git a/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/AuthProvider.interface.ts b/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/AuthProvider.interface.ts index 7224f07d388..c8cdb73cda5 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/AuthProvider.interface.ts +++ b/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/AuthProvider.interface.ts @@ -24,7 +24,7 @@ export type UserProfile = { name: string; picture: string; locale?: string; -} & Pick; +} & Pick; export type OidcUser = { id_token: string; @@ -41,4 +41,5 @@ export interface AuthenticatorRef { export enum JWT_PRINCIPAL_CLAIMS { EMAIL = 'email', PREFERRED_USERNAME = 'preferred_username', + SUB = 'sub', } diff --git a/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/basic-auth.provider.tsx b/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/basic-auth.provider.tsx index 8f3512b4f0e..0a9ab643bf8 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/basic-auth.provider.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/basic-auth.provider.tsx @@ -111,6 +111,7 @@ const BasicAuthProvider = ({ email, name: '', picture: '', + sub: '', }, scope: '', }); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/okta-auth-provider.tsx b/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/okta-auth-provider.tsx index b2ff7ccb731..36f30597099 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/okta-auth-provider.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/authentication/auth-provider/okta-auth-provider.tsx @@ -59,6 +59,7 @@ export const OktaAuthProvider: FunctionComponent = ({ // eslint-disable-next-line @typescript-eslint/no-explicit-any picture: (info as any).imageUrl || '', locale: info.locale || '', + sub: info.sub, }, }; onLoginSuccess(user); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/authentication/authenticators/MsalAuthenticator.tsx b/openmetadata-ui/src/main/resources/ui/src/components/authentication/authenticators/MsalAuthenticator.tsx index 7f4ac7d33f7..27c629d86c2 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/authentication/authenticators/MsalAuthenticator.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/authentication/authenticators/MsalAuthenticator.tsx @@ -83,6 +83,7 @@ const MsalAuthenticator = forwardRef( email: account?.username || '', name: account?.name || '', picture: '', + sub: '', }, }; diff --git a/openmetadata-ui/src/main/resources/ui/src/components/authentication/callbacks/Auth0Callback/Auth0Callback.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/authentication/callbacks/Auth0Callback/Auth0Callback.test.tsx index 412149bbbe7..04fcf8e6a4d 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/authentication/callbacks/Auth0Callback/Auth0Callback.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/authentication/callbacks/Auth0Callback/Auth0Callback.test.tsx @@ -121,6 +121,7 @@ describe('Test Auth0Callback component', () => { name: 'test_user', picture: 'test_picture', locale: 'test_locale', + sub: '', }, scope: '', }); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/authentication/callbacks/Auth0Callback/Auth0Callback.tsx b/openmetadata-ui/src/main/resources/ui/src/components/authentication/callbacks/Auth0Callback/Auth0Callback.tsx index 0a0c4626b1e..7fe0b9ea6a8 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/authentication/callbacks/Auth0Callback/Auth0Callback.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/authentication/callbacks/Auth0Callback/Auth0Callback.tsx @@ -34,6 +34,7 @@ const Auth0Callback: VFC = () => { name: user?.name || '', picture: user?.picture || '', locale: user?.locale || '', + sub: user?.sub || '', }, }; handleSuccessfulLogin(oidcUser); diff --git a/openmetadata-ui/src/main/resources/ui/src/pages/signup/index.test.tsx b/openmetadata-ui/src/main/resources/ui/src/pages/signup/index.test.tsx index bfaf145bd56..937a3719357 100644 --- a/openmetadata-ui/src/main/resources/ui/src/pages/signup/index.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/pages/signup/index.test.tsx @@ -19,6 +19,8 @@ import AppState from '../../AppState'; import { getImages } from '../../utils/CommonUtils'; import { mockCreateUser } from './mocks/signup.mock'; +let letExpectedUserName = 'sample123'; + const mockChangeHandler = jest.fn(); const mockSubmitHandler = jest.fn(); const mockShowErrorToast = jest.fn(); @@ -84,6 +86,10 @@ jest.mock('../../utils/CommonUtils', () => ({ ), })); +jest.mock('utils/AuthProvider.util', () => ({ + getNameFromUserData: jest.fn().mockImplementation(() => letExpectedUserName), +})); + describe('Signup page', () => { beforeEach(() => { jest.clearAllMocks(); @@ -222,6 +228,7 @@ describe('Signup page', () => { it('Handlers in form should work if data is empty', async () => { (getImages as jest.Mock).mockImplementationOnce(() => Promise.reject('')); + letExpectedUserName = ''; AppState.newUser = { name: '', diff --git a/openmetadata-ui/src/main/resources/ui/src/pages/signup/index.tsx b/openmetadata-ui/src/main/resources/ui/src/pages/signup/index.tsx index 2ac687fbe7f..614ed749fbb 100644 --- a/openmetadata-ui/src/main/resources/ui/src/pages/signup/index.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/pages/signup/index.tsx @@ -21,12 +21,12 @@ import { CookieStorage } from 'cookie-storage'; import React, { useState } from 'react'; import { useHistory } from 'react-router-dom'; import { createUser } from 'rest/userAPI'; +import { getNameFromUserData } from 'utils/AuthProvider.util'; import appState from '../../AppState'; import { REDIRECT_PATHNAME, ROUTES } from '../../constants/constants'; import { CreateUser } from '../../generated/api/teams/createUser'; import { User } from '../../generated/entity/teams/user'; import jsonData from '../../jsons/en'; -import { getNameFromUserData } from '../../utils/AuthProvider.util'; import { getImages } from '../../utils/CommonUtils'; import SVGIcons, { Icons } from '../../utils/SvgUtils'; import { showErrorToast } from '../../utils/ToastUtils'; 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 fb95fb0688d..095416bc61b 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 @@ -18,10 +18,7 @@ import { PopupRequest, PublicClientApplication, } from '@azure/msal-browser'; -import { - JWT_PRINCIPAL_CLAIMS, - UserProfile, -} from 'components/authentication/auth-provider/AuthProvider.interface'; +import { UserProfile } from 'components/authentication/auth-provider/AuthProvider.interface'; import jwtDecode, { JwtPayload } from 'jwt-decode'; import { first, isNil } from 'lodash'; import { WebStorageStateStore } from 'oidc-client'; @@ -212,18 +209,32 @@ export const getNameFromUserData = ( user: UserProfile, jwtPrincipalClaims: AuthenticationConfiguration['jwtPrincipalClaims'] = [] ) => { - // get the first claim from claim list - const firstClaim = first(jwtPrincipalClaims); - const nameFromEmail = getNameFromEmail(user.email); + // filter and extract the present claims in user profile + const jwtClaims = jwtPrincipalClaims.reduce( + (prev: string[], curr: string) => { + const currentClaim = user[curr as keyof UserProfile]; + if (currentClaim) { + return [...prev, currentClaim]; + } else { + return prev; + } + }, + [] + ); - /* if first claim is preferred_username then return preferred_username - * for fallback if preferred_username is not present then return the name from email - */ - if (firstClaim === JWT_PRINCIPAL_CLAIMS.PREFERRED_USERNAME) { - return user.preferred_username ?? nameFromEmail; + // get the first claim from claims list + const firstClaim = first(jwtClaims); + + let userName = ''; + + // if claims contains the "@" then split it out otherwise assign it to username as it is + if (firstClaim?.includes('@')) { + userName = getNameFromEmail(firstClaim); + } else { + userName = firstClaim ?? ''; } - return nameFromEmail; + return userName; }; export const isProtectedRoute = (pathname: string) => { 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 new file mode 100644 index 00000000000..853efaad3b9 --- /dev/null +++ b/openmetadata-ui/src/main/resources/ui/src/utils/AuthProviderUtil.test.ts @@ -0,0 +1,94 @@ +/* + * Copyright 2023 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { getNameFromUserData } from './AuthProvider.util'; + +const userProfile = { + email: 'testUser@gmail.com', + sub: 'i_am_sub', + preferred_username: 'i_am_preferred_username', + name: 'Test User', + picture: '', +}; + +describe('Test Auth Provider utils', () => { + it('getNameFromUserData should return the userName for first claim', () => { + // should return the preferred username + const userName1 = getNameFromUserData(userProfile, [ + 'preferred_username', + 'email', + 'sub', + ]); + + expect(userName1).toEqual('i_am_preferred_username'); + + // should return the name from email + const userName2 = getNameFromUserData(userProfile, [ + 'email', + 'preferred_username', + 'sub', + ]); + + expect(userName2).toEqual('testUser'); + + // should return the sub + const userName3 = getNameFromUserData(userProfile, [ + 'sub', + 'email', + 'preferred_username', + ]); + + expect(userName3).toEqual('i_am_sub'); + }); + + it('getNameFromUserData should fallback to next claim if first claim is not present', () => { + // should return the name from email as fallback + const userName1 = getNameFromUserData( + { ...userProfile, preferred_username: '' }, + ['preferred_username', 'email', 'sub'] + ); + + expect(userName1).toEqual('testUser'); + + // should return the sub as fallback + const userName2 = getNameFromUserData( + { ...userProfile, preferred_username: '' }, + ['preferred_username', 'sub', 'email'] + ); + + expect(userName2).toEqual('i_am_sub'); + + // should return the name from email as fallback if both 'preferred_username' and 'sub' are not present + const userName3 = getNameFromUserData( + { ...userProfile, preferred_username: '', sub: '' }, + ['preferred_username', 'sub', 'email'] + ); + + expect(userName3).toEqual('testUser'); + }); + + it('getNameFromUserData should handle the claim if it contains @', () => { + const userName1 = getNameFromUserData( + { ...userProfile, preferred_username: 'test@gmail.com' }, + ['preferred_username', 'email', 'sub'] + ); + + expect(userName1).toEqual('test'); + + const userName2 = getNameFromUserData( + { ...userProfile, sub: 'test-1@gmail.com' }, + ['sub', 'preferred_username', 'email'] + ); + + expect(userName2).toEqual('test-1'); + }); +});