From 7b363d4624ef2e9b83705b1cd2b37008fe96152f Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Wed, 21 May 2025 16:19:34 +0200 Subject: [PATCH 1/6] =?UTF-8?q?=E2=9A=A1=EF=B8=8F(frontend)=20prevent=20au?= =?UTF-8?q?thentication=20retry=20on=20401=20responses?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stop retry attempts when receiving 401 Unauthorized from /me endpoint since this clearly indicates authentication status. The original purpose of the /me call is simply to determine if user is authenticated, and a 401 provides sufficient information. Prevents unnecessary network requests caused by React Query's automatic retry behavior when re-raising exceptions, which was hiding the 401 status. Improves performance and reduces server load during authentication failures. --- src/frontend/apps/impress/src/core/AppProvider.tsx | 4 +++- .../apps/impress/src/features/auth/api/useAuthQuery.tsx | 9 +++++++++ .../features/language/hooks/useLanguageSynchronizer.ts | 2 +- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/frontend/apps/impress/src/core/AppProvider.tsx b/src/frontend/apps/impress/src/core/AppProvider.tsx index 03ce5097d..9f18e1142 100644 --- a/src/frontend/apps/impress/src/core/AppProvider.tsx +++ b/src/frontend/apps/impress/src/core/AppProvider.tsx @@ -9,6 +9,8 @@ import { useResponsiveStore } from '@/stores/'; import { ConfigProvider } from './config/'; +export const DEFAULT_QUERY_RETRY = 1; + /** * QueryClient: * - defaultOptions: @@ -19,7 +21,7 @@ import { ConfigProvider } from './config/'; const defaultOptions = { queries: { staleTime: 1000 * 60 * 3, - retry: 1, + retry: DEFAULT_QUERY_RETRY, }, }; const queryClient = new QueryClient({ diff --git a/src/frontend/apps/impress/src/features/auth/api/useAuthQuery.tsx b/src/frontend/apps/impress/src/features/auth/api/useAuthQuery.tsx index 026beec9f..4c15e39ce 100644 --- a/src/frontend/apps/impress/src/features/auth/api/useAuthQuery.tsx +++ b/src/frontend/apps/impress/src/features/auth/api/useAuthQuery.tsx @@ -1,6 +1,7 @@ import { UseQueryOptions, useQuery } from '@tanstack/react-query'; import { APIError, errorCauses, fetchAPI } from '@/api'; +import { DEFAULT_QUERY_RETRY } from '@/core'; import { User } from './types'; @@ -16,6 +17,7 @@ import { User } from './types'; */ export const getMe = async (): Promise => { const response = await fetchAPI(`users/me/`); + if (!response.ok) { throw new APIError( `Couldn't fetch user data: ${response.statusText}`, @@ -34,6 +36,13 @@ export function useAuthQuery( queryKey: [KEY_AUTH], queryFn: getMe, staleTime: 1000 * 60 * 15, // 15 minutes + retry: (failureCount, error) => { + // we assume that a 401 means the user is not logged in + if (error.status == 401) { + return false; + } + return failureCount < DEFAULT_QUERY_RETRY; + }, ...queryConfig, }); } diff --git a/src/frontend/apps/impress/src/features/language/hooks/useLanguageSynchronizer.ts b/src/frontend/apps/impress/src/features/language/hooks/useLanguageSynchronizer.ts index e6bb23b99..7ecd2f3ad 100644 --- a/src/frontend/apps/impress/src/features/language/hooks/useLanguageSynchronizer.ts +++ b/src/frontend/apps/impress/src/features/language/hooks/useLanguageSynchronizer.ts @@ -2,7 +2,7 @@ import { useCallback, useMemo, useRef } from 'react'; import { useTranslation } from 'react-i18next'; import { useConfig } from '@/core'; -import { useAuthQuery } from '@/features/auth/api'; +import { useAuthQuery } from '@/features/auth'; import { useChangeUserLanguage } from '@/features/language/api/useChangeUserLanguage'; import { getMatchingLocales } from '@/features/language/utils/locale'; import { availableFrontendLanguages } from '@/i18n/initI18n'; From 4f3e0ef5b30f9165ad0cb93aa752718968d146b6 Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Wed, 21 May 2025 18:55:27 +0200 Subject: [PATCH 2/6] =?UTF-8?q?=E2=9A=A1=EF=B8=8F(frontend)=20implement=20?= =?UTF-8?q?Mozilla=20Django=20OIDC=20returnTo=20for=20login=20flow?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use returnTo capability to pass target URL during authentication, eliminating redundant renders when returning to app. Work around Next.js router limitations (can't pass data when replacing URL) by adding temporary target parameter. Discovered issue: 401 responses on document fetch trigger retry loop instead of redirecting to login. Will address in subsequent commits. --- src/backend/impress/settings.py | 3 +++ .../apps/impress/src/core/AppProvider.tsx | 7 ++--- .../src/features/auth/components/Auth.tsx | 18 +------------ .../apps/impress/src/features/auth/conf.ts | 1 - .../apps/impress/src/features/auth/utils.ts | 26 ++++--------------- src/frontend/apps/impress/src/pages/401.tsx | 17 +++++++++--- .../impress/src/pages/docs/[id]/index.tsx | 7 ++--- 7 files changed, 31 insertions(+), 48 deletions(-) diff --git a/src/backend/impress/settings.py b/src/backend/impress/settings.py index 571d7052d..ddfedd892 100755 --- a/src/backend/impress/settings.py +++ b/src/backend/impress/settings.py @@ -551,6 +551,9 @@ class Base(Configuration): environ_name="OIDC_STORE_REFRESH_TOKEN_KEY", environ_prefix=None, ) + OIDC_REDIRECT_FIELD_NAME = values.Value( + "returnTo", environ_name="OIDC_REDIRECT_FIELD_NAME", environ_prefix=None + ) # WARNING: Enabling this setting allows multiple user accounts to share the same email # address. This may cause security issues and is not recommended for production use when diff --git a/src/frontend/apps/impress/src/core/AppProvider.tsx b/src/frontend/apps/impress/src/core/AppProvider.tsx index 9f18e1142..49804419f 100644 --- a/src/frontend/apps/impress/src/core/AppProvider.tsx +++ b/src/frontend/apps/impress/src/core/AppProvider.tsx @@ -4,7 +4,7 @@ import { useRouter } from 'next/router'; import { useEffect } from 'react'; import { useCunninghamTheme } from '@/cunningham'; -import { Auth, KEY_AUTH, setAuthUrl } from '@/features/auth'; +import { Auth, KEY_AUTH } from '@/features/auth'; import { useResponsiveStore } from '@/stores/'; import { ConfigProvider } from './config/'; @@ -53,8 +53,9 @@ export function AppProvider({ children }: { children: React.ReactNode }) { void queryClient.resetQueries({ queryKey: [KEY_AUTH], }); - setAuthUrl(); - void replace(`/401`); + void replace( + `/401?returnTo=${encodeURIComponent(window.location.pathname)}`, + ); } }, }, diff --git a/src/frontend/apps/impress/src/features/auth/components/Auth.tsx b/src/frontend/apps/impress/src/features/auth/components/Auth.tsx index addb481b3..84800d877 100644 --- a/src/frontend/apps/impress/src/features/auth/components/Auth.tsx +++ b/src/frontend/apps/impress/src/features/auth/components/Auth.tsx @@ -7,7 +7,7 @@ import { useConfig } from '@/core'; import { HOME_URL } from '../conf'; import { useAuth } from '../hooks'; -import { getAuthUrl, gotoLogin } from '../utils'; +import { gotoLogin } from '../utils'; export const Auth = ({ children }: PropsWithChildren) => { const { isLoading, pathAllowed, isFetchedAfterMount, authenticated } = @@ -23,22 +23,6 @@ export const Auth = ({ children }: PropsWithChildren) => { ); } - /** - * If the user is authenticated and wanted initially to access a document, - * we redirect to the document page. - */ - if (authenticated) { - const authUrl = getAuthUrl(); - if (authUrl) { - void replace(authUrl); - return ( - - - - ); - } - } - /** * If the user is not authenticated and the path is not allowed, we redirect to the login page. */ diff --git a/src/frontend/apps/impress/src/features/auth/conf.ts b/src/frontend/apps/impress/src/features/auth/conf.ts index c44fe0188..024c07dd5 100644 --- a/src/frontend/apps/impress/src/features/auth/conf.ts +++ b/src/frontend/apps/impress/src/features/auth/conf.ts @@ -3,4 +3,3 @@ import { baseApiUrl } from '@/api'; export const HOME_URL = '/home'; export const LOGIN_URL = `${baseApiUrl()}authenticate/`; export const LOGOUT_URL = `${baseApiUrl()}logout/`; -export const PATH_AUTH_LOCAL_STORAGE = 'docs-path-auth'; diff --git a/src/frontend/apps/impress/src/features/auth/utils.ts b/src/frontend/apps/impress/src/features/auth/utils.ts index 41d50cf01..806a75652 100644 --- a/src/frontend/apps/impress/src/features/auth/utils.ts +++ b/src/frontend/apps/impress/src/features/auth/utils.ts @@ -1,27 +1,11 @@ +import { backendUrl } from '@/api'; import { terminateCrispSession } from '@/services/Crisp'; -import { LOGIN_URL, LOGOUT_URL, PATH_AUTH_LOCAL_STORAGE } from './conf'; +import { LOGIN_URL, LOGOUT_URL } from './conf'; -export const getAuthUrl = () => { - const path_auth = localStorage.getItem(PATH_AUTH_LOCAL_STORAGE); - if (path_auth) { - localStorage.removeItem(PATH_AUTH_LOCAL_STORAGE); - return path_auth; - } -}; - -export const setAuthUrl = () => { - if (window.location.pathname !== '/') { - localStorage.setItem(PATH_AUTH_LOCAL_STORAGE, window.location.pathname); - } -}; - -export const gotoLogin = (withRedirect = true) => { - if (withRedirect) { - setAuthUrl(); - } - - window.location.replace(LOGIN_URL); +export const gotoLogin = (returnTo = '/') => { + const authenticateUrl = LOGIN_URL + `?returnTo=${backendUrl() + returnTo}`; + window.location.replace(authenticateUrl); }; export const gotoLogout = () => { diff --git a/src/frontend/apps/impress/src/pages/401.tsx b/src/frontend/apps/impress/src/pages/401.tsx index 995bcbe55..1ebf06efa 100644 --- a/src/frontend/apps/impress/src/pages/401.tsx +++ b/src/frontend/apps/impress/src/pages/401.tsx @@ -1,7 +1,7 @@ import { Button } from '@openfun/cunningham-react'; import Image from 'next/image'; import { useRouter } from 'next/router'; -import { ReactElement, useEffect } from 'react'; +import { ReactElement, useEffect, useState } from 'react'; import { useTranslation } from 'react-i18next'; import img401 from '@/assets/icons/icon-401.png'; @@ -13,7 +13,18 @@ import { NextPageWithLayout } from '@/types/next'; const Page: NextPageWithLayout = () => { const { t } = useTranslation(); const { authenticated } = useAuth(); - const { replace } = useRouter(); + const router = useRouter(); + const { replace } = router; + + const [returnTo, setReturnTo] = useState(undefined); + const { returnTo: returnToParams } = router.query; + + useEffect(() => { + if (returnToParams) { + setReturnTo(returnToParams as string); + void replace('/401'); + } + }, [returnToParams, replace]); useEffect(() => { if (authenticated) { @@ -42,7 +53,7 @@ const Page: NextPageWithLayout = () => { {t('Log in to access the document.')} - diff --git a/src/frontend/apps/impress/src/pages/docs/[id]/index.tsx b/src/frontend/apps/impress/src/pages/docs/[id]/index.tsx index 295672436..01aae751d 100644 --- a/src/frontend/apps/impress/src/pages/docs/[id]/index.tsx +++ b/src/frontend/apps/impress/src/pages/docs/[id]/index.tsx @@ -14,7 +14,7 @@ import { useDoc, useDocStore, } from '@/docs/doc-management/'; -import { KEY_AUTH, setAuthUrl } from '@/features/auth'; +import { KEY_AUTH } from '@/features/auth'; import { MainLayout } from '@/layouts'; import { useBroadcastStore } from '@/stores'; import { NextPageWithLayout } from '@/types/next'; @@ -115,8 +115,9 @@ const DocPage = ({ id }: DocProps) => { void queryClient.resetQueries({ queryKey: [KEY_AUTH], }); - setAuthUrl(); - void replace(`/401`); + void replace( + `/401?returnTo=${encodeURIComponent(window.location.pathname)}`, + ); return null; } From f66a8e3f1dad11f1fe8c517ab0d648abdca630d7 Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Wed, 21 May 2025 19:07:29 +0200 Subject: [PATCH 3/6] =?UTF-8?q?=F0=9F=94=A5(frontend)=20delete=20unused=20?= =?UTF-8?q?variable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove variable that is no longer referenced in the codebase to improve code cleanliness and reduce potential confusion. --- .../apps/impress/src/features/docs/doc-management/api/useDoc.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/api/useDoc.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/api/useDoc.tsx index ebbb1d543..9aeb9bdad 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/api/useDoc.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-management/api/useDoc.tsx @@ -19,7 +19,6 @@ export const getDoc = async ({ id }: DocParams): Promise => { }; export const KEY_DOC = 'doc'; -export const KEY_DOC_VISIBILITY = 'doc-visibility'; export function useDoc( param: DocParams, From 4de26c331bd846c4b23901b548626be303e2e49e Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Thu, 22 May 2025 00:27:42 +0200 Subject: [PATCH 4/6] =?UTF-8?q?=E2=9A=A1=EF=B8=8F(frontend)=20optimize=20d?= =?UTF-8?q?ocument=20fetch=20error=20handling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reduce unnecessary fetch requests when retrieving documents with permission or authentication issues. Previous implementation was triggering multiple document requests despite having sufficient error information from initial attempt to determine appropriate user redirection. Additionally, fix issue where resetting the auth cache was triggering redundant authentication verification requests. The responsibility for checking auth status should belong to the 401 page component on mount, rather than being triggered by cache resets during error handling. Known limitations: - Not waiting for async function completion makes code harder to maintain - Added loading spinner as temporary solution to prevent UI flicker - Future improvement should implement consistent error-based redirects rather than rendering error messages directly on document page --- .../impress/src/pages/docs/[id]/index.tsx | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/frontend/apps/impress/src/pages/docs/[id]/index.tsx b/src/frontend/apps/impress/src/pages/docs/[id]/index.tsx index 01aae751d..8aa0090de 100644 --- a/src/frontend/apps/impress/src/pages/docs/[id]/index.tsx +++ b/src/frontend/apps/impress/src/pages/docs/[id]/index.tsx @@ -6,6 +6,7 @@ import { useEffect, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { Box, Icon, TextErrors } from '@/components'; +import { DEFAULT_QUERY_RETRY } from '@/core'; import { DocEditor } from '@/docs/doc-editor'; import { Doc, @@ -14,7 +15,6 @@ import { useDoc, useDocStore, } from '@/docs/doc-management/'; -import { KEY_AUTH } from '@/features/auth'; import { MainLayout } from '@/layouts'; import { useBroadcastStore } from '@/stores'; import { NextPageWithLayout } from '@/types/next'; @@ -56,6 +56,14 @@ const DocPage = ({ id }: DocProps) => { { staleTime: 0, queryKey: [KEY_DOC, { id }], + retryDelay: 1000, + retry: (failureCount, error) => { + if (error.status == 403 || error.status == 401 || error.status == 404) { + return false; + } else { + return failureCount < DEFAULT_QUERY_RETRY; + } + }, }, ); @@ -101,24 +109,17 @@ const DocPage = ({ id }: DocProps) => { }, [addTask, doc?.id, queryClient]); if (isError && error) { - if (error.status === 403) { - void replace(`/403`); - return null; - } - - if (error.status === 404) { - void replace(`/404`); - return null; - } - - if (error.status === 401) { - void queryClient.resetQueries({ - queryKey: [KEY_AUTH], - }); + if ([403, 404, 401].includes(error.status)) { void replace( - `/401?returnTo=${encodeURIComponent(window.location.pathname)}`, + error.status === 401 + ? `/${error.status}?returnTo=${encodeURIComponent(window.location.pathname)}` + : `/${error.status}`, + ); + return ( + + + ); - return null; } return ( From ec663b601fb1f25db33fbecd25b1e3a75596f763 Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Thu, 22 May 2025 01:04:56 +0200 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=9A=B8(auth)=20implement=20first=20dr?= =?UTF-8?q?aft=20of=20silent=20login=20with=20prompt=3DNone?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add initial naive implementation of silent login functionality using prompt=None parameter to request OIDC provider to skip login screen and return proper HTTP error response instead. --- src/backend/impress/settings.py | 2 ++ .../src/features/auth/api/useAuthQuery.tsx | 13 +++++++ .../impress/src/features/auth/silentLogin.ts | 35 +++++++++++++++++++ .../apps/impress/src/features/auth/utils.ts | 7 ++-- 4 files changed, 54 insertions(+), 3 deletions(-) create mode 100644 src/frontend/apps/impress/src/features/auth/silentLogin.ts diff --git a/src/backend/impress/settings.py b/src/backend/impress/settings.py index ddfedd892..815c2d8d3 100755 --- a/src/backend/impress/settings.py +++ b/src/backend/impress/settings.py @@ -467,6 +467,8 @@ class Base(Configuration): ) # OIDC - Authorization Code Flow + OIDC_AUTHENTICATE_CLASS = "lasuite.oidc_login.views.OIDCAuthenticationRequestView" + OIDC_CALLBACK_CLASS = "lasuite.oidc_login.views.OIDCAuthenticationCallbackView" OIDC_CREATE_USER = values.BooleanValue( default=True, environ_name="OIDC_CREATE_USER", diff --git a/src/frontend/apps/impress/src/features/auth/api/useAuthQuery.tsx b/src/frontend/apps/impress/src/features/auth/api/useAuthQuery.tsx index 4c15e39ce..90a87f48c 100644 --- a/src/frontend/apps/impress/src/features/auth/api/useAuthQuery.tsx +++ b/src/frontend/apps/impress/src/features/auth/api/useAuthQuery.tsx @@ -2,6 +2,10 @@ import { UseQueryOptions, useQuery } from '@tanstack/react-query'; import { APIError, errorCauses, fetchAPI } from '@/api'; import { DEFAULT_QUERY_RETRY } from '@/core'; +import { + attemptSilentLogin, + canAttemptSilentLogin, +} from '@/features/auth/silentLogin'; import { User } from './types'; @@ -18,6 +22,15 @@ import { User } from './types'; export const getMe = async (): Promise => { const response = await fetchAPI(`users/me/`); + if (!response.ok && response.status == 401 && canAttemptSilentLogin()) { + const currentLocation = window.location.href; + attemptSilentLogin(3600); + + while (window.location.href === currentLocation) { + await new Promise((resolve) => setTimeout(resolve, 100)); + } + } + if (!response.ok) { throw new APIError( `Couldn't fetch user data: ${response.statusText}`, diff --git a/src/frontend/apps/impress/src/features/auth/silentLogin.ts b/src/frontend/apps/impress/src/features/auth/silentLogin.ts new file mode 100644 index 000000000..2af78c3ba --- /dev/null +++ b/src/frontend/apps/impress/src/features/auth/silentLogin.ts @@ -0,0 +1,35 @@ +import { gotoLogin } from '@/features/auth'; + +const SILENT_LOGIN_RETRY_KEY = 'silent-login-retry'; + +const isRetryAllowed = () => { + const lastRetryDate = localStorage.getItem(SILENT_LOGIN_RETRY_KEY); + if (!lastRetryDate) { + return true; + } + const now = new Date(); + return now.getTime() > Number(lastRetryDate); +}; + +const setNextRetryTime = (retryIntervalInSeconds: number) => { + const now = new Date(); + const nextRetryTime = now.getTime() + retryIntervalInSeconds * 1000; + localStorage.setItem(SILENT_LOGIN_RETRY_KEY, String(nextRetryTime)); +}; + +const initiateSilentLogin = () => { + const currentPath = window.location.pathname; + gotoLogin(currentPath, true); +}; + +export const canAttemptSilentLogin = () => { + return isRetryAllowed(); +}; + +export const attemptSilentLogin = (retryIntervalInSeconds: number) => { + if (!isRetryAllowed()) { + return; + } + setNextRetryTime(retryIntervalInSeconds); + initiateSilentLogin(); +}; diff --git a/src/frontend/apps/impress/src/features/auth/utils.ts b/src/frontend/apps/impress/src/features/auth/utils.ts index 806a75652..77f0605c7 100644 --- a/src/frontend/apps/impress/src/features/auth/utils.ts +++ b/src/frontend/apps/impress/src/features/auth/utils.ts @@ -1,10 +1,11 @@ -import { backendUrl } from '@/api'; import { terminateCrispSession } from '@/services/Crisp'; import { LOGIN_URL, LOGOUT_URL } from './conf'; -export const gotoLogin = (returnTo = '/') => { - const authenticateUrl = LOGIN_URL + `?returnTo=${backendUrl() + returnTo}`; +export const gotoLogin = (returnTo = '/', isSilent = false) => { + const authenticateUrl = + LOGIN_URL + + `?silent=${encodeURIComponent(isSilent)}&returnTo=${window.location.origin + returnTo}`; window.location.replace(authenticateUrl); }; From 4cecfec0b71ce63110d029d0be81030c29d701dd Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Sat, 24 May 2025 23:52:05 +0200 Subject: [PATCH 6/6] =?UTF-8?q?=F0=9F=A9=B9(frontend)=20correct=20misleadi?= =?UTF-8?q?ng=20error=20message=20for=20config=20fetch=20failure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update APIError message that incorrectly mentioned document when configuration fetching fails. --- src/frontend/apps/impress/src/core/config/api/useConfig.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/frontend/apps/impress/src/core/config/api/useConfig.tsx b/src/frontend/apps/impress/src/core/config/api/useConfig.tsx index fdfe8c97e..b245d1b91 100644 --- a/src/frontend/apps/impress/src/core/config/api/useConfig.tsx +++ b/src/frontend/apps/impress/src/core/config/api/useConfig.tsx @@ -38,7 +38,10 @@ export const getConfig = async (): Promise => { const response = await fetchAPI(`config/`); if (!response.ok) { - throw new APIError('Failed to get the doc', await errorCauses(response)); + throw new APIError( + 'Failed to get the configurations', + await errorCauses(response), + ); } const config = response.json() as Promise;