-
Notifications
You must be signed in to change notification settings - Fork 0
Feature: add detailed user info update for admin panel #14
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
Conversation
- add `features` for UserApi - add `reset password` and `Features` for user info update
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e967ba486d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| let isPasswordReused | ||
| try { | ||
| isPasswordReused = await HaveIBeenPwned.promises.isPasswordReused(value) |
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.
Call exported HIBP checker for admin password updates
When an admin sets a password, this calls HaveIBeenPwned.promises.isPasswordReused, but HaveIBeenPwned only exports promises.checkPasswordForReuse (see services/web/app/src/Features/Authentication/HaveIBeenPwned.mjs). The resulting TypeError is swallowed by the catch, so the reused-password check is silently skipped and weak/reused passwords can still be accepted.
Useful? React with 👍 / 👎.
| // delete password from response for security reasons | ||
| if (update.password) { | ||
| delete update.password |
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.
Remove hashed password from update response
In the password-update path, update.hashedPassword is set and later returned via res.json(update), but this sanitization block only deletes update.password. As a result, changing a user's password exposes the bcrypt hash in the API response and propagates it to frontend state (performUpdateUser merges response fields), which is an unnecessary credential leak.
Useful? React with 👍 / 👎.
|
@codex, I have fixed now |
|
Thanks — I verified the two previously flagged P1 issues appear fixed in the current branch. What I checked
Checks run
No additional code changes were needed from my side, so I did not create a commit or PR. |
|
Need to test on cloud VM before merged. I am interested in if some features is really limited by db params. |
User Admin Panel need to be more specific, so we add some changes to CEP admin panel.
featuresfor UserApireset passwordandFeaturesfor user info updateDescription
We want a more detailed admin panel for user management.
Related issues / Pull Requests
Contributor Agreement