fix(admin-ui): unable to remove or reassign admin roles from user profile#2697
Merged
fix(admin-ui): unable to remove or reassign admin roles from user profile#2697
Conversation
…file Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
…file Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Contributor
📝 WalkthroughWalkthroughMoves GluuToast out of AppAuthProvider and above AppLayout; adds an onCloseRedirectUrl field to Redux toast state and updateToast action; AppAuthProvider dispatches updateToast with a redirect URL for logout errors; GluuToast reads the redirect URL and performs a full-page redirect on close; preserves empty arrays in UserForm modified-fields logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppAuthProvider
participant Redux
participant GluuToast
participant Browser
User->>AppAuthProvider: Trigger action (e.g., logout/no-valid-role)
AppAuthProvider->>Redux: dispatch updateToast(show=true, type='error', message, onCloseRedirectUrl=sessionEndpoint)
Redux->>Redux: store showToast, type, message, onCloseRedirectUrl
Redux->>GluuToast: state update -> render toast with onClose handler
GluuToast->>User: display error toast
User->>GluuToast: close toast (user or auto)
GluuToast->>Browser: window.location.href = onCloseRedirectUrl (full-page redirect)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mo-auto
added
comp-admin-ui
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
admin-ui/app/routes/Apps/Gluu/GluuToast.tsx (1)
13-48:⚠️ Potential issue | 🟠 MajorCapture the redirect URL per toast, not in a shared ref.
redirectUrlRef.currentis mutable process-wide state for every active toast. If another toast is dispatched before the first one closes, Line 46 overwrites the ref, so the first toast'sonClosecan redirect to the wrong place or skip the redirect entirely.Proposed fix
-import { useEffect, useRef } from 'react' +import { useEffect } from 'react' @@ - const redirectUrlRef = useRef('') - - const handleToastClose = () => { - if (redirectUrlRef.current) { - window.location.href = redirectUrlRef.current - } - } - const showTheToast = () => { - const options = onCloseRedirectUrl ? { onClose: handleToastClose } : {} + const redirectUrl = onCloseRedirectUrl + const options = redirectUrl + ? { + onClose: () => { + window.location.assign(redirectUrl) + }, + } + : {} if (type == 'success') { toast.success(<ToastDesign />, options) } else { toast.error(<ToastDesign />, options) } @@ if (showToast) { - redirectUrlRef.current = onCloseRedirectUrl || '' showTheToast() dispatch(updateToast(false, 'success', '')) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuToast.tsx` around lines 13 - 48, The shared mutable ref redirectUrlRef is overwritten across concurrent toasts; instead capture the per-toast redirect URL in a local variable and pass a dedicated onClose closure to the toast so each toast uses its own URL. Modify showTheToast to read const redirect = onCloseRedirectUrl || '' and create a local handleClose() { if (redirect) window.location.href = redirect } (use this closure when building options passed to toast.success/toast.error) and remove reliance on redirectUrlRef and the separate handleToastClose function; keep dispatch(updateToast(false, 'success', '')) inside the same useEffect after showing the toast.admin-ui/plugins/user-management/components/UserForm.tsx (1)
190-203:⚠️ Potential issue | 🟠 MajorDon't preserve
[]for claims that were never persisted.Both paths now keep empty arrays in
modifiedFields, butadmin-ui/plugins/user-management/utils/attributeTransformUtils.ts:189-206treats any present multi-valued key as a real update and will create a newcustomAttributewithvalues: []when the attribute did not exist before. That turns “add claim, then remove it before saving” into a bogus clear operation instead of dropping the change. Only keep an empty array when the original user already had that multi-valued attribute.Also applies to: 227-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/user-management/components/UserForm.tsx` around lines 190 - 203, The current updateModifiedFields callback (and the similar block at the other location) treats an empty array as a deliberate change even if the attribute didn't previously exist, which later causes attributeTransformUtils to create a bogus empty customAttribute; modify the logic in updateModifiedFields (and the other matching setter) to check the original user's attributes/state before deciding to keep an empty array: if isEmptyValue(value) && Array.isArray(value) then remove the key from modifiedFields unless the original user already had that multi-valued attribute (use the component's existing user/originalUser prop/state to determine existence); otherwise keep non-empty arrays and other values as before. Ensure you reference updateModifiedFields (and the second setter block) and attributeTransformUtils behavior when implementing this conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/app/utils/AppAuthProvider.tsx`:
- Around line 245-253: The logout URL is built by string interpolation which
breaks if the stored postLogoutRedirectUri contains its own query/fragment;
replace the manual template with URL and URL.searchParams to construct
sessionEndpoint robustly: create a new URL using authConfigs?.endSessionEndpoint
(or fallback to empty string/throw if invalid), then call
searchParams.set('state', state) and
searchParams.set('post_logout_redirect_uri',
localStorage.getItem('postLogoutRedirectUri') ?? '') before passing the
resulting URL.toString() into updateToast (where sessionEndpoint is currently
used); update references to sessionEndpoint and keep the same
dispatch/updateToast usage and message (t('messages.no_valid_role_logout', {
seconds: LOGOUT_DELAY_SECONDS })) so behavior doesn’t change.
---
Outside diff comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuToast.tsx`:
- Around line 13-48: The shared mutable ref redirectUrlRef is overwritten across
concurrent toasts; instead capture the per-toast redirect URL in a local
variable and pass a dedicated onClose closure to the toast so each toast uses
its own URL. Modify showTheToast to read const redirect = onCloseRedirectUrl ||
'' and create a local handleClose() { if (redirect) window.location.href =
redirect } (use this closure when building options passed to
toast.success/toast.error) and remove reliance on redirectUrlRef and the
separate handleToastClose function; keep dispatch(updateToast(false, 'success',
'')) inside the same useEffect after showing the toast.
In `@admin-ui/plugins/user-management/components/UserForm.tsx`:
- Around line 190-203: The current updateModifiedFields callback (and the
similar block at the other location) treats an empty array as a deliberate
change even if the attribute didn't previously exist, which later causes
attributeTransformUtils to create a bogus empty customAttribute; modify the
logic in updateModifiedFields (and the other matching setter) to check the
original user's attributes/state before deciding to keep an empty array: if
isEmptyValue(value) && Array.isArray(value) then remove the key from
modifiedFields unless the original user already had that multi-valued attribute
(use the component's existing user/originalUser prop/state to determine
existence); otherwise keep non-empty arrays and other values as before. Ensure
you reference updateModifiedFields (and the second setter block) and
attributeTransformUtils behavior when implementing this conditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e741f78-c408-4fdd-ad23-a7806f90e109
📒 Files selected for processing (6)
admin-ui/app/components/App/AuthenticatedRouteSelector.tsxadmin-ui/app/redux/features/toastSlice.tsadmin-ui/app/redux/types/index.tsadmin-ui/app/routes/Apps/Gluu/GluuToast.tsxadmin-ui/app/utils/AppAuthProvider.tsxadmin-ui/plugins/user-management/components/UserForm.tsx
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
admin-ui/app/utils/AppAuthProvider.tsx (1)
249-252: 🧹 Nitpick | 🔵 TrivialMinor: Consider omitting
post_logout_redirect_uriwhen empty.Currently, if
localStorage.getItem('postLogoutRedirectUri')returnsnull, an empty string is set as the parameter value (post_logout_redirect_uri=). Some OAuth providers may behave unexpectedly with an empty value. Consider conditionally adding the parameter only when a value exists:Suggested improvement
endSessionUrl.searchParams.set('state', state) -endSessionUrl.searchParams.set( - 'post_logout_redirect_uri', - localStorage.getItem('postLogoutRedirectUri') ?? '', -) +const postLogoutRedirectUri = localStorage.getItem('postLogoutRedirectUri') +if (postLogoutRedirectUri) { + endSessionUrl.searchParams.set('post_logout_redirect_uri', postLogoutRedirectUri) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/utils/AppAuthProvider.tsx` around lines 249 - 252, The code currently always sets post_logout_redirect_uri to localStorage.getItem('postLogoutRedirectUri') (possibly an empty string) on endSessionUrl; change this so endSessionUrl.searchParams.set('post_logout_redirect_uri', ...) is only called when the retrieved value is non-empty (truthy) — e.g., read const redirect = localStorage.getItem('postLogoutRedirectUri') and if (redirect) then call endSessionUrl.searchParams.set(...) — locate this logic in AppAuthProvider (the endSessionUrl handling) and ensure no parameter is added when redirect is null/empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@admin-ui/app/utils/AppAuthProvider.tsx`:
- Around line 249-252: The code currently always sets post_logout_redirect_uri
to localStorage.getItem('postLogoutRedirectUri') (possibly an empty string) on
endSessionUrl; change this so
endSessionUrl.searchParams.set('post_logout_redirect_uri', ...) is only called
when the retrieved value is non-empty (truthy) — e.g., read const redirect =
localStorage.getItem('postLogoutRedirectUri') and if (redirect) then call
endSessionUrl.searchParams.set(...) — locate this logic in AppAuthProvider (the
endSessionUrl handling) and ensure no parameter is added when redirect is
null/empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b0ec723-9844-4b5b-808c-73da2b31c055
📒 Files selected for processing (2)
admin-ui/app/utils/AppAuthProvider.tsxadmin-ui/plugins/user-management/components/PasswordChangeModal.tsx
duttarnab
approved these changes
Mar 10, 2026
moabu
approved these changes
Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(admin-ui): unable to remove or reassign admin roles from user profile (#2684)
Summary
This PR fixes an issue where admin roles could not be removed or reassigned from a user's profile in the Admin UI.
Issue
When attempting to remove roles such as
jansAdminRoleoruserRolefrom a user (e.g., admin user), the operation fails and the roles cannot be updated from the UI.Fix
Result
🔗 Ticket
Closes: #2684
Summary by CodeRabbit
New Features
Bug Fixes