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

refactor(web): remove jwt-decode and refactor auth logic #5620

Merged
merged 2 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 0 additions & 1 deletion apps/web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@
"eslint-plugin-react-hooks": "^4.4.0",
"handlebars": "^4.7.7",
"html-webpack-plugin": "5.5.3",
"jwt-decode": "^3.1.2",
"launchdarkly-react-client-sdk": "^3.0.6",
"less": "^4.1.0",
"localforage": "^1.10.0",
Expand Down
10 changes: 5 additions & 5 deletions apps/web/src/AppRoutes.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { FeatureFlagsKeysEnum } from '@novu/shared';
import { Route, Routes } from 'react-router-dom';
import { AppLayout } from './components/layout/AppLayout';
import { RequiredAuth } from './components/layout/RequiredAuth';
import { EnsureOnboardingComplete } from './components/layout/EnsureOnboardingComplete';
import { ROUTES } from './constants/routes.enum';
import { useFeatureFlag } from './hooks';
import { ActivitiesPage } from './pages/activities/ActivitiesPage';
Expand Down Expand Up @@ -62,17 +62,17 @@ export const AppRoutes = () => {
<Route
path={ROUTES.PARTNER_INTEGRATIONS_VERCEL_LINK_PROJECTS}
element={
<RequiredAuth>
<EnsureOnboardingComplete>
<LinkVercelProjectPage type="create" />
</RequiredAuth>
</EnsureOnboardingComplete>
}
/>
<Route
path={ROUTES.PARTNER_INTEGRATIONS_VERCEL_LINK_PROJECTS_EDIT}
element={
<RequiredAuth>
<EnsureOnboardingComplete>
<LinkVercelProjectPage type="edit" />
</RequiredAuth>
</EnsureOnboardingComplete>
}
/>
<Route element={<AppLayout />}>
Expand Down
26 changes: 14 additions & 12 deletions apps/web/src/Providers.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Loader } from '@mantine/core';
import { ColorSchemeProvider, Loader } from '@mantine/core';
import { colors } from '@novu/design-system';
import { CONTEXT_PATH, SegmentProvider } from '@novu/shared-web';
import * as Sentry from '@sentry/react';
Expand Down Expand Up @@ -48,17 +48,19 @@ const fallbackDisplay = (
*/
const Providers: React.FC<PropsWithChildren<{}>> = ({ children }) => {
return (
<SegmentProvider>
<QueryClientProvider client={queryClient}>
<BrowserRouter basename={CONTEXT_PATH}>
<AuthProvider>
<LaunchDarklyProvider fallbackDisplay={fallbackDisplay}>
<HelmetProvider>{children}</HelmetProvider>
</LaunchDarklyProvider>
</AuthProvider>
</BrowserRouter>
</QueryClientProvider>
</SegmentProvider>
<ColorSchemeProvider colorScheme={'dark'} toggleColorScheme={() => {}}>
<SegmentProvider>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporarily adding this to fix the broken next.

<QueryClientProvider client={queryClient}>
<BrowserRouter basename={CONTEXT_PATH}>
<AuthProvider fallbackComponent={fallbackDisplay}>
<LaunchDarklyProvider>
<HelmetProvider>{children}</HelmetProvider>
</LaunchDarklyProvider>
</AuthProvider>
</BrowserRouter>
</QueryClientProvider>
</SegmentProvider>
</ColorSchemeProvider>
);
};

Expand Down
26 changes: 3 additions & 23 deletions apps/web/src/components/launch-darkly/LaunchDarklyProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,33 +2,23 @@ import * as Sentry from '@sentry/react';

import { IOrganizationEntity } from '@novu/shared';
import { asyncWithLDProvider } from 'launchdarkly-react-client-sdk';
import { PropsWithChildren, ReactNode, useEffect, useMemo, useRef, useState } from 'react';
import { PropsWithChildren, useEffect, useMemo, useRef, useState } from 'react';
import { useFeatureFlags, useAuthContext, LAUNCH_DARKLY_CLIENT_SIDE_ID } from '@novu/shared-web';
import { selectShouldInitializeLaunchDarkly } from './utils/selectShouldInitializeLaunchDarkly';
import { selectShouldShowLaunchDarklyFallback } from './utils/selectShouldShowLaunchDarklyFallback';

/** A provider with children required */
type GenericLDProvider = Awaited<ReturnType<typeof asyncWithLDProvider>>;

/** Simply renders the children */
const DEFAULT_GENERIC_PROVIDER: GenericLDProvider = ({ children }) => <>{children}</>;

export interface ILaunchDarklyProviderProps {
/** Renders when LaunchDarkly is enabled and is awaiting initialization */
fallbackDisplay: ReactNode;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed, this is moved to the AuthProvider. This is suitable because Launch Darkly was actually dependent on auth context being available, now it will always be there.

/**
* Async provider for feature flags.
*
* @requires AuthProvider must be wrapped in the AuthProvider.
*/
export const LaunchDarklyProvider: React.FC<PropsWithChildren<ILaunchDarklyProviderProps>> = ({
children,
fallbackDisplay,
}) => {
export const LaunchDarklyProvider: React.FC<PropsWithChildren<{}>> = ({ children }) => {
const LDProvider = useRef<GenericLDProvider>(DEFAULT_GENERIC_PROVIDER);
const [isLDReady, setIsLDReady] = useState<boolean>(false);

const authContext = useAuthContext();
if (!authContext) {
Expand Down Expand Up @@ -68,21 +58,11 @@ export const LaunchDarklyProvider: React.FC<PropsWithChildren<ILaunchDarklyProvi
});
} catch (err: unknown) {
Sentry.captureException(err);
} finally {
setIsLDReady(true);
}
};

fetchLDProvider();
}, [setIsLDReady, shouldInitializeLd, currentOrganization]);

/**
* For self-hosted, LD will not be enabled, so do not block initialization.
* Must not show the fallback if the user isn't logged-in to avoid issues with un-authenticated routes (i.e. login).
*/
if (selectShouldShowLaunchDarklyFallback(authContext, isLDReady)) {
return <>{fallbackDisplay}</>;
}
}, [shouldInitializeLd, currentOrganization]);

return (
<LDProvider.current>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { selectHasUserCompletedSignUp, UserContext } from '@novu/shared-web';
import { UserContext } from '@novu/shared-web';
import { checkShouldUseLaunchDarkly } from '@novu/shared-web';

/** Determine if LaunchDarkly should be initialized based on the current auth context */
export function selectShouldInitializeLaunchDarkly(userCtx: UserContext): boolean {
const { isLoggedIn, currentOrganization } = userCtx;
const { isLoggedIn, currentUser, currentOrganization } = userCtx;
// don't show fallback if LaunchDarkly isn't enabled
if (!checkShouldUseLaunchDarkly()) {
return false;
Expand All @@ -22,7 +22,7 @@ export function selectShouldInitializeLaunchDarkly(userCtx: UserContext): boolea
* have an organizationId yet that we can use for org-based feature flags. To prevent from blocking this page
* from loading during this "limbo" state, we should initialize LD with the anonymous context.
*/
if (!selectHasUserCompletedSignUp(userCtx)) {
if (!currentUser?.organizationId) {
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Available through user context now.

}

Expand Down

This file was deleted.

6 changes: 3 additions & 3 deletions apps/web/src/components/layout/AppLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { HeaderNav } from './components/HeaderNav';
import { SideNav } from './components/SideNav';
import { IntercomProvider } from 'react-use-intercom';
import { INTERCOM_APP_ID } from '../../config';
import { RequiredAuth } from './RequiredAuth';
import { EnsureOnboardingComplete } from './EnsureOnboardingComplete';
import { SpotLight } from '../utils/Spotlight';
import { SpotLightProvider } from '../providers/SpotlightProvider';
import { useFeatureFlag } from '@novu/shared-web';
Expand Down Expand Up @@ -37,7 +37,7 @@ export function AppLayout() {
const isInformationArchitectureEnabled = useFeatureFlag(FeatureFlagsKeysEnum.IS_INFORMATION_ARCHITECTURE_ENABLED);

return (
<RequiredAuth>
<EnsureOnboardingComplete>
<SpotLightProvider>
<ThemeProvider>
<IntercomProvider
Expand Down Expand Up @@ -79,6 +79,6 @@ export function AppLayout() {
</IntercomProvider>
</ThemeProvider>
</SpotLightProvider>
</RequiredAuth>
</EnsureOnboardingComplete>
);
}
15 changes: 15 additions & 0 deletions apps/web/src/components/layout/EnsureOnboardingComplete.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { Navigate, useLocation } from 'react-router-dom';
import { ROUTES } from '../../constants/routes.enum';
import { useBlueprint, useAuthController } from '../../hooks/index';

export function EnsureOnboardingComplete({ children }: any) {
useBlueprint();
const location = useLocation();
const { user } = useAuthController();

if ((!user?.organizationId || !user?.environmentId) && location.pathname !== ROUTES.AUTH_APPLICATION) {
return <Navigate to={ROUTES.AUTH_APPLICATION} replace />;
} else {
return children;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file's contents came from RequiredAuth, and is heavily simplified (expand diff below). The useBlueprint hook call here is uncomfortable, but I've left it to avoid further scope in this PR.

46 changes: 0 additions & 46 deletions apps/web/src/components/layout/RequiredAuth.tsx

This file was deleted.

4 changes: 2 additions & 2 deletions apps/web/src/hooks/useAuthController.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
import { useAuthController, applyToken, getTokenPayload, getToken } from '@novu/shared-web';
import { useAuthController, applyToken } from '@novu/shared-web';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { useAuthController, applyToken } from '@novu/shared-web';
export { useAuthController, applyToken } from '@novu/shared-web';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a bunch of these import/re-export patterns for @novu/shared-web, I'll suggest we handle them separately in batch.

export { useAuthController, applyToken, getTokenPayload, getToken };
export { useAuthController, applyToken };
6 changes: 3 additions & 3 deletions apps/web/src/hooks/useBlueprint.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { useNavigate, useLocation, useSearchParams } from 'react-router-dom';
import { useEffect } from 'react';

import { getToken } from './useAuthController';
import { useAuthController } from './useAuthController';
import { useSegment } from '../components/providers/SegmentProvider';
import { ROUTES } from '../constants/routes.enum';

Expand All @@ -12,10 +12,10 @@ export const useBlueprint = () => {
const { pathname } = useLocation();
const segment = useSegment();
const id = localStorage.getItem('blueprintId');
const token = getToken();
const { token } = useAuthController();

useEffect(() => {
if (id && token !== null) {
if (id && !!token) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From here on, we'll see several files be updated to pull from the auth controller, this ensures only the auth controller deals with surfacing auth concerns as the source of truth.

navigate(ROUTES.WORKFLOWS_CREATE, {
replace: true,
});
Expand Down
4 changes: 1 addition & 3 deletions apps/web/src/hooks/useVercelIntegration.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import axios from 'axios';
import { useCallback } from 'react';
import { useMutation } from '@tanstack/react-query';
import { useNavigate } from 'react-router-dom';
Expand All @@ -11,11 +10,10 @@ import { vercelIntegrationSetup } from '../api/vercel-integration';
export function useVercelIntegration() {
const { token } = useAuthContext();
const isLoggedIn = !!token;
const isAxiosAuthorized = axios.defaults.headers.common.Authorization;

const { code, next, configurationId } = useVercelParams();

const canStartSetup = Boolean(code && next && isLoggedIn && isAxiosAuthorized);
const canStartSetup = Boolean(code && next && isLoggedIn);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isLoggedIn takes care of authorization concerns by itself.

const navigate = useNavigate();

Expand Down
5 changes: 0 additions & 5 deletions apps/web/src/initializeApp.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { library } from '@fortawesome/fontawesome-svg-core';
import { far } from '@fortawesome/free-regular-svg-icons';
import { fas } from '@fortawesome/free-solid-svg-icons';
import { applyToken, getToken } from '@novu/shared-web';
import * as Sentry from '@sentry/react';
import { Integrations } from '@sentry/tracing';
import { ENV, SENTRY_DSN } from './config';
Expand Down Expand Up @@ -57,8 +56,4 @@ export const initializeApp = () => {
},
});
}

const tokenStoredToken: string = getToken();

applyToken(tokenStoredToken);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are no longer using the defaults config of Axios, so this is no longer required during initialization.

};
8 changes: 2 additions & 6 deletions apps/web/src/pages/auth/LoginPage.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { useEffect } from 'react';
import { useNavigate, useSearchParams } from 'react-router-dom';
import jwtDecode from 'jwt-decode';
import { IJwtPayload } from '@novu/shared';

import { useAuthContext } from '../../components/providers/AuthProvider';
import { LoginForm } from './components/LoginForm';
Expand All @@ -15,7 +13,7 @@ import { ROUTES } from '../../constants/routes.enum';

export default function LoginPage() {
useBlueprint();
const { setToken, token: oldToken } = useAuthContext();
const { setToken, token: oldToken, currentUser } = useAuthContext();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decoded jwt params are available on currentUser now.

const segment = useSegment();
const navigate = useNavigate();
const [params] = useSearchParams();
Expand All @@ -31,9 +29,7 @@ export default function LoginPage() {

useEffect(() => {
if (token) {
const user = jwtDecode<IJwtPayload>(token);

if (!invitationToken && (!user.organizationId || !user.environmentId)) {
if (!invitationToken && currentUser?._id && (!currentUser?.organizationId || !currentUser?.environmentId)) {
Copy link
Contributor Author

@rifont rifont May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional check here for currentUser?._id ensures that the User is updated in Auth Context (it's null by default) before a redirection can take place, otherwise the page will flicker to ROUTES.AUTH_APPLICATION when loading.

const authApplicationLink = isFromVercel
? `${ROUTES.AUTH_APPLICATION}?code=${code}&next=${next}`
: ROUTES.AUTH_APPLICATION;
Expand Down
Loading
Loading