Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix(ui): refresh call concurrency for multiple browser tabs #19303

Merged
merged 19 commits into from
Jan 20, 2025
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions openmetadata-ui/src/main/resources/ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,10 @@
"crypto-random-string-with-promisify-polyfill": "^5.0.0",
"diff": "^5.0.0",
"dompurify": "^3.1.5",
"elkjs": "^0.9.3",
"eventemitter3": "^5.0.1",
"fast-json-patch": "^3.1.1",
"history": "4.5.1",
"elkjs": "^0.9.3",
"html-react-parser": "^1.4.14",
"https-browserify": "^1.0.0",
"i18next": "^21.10.0",
Expand Down Expand Up @@ -150,7 +150,7 @@
"@babel/preset-env": "^7.11.0",
"@babel/preset-react": "^7.10.4",
"@estruyf/github-actions-reporter": "^1.7.0",
"@playwright/test": "^1.44.1",
"@playwright/test": "1.48.2",
"@svgr/webpack": "^6.5.0",
"@testing-library/jest-dom": "^5.11.10",
"@testing-library/react": "^11.2.7",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
* limitations under the License.
*/
export const JWT_EXPIRY_TIME_MAP = {
'3 minutes': 180,
'1 hour': 3600,
'2 hours': 7200,
'3 hours': 10800,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const test = base.extend<{
// Set a new value for a key in localStorage
localStorage.setItem(
'om-session',
JSON.stringify({ state: { oidcIdToken: token } })
JSON.stringify({ oidcIdToken: token })
);
}, tokenData.config.JWTToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,53 @@
* limitations under the License.
*/
import { expect, test } from '@playwright/test';
import { LOGIN_ERROR_MESSAGE } from '../../constant/login';
import { JWT_EXPIRY_TIME_MAP, LOGIN_ERROR_MESSAGE } from '../../constant/login';
import { UserClass } from '../../support/user/UserClass';
import { performAdminLogin } from '../../utils/admin';
import { redirectToHomePage } from '../../utils/common';
import { updateJWTTokenExpiryTime } from '../../utils/login';
import { visitUserProfilePage } from '../../utils/user';

const user = new UserClass();
const CREDENTIALS = user.data;
const invalidEmail = '[email protected]';
const invalidPassword = 'testUsers@123';

test.describe.configure({
// 5 minutes max for refresh token tests
timeout: 5 * 60 * 1000,
});

test.describe('Login flow should work properly', () => {
test.afterAll('Cleanup', async ({ browser }) => {
const { apiContext, afterAction, page } = await performAdminLogin(browser);
const response = await page.request.get(
`/api/v1/users/name/${user.getUserName()}`
);

// reset token expiry to 4 hours
await updateJWTTokenExpiryTime(apiContext, JWT_EXPIRY_TIME_MAP['4 hours']);

user.responseData = await response.json();
await user.delete(apiContext);
await afterAction();
});

test.beforeAll(
'Update token timer to be 3 minutes for new token created',
async ({ browser }) => {
const { apiContext, afterAction } = await performAdminLogin(browser);

// update expiry for 3 mins
await updateJWTTokenExpiryTime(
apiContext,
JWT_EXPIRY_TIME_MAP['3 minutes']
);

await afterAction();
}
);

test('Signup and Login with signed up credentials', async ({ page }) => {
await page.goto('/');

Expand Down Expand Up @@ -111,4 +138,36 @@ test.describe('Login flow should work properly', () => {
await page.getByRole('button', { name: 'Submit' }).click();
await page.locator('[data-testid="go-back-button"]').click();
});

test.fixme('Refresh should work', async ({ browser }) => {
const browserContext = await browser.newContext();
const { apiContext, afterAction } = await performAdminLogin(browser);
const page1 = await browserContext.newPage(),
page2 = await browserContext.newPage();

const testUser = new UserClass();
await testUser.create(apiContext);

await afterAction();

await test.step('Login and wait for refresh call is made', async () => {
// User login

await testUser.login(page1);
await redirectToHomePage(page1);
await redirectToHomePage(page2);

const refreshCall = page1.waitForResponse('**/refresh', {
timeout: 3 * 60 * 1000,
});

await refreshCall;

await redirectToHomePage(page1);

await visitUserProfilePage(page1, testUser.responseData.name);
await redirectToHomePage(page2);
await visitUserProfilePage(page2, testUser.responseData.name);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ export const NAME_MAX_LENGTH_VALIDATION_ERROR =
export const getToken = async (page: Page) => {
return page.evaluate(
() =>
JSON.parse(localStorage.getItem('om-session') ?? '{}')?.state
?.oidcIdToken ?? ''
JSON.parse(localStorage.getItem('om-session') ?? '{}')?.oidcIdToken ?? ''
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
isTourRoute,
} from '../../utils/AuthProvider.util';
import { addToRecentSearched } from '../../utils/CommonUtils';
import { getOidcToken } from '../../utils/LocalStorageUtils';
import searchClassBase from '../../utils/SearchClassBase';
import NavBar from '../NavBar/NavBar';
import './app-bar.style.less';
Expand All @@ -37,7 +38,7 @@ const Appbar: React.FC = (): JSX.Element => {
const { isTourOpen, updateTourPage, updateTourSearch, tourSearchValue } =
useTourProvider();

const { isAuthenticated, searchCriteria, getOidcToken, trySilentSignIn } =
const { isAuthenticated, searchCriteria, trySilentSignIn } =
useApplicationStore();

const parsedQueryString = Qs.parse(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { useTranslation } from 'react-i18next';
import { AuthProvider } from '../../../generated/settings/settings';

import { useApplicationStore } from '../../../hooks/useApplicationStore';
import { setOidcToken } from '../../../utils/LocalStorageUtils';
import { AuthenticatorRef } from '../AuthProviders/AuthProvider.interface';

interface Props {
Expand All @@ -31,8 +32,7 @@ interface Props {

const Auth0Authenticator = forwardRef<AuthenticatorRef, Props>(
({ children, onLogoutSuccess }: Props, ref) => {
const { setIsAuthenticated, authConfig, setOidcToken } =
useApplicationStore();
const { setIsAuthenticated, authConfig } = useApplicationStore();
const { t } = useTranslation();
const { loginWithRedirect, getAccessTokenSilently, getIdTokenClaims } =
useAuth0();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ import {
} from '../../../rest/auth-API';

import { useApplicationStore } from '../../../hooks/useApplicationStore';
import {
getRefreshToken,
setOidcToken,
setRefreshToken,
} from '../../../utils/LocalStorageUtils';
import Loader from '../../common/Loader/Loader';
import { useBasicAuth } from '../AuthProviders/BasicAuthProvider';

Expand All @@ -40,9 +45,7 @@ const BasicAuthenticator = forwardRef(
const {
setIsAuthenticated,
authConfig,
getRefreshToken,
setRefreshToken,
setOidcToken,

isApplicationLoading,
} = useApplicationStore();

Expand All @@ -54,7 +57,13 @@ const BasicAuthenticator = forwardRef(
authConfig?.provider !== AuthProvider.Basic &&
authConfig?.provider !== AuthProvider.LDAP
) {
Promise.reject(t('message.authProvider-is-not-basic'));
return Promise.reject(
new Error(t('message.authProvider-is-not-basic'))
);
}

if (!refreshToken) {
return Promise.reject(new Error(t('message.no-token-available')));
}

const response = await getAccessTokenOnExpiry({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,11 @@ import { useHistory } from 'react-router-dom';
import { ROUTES } from '../../../constants/constants';
import { useApplicationStore } from '../../../hooks/useApplicationStore';
import { logoutUser, renewToken } from '../../../rest/LoginAPI';
import { setOidcToken } from '../../../utils/LocalStorageUtils';

export const GenericAuthenticator = forwardRef(
({ children }: { children: ReactNode }, ref) => {
const {
setIsAuthenticated,
setIsSigningUp,
removeOidcToken,
setOidcToken,
} = useApplicationStore();
const { setIsAuthenticated, setIsSigningUp } = useApplicationStore();
const history = useHistory();

const handleLogin = () => {
Expand All @@ -42,7 +38,7 @@ export const GenericAuthenticator = forwardRef(
await logoutUser();

history.push(ROUTES.SIGNIN);
removeOidcToken();
setOidcToken('');
setIsAuthenticated(false);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import { ROUTES } from '../../../constants/constants';
import { useApplicationStore } from '../../../hooks/useApplicationStore';
import useCustomLocation from '../../../hooks/useCustomLocation/useCustomLocation';
import SignInPage from '../../../pages/LoginPage/SignInPage';
import TokenService from '../../../utils/Auth/TokenService/TokenServiceUtil';
import { setOidcToken } from '../../../utils/LocalStorageUtils';
import { showErrorToast } from '../../../utils/ToastUtils';
import Loader from '../../common/Loader/Loader';
import {
Expand Down Expand Up @@ -71,7 +73,6 @@ const OidcAuthenticator = forwardRef<AuthenticatorRef, Props>(
updateAxiosInterceptors,
currentUser,
newUser,
setOidcToken,
isApplicationLoading,
} = useApplicationStore();
const history = useHistory();
Expand Down Expand Up @@ -105,6 +106,9 @@ const OidcAuthenticator = forwardRef<AuthenticatorRef, Props>(
// On success update token in store and update axios interceptors
setOidcToken(user.id_token);
updateAxiosInterceptors();
// Clear the refresh token in progress flag
// Since refresh token request completes with a callback
TokenService.getInstance().clearRefreshInProgress();
};

const handleSilentSignInFailure = (error: unknown) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import React, {
} from 'react';

import { useApplicationStore } from '../../../hooks/useApplicationStore';
import { setOidcToken } from '../../../utils/LocalStorageUtils';
import { AuthenticatorRef } from '../AuthProviders/AuthProvider.interface';

interface Props {
Expand All @@ -30,7 +31,7 @@ interface Props {
const OktaAuthenticator = forwardRef<AuthenticatorRef, Props>(
({ children, onLogoutSuccess }: Props, ref) => {
const { oktaAuth } = useOktaAuth();
const { setIsAuthenticated, setOidcToken } = useApplicationStore();
const { setIsAuthenticated } = useApplicationStore();

const login = async () => {
oktaAuth.signInWithRedirect();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ import { showErrorToast } from '../../../utils/ToastUtils';
import { ROUTES } from '../../../constants/constants';
import { useApplicationStore } from '../../../hooks/useApplicationStore';
import { AccessTokenResponse, refreshSAMLToken } from '../../../rest/auth-API';
import {
getOidcToken,
getRefreshToken,
setOidcToken,
setRefreshToken,
} from '../../../utils/LocalStorageUtils';
import { AuthenticatorRef } from '../AuthProviders/AuthProvider.interface';

interface Props {
Expand All @@ -45,14 +51,7 @@ interface Props {

const SamlAuthenticator = forwardRef<AuthenticatorRef, Props>(
({ children, onLogoutSuccess }: Props, ref) => {
const {
setIsAuthenticated,
authConfig,
getOidcToken,
getRefreshToken,
setRefreshToken,
setOidcToken,
} = useApplicationStore();
const { setIsAuthenticated, authConfig } = useApplicationStore();
const config = authConfig?.samlConfiguration as SamlSSOClientConfig;

const handleSilentSignIn = async (): Promise<AccessTokenResponse> => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,14 @@ jest.mock('../../../../hooks/useApplicationStore', () => {
useApplicationStore: jest.fn(() => ({
authConfig: {},
handleSuccessfulLogin: mockHandleSuccessfulLogin,
setOidcToken: jest.fn(),
})),
};
});

jest.mock('../../../../utils/LocalStorageUtils', () => ({
setOidcToken: jest.fn(),
}));

describe('Test Auth0Callback component', () => {
afterEach(() => {
jest.clearAllMocks();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@ import { t } from 'i18next';
import React, { VFC } from 'react';

import { useApplicationStore } from '../../../../hooks/useApplicationStore';
import { setOidcToken } from '../../../../utils/LocalStorageUtils';
import { OidcUser } from '../../AuthProviders/AuthProvider.interface';

const Auth0Callback: VFC = () => {
const { isAuthenticated, user, getIdTokenClaims, error } = useAuth0();
const { handleSuccessfulLogin, setOidcToken } = useApplicationStore();
const { handleSuccessfulLogin } = useApplicationStore();
if (isAuthenticated) {
getIdTokenClaims()
.then((token) => {
Expand Down
Loading
Loading