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> (cherry picked from commit 46550b3a881c973c40d08612d22c6df17b522089)
2025-10-31 02:29:03 +00:00 · 2024-09-17 19:16:59 +05:30 · 2024-09-17 19:16:59 +05:30 · 3c8603e3a5
commit 3c8603e3a5
parent f08e643152
10 changed files with 159 additions and 99 deletions
--- a/openmetadata-ui/src/main/resources/ui/package.json
+++ b/openmetadata-ui/src/main/resources/ui/package.json
@ -109,7 +109,6 @@
    "react": "^16.14.0",
    "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": "^16.14.0",
--- a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx
+++ b/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 { instance, accounts, inProgress } = useMsal();
+    const { setOidcToken } = useApplicationStore();
+    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,
--- 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
@ -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, {
@ -62,7 +61,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,
@ -74,10 +73,7 @@ import {
 } from '../../../utils/AuthProvider.util';
 import { escapeESReservedCharacters } from '../../../utils/StringsUtils';
 import { showErrorToast, showInfoToast } from '../../../utils/ToastUtils';
-import {
-  getUserDataFromOidc,
-  matchUserDetails,
-} from '../../../utils/UserDataUtils';
+import { checkIfUpdateRequired } from '../../../utils/UserDataUtils';
 import { resetWebAnalyticSession } from '../../../utils/WebAnalyticsUtils';
 import Loader from '../../common/Loader/Loader';
 import Auth0Authenticator from '../AppAuthenticators/Auth0Authenticator';
@ -267,29 +263,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.
@ -426,12 +399,8 @@ export const AuthProvider = ({

        const res = await getLoggedInUser({ fields });
        if (res) {
-          const updatedUserData = getUserDataFromOidc(res, newUser);
-          if (!matchUserDetails(res, updatedUserData, ['profile', 'email'])) {
-            getUpdatedUser(updatedUserData, res);
-          } else {
-            setCurrentUser(res);
-          }
+          const userDetails = await checkIfUpdateRequired(res, newUser);
+          setCurrentUser(userDetails);

          // Fetch domains at the start
          await fetchDomainList();
--- a/openmetadata-ui/src/main/resources/ui/src/components/Settings/Users/UserProfileIcon/UserProfileIcon.component.tsx
+++ b/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),
--- a/openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts
+++ b/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;
--- a/openmetadata-ui/src/main/resources/ui/src/utils/UserDataUtils.test.ts
+++ b/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);
+  });
+});
--- a/openmetadata-ui/src/main/resources/ui/src/utils/UserDataUtils.ts
+++ b/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;
+};
--- a/openmetadata-ui/src/main/resources/ui/webpack.config.dev.js
+++ b/openmetadata-ui/src/main/resources/ui/webpack.config.dev.js
@ -202,6 +202,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,
--- a/openmetadata-ui/src/main/resources/ui/webpack.config.prod.js
+++ b/openmetadata-ui/src/main/resources/ui/webpack.config.prod.js
@ -198,6 +198,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,
--- a/openmetadata-ui/src/main/resources/ui/yarn.lock
+++ b/openmetadata-ui/src/main/resources/ui/yarn.lock
@ -12338,11 +12338,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"