-
Notifications
You must be signed in to change notification settings - Fork 404
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?
Conversation
… replace scattered authentication logic in the frontend.
…d `useAPI`, streamlining state management and simplifying configuration retrieval logic.
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.
Will review this soon, but before starting -
@Ben-El can you please describe how this was tested?
This change touches a lot of sensitive flows, in different system setups.
We should be very careful with QA and testing this change, so I'd be happy for more details about it.
Also, @Annaseli may have some more context as the one recently updated these files.
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.
Thanks for this massive change -
It's a very good step forward!
And I like the RequiresAuth idea, it makes a lot of sense.
Blocking mainly since the login flow is still scattered, and I think it can be further encapsulated.
And in addition:
These are maybe the most delicate flows in the WebUI, require proper testing, and are not properly covered by unit or integration tests.
Hence, IMHO the hardest part in this change is validating and QA-ing the different flows.
So please make sure that you have validated all the different flows yourself, including Cloud, Enterprise, SCIM and RBAC. According to the PR description, these are still missing.
webui/src/lib/auth/status.ts
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.
In other contexts (for example configProvider.tsx) all the types are on the same file.
I think we should keep a unified pattern.
webui/src/lib/auth/authContext.tsx
Outdated
| import React, { createContext, useContext, useMemo, useState, ReactNode } from "react"; | ||
| import { AUTH_STATUS, type AuthStatus } from "./status"; | ||
|
|
||
| type AuthContextShape = { |
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 "shape"?
Let's align to AuthContextType like the other contexts we have.
webui/src/lib/auth/authContext.tsx
Outdated
|
|
||
| function readPersistedStatus(): AuthStatus { | ||
| try { | ||
| const v = window.localStorage.getItem(STORAGE_KEY); |
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.
In api/index.js, the "user" is persisted into cache (see auth.login()).
Why do we need to save and maintain this information twice, instead of encapsulating it into a single place?
webui/src/lib/auth/authContext.tsx
Outdated
| status, | ||
| markAuthenticated: () => { | ||
| setStatus(AUTH_STATUS.AUTHENTICATED); | ||
| try { window.localStorage.setItem(STORAGE_KEY, AUTH_STATUS.AUTHENTICATED); } catch { return } |
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.
An empty catch clause is a bad practice.
At least log an error there.
webui/src/lib/auth/authContext.tsx
Outdated
|
|
||
| type AuthContextShape = { | ||
| status: AuthStatus; | ||
| markAuthenticated: () => void; |
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 do you need two functions here, and not unify to a single setStatus() function?
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.
Or even better - maybe use useEffect() to update the localStorage as a side effect (as done in configProvider, for example)?
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.
Since the recent changes, useEffect is no longer necessary 👍
webui/src/lib/components/navbar.jsx
Outdated
| auth.clearCurrentUser(); | ||
| window.location = logoutUrl; | ||
| markUnauthenticated(); | ||
| if (logoutUrl !== "/logout") { |
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.
Please explain this change.
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.
For SSO: external IdP logout must be a full navigation and replace history
| import {useAuth} from "../auth/authContext"; | ||
| import {AUTH_STATUS} from "../auth/status"; | ||
|
|
||
| export default function RequireAuth() { |
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 but important - should be RequiresAuth, since it's a trait of a component.
webui/src/lib/components/navbar.jsx
Outdated
| return; | ||
| } | ||
|
|
||
| router.navigate("/auth/login", { replace: true }); |
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 would expect a redirection to "/", and have the "/" to redirect to login if needed.
In other words, have the "/auth/login" redirection in a single place.
…e redundant `status.ts`, and rename `RequireAuth` to `RequiresAuth` for consistency.
…d `markUnauthenticated` into `setAuthStatus`, simplifying auth state management.
…alizing auth redirection logic and simplifying `login.tsx`.
…simplify child rendering logic.
… and improve readability of authentication logic.
…istency with functional component style.
… definitions for improved readability and organization.
…` and `setAuthStatus`, simplifying and centralizing auth logic.
…ng redundant type assertions.
… maintain consistency in authentication flow.
…avigation logic and simplify SSO handling flow.
…riable assignment.
…r cleaner and simplified authentication flow.
… destructuring for cleaner code.
… centralize redirect logic and ensure consistent handling of authenticated and unauthenticated states.
…ng redundant parentheses.
…eamline sessionStorage usage.
…ze URLs, and enhance `next` state persistence logic.
… unnecessary try-catch block.
…uth checks, and simplify `next` state handling.
…t` state handling, and improve redirection cleanup logic.
…` navigation from sessionStorage.
…in config handling, and consolidate rendering logic.
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.
Asking again since it wasn't addressed yet, and it's a very important part of this feature:
@Ben-El can you please provide details on how was this tested?
Besides the different flows, how about the different auth types - Cloud, Enterprise, SCIM and RBAC?
And on the same subject, did you consider adding some automated tests (E2E or integration), so that we'll have more confidence when updating this area of the WebUI in the future?
…N_CONFIG`, and enhance redirection handling with `Navigate`.
…edirection logic, and improve query string handling.
… and `ExternalRedirect` components, and enhance `next` state handling.
My Thoughts About the TestingYour changes are great! but they affect the sensitive authentication flow, which we want to be cautious about because it could easily break when interacting with different integrations and environments. What I Think Should Be Done Regarding TestingPrepare a test plan that details:
That way, we can review it together, identify any missing flows, scenarios, or environments that still need coverage, and prioritize what to test with which setups. Suggested Structure for the Test Plan
Current My Understanding of the Different Setups(Please correct or add if I missed anything.) We currently have multiple ways to handle authentication (verifying identity) and several methods for authorization (managing permissions). Authentication Options (lakeFS Enterprise)
Authorization Options (lakeFS Enterprise)
Authorization Options (lakeFS)
I’m not sure which authorization and authentication is used in Cloud. Maybe SCIM and GIAM are relevant here so you might need to check that. Essentially, the goal is to ensure that authentication works properly with:
But we only need to check the combinations that our customers might actually use. Regarding Automated Tests
Things to Consider Regarding the Test Plan
|
…s and simplify redirection logic.
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.
Great progress here @Ben-El .
I think that the architecture is now in place,
And the AuthProvider and RequiresAuth are a good solution,
And the code looks good.
Added some comments, most of them mainly revolve around cleaning up the code and readability.
In addition, please address the questions raised about testing this:
We've already had some changes in the auth area that ended up in either new production bugs or in regression. Since the changes in this PR are major, we should do the most for finding these bugs before releasing. Since these area isn't properly covered by tests, the whole testing / QA of this feature must be addressed.
webui/src/lib/auth/authContext.tsx
Outdated
| if (path === "/auth/login") return true; | ||
| return path.startsWith("/auth/oidc"); |
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.
| if (path === "/auth/login") return true; | |
| return path.startsWith("/auth/oidc"); | |
| if (path === "/auth/login") return true; | |
| if (path.startsWith("/auth/oidc")) return true; | |
| return false; |
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.
| if (path === "/auth/login") return true; | |
| return path.startsWith("/auth/oidc"); | |
| return path === "/auth/login" || path.startsWith("/auth/oidc"); |
webui/src/lib/auth/authContext.tsx
Outdated
| export const AUTH_STATUS = { | ||
| AUTHENTICATED: "authenticated", | ||
| UNAUTHENTICATED: "unauthenticated", | ||
| UNKNOWN: "unknown", |
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.
Maybe replace UKNOWN with PENDING?
Is there a scenario in which there's no auth request initiated?
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 UNKNOWN (or PENDING) state just represents this short initial phase before that request completes, not a scenario where the request doesn't happen.
So PENDING might indeed be a clearer name, since it better conveys "auth check in progress" rather than "no request".
|
|
||
| export const useAuth = (): AuthContextType => { | ||
| const ctx = useContext(AuthContext); | ||
| if (!ctx) throw new Error("useAuth must be used within <AuthProvider>"); |
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.
webui/src/lib/auth/authContext.tsx
Outdated
| auth.clearCurrentUser(); | ||
| setStatus(AUTH_STATUS.UNAUTHENTICATED); | ||
| const path = window.location.pathname; | ||
| const next = path + (window.location.search || "") + (window.location.hash || ""); |
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 please explain this hashing?
What it does and why it's needed?
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 hash was Anna's suggestion, the "hash" here refers to the fragment part of the URL (the part after #, not cryptographic hashing).
We include both location.search and location.hash in the next value so that when the user gets redirected back after login, they return to the exact same URL, including query params and hash anchors.
Without this, the redirect would lose any #fragment and land the user at the base page instead.
Maybe hashing is not critical for now, but in the future we may have anchors or something, so it'll cover that too.
webui/src/lib/auth/authContext.tsx
Outdated
| const AuthContext = createContext<AuthContextType | null>(null); | ||
|
|
||
| const isPublicAuthRoute = (path: string) => { | ||
| if (path === "/auth/login") return true; |
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.
Both "/auth/login" and "/auth/oidc" should be consts.
|
|
||
| if (loading) return <Loading/>; | ||
| if (!user) { | ||
| const next = location.pathname + (location.search || "") + (location.hash || ""); |
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.
Is this logic also required here (and not only in authContext)? Isn't this covered by flows catched by the context?
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.
Good question!
The check in RequiresAuth is intentional.
While authContext handles unauthorized events globally (e.g. when a token expires or the server returns 401), RequiresAuth acts as a route guard that ensures protected pages aren’t rendered even briefly when there's no active user yet.
So the logic overlaps a bit but serves a different timing:
authContext reacts after unauthorized events, while RequiresAuth prevents access before rendering a protected route.
| const pluginManager = usePluginManager(); | ||
| const {user} = useUser(); | ||
| const [storageConfig, setConfig] = useState<ConfigContextType>(configInitialState); | ||
| const { response, loading, error } = useAPI(() => config.getConfig(), [user]); |
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.
🔥
webui/src/lib/hooks/user.jsx
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.
I agree with @Annaseli here.
This hook is already minimal -
It can be unified with the authContext for simplicity and easier tracking.
webui/src/pages/auth/login.tsx
Outdated
| } | ||
| }; | ||
|
|
||
| const getLoginIntent = (location: ReturnType<typeof useLocation>) => { |
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.
Please make this code more readable, either by extracting to sub-functions, renaming vars or adding comments.
Currently it's pretty hard to understand what it does.
webui/src/pages/auth/login.tsx
Outdated
| await auth.login(username, password); | ||
| router.push(next || '/'); | ||
| navigate(0); | ||
| setAuthStatus(AUTH_STATUS.AUTHENTICATED); |
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 suspect there might be a race condition here, between setAuthStatus and router.navigate.
How about making sure that the router.navigate will happen after the setAuthStatus?
…s`, replace `AUTH_STATUS.UNKNOWN` with `AUTH_STATUS.PENDING`, and centralize utility functions for redirection and route management.
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.
@Ben-El great progress, it's almost there.
Still some questions,
And some small improvements required.
webui/src/lib/auth/authContext.tsx
Outdated
|
|
||
| useEffect(() => { | ||
| if (status === AUTH_STATUS.AUTHENTICATED) { | ||
| const stored = window.sessionStorage.getItem("lakefs_post_login_next"); |
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.
Make lakefs_post_login_next a const common to all relevant files.
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: rename stored to postLoginNext or something like this.
webui/src/lib/utils.ts
Outdated
| export const isPublicAuthRoute = (path: string) => | ||
| path === ROUTES.LOGIN || path.startsWith(ROUTES.OIDC_PREFIX); | ||
|
|
||
| export const buildNextFromWindow = () => |
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 read this function a few time, and still can't figure out what it's doing,
And what "build next from window" means.
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 builds the redirect target URL from window.location.
I will change to a clearer name, getCurrentRelativeUrl or something.
Thx.
webui/src/lib/auth/authContext.tsx
Outdated
| auth.clearCurrentUser(); | ||
| setStatus(AUTH_STATUS.UNAUTHENTICATED); | ||
| const path = window.location.pathname; | ||
| const next = buildNextFromWindow(); |
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: this can be inside the if.
| // handle global dark mode here | ||
| const {state} = useContext(AppContext); | ||
| document.documentElement.setAttribute('data-bs-theme', state.settings.darkMode ? 'dark' : 'light') | ||
| document.documentElement.setAttribute( |
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 this is inside useEffect?
Did you test the behavior of the Dark Mode after this change?
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.
Good question!
I moved this logic into useEffect because setting a DOM attribute is a side-effect,
and React best practices recommend running side-effects inside useEffect rather than during render.
This ensures that the attribute is updated only when darkMode changes, and prevents double execution under strict mode.
I tested it and confirmed that dark mode toggling and initial theme load still behave as expected.
| if (loading) return <Loading/>; | ||
| if (!user) { | ||
| const next = buildNextFromWindow(); | ||
| const url = `${ROUTES.LOGIN}?redirected=true&next=${encodeURIComponent(next)}`; |
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 is both hard to read and tricky to maintain.
Please use URLSearchParams() or something similar to create this in a more robust way.
webui/src/lib/utils.ts
Outdated
| qp: URLSearchParams | ||
| ) => { | ||
| const qs = qp.toString(); | ||
| return `${loc.pathname}${qs ? `?${qs}` : ""}${loc.hash ?? ""}`; |
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 is both hard to read and tricky to maintain.
Please use URLSearchParams() or something similar to create this in a more robust way.
webui/src/lib/hooks/user.jsx
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.
@Ben-El this comment still awaits your addressing.
webui/src/lib/utils.ts
Outdated
| export const queryOf = (loc: ReturnType<typeof useLocation>) => | ||
| new URLSearchParams(loc.search); | ||
|
|
||
| export const isTrue = (v: string | null) => v === "true"; |
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.
🤔
webui/src/pages/auth/login.tsx
Outdated
| u.searchParams.set("next", safeNext); | ||
| return u.toString(); | ||
| } catch { | ||
| return `${url}${url.includes("?") ? "&" : "?"}next=${encodeURIComponent(safeNext)}`; |
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 is both hard to read and tricky to maintain.
Please use URLSearchParams() or something similar to create this in a more robust way.
webui/src/pages/auth/login.tsx
Outdated
| if (setupResponse && (setupResponse.state !== SETUP_STATE_INITIALIZED || setupResponse.comm_prefs_missing)) { | ||
| router.push({pathname: '/setup', params: {}, query: router.query as Record<string, string>}) | ||
| return null; | ||
| const qs = new URLSearchParams(location.search); |
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.
TBH, this whole login algorithm/flow is still not clear.
Note that there's been a decent amount of comments in the original code, for a reason.
Making this flow clearer will make it much easier to review.
…CurrentRelativeUrl`, unify `lakefs_post_login_next` handling, and centralize redirection utilities for consistency.
…management in `authContext`, and update components accordingly.
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.
LGTM, nice work!
I hope the testing part will work out fine 🤞
| 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.
Nit: what's the style, {aaa} or { aaa }?
| if (error) { | ||
| return <AlertError error={error} className={"mt-1 w-50 m-auto"} onDismiss={() => window.location.reload()} />; | ||
| } | ||
| // Setup doesn't complete, send to /setup with redirected=true&next=... |
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 comment belongs to the next line...
Closes #9544
Summary
This PR refactors the WebUI authentication and navigation flow to improve reliability and consistency across login, logout, and protected routes.
Motivation
Previously, authentication state and redirects were handled in multiple components, which caused inconsistencies:
Etc.
Changes
AuthContextand unifiedmarkAuthenticated/markUnauthenticatedAPI.RequireAuthto handle redirects for unauthenticated users consistently.useUserto rely onAuthContextstatus and cache only when authenticated.ConfigProviderto re-fetch configuration when user state changes.navbar.jsxto cleanly reset user state and redirect properly./auth/loginno longer shows when the user is already authenticated.navbarwhen logged out.Testings
Tested manually across the following flows:
https://docs.google.com/spreadsheets/d/1-1UFTf0sbI16325zvV8FYfnoc7y8i62Mb_GbVHhXdv8/edit?gid=0#gid=0