Fix login flow issues #9506

Annaseli · 2025-09-11T19:10:04Z

Description

This PR fixes all the issues mentioned in the related issue.

I tested the login flow and confirmed that login flow works as expected, with all previously reported problems resolved in the following environments:

Local lakeFS OSS with no RBAC
Local lakeFS OSS with ACL
Local lakeFS Enterprise with local RBAC

Annaseli · 2025-09-11T19:12:02Z

webui/src/lib/components/auth/users.jsx

+        hasMore = results.pagination.has_more;
+    } while (hasMore);
+    usersList.sort((a, b) => resolveUserDisplayNameFN(a).localeCompare(resolveUserDisplayNameFN(b)));
+    return usersList;


The catch here prevented the error from being caught by useAPI, which is responsible for redirecting to /auth/login. Since this catch block didn’t do anything with the error, I removed it.

Annaseli · 2025-09-11T19:13:41Z

webui/src/lib/components/auth/layout.tsx

+            }
+            setIsLogged(true);
+        }
+    }, [userWithId, loading, error, setIsLogged, router]);


By using useEffect, we can redirect to the login page from all of the Administration tabs instead of showing errors or an empty pages.

You're already handling this in layout.tsx -
I think it's way better to do this routing there.
It keeps everything in one place, and doesn't require importing the useLayoutOutletContext.
It might catch future redirections that might occur in the "app layout" level.

As discussed, I’m keeping this router.push() code here and will open a relevant issue for refactoring.

Annaseli · 2025-09-11T19:19:58Z

webui/src/lib/components/layout.tsx

+            // If there's no user or there's an authentication error, show simple navbar
+            setIsLogged(Boolean((userWithId && userWithId.id !== "") && !error));
+        }
+    }, [userWithId, loading, error]);


This fix ensures the navbar is displayed according to the user’s current authentication status.

Annaseli · 2025-09-11T19:23:47Z

webui/src/pages/repositories/repository/error.jsx

+            query: {next: router.route, redirected: true},
+        });
+        return null;
+    }


Endpoints starting with /repositories return AlertError here before reaching the useAPI func that should redirect to /auth/login when the user is unauthenticated. i.e, the error occurs before the useAPI code executes, so before this change the error was simply displayed instead of triggering the redirect.

itaigilo

Nice catch - good handling of some nasty bugs.

Blocking for the RepoError comments -
The code should be simplified and cleaned.

itaigilo · 2025-09-12T22:27:45Z

webui/src/lib/components/auth/layout.tsx

+                    params: {},
+                    query: { next: router.route, redirected: 'true' },
+                });
+                return;


This is very confusing.
Prefer else instead of return in this case.

itaigilo · 2025-09-12T22:30:16Z

webui/src/lib/components/auth/layout.tsx

-        setIsLogged(true);
-    }, [setIsLogged]);
+        if (!loading) {
+            if (!userWithId || userWithId.id === "" || error) {


Can there be a userWithId which is not null but without id?

we get userWithId from useUser(), which calls auth.getCurrentUserWithCache(). If the user data isn’t cached, this function fetches it from the /user endpoint.

According to Swagger, the /user endpoint when returns status code 200, returns a CurrentUser schema, which returns the User schema:

User: type: object required: - id - creation_date properties: id: type: string description: A unique identifier for the user. Cannot be edited. creation_date: type: integer format: int64 description: Unix Epoch in seconds ...

Because id is a required field in this schema, the id property must exist when userWithId is not null.

Also in the browser, when the user is unauthenticated, we see:

So if (!userWithId || error) { should be enough, no?
Since id can't be empty?

(It's nit, ofc.)

@itaigilo
userWithId.id is always present, but it can be an empty string. It’s empty when the user is logged out. So, checking whether this value is empty is the key test for determining if the user is logged in or not.

itaigilo · 2025-09-12T22:31:28Z

webui/src/lib/components/layout.tsx

+        if (!loading) {
+            // If there's a user and no error, show authenticated (full) navbar
+            // If there's no user or there's an authentication error, show simple navbar
+            setIsLogged(Boolean((userWithId && userWithId.id !== "") && !error));


Suggested change

setIsLogged(Boolean((userWithId && userWithId.id !== "") && !error));

setIsLogged(userWithId?.id !== "" && !error);

But double-check this one ☝️ 😄

For the:
setIsLogged(userWithId?.id !== "" && !error);

there will be an issue when userWithId is null or undefined:
null?.id !== "" → undefined !== "" → true so if there is no error we will get: true && !error -> true

while with the current code:
null && * → false

So the behavior differs.

But I did remove the redundant parentheses around (userWithId && userWithId.id !== "") and replaced the Boolean() cast with !! (since omitting it leads to a TypeScript error):

itaigilo · 2025-09-12T22:41:42Z

webui/src/pages/repositories/repository/error.jsx

 };

-export const RepoError = ({error}) => {
+export const RepoError = ({error, router}) => {


There's no need to pass router as param -
You can have useRouter() here.

itaigilo · 2025-09-12T22:47:56Z

webui/src/pages/repositories/repository/error.jsx

-export const RepoError = ({error}) => {
+export const RepoError = ({error, router}) => {
+    if (error instanceof AuthenticationError) {
+        router.push({


I would expect this RepoError component to contain only a rendering of the UI, not routing logic.
We should strive to have the routing logic at the "top" of the components tree, otherwise it becomes a nightmare to understand, for example, which component ended up causing the router.push(), making it very hard to maintain and to debug.

Have you tried placing it in the place where this error is originally produced?
I think that RefContextProvider should be it, but I'm not sure it's the source for all these RepoErrors.

I changed the code so that the RefContextProvider now uses useAPI() since useAPI() automatically redirects to the login page when the user is unauthenticated. Changed the RefContextProvider because useRefs() receives its data from the RefContextProvider, and the errors returned from useRefs() are passed to the RepoError component for display.

nopcoder

Don't think that RepoError should redirect - at this level when we render a control to display an error we should not make this call.

The useAPI and similar that handle the API call should handle this state and update that we need to redirect user to the login page.

Annaseli · 2025-09-17T17:31:28Z

Don't think that RepoError should redirect - at this level when we render a control to display an error we should not make this call.

The useAPI and similar that handle the API call should handle this state and update that we need to redirect user to the login page.

I changed the code so that the RefContextProvider now uses useAPI() since useAPI() automatically redirects to the login page when the user is unauthenticated. Changed the RefContextProvider because useRefs() receives its data from the RefContextProvider, and the errors returned from useRefs() are passed to the RepoError component for display.

itaigilo

Looking much better now.

Still some comments about the redirection.

itaigilo · 2025-09-18T07:11:54Z

webui/src/lib/components/auth/layout.tsx

-        setIsLogged(true);
-    }, [setIsLogged]);
+        if (!loading) {
+            if (!userWithId || userWithId.id === "" || error) {


So if (!userWithId || error) { should be enough, no?
Since id can't be empty?

(It's nit, ofc.)

itaigilo · 2025-09-18T07:13:42Z

webui/src/lib/components/auth/layout.tsx

+    }
+
+    // Don't render anything if not authenticated (will redirect)
+    if (!userWithId || userWithId.id === "" || error) {


I suggest loading in this case as well (better see a loader than a blank screen that might look like something is stuck) -
But up to you.

consulted about it also with chatGPT and it says:

short answer: show a loading state only when the auth status is still unknown; otherwise keep return null while you redirect.

Why

If you’re already sure isLogged === false, you immediately redirect in the effect. Rendering a spinner for a frame just before redirect often causes a flicker. return null is fine here.

Showing a spinner makes sense when auth is still loading (e.g., useUser() hasn’t resolved yet).

so I think i'll leave the return null and return the only while loading.

itaigilo · 2025-09-18T07:23:45Z

webui/src/lib/components/layout.tsx

    return (
        <ConfigProvider>
-            <TopNav logged={isLogged}/>
+            {!loading && <TopNav logged={isLogged}/>}


Is loading == true only after the login form is submitted?
Or are there other situations?

The loading comes from the useUser() call. It is loading == true only while waiting for the /user data to be fetched.

itaigilo · 2025-09-18T07:25:54Z

webui/src/lib/hooks/repo.jsx

+    const { response, error, loading } = useAPI(async () => {
+        if (!repoId) return null;
+        const repo = await repositories.get(repoId);
+        const reference = await resolveRef(repoId, (ref) ? ref : repo.default_branch);


While you're at it:

itaigilo · 2025-09-18T07:26:13Z

webui/src/lib/hooks/repo.jsx

+    const { response, error, loading } = useAPI(async () => {
+        if (!repoId) return null;
+        const repo = await repositories.get(repoId);
+        const reference = await resolveRef(repoId, (ref) ? ref : repo.default_branch);


Suggested change

const reference = await resolveRef(repoId, (ref) ? ref : repo.default_branch);

const reference = await resolveRef(repoId, ref || repo.default_branch);

itaigilo · 2025-09-18T07:26:35Z

webui/src/lib/hooks/repo.jsx

+        if (!repoId) return null;
+        const repo = await repositories.get(repoId);
+        const reference = await resolveRef(repoId, (ref) ? ref : repo.default_branch);
+        const comparedRef = await resolveRef(repoId, (compare)? compare : repo.default_branch);


Suggested change

const comparedRef = await resolveRef(repoId, (compare)? compare : repo.default_branch);

const comparedRef = await resolveRef(repoId, compare || repo.default_branch);

itaigilo · 2025-09-18T07:27:52Z

webui/src/lib/components/auth/layout.tsx

+            }
+            setIsLogged(true);
+        }
+    }, [userWithId, loading, error, setIsLogged, router]);


You're already handling this in layout.tsx -
I think it's way better to do this routing there.
It keeps everything in one place, and doesn't require importing the useLayoutOutletContext.
It might catch future redirections that might occur in the "app layout" level.

itaigilo · 2025-09-22T10:02:30Z

webui/src/lib/components/auth/layout.tsx

+    const [activeTab, setActiveTab] = useState("credentials");
    const {RBAC: rbac} = useLoginConfigContext();
-    const [setIsLogged] = useLayoutOutletContext();
+    const [loading, isLogged] = useLayoutOutletContext();


Shouldn't this be:
const [isLogged, setIsLogged] = useLayoutOutletContext();
?

Why it has a different structure?

@itaigilo
I updated the parent Layout component to:
<Outlet context={[loading, isLogged, setIsLogged] satisfies LayoutOutletContext}/>

instead of:
<Outlet context={[setIsLogged] satisfies LayoutOutletContext}/>

I need const [loading, isLogged] = useLayoutOutletContext(); and not const [isLogged, setIsLogged] = useLayoutOutletContext(); because in AuthLayout I check whether the user is logged in. Since this check is already done in Layout, I can use the isLogged value passed through the context, rather than re-calling useUser(). So I needed the isLogged and didn't need the setIsLogged . There is no usage in setIsLogged in the updated AuthLayout component. (It also doesn't pass it to other components)

And I added loading to the useLayoutOutletContext () because it is returned from calling to the useUser (which is called in Layout). I need the loading in the AuthLayout so that I can add this:
if (loading) return <Loading/>

So AuthLayout needs both isLogged and loading via useLayoutOutletContext, and not the setIsLogged

itaigilo · 2025-09-22T14:05:24Z

webui/src/pages/repositories/repository/objects.jsx

Pretty weird, to have only whitespace changes for files.
Next time, please try to avoid this.

itaigilo · 2025-09-22T14:45:32Z

webui/src/lib/components/layout.tsx

+    useEffect(() => {
+        if (!loading) {
+            // If there's a user and no error, show authenticated (full) navbar
+            setIsLogged(!!(userWithId && userWithId.id !== "" && !error));


Suggested change

setIsLogged(!!(userWithId && userWithId.id !== "" && !error));

setIsLogged(!(userWithId?.id) && !error);

I think that !(userWithId?.id) && !error is the opposite of !!(userWithId && userWithId.id !== "" && !error) because:

for !(userWithId?.id) && !error:
if user is logged out and there is no error => userWithId.id=="" => userWithId?.id is false => !(userWithId?.id) is true => !(userWithId?.id) && !error is true
so in this case when user is logged out we get setIsLogged(true) instead of setIsLogged(false)

Did you mean this?
setIsLogged(!!userWithId?.id && !error); (!! equals Boolean())

I updated the code to this - setIsLogged(!!userWithId?.id && !error);
but LMK if you meant something else.

itaigilo · 2025-09-22T14:56:20Z

webui/src/lib/components/auth/layout.tsx

-        setIsLogged(true);
-    }, [setIsLogged]);
+        if (!isLogged) {
+            router.push({


Can you please add a comment here, explaining why this redirection is done here, and not it the place that gets the isLogged in first place (the top-level layout)?

itaigilo · 2025-09-22T15:35:39Z

webui/src/lib/hooks/repo.jsx

+        return { repo, reference, compare: comparedRef };
    }, [repoId, ref, compare]);

+    const refState = {


So refState is a const, and nothing changes it?
Or am I missing something?
What makes it a state?

The "state" behavior comes from the useAPI hook.
When the async operation in useAPI completes or dependencies [repoId, ref, compare] change, useAPI triggers a re-render.
On each re-render, a new refState object is created with the updated values from useAPI.
This new object gets passed through the context, notifying all consumers.
So while refState is const within each render, it's recreated on every render with potentially different values.
So It's a derived state - refState is derived from the useAPI hook's internal state, not a state variable itself.

And useAPI already uses useEffect to execute the promise (fetch the data in this case).
It initializes state and manages loading and error handling internally, so we delegate that responsibility to useAPI.

Great, thanks for the details.

To make things (maybe) clearer, I would've renamed refState to refsState (which is nit),
And change the const [ refs ] = useContext(RefContext); above to const [ refsState ] = useContext(RefContext);. This way, it might be clearer that this const is the only object that actually accesses the refsState you create here.

But that's not blocking.

itaigilo

LGTM -
Nice effort in a messy part of the code 💪

itaigilo · 2025-09-23T08:32:04Z

webui/src/lib/hooks/repo.jsx

+        return { repo, reference, compare: comparedRef };
    }, [repoId, ref, compare]);

+    const refState = {


Great, thanks for the details.

To make things (maybe) clearer, I would've renamed refState to refsState (which is nit),
And change the const [ refs ] = useContext(RefContext); above to const [ refsState ] = useContext(RefContext);. This way, it might be clearer that this const is the only object that actually accesses the refsState you create here.

But that's not blocking.

Addressed Barak Amar’s comment and he didn’t request any further changes. After Barak’s review, Itai Gilo also reviewed the code and approved it.

Annaseli added 4 commits September 11, 2025 20:19

added the code for fixing the redirection and navbar behaviour

9507ef5

simplified the fix to /auth/login

7f3da32

cleanup

637532f

added rerouting for login page from all the Administration tabs

f1be107

Annaseli requested review from itaigilo and nopcoder September 11, 2025 19:10

Annaseli added the include-changelog PR description should be included in next release changelog label Sep 11, 2025

Annaseli commented Sep 11, 2025

View reviewed changes

itaigilo requested changes Sep 12, 2025

View reviewed changes

nopcoder previously requested changes Sep 14, 2025

View reviewed changes

Annaseli added 3 commits September 17, 2025 20:42

addressed comments on the PR

d6736c7

Merge branch 'master' into task/fix-missing-redirection-to-login

f18ef86

fixed lint issue

796c448

Annaseli requested review from itaigilo and nopcoder September 17, 2025 17:56

itaigilo requested changes Sep 18, 2025

View reviewed changes

cleaned code - addressed PR comments

a5155c8

Annaseli requested a review from itaigilo September 21, 2025 14:10

added redirection to main after login

b6a82cb

itaigilo reviewed Sep 22, 2025

View reviewed changes

Annaseli requested a review from itaigilo September 22, 2025 12:23

removed the loading bool from the useLayoutOutletContext

ae8acdd

itaigilo reviewed Sep 22, 2025

View reviewed changes

Annaseli added 2 commits September 22, 2025 18:54

removed setIsLogged

c3f403e

addressed more PR comment

b64243d

Annaseli requested a review from itaigilo September 22, 2025 22:49

itaigilo approved these changes Sep 23, 2025

View reviewed changes

renamed param

7535ed4

Annaseli merged commit 36d3e1b into master Sep 23, 2025
41 checks passed

Annaseli deleted the task/fix-missing-redirection-to-login branch September 23, 2025 09:12

	setIsLogged(Boolean((userWithId && userWithId.id !== "") && !error));
	setIsLogged(userWithId?.id !== "" && !error);

	const reference = await resolveRef(repoId, (ref) ? ref : repo.default_branch);
	const reference = await resolveRef(repoId, ref \|\| repo.default_branch);

	const comparedRef = await resolveRef(repoId, (compare)? compare : repo.default_branch);
	const comparedRef = await resolveRef(repoId, compare \|\| repo.default_branch);

	setIsLogged(!!(userWithId && userWithId.id !== "" && !error));
	setIsLogged(!(userWithId?.id) && !error);

Fix login flow issues #9506

Fix login flow issues #9506

Uh oh!

Conversation

Annaseli commented Sep 11, 2025

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaigilo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Annaseli Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Annaseli Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nopcoder left a comment

Choose a reason for hiding this comment

Uh oh!

Annaseli commented Sep 17, 2025

Uh oh!

itaigilo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Annaseli Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Annaseli Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Annaseli Sep 21, 2025 •

edited

Loading

Annaseli Sep 16, 2025 •

edited

Loading

Annaseli Sep 21, 2025 •

edited

Loading

Annaseli Sep 22, 2025 •

edited

Loading

Annaseli Sep 22, 2025 •

edited

Loading