-
Notifications
You must be signed in to change notification settings - Fork 13
Added multi auth cluster #952
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's gonna be cleaner to convert the empty component AuthValidator into a useValidateAuth hook that has a clientId as parameter instead of an object?
function useValidateAuth(clientId: string) {
const { logout } = useAuth0();
useEffect(() => {
const storedToken = localStorage.getItem(STORAGE_KEY_AUTH_TOKEN);
if (!isTokenValid(clientId) && storedToken) {
localStorage.removeItem(STORAGE_KEY_AUTH_TOKEN);
logout({ logoutParams: { returnTo: window.location.href } });
}
}, [clientId, logout]);
}This way, we would have Auth0Provider returning just its children.
export function AuthProvider({ configs, children }: PropsWithChildren<AuthProviderProps>) {
if (!configs?.length) {
return <>{children}</>;
}
const config = getApiConfigOrThrow(configs);
useValidateAuth(config.clientId);
return (
<Auth0Provider
domain={config.domain}
clientId={config.clientId}
authorizationParams={config.authorizationParams}
context={config.context}
>
{children}
</Auth0Provider>
);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useAuth0 hook must be used within an Auth0Provider component; otherwise, it will throw an error
| */ | ||
| export function isTokenValid(clientID: string): boolean { | ||
| const fetchedToken = localStorage.getItem(STORAGE_KEY_AUTH_TOKEN); | ||
| const fetchedTokenDecoded = fetchedToken ? decodeMonkJwt(fetchedToken).azp : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should fetchedTokenDecoded be named as fetchedClientID since we are specifically looking for .azp | null and compare it to clientID?
| * Utility function that retrieves the appropriate AuthConfig based on the URL search params | ||
| * (ie. MonkSearchParam.CLIENT_ID). | ||
| */ | ||
| export function getApiConfigOrThrow(configs: AuthConfig[]): AuthConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the function's name, it is not meant to use any kind of React, but we are using useMonkSearchParams hook.
We would better refactor it as either a useApiConfigOrThrow hook or pure function.
| export function getApiConfigOrThrow(configs: AuthConfig[]): AuthConfig { | ||
| const { get } = useMonkSearchParams(); | ||
|
|
||
| if (!configs.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, having a config file is crucial for the app and has to be loaded at the very high-level of the app, in LiveConfigAppProvider.
If we throw something from here, there's no message except the Uncaught error in the browser's console.
This utility function is used in 2 places:
authProvider.tsx, which has its back covered through this line:
if (!configs?.length) {
return <>{children}</>;
}App.tsxwhich passes it toLiveConfigAppProvideras a prop:
<LiveConfigAppProvider
...
apiDomain={getApiConfigOrThrow(authConfigs).apiDomain}
/>Would it be easier to avoid the throw here and make sure we throw just one time, at the very beginning, such as inside LiveConfigAppProvider? Non-existing apiDomain could be covered by extending this verification:
if (loading.isLoading || loading.error || !config) {
return (
<div style={styles['container']}>
{loading.isLoading && <Spinner primaryColor='primary' size={70} />}
{!loading.isLoading && (
<>
<div style={styles['errorMessage']} data-testid='error-msg'>
Unable to fetch application configuration. Please try again in a few minutes.
</div>
<Button variant='outline' icon='refresh' onClick={() => setRetry((value) => value + 1)}>
Retry
</Button>
</>
)}
</div>
);
}
Overview
Jira Ticket Reference : MN-784
Added a new AuthProvider component.
It reads the MonkSearchParam.CLIENT_ID from the URL and redirects the user to the appropriate authentication domain.
Checklist before requesting a review