Fix login by duzda · Pull Request #1160 · freeipa/freeipa-webui

duzda · 2026-08-13T11:44:18Z

This may be tested with the development, but it fixes a bug that exists in production. To test and replicate the fixed bug in production:

Run kinit
Navigate to Modern WebUI (You should get automatically logged in)
Log out -> Stuck in the loop.

The fix allows you to log in as another user on log out, but at the same time if you refresh page, it will still pick up kerberos. The rest is just a simplifications and few other changes, I added a link that takes you back to the old webui, I'd wish I knew who and where was asking for that, but the change seemed minor and made sense. There is also a bunch of simplifications and fixes regarding getting stuck in the login loop.

Summary by Sourcery

Simplify authentication flow and resolve login/logout issues in the Modern WebUI while improving login UX and global auth state handling.

Bug Fixes:

Fix login loop when logging out of a Kerberos-authenticated session so users can cleanly log in again.
Ensure application correctly reflects logged-in status based on backend whoami response without stale local state.
Prevent failed Kerberos or certificate logins from leaving the UI in an inconsistent authenticating state.

Enhancements:

Replace per-user reload-based navigation with state-driven auth (loggedIn flag) and route guards.
Centralize logged-in user information in the global store and use it to fetch and display user details in the layout header.
Allow temporarily disabling Kerberos on logout to support switching users while still supporting Kerberos on refresh.
Refactor login page to use a custom LoginPage wrapper component and structured helper content.
Remove initial data loading spinner from routing and rely on global app-level loading instead.

Documentation:

Add inline login page helper content describing username/password, Kerberos, and certificate login options, including a link back to the old WebUI.

sourcery-ai

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="src/AppLayout.tsx" line_range="58-65" />
<code_context>
+  const { data: userDetails, isFetching } = useGetUserByUidQuery(loggedInUser, {
</code_context>
<issue_to_address>
**issue:** Guard against undefined `givenname` when computing `fullName` to avoid rendering `"undefined"`.

Because `userDetails?.givenname !== ""` is true when `givenname` is `undefined`, the header can show `"undefined <sn>"`. Consider a truthiness check or explicitly handling both `undefined` and empty string:

```ts
const fullName = React.useMemo(() => {
  if (!userDetails) return "";
  if (userDetails.givenname) {
    return `${userDetails.givenname} ${userDetails.sn}`;
  }
  return userDetails.sn;
}, [userDetails]);
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai · 2026-08-13T11:45:27Z

+  const { data: userDetails, isFetching } = useGetUserByUidQuery(loggedInUser, {
+    skip: !loggedInUser,
+  });

  // Retrieve and assign user full name
-  const [fullName, setFullName] = React.useState<string>("");
-
-  React.useEffect(() => {
-    if (props.loggedInUser) {
-      getUserDetails(props.loggedInUser).then((response) => {
-        if ("data" in response) {
-          const first = response.data?.result.result.givenname;
-          const last = response.data?.result.result.sn;
-          // Some users (e.g., admin) don't have first name
-          if (!first) {
-            setFullName(last as string);
-          } else {
-            setFullName(first + " " + last);
-          }
-        }
-      });
+  const fullName = React.useMemo(() => {
+    if (!userDetails) return "";
+    if (userDetails?.givenname !== "") {


issue: Guard against undefined givenname when computing fullName to avoid rendering "undefined".

Because userDetails?.givenname !== "" is true when givenname is undefined, the header can show "undefined <sn>". Consider a truthiness check or explicitly handling both undefined and empty string:

const fullName = React.useMemo(() => { if (!userDetails) return ""; if (userDetails.givenname) { return `${userDetails.givenname} ${userDetails.sn}`; } return userDetails.sn; }, [userDetails]);

…login after logout Changes: - Create a custom `LoginPage` component that accepts `loginPageContent` as a React node and renders it in the login footer - Update `LoginMainPage` to use the custom `LoginPage` and render login instructions as a `List`, including a link back to the old WebUI - Set an `isKerberosDisabled` flag in `localStorage` on logout to prevent automatic Kerberos re-login on the next visit - Read and clear the flag in `LoginMainPage` to control Kerberos auto-login behavior - Add CSS styling for the login page list bullets Fixes: freeipa#570 Signed-off-by: David Hanina <dhanina@redhat.com>

Changes: - Simplify `auth-slice` to track only `loggedUser` instead of separate `isUserLoggedIn`, `user`, and `error` fields - Rename auth actions to `setLoggedUser` and `setLoggedOut` - Remove local auth state from `App.tsx` and use the Redux `loggedUser` value directly - Use RTK Query's `isFetching` flag for the initial batch loading state - Remove `window.location.reload()` calls after login and logout - Simplify `AppRoutes` by removing the `isInitialDataLoaded` prop and the `DataSpinner` fallback Signed-off-by: David Hanina <dhanina@redhat.com>

carma12

Overall nice solution. Just some details...

carma12 · 2026-08-13T13:58:47Z

 }

+.login-page-list {
+  list-style-type: "· ";


Not sure if I understand this...

I've removed the · from each string, instead it's part of the styling of the element, this more follows the common html and css schemantics. The true reason is, that I wanted to insert a link and the PF component only accepts strings...

carma12 · 2026-08-13T14:11:35Z

-  isUserLoggedIn: boolean;
-  user: string | null;
-  error: string | null;
+  loggedUser: string | null;


This is the same as the user parameter you just deleted. But I understand that the new name is more descriptive... Not sure if it make sense to remove the error parameter, just in case the API call returns an error response (can't recall now the chances of that happening).

Scrapped, the user now lives only in the global slice, which makes more sense, as this info was duplicated.

carma12 · 2026-08-13T14:21:17Z

  // Store data in global slice (Redux)
  React.useEffect(() => {
-    if (!isInitialBatchLoading && initialBatchResponse === undefined) {
+    if (initialBatchResponse === undefined) {


The old code had if (!isInitialBatchLoading && initialBatchResponse === undefined) which properly guarded against transient undefined states. This PR removed the !isInitialBatchLoading guard, causing a briefly flash the login page during the refetch window. Maybe this change can be be reverted?

I've scraped this idea, in favor of fully relying on RTK Query, I'm curious about some checks there...

carma12 · 2026-08-13T14:24:01Z

+  React.useEffect(() => {
+    // We need to refetch data on user change
+    if (!isInitialBatchLoading && loggedIn) {
+      refetch();
+    }
+  }, [loggedIn]);


This useEffect uses isInitialBatchLoading and refetch in its body, but only declares [loggedIn] as a dependency.

I've scraped this idea, all of this is replace by rtk query and immediate caching, instead we refetch whenever we login.

carma12

Overall nice solution. Just some details...

carma12 · 2026-08-13T14:42:39Z

-        // Forcing full page to reload and redirect to login page
-        window.location.reload();
+        sessionStorage.setItem("isKerberosDisabled", "true");
+        dispatch(setLoggedOut());


I'm thinking that there is no safeguard here (e.g. error shown) in case the logout operation fails, e.g., due to a failed response, network error, server unreachable, 500, etc. Maybe we should consider to add something here? This can be done in a different PR if needed.

carma12 · 2026-08-13T14:54:06Z

-        // Forcing full page to reload and redirect to login page
-        window.location.reload();
+        sessionStorage.setItem("isKerberosDisabled", "true");
+        dispatch(setLoggedOut());


I'm thinking that there is no safeguard here (e.g. error shown) in case the logout operation fails, e.g., due to a failed response, network error, server unreachable, 500, etc. Maybe we should consider to add something here? This can be done in a different PR if needed.

Changes: - Replace `loggedUser` string state with `loggedIn` boolean in auth slice - Rename `setLoggedUser` action to `setLoggedIn` - Retrieve logged-in user UID from global slice in `AppLayout` - Use `useGetUserByUidQuery` instead of mutation for user details - Refetch initial batch data when login state changes - Move `isKerberosDisabled` flag from localStorage to sessionStorage - Improve login error handling and validation state updates Signed-off-by: David Hanina <dhanina@redhat.com>

Move AppLayout from a conditional wrapper in App.tsx to a parent route in AppRoutes, rendering nested routes via Outlet. This removes the children prop from AppLayout and simplifies the top-level App rendering. This also fixes a bug where when logged it it incorrectly renders sync-otp or browser-config pages. Signed-off-by: David Hanina <dhanina@redhat.com>

- Add a `userMetadata` query to `rpcAuth` that batches the initial configuration commands (config_show, whoami, env, dns_is_enabled, etc.) and returns a typed `UserMetadata` object. - Simplify `global-slice` to hold `UserMetadata` directly, populating it from the query matcher and clearing `loggedInUser` on rejection. - Remove the dedicated `auth-slice`; derive login state from whether `loggedInUser` is non-empty. - Update `App`, `AppLayout`, `AppRoutes`, `LoginMainPage`, and `ResetPassword` to use the new query and the `loggedInUser` global value. - Refetch `userMetadata` on successful login and logout instead of toggling a boolean auth flag. Assisted-by: Cursor <cursoragent@curosr.com> Signed-off-by: David Hanina <dhanina@redhat.com>

Decouple the two different testing utils as importing from store initializes API, which we want to avoid in some cases. This can be refactored in a nicer way later on. Signed-off-by: David Hanina <dhanina@redhat.com>

sourcery-ai Bot reviewed Aug 13, 2026

View reviewed changes

duzda force-pushed the fix-login branch from 5f1a756 to 2b2e9a3 Compare August 13, 2026 11:54

duzda added 2 commits August 13, 2026 15:41

duzda force-pushed the fix-login branch from 2b2e9a3 to 41ab00a Compare August 13, 2026 14:18

carma12 reviewed Aug 13, 2026

View reviewed changes

duzda added 3 commits August 14, 2026 12:04

duzda force-pushed the fix-login branch from 41ab00a to 55f5f9b Compare August 14, 2026 11:28

Extract router test utilities into dedicated file

a654830

Decouple the two different testing utils as importing from store initializes API, which we want to avoid in some cases. This can be refactored in a nicer way later on. Signed-off-by: David Hanina <dhanina@redhat.com>

duzda added the needs-review This PR is waiting on a review label Aug 14, 2026

Uh oh!

Conversation

duzda commented Aug 13, 2026 • edited by sourcery-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

carma12 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!

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!

carma12 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

duzda commented Aug 13, 2026 •

edited by sourcery-ai Bot

Loading