fix(ui): overriding user info after login (#17825)

* fix(ui): overriding user info after login

* fix displayName override unnecessarily

* address comments

Co-authored-by: Sachin Chaurasiya <sachinchaurasiyachotey87@gmail.com>

* fix MSAL login and refresh token issue

* remove unwanted changes

---------

Co-authored-by: Sachin Chaurasiya <sachinchaurasiyachotey87@gmail.com>

openmetadata-ui/src/main/resources/ui/package.json

@@ -109,7 +109,6 @@
     "react-antd-column-resize": "1.0.3",
     "react-awesome-query-builder": "5.1.2",
     "react-codemirror2": "^7.2.1",
-    "react-context-mutex": "^2.0.0",
     "react-dnd": "14.0.2",
     "react-dnd-html5-backend": "14.0.2",
     "react-dom": "^17.0.2",

openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx

@@ -14,7 +14,6 @@
 import {
   AuthenticationResult,
   InteractionRequiredAuthError,
-  InteractionStatus,
 } from '@azure/msal-browser';
 import { useAccount, useMsal } from '@azure/msal-react';
 import { AxiosError } from 'axios';
@@ -23,10 +22,8 @@ import React, {
   forwardRef,
   Fragment,
   ReactNode,
-  useEffect,
   useImperativeHandle,
 } from 'react';
-import { useMutex } from 'react-context-mutex';
 import { toast } from 'react-toastify';
 import { useApplicationStore } from '../../../hooks/useApplicationStore';
 import { msalLoginRequest } from '../../../utils/AuthProvider.util';
@@ -50,34 +47,9 @@ const MsalAuthenticator = forwardRef<AuthenticatorRef, Props>(
     { children, onLoginSuccess, onLogoutSuccess, onLoginFailure }: Props,
     ref
   ) => {
-    const { setOidcToken, getOidcToken } = useApplicationStore();
+    const { setOidcToken } = useApplicationStore();
-    const { instance, accounts, inProgress } = useMsal();
+    const { instance, accounts } = useMsal();
     const account = useAccount(accounts[0] || {});
-    const MutexRunner = useMutex();
-    const mutex = new MutexRunner('fetchIdToken');
-
-    const handleOnLogoutSuccess = () => {
-      for (const key in localStorage) {
-        if (key.includes('-login.windows.net-') || key.startsWith('msal.')) {
-          localStorage.removeItem(key);
-        }
-      }
-      onLogoutSuccess();
-    };
-
-    const login = () => {
-      try {
-        instance.loginPopup(msalLoginRequest);
-      } catch (error) {
-        // eslint-disable-next-line no-console
-        console.error(error);
-        onLoginFailure(error as AxiosError);
-      }
-    };
-
-    const logout = () => {
-      handleOnLogoutSuccess();
-    };
 
     const parseResponse = (response: AuthenticationResult): OidcUser => {
       // Call your API with the access token and return the data you need to save in state
@@ -104,6 +76,29 @@ const MsalAuthenticator = forwardRef<AuthenticatorRef, Props>(
       return user;
     };
 
+    const handleOnLogoutSuccess = () => {
+      for (const key in localStorage) {
+        if (key.includes('-login.windows.net-') || key.startsWith('msal.')) {
+          localStorage.removeItem(key);
+        }
+      }
+      onLogoutSuccess();
+    };
+
+    const login = async () => {
+      try {
+        const response = await instance.loginPopup(msalLoginRequest);
+
+        onLoginSuccess(parseResponse(response));
+      } catch (error) {
+        onLoginFailure(error as AxiosError);
+      }
+    };
+
+    const logout = () => {
+      handleOnLogoutSuccess();
+    };
+
     const fetchIdToken = async (
       shouldFallbackToPopup = false
     ): Promise<OidcUser> => {
@@ -154,34 +149,11 @@ const MsalAuthenticator = forwardRef<AuthenticatorRef, Props>(
     };
 
     const renewIdToken = async () => {
-      const user = await fetchIdToken();
+      const user = await fetchIdToken(true);
 
       return user.id_token;
     };
 
-    useEffect(() => {
-      const oidcUserToken = getOidcToken();
-      if (
-        !oidcUserToken &&
-        inProgress === InteractionStatus.None &&
-        (accounts.length > 0 || account?.idTokenClaims)
-      ) {
-        mutex.run(async () => {
-          mutex.lock();
-          fetchIdToken(true)
-            .then((user) => {
-              if ((user as OidcUser).id_token) {
-                onLoginSuccess(user as OidcUser);
-              }
-            })
-            .catch(onLoginFailure)
-            .finally(() => {
-              mutex.unlock();
-            });
-        });
-      }
-    }, [inProgress, accounts, instance, account]);
-
     useImperativeHandle(ref, () => ({
       invokeLogin: login,
       invokeLogout: logout,

openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx

@@ -25,7 +25,6 @@ import {
   InternalAxiosRequestConfig,
 } from 'axios';
 import { CookieStorage } from 'cookie-storage';
-import { compare } from 'fast-json-patch';
 import { isEmpty, isNil, isNumber } from 'lodash';
 import Qs from 'qs';
 import React, {
@@ -63,7 +62,7 @@ import {
   fetchAuthenticationConfig,
   fetchAuthorizerConfig,
 } from '../../../rest/miscAPI';
-import { getLoggedInUser, updateUserDetail } from '../../../rest/userAPI';
+import { getLoggedInUser } from '../../../rest/userAPI';
 import TokenService from '../../../utils/Auth/TokenService/TokenServiceUtil';
 import {
   extractDetailsFromToken,
@@ -76,10 +75,7 @@ import {
 import { getPathNameFromWindowLocation } from '../../../utils/RouterUtils';
 import { escapeESReservedCharacters } from '../../../utils/StringsUtils';
 import { showErrorToast, showInfoToast } from '../../../utils/ToastUtils';
-import {
+import { checkIfUpdateRequired } from '../../../utils/UserDataUtils';
-  getUserDataFromOidc,
-  matchUserDetails,
-} from '../../../utils/UserDataUtils';
 import { resetWebAnalyticSession } from '../../../utils/WebAnalyticsUtils';
 import Loader from '../../common/Loader/Loader';
 import Auth0Authenticator from '../AppAuthenticators/Auth0Authenticator';
@@ -269,29 +265,6 @@ export const AuthProvider = ({
     }
   };
 
-  const getUpdatedUser = async (updatedData: User, existingData: User) => {
-    // PUT method for users api only excepts below fields
-    const updatedUserData = { ...existingData, ...updatedData };
-    const jsonPatch = compare(existingData, updatedUserData);
-
-    try {
-      const res = await updateUserDetail(existingData.id, jsonPatch);
-      if (res) {
-        setCurrentUser({ ...existingData, ...res });
-      } else {
-        throw t('server.unexpected-response');
-      }
-    } catch (error) {
-      setCurrentUser(existingData);
-      showErrorToast(
-        error as AxiosError,
-        t('server.entity-updating-error', {
-          entity: t('label.admin-profile'),
-        })
-      );
-    }
-  };
-
   /**
    * Renew Id Token handler for all the SSOs.
    * This method will be called when the id token is about to expire.
@@ -428,12 +401,8 @@ export const AuthProvider = ({
 
         const res = await getLoggedInUser({ fields });
         if (res) {
-          const updatedUserData = getUserDataFromOidc(res, newUser);
+          const userDetails = await checkIfUpdateRequired(res, newUser);
-          if (!matchUserDetails(res, updatedUserData, ['profile', 'email'])) {
+          setCurrentUser(userDetails);
-            getUpdatedUser(updatedUserData, res);
-          } else {
-            setCurrentUser(res);
-          }
 
           // Fetch domains at the start
           await fetchDomainList();

openmetadata-ui/src/main/resources/ui/src/components/Settings/Users/UserProfileIcon/UserProfileIcon.component.tsx

@@ -47,7 +47,7 @@ type ListMenuItemProps = {
   listItems: EntityReference[];
   labelRenderer: (item: EntityReference) => ReactNode;
   readMoreLabelRenderer: (count: number) => ReactNode;
-  readMoreKey?: string;
+  readMoreKey: string;
   sizeLimit?: number;
 };
 
@@ -66,7 +66,7 @@ const renderLimitedListMenuItem = ({
   const items = listItems.slice(0, sizeLimit);
 
   return isEmpty(items)
-    ? [{ label: NO_DATA_PLACEHOLDER, key: 'no-teams' }]
+    ? [{ label: NO_DATA_PLACEHOLDER, key: readMoreKey.replace('more', 'no') }]
     : [
         ...(items?.map((item) => ({
           label: labelRenderer(item),

openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts

@@ -73,6 +73,7 @@ class TokenService {
         response = await this.renewToken();
         this.tokeUpdateInProgress = false;
       } catch (error) {
+        useApplicationStore.getState().onLogoutHandler();
         // Do nothing
       } finally {
         this.tokeUpdateInProgress = false;

openmetadata-ui/src/main/resources/ui/src/utils/UserDataUtils.test.ts

@@ -10,8 +10,9 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
+import { OidcUser } from '../components/Auth/AuthProviders/AuthProvider.interface';
 import { User } from '../generated/entity/teams/user';
-import { getUserWithImage } from './UserDataUtils';
+import { checkIfUpdateRequired, getUserWithImage } from './UserDataUtils';
 
 describe('getUserWithImage', () => {
   it('should return the correct user based on profile and isBot status', () => {
@@ -88,3 +89,64 @@ describe('getUserWithImage', () => {
     expect(result).toEqual(userWithoutProfile);
   });
 });
+
+describe('checkIfUpdateRequired', () => {
+  it('should return the updated user details if update is required', async () => {
+    const existingUserDetails: User = {
+      email: 'a@a.com',
+      id: '1',
+      name: 'user',
+      isBot: false,
+    };
+
+    const newUser: OidcUser = {
+      id_token: 'idToken',
+      scope: 'scope',
+      profile: {
+        email: 'a@a.com',
+        name: 'updatedUser',
+        picture: '',
+        preferred_username: 'preferred_username',
+        sub: 'sub',
+      },
+    };
+
+    const updatedUserDetails = await checkIfUpdateRequired(
+      existingUserDetails,
+      newUser
+    );
+
+    expect(updatedUserDetails).toEqual({
+      ...existingUserDetails,
+      name: 'user',
+    });
+  });
+
+  it('should return the existing user details if update is not required', async () => {
+    const existingUserDetails: User = {
+      email: 'a@a.com',
+      id: '1',
+      name: 'user',
+      isBot: false,
+    };
+
+    const newUser: OidcUser = {
+      id_token: 'idToken',
+      scope: 'scope',
+      profile: {
+        email: 'a@a.com',
+        name: 'user',
+        picture: '',
+        preferred_username: 'preferred_username',
+        sub: 'sub',
+      },
+    };
+
+    const updatedUserDetails = await checkIfUpdateRequired(
+      existingUserDetails,
+      newUser
+    );
+
+    expect(updatedUserDetails).toEqual(existingUserDetails);
+  });
+});

openmetadata-ui/src/main/resources/ui/src/utils/UserDataUtils.ts

@@ -11,6 +11,8 @@
  *  limitations under the License.
  */
 
+import { AxiosError } from 'axios';
+import { compare } from 'fast-json-patch';
 import { isEqual } from 'lodash';
 import { OidcUser } from '../components/Auth/AuthProviders/AuthProvider.interface';
 import { WILD_CARD_CHAR } from '../constants/char.constants';
@@ -18,13 +20,16 @@ import { SettledStatus } from '../enums/Axios.enum';
 import { SearchIndex } from '../enums/search.enum';
 import { SearchResponse } from '../interface/search.interface';
 import { getSearchedTeams, getSearchedUsers } from '../rest/miscAPI';
+import { updateUserDetail } from '../rest/userAPI';
 import { User } from './../generated/entity/teams/user';
 import { formatTeamsResponse, formatUsersResponse } from './APIUtils';
 import { getImages } from './CommonUtils';
+import i18n from './i18next/LocalUtil';
 import {
   getImageWithResolutionAndFallback,
   ImageQuality,
 } from './ProfilerUtils';
+import { showErrorToast } from './ToastUtils';
 import userClassBase from './UserClassBase';
 
 export const getUserDataFromOidc = (
@@ -44,7 +49,7 @@ export const getUserDataFromOidc = (
     ...userData,
     email,
     displayName: oidcUser.profile.name,
-    profile: images ? { images } : userData.profile,
+    profile: images ? { ...userData.profile, images } : undefined,
   };
 };
 
@@ -124,3 +129,44 @@ export const getUserWithImage = (user: User) => {
 
   return user;
 };
+
+export const checkIfUpdateRequired = async (
+  existingUserDetails: User,
+  newUser: OidcUser
+): Promise<User> => {
+  const updatedUserData = getUserDataFromOidc(existingUserDetails, newUser);
+
+  if (existingUserDetails.email !== updatedUserData.email) {
+    return existingUserDetails;
+  } else if (
+    existingUserDetails.email === updatedUserData.email &&
+    // We only want to update images / profile info not any other information
+    updatedUserData.profile?.images &&
+    !matchUserDetails(existingUserDetails, updatedUserData, ['profile'])
+  ) {
+    const finalData = {
+      ...existingUserDetails,
+      //   We want to override any profile information that is coming from the OIDC provider
+      profile: {
+        ...existingUserDetails.profile,
+        ...updatedUserData.profile
+      },
+    };
+    const jsonPatch = compare(existingUserDetails, finalData);
+
+    try {
+      const res = await updateUserDetail(existingUserDetails.id, jsonPatch);
+
+      return res;
+    } catch (error) {
+      showErrorToast(
+        error as AxiosError,
+        i18n.t('server.entity-updating-error', {
+          entity: i18n.t('label.admin-profile'),
+        })
+      );
+    }
+  }
+
+  return existingUserDetails;
+};

openmetadata-ui/src/main/resources/ui/webpack.config.dev.js

@@ -205,6 +205,14 @@ module.exports = {
           from: path.join(__dirname, 'public/favicon.png'),
           to: outputPath,
         },
+        {
+          from: path.join(__dirname, 'public/favicons/favicon-16x16.png'),
+          to: outputPath,
+        },
+        {
+          from: path.join(__dirname, 'public/favicons/favicon-32x32.png'),
+          to: outputPath,
+        },
         {
           from: path.join(__dirname, 'public/logo192.png'),
           to: outputPath,

openmetadata-ui/src/main/resources/ui/webpack.config.prod.js

@@ -199,6 +199,14 @@ module.exports = {
           from: path.join(__dirname, 'public/favicon.png'),
           to: outputPath,
         },
+        {
+          from: path.join(__dirname, 'public/favicons/favicon-16x16.png'),
+          to: outputPath,
+        },
+        {
+          from: path.join(__dirname, 'public/favicons/favicon-32x32.png'),
+          to: outputPath,
+        },
         {
           from: path.join(__dirname, 'public/logo192.png'),
           to: outputPath,

openmetadata-ui/src/main/resources/ui/yarn.lock

@@ -12186,11 +12186,6 @@ react-codemirror2@^7.2.1:
   resolved "https://registry.yarnpkg.com/react-codemirror2/-/react-codemirror2-7.2.1.tgz#38dab492fcbe5fb8ebf5630e5bb7922db8d3a10c"
   integrity sha512-t7YFmz1AXdlImgHXA9Ja0T6AWuopilub24jRaQdPVbzUJVNKIYuy3uCFZYa7CE5S3UW6SrSa5nAqVQvtzRF9gw==
 
-react-context-mutex@^2.0.0:
-  version "2.0.0"
-  resolved "https://registry.yarnpkg.com/react-context-mutex/-/react-context-mutex-2.0.0.tgz#4060bc8fbced6d2680ab093139c727201d402b88"
-  integrity sha512-Qss0YfJAKQ/+CYdVjHB3w7wRLdeetSrp2IMEEouXEpoTdohaCXIKZtFyNMMKDytLVWqHiiXg0RDA2kj/DQbeMQ==
-
 react-dnd-html5-backend@14.0.2:
   version "14.0.2"
   resolved "https://registry.yarnpkg.com/react-dnd-html5-backend/-/react-dnd-html5-backend-14.0.2.tgz#25019388f6abdeeda3a6fea835dff155abb2085c"