-
Notifications
You must be signed in to change notification settings - Fork 406
Centralize Authentication #9593
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: master
Are you sure you want to change the base?
Changes from 144 commits
11519b2
9ef2f9d
69a0025
626debf
4bca6ab
4796e8b
d8368f3
0ff7e8d
3352594
95b9d5a
5d881bc
ca2f40d
46038ea
6f930a2
e3a698d
a61eb21
b8260b5
f33070a
5cd834e
2ced21e
c13ade2
2cfcb92
1cbbcf2
13a8f05
467380e
c3a9aa1
708aa7e
df0e3d3
5cff9a9
3784285
b64d6c9
9b354b6
0ee5fdf
3727dbc
f660e5b
af7cbc0
ba0c84e
653134b
24664d9
505d523
4d859f9
b520906
48f871b
3e18e3a
107ce94
d225ca5
86237cf
4f5679e
2b700e5
1f03d70
a407732
b6b4ee8
a8f7a87
047876d
b954953
200ebfc
96dbec9
bb29497
addbe21
ecfc0ec
6410f9b
b5fc52c
05e8336
1a6773e
10c074b
d6a3cca
2a14c15
b32a916
6a2193b
1f3b538
55d22a7
c96a60b
f36cdcf
595a4ab
9ed12da
1a8074e
1593245
a561a28
a19849a
a1188ba
b021638
7e90052
e89c4c9
90f9f74
d5f1bf0
ef59744
2d586a7
f69d2fe
2052bec
5ebe478
02bc69d
9c3e1a0
221418a
22ed250
7dd0fce
bc11b5a
c0ccc4e
3fbc0d9
46b8b55
f51d9a5
973e1de
1996fa2
c006d22
5a0adaf
ce93a8b
3fbe976
4617168
f8a68c6
f2482e3
1d7ef86
6d73424
3a3fb5c
ee4b785
08bd31a
c0b6608
24de77b
0252da3
3bf75af
f161ee7
483df71
3c52738
82ec737
ea8e1db
2319d8a
aef4d21
31855dd
c8ab095
f028b0b
c7de7fc
521f459
c7c75e7
3492a62
9ebeeb0
b54602d
4854532
6ddf7d5
c0d46fc
92dfe09
b3675eb
2ce217a
474adc6
64cc7de
4702fd5
ab7efcb
6bc2e24
dbb9341
d0ecd0b
95ccf52
c39f938
917f6ae
91eef37
717bf67
9c787c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,89 @@ | ||||||||||||||||||
| import React, {createContext, useContext, useMemo, useState, ReactNode, useCallback, useEffect} from "react"; | ||||||||||||||||||
| import { useNavigate } from "react-router-dom"; | ||||||||||||||||||
| import { auth } from "../api"; | ||||||||||||||||||
| import {getCurrentRelativeUrl, isPublicAuthRoute, ROUTES} from "../utils"; | ||||||||||||||||||
|
||||||||||||||||||
| import React, {createContext, useContext, useMemo, useState, ReactNode, useCallback, useEffect} from "react"; | |
| import { useNavigate } from "react-router-dom"; | |
| import { auth } from "../api"; | |
| import {getCurrentRelativeUrl, isPublicAuthRoute, ROUTES} from "../utils"; | |
| import React, { createContext, useContext, useMemo, useState, ReactNode, useCallback, useEffect } from "react"; | |
| import { useNavigate } from "react-router-dom"; | |
| import { auth } from "../api"; | |
| import { getCurrentRelativeUrl, isPublicAuthRoute, ROUTES } from "../utils"; |
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.
Can you add a comment that explains this code?
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 idea is that refreshUser can optionally receive an object of options, currently only { useCache?: boolean }.
refreshUser is an async method to reload the current user state from the API.
It accepts an optional options object (currently { useCache?: boolean }),
allowing callers to control whether to use cached data or force a fresh fetch.
Outdated
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.
Why onUnauthorized and not onUnauthenticated?
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.
Why aren’t you wrapping await auth.getCurrentUserWithCache() with useAPI?
Also, related to this, if you’re recalculating this while the user is in a loading state, shouldn’t you handle it by setting setStatus(AUTH_STATUS.PENDING)?
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.
I decided not to use useAPI here because the AuthContext runs outside the component lifecycle - we call refreshUser on app startup, after login/logout, and on pageshow events.
Using useAPI here would introduce redundant redirects and implicit 401 handling that conflict with onUnauthorized.
Regarding PENDING, I will add a selective state update: it will set only when performing a cold refresh (useCache: false) and the user isn't already authenticated.
This will avoid flicker during background updates while still providing a proper loading state.
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.
why is this needed here?
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.
This callback centralizes the "unauthorized" handling logic for the app.
It's used whenever the backend returns a 401 or when the user session becomes invalid.
Calling auth.clearCurrentUser() here ensures that any cached user info is cleared immediately, before redirecting to the login page.
Having this as a dedicated onUnauthorized function makes the flow reusable - we can call it both from API hooks and from other parts of the app that need to handle session expiration consistently.
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.
Why redirect to ROUTES.LOGIN instead of to the root, which already redirects to the login page?
Also, shouldn’t the /logout endpoints handle this redirection already?
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.
We redirect explicitly to ROUTES.LOGIN instead of / for a few reasons:
onUnauthorized is triggered by 401s from XHR/fetch calls anywhere in the app.
Redirects returned by the /logout endpoint don't affect SPA navigation (the browser will not change window.location), so the client must navigate explicitly.
Going directly to /auth/login avoids an extra hop (root → login) and reduces flicker.
It also guarantees we keep the intended next target and doesn't rely on whatever logic the root route currently implements.
We keep the isPublicAuthRoute guard to avoid redirect loops if we are already on the login/oidc/sso paths.
TL;DR: direct, deterministic navigation to the login page is safer and smoother here.
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.
Can this happen?
Isn't AuthContext initialized no matter what?
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.
It can happen if a component using useAuth() is rendered outside the <AuthProvider>, for example in a storybook, or future refactors where a subtree is mounted separately.
The guard makes the failure explicit and easy to debug, instead of causing null errors later.
So it's a safety net, it should never trigger in production, but it's still valuable during development.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,41 +1,30 @@ | ||
| import React, { FC, useContext, useState, useEffect } from "react"; | ||
| import { Outlet, useOutletContext } from "react-router-dom"; | ||
| import React, { FC, useContext, useEffect } from "react"; | ||
| import { Outlet } from "react-router-dom"; | ||
| import { ConfigProvider } from "../hooks/configProvider"; | ||
| import TopNav from './navbar'; | ||
| import { AppContext } from "../hooks/appContext"; | ||
| import useUser from "../hooks/user"; | ||
| import {AUTH_STATUS, useAuth} from "../auth/authContext"; | ||
|
|
||
| type LayoutOutletContext = [boolean]; | ||
| const Layout: FC = () => { | ||
| const { status } = useAuth(); | ||
| const showTopNav = status === AUTH_STATUS.AUTHENTICATED; | ||
|
|
||
| const Layout: FC<{logged: boolean}> = ({logged}) => { | ||
| const [isLogged, setIsLogged] = useState(logged ?? true); | ||
| const { user, loading, error } = useUser(); | ||
| const userWithId = user as { id?: string } | null; | ||
|
|
||
| // Update isLogged state based on actual authentication status | ||
| const { state } = useContext(AppContext); | ||
| useEffect(() => { | ||
| if (!loading) { | ||
| // If there's a user and no error, show authenticated (full) navbar | ||
| setIsLogged(!!userWithId?.id && !error); | ||
| } | ||
| }, [userWithId, loading, error]); | ||
|
|
||
| // handle global dark mode here | ||
| const {state} = useContext(AppContext); | ||
| document.documentElement.setAttribute('data-bs-theme', state.settings.darkMode ? 'dark' : 'light') | ||
| document.documentElement.setAttribute( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this is inside
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! |
||
| "data-bs-theme", | ||
| state.settings.darkMode ? "dark" : "light" | ||
| ); | ||
| }, [state.settings.darkMode]); | ||
|
|
||
| return ( | ||
| <ConfigProvider> | ||
| {!loading && <TopNav logged={isLogged}/>} | ||
| {showTopNav && <TopNav/>} | ||
| <div className="main-app"> | ||
| <Outlet context={[isLogged] satisfies LayoutOutletContext}/> | ||
| <Outlet /> | ||
| </div> | ||
| </ConfigProvider> | ||
| ); | ||
| }; | ||
|
|
||
| export function useLayoutOutletContext() { | ||
| return useOutletContext<LayoutOutletContext>(); | ||
| } | ||
|
|
||
| export default Layout; | ||
| export default Layout; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,4 @@ | ||
| import React from "react"; | ||
| import useUser from '../hooks/user' | ||
| import {auth} from "../api"; | ||
| import {useRouter} from "../hooks/router"; | ||
| import {Link} from "./nav"; | ||
| import DarkModeToggle from "./darkModeToggle"; | ||
|
|
@@ -9,15 +7,18 @@ import Container from "react-bootstrap/Container"; | |
| import {useLoginConfigContext} from "../hooks/conf"; | ||
| import {FeedPersonIcon} from "@primer/octicons-react"; | ||
| import {useConfigContext} from "../hooks/configProvider"; | ||
| import {auth} from "../api"; | ||
| import {AUTH_STATUS, useAuth} from "../auth/authContext"; | ||
|
|
||
| const NavUserInfo = () => { | ||
| const { user, loading: userLoading, error } = useUser(); | ||
| const { user, status } = useAuth(); | ||
| const userLoading = status === AUTH_STATUS.PENDING; | ||
| const logoutUrl = useLoginConfigContext()?.logout_url || "/logout" | ||
| const {config, error: versionError, loading: versionLoading} = useConfigContext(); | ||
| const versionConfig = config?.versionConfig || {}; | ||
|
|
||
| if (userLoading || versionLoading) return <Navbar.Text>Loading...</Navbar.Text>; | ||
| if (!user || !!error) return (<></>); | ||
| if (!user) return (<></>); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think that this is an example why await auth.getCurrentUserWithCache() should be wrapped in useAPI ,so errors can be returned and handled like before: if (!user || !!error) return (<></>);
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In our current setup, all authentication errors are handled centrally in The TL;DR: Auth errors are now global, not per-component - that's why this simplified check ( |
||
| const notifyNewVersion = !versionLoading && !versionError && versionConfig.upgrade_recommended | ||
| const NavBarTitle = () => { | ||
| return ( | ||
|
|
@@ -37,9 +38,9 @@ const NavUserInfo = () => { | |
| </> | ||
| </NavDropdown.Item><NavDropdown.Divider/></>} | ||
| <NavDropdown.Item | ||
| onClick={()=> { | ||
| onClick={() => { | ||
| auth.clearCurrentUser(); | ||
| window.location = logoutUrl; | ||
| window.location.replace(logoutUrl); | ||
| }}> | ||
| Logout | ||
| </NavDropdown.Item> | ||
|
|
@@ -63,8 +64,8 @@ const TopNavLink = ({ href, children }) => { | |
| ); | ||
| }; | ||
|
|
||
| const TopNav = ({logged = true}) => { | ||
| return logged && ( | ||
| const TopNav = () => { | ||
| return ( | ||
| <Navbar variant="dark" bg="dark" expand="md" className="border-bottom"> | ||
| <Container fluid={true}> | ||
| <Link component={Navbar.Brand} href="/"> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import React from "react"; | ||
| import {Navigate, Outlet} from "react-router-dom"; | ||
| import {Loading} from "./controls"; | ||
| import {ROUTES} from "../utils"; | ||
| import {getCurrentRelativeUrl} from "../utils"; | ||
| import {AUTH_STATUS, useAuth} from "../auth/authContext"; | ||
|
|
||
| const RequiresAuth: React.FC = () => { | ||
| const { user, status } = useAuth(); | ||
|
|
||
| if (status === AUTH_STATUS.PENDING) return <Loading />; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think thay when the user is recalculated, AUTH_STATUS.PENDING should be set in the meantime in authContext since ; should appear while loading |
||
| if (!user) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also handle !user.id , the user might exist, but a null id also means no authenticated user.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A missing |
||
| const next = getCurrentRelativeUrl(); | ||
| const params = new URLSearchParams({ redirected: "true", next }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why URLSearchParams gets { redirected: "true", next } as input and not location.search ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm explicitly creating a new Using This way we ensure that the login page only receives the parameters it actually expects: |
||
| return <Navigate to={{ pathname: ROUTES.LOGIN, search: `?${params.toString()}` }} replace state={{ redirected: true, next }}/>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use location.search instead of params.toString()?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used a new
|
||
| } | ||
|
|
||
| return <Outlet/>; | ||
| }; | ||
|
|
||
| export default RequiresAuth; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import {useEffect, useState} from 'react'; | ||
| import {AuthenticationError} from "../api"; | ||
| import {useRouter} from "./router"; | ||
| import {useAuth} from "../auth/authContext"; | ||
|
|
||
| const initialPaginationState = { | ||
| loading: true, | ||
|
|
@@ -50,46 +50,21 @@ const initialAPIState = { | |
| }; | ||
|
|
||
| export const useAPI = (promise, deps = []) => { | ||
| const router = useRouter(); | ||
| const [request, setRequest] = useState(initialAPIState); | ||
| const [needToLogin, setNeedToLogin] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| if (needToLogin) { | ||
| const loginPathname = '/auth/login'; | ||
| if (router.route === loginPathname) { | ||
| return; | ||
| } | ||
| // If the user is not logged in and attempts to access a lakeFS endpoint other than '/auth/login', | ||
| // they are first redirected to the '/auth/login' endpoint. For users logging in via lakeFS | ||
| // (not via SSO), after successful authentication they will be redirected back to the original endpoint | ||
| // they attempted to access. The redirected flag is set here so it can later be used to properly | ||
| // handle SSO redirection when login via SSO is configured. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should keep a comment (where it should be in the new version) explaining why the redirected flag is set to true, since it wasn’t clear to either Barak or me when we first saw it. |
||
| router.push({ | ||
| pathname: loginPathname, | ||
| query: {next: router.route, redirected: true}, | ||
| }); | ||
| setNeedToLogin(false); | ||
| } | ||
| }, [needToLogin, router]) | ||
| const { onUnauthorized } = useAuth(); | ||
|
|
||
| useEffect(() => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. f we’re changing this anyway, maybe it’s worth also adding the call to guard against updating state after the component unmounts? - WDYT
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| let isMounted = true; | ||
| setRequest(initialAPIState); | ||
| const execute = async () => { | ||
| try { | ||
| const response = await promise(); | ||
| setRequest({ | ||
| loading: false, | ||
| error: null, | ||
| response, | ||
| }); | ||
| if (!isMounted) return; | ||
| setRequest({ loading: false, error: null, response }); | ||
| } catch (error) { | ||
| if (error instanceof AuthenticationError) { | ||
| if (isMounted) { | ||
| setNeedToLogin(true); | ||
| } | ||
| return; | ||
| if (!isMounted) return; | ||
| if (error instanceof AuthenticationError && error.status === 401) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The AuthenticationError is the status code 401 isn't it? why did you add && error.status === 401?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Adding the explicit In short: it's a defensive check that prevents false positives and makes the logic more robust. |
||
| onUnauthorized(); | ||
| } | ||
| setRequest({ | ||
| loading: false, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,9 @@ | ||
| import React, { createContext, FC, useContext, useEffect, useState, } from "react"; | ||
| import React, {createContext, FC, useContext, useEffect, useMemo} from "react"; | ||
|
|
||
| import { config } from "../api"; | ||
| import useUser from "./user"; | ||
| import { usePluginManager } from "../../extendable/plugins/pluginsContext"; | ||
| import {useAPI} from "./api"; | ||
| import {useAuth} from "../auth/authContext"; | ||
|
|
||
| type ConfigContextType = { | ||
| error: Error | null; | ||
|
|
@@ -57,21 +58,23 @@ const useConfigContext = () => useContext(configContext); | |
|
|
||
| const ConfigProvider: FC<{children: React.ReactNode}> = ({children}) => { | ||
| const pluginManager = usePluginManager(); | ||
| const {user} = useUser(); | ||
| const [storageConfig, setConfig] = useState<ConfigContextType>(configInitialState); | ||
| const {user} = useAuth(); | ||
| const { response, loading, error } = useAPI(() => config.getConfig(), [user]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 |
||
|
|
||
| useEffect(() => { | ||
| config.getConfig() | ||
| .then(configData => { | ||
| pluginManager.customObjectRenderers?.init(configData); | ||
| setConfig({config: configData, loading: false, error: null}); | ||
| }) | ||
| .catch((error) => | ||
| setConfig({config: null, loading: false, error})); | ||
| }, [user]); | ||
| if (response) { | ||
| pluginManager.customObjectRenderers?.init(response); | ||
| } | ||
| }, [response, pluginManager]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| const value = useMemo( | ||
| () => ( | ||
| { config: response ?? null, loading, error } satisfies ConfigContextType | ||
| ), | ||
| [response, loading, error]); | ||
|
|
||
| return ( | ||
| <configContext.Provider value={storageConfig}> | ||
| <configContext.Provider value={value}> | ||
| {children} | ||
| </configContext.Provider> | ||
| ); | ||
|
|
||
|
This file was deleted.
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.
Nit: what's the style,
{aaa}or{ aaa }?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.
{ aaa }