-
Notifications
You must be signed in to change notification settings - Fork 49
fix(common-auth): current user cannot be disabled #1852
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
fix(common-auth): current user cannot be disabled #1852
Conversation
Reviewer's GuideDisable the activation toggle for the currently logged-in user by integrating the frontend auth context and binding a disable state to the switch, preventing users from locking themselves out. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
cb7917e
to
0bd9092
Compare
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.
Hey @karelhala - I've reviewed your changes - here's some feedback:
- Consider moving the current username fetch out of each ActivateToggle into a parent/context so you don’t call auth.getUser for every row (and avoid flicker/performance hits).
- Initialize isDisabled to a boolean (e.g. false) instead of undefined to prevent the Switch from being in an indeterminate state before the async effect resolves.
- Use strict equality (===) when comparing user.username and the fetched username to avoid unintended type coercion.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
useEffect(() => { | ||
const getUser = async () => { | ||
const username = (await auth.getUser())?.identity?.user?.username as string; | ||
setIsDisabled(user.username == username); | ||
}; | ||
getUser(); | ||
}, [auth]); |
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.
issue (bug_risk): Include user.username in the dependency array
Since isDisabled depends on user.username, include it in the dependency array to ensure the effect updates when user changes.
0bd9092
to
3f01099
Compare
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.
Code looks good. Do we need some tooltip or message? Is it clear to the user what they have to do?
3f01099
to
838e29d
Compare
@Hyperkid123 good point on the tooltip. The option to mark user as org admin or not is disabled as well. Let's see if customers will want some tooltip or not. FYI I've changed the logic as username felt too britle (users can change their username) so I'm comparing external source ID and account ID. |
Description
If logged in user disables itself and is last org admin they have to contact redhat in order to re-enable the account back. API doesn't have a check for this for now and so users could block themselves from accessing any redhat app. This PR checks if external ID is the same as currently logged in user's account ID and if so disable the option to disable / enable user activity.
RHCLOUD-40473
Screenshots
Before:
After:
Checklist ☑️
Summary by Sourcery
Bug Fixes: