Skip to content
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

ui/user: Allow setting/changing basic auth passwords in UI #2991

Merged
merged 33 commits into from
Jun 8, 2023

Conversation

allending313
Copy link
Contributor

@allending313 allending313 commented May 4, 2023

  • Identified the issue which this PR solves.
  • Read the CONTRIBUTING document.
  • Code builds clean without any errors or warnings.
  • Added appropriate tests for any new functionality.
  • All new and existing tests passed.
  • Added comments in the code, where necessary.
  • Ran make check to catch common errors. Fixed any that came up.

Description:
Provides a way for users and admins to reset a basic auth password through the UI.

  • Updated the edit user detail modal to support resetting a password.
  • The edit detail button will be available to non-admin users when viewing their own user profile page (edit role checkbox will be unavailable if they are not an admin).
  • Admins will be able to update users' passwords without needing to enter the user's old password.

Which issue(s) this PR fixes:
Fixes #2667

Out of Scope:
Forgot password feature

Screenshots:
Non-Admin view
Screenshot 2023-05-04 at 6 08 17 PM

Admin view
Screenshot 2023-05-04 at 6 08 41 PM

Describe any introduced user-facing changes:

  • Updated user edit details modal to support resetting password.
  • Slightly refactored user profile page option buttons (delete, edit, favorite)

Describe any introduced API changes:

  • Created gql endpoint to update user password

@allending313 allending313 self-assigned this May 4, 2023
@github-actions github-actions bot added the size/l label May 4, 2023
@allending313 allending313 changed the title feat(#2667) basic auth reset password feat: basic auth reset password May 4, 2023
@allending313 allending313 requested a review from Forfold May 5, 2023 14:15
@AlaricWhitney
Copy link
Collaborator

@allending313 it looks like there's some conflicts that need to be resolved.

allending313 and others added 5 commits May 22, 2023 08:19
Bumps [playwright](https://github.com/Microsoft/playwright) from 1.32.0 to 1.33.0.
- [Release notes](https://github.com/Microsoft/playwright/releases)
- [Commits](microsoft/playwright@v1.32.0...v1.33.0)

---
updated-dependencies:
- dependency-name: playwright
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github.com/coreos/go-oidc/v3](https://github.com/coreos/go-oidc) from 3.5.0 to 3.6.0.
- [Release notes](https://github.com/coreos/go-oidc/releases)
- [Commits](coreos/go-oidc@v3.5.0...v3.6.0)

---
updated-dependencies:
- dependency-name: github.com/coreos/go-oidc/v3
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [@mui/lab](https://github.com/mui/material-ui/tree/HEAD/packages/mui-lab) from 5.0.0-alpha.128 to 5.0.0-alpha.130.
- [Release notes](https://github.com/mui/material-ui/releases)
- [Changelog](https://github.com/mui/material-ui/blob/master/CHANGELOG.md)
- [Commits](https://github.com/mui/material-ui/commits/HEAD/packages/mui-lab)

---
updated-dependencies:
- dependency-name: "@mui/lab"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
@allending313 allending313 force-pushed the feat/2667-reset-password branch from f56b49f to 2dcbd4b Compare May 22, 2023 15:46
allending313 and others added 3 commits May 23, 2023 13:42
Copy link
Member

@mastercactapus mastercactapus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good. Functionally validated and seeing a couple of issues to address:

  • if Auth.DisableBasic is set, this form should be disabled with a message (e.g., "password authentication is currently disabled")
  • It might be a good idea to require oldPassword for the current user, even if they are admin
  • user accounts created via OIDC or GitHub (i.e., no entry in auth_basic_users) don't get one set by admin -- "succeeds" but basic auth doesn't work for the user
  • user accounts created via OIDC or GitHub (i.e., no entry in auth_basic_users) get an invalid ID when trying to set it for themselves

@allending313
Copy link
Contributor Author

allending313 commented May 30, 2023

for accounts created via OIDC/Github, should we hide the pw reset fields for those user accounts? (not quite sure how to check for account creation method)
or should the backend check for this and return an error message if users try to reset the pw for an account not created via basic auth (entry not found in auth_basic_users)

@mastercactapus
Copy link
Member

@allending313 Based on recent discussions, disable rather than hide. It tends to be confusing when pieces of the UI are missing entirely. A little text about why it's disabled would also probably help.

I'm thinking maybe we can add a hasPassword to the User type, which is only accessible to the current user, or an admin, and in the case of hasPassword === false, then we'd hide the "Old password" field and not require it.

We just need to be sure that while an admin can "upsert" a new password, a user can only update an existing one if the old password is specified.

@mastercactapus mastercactapus changed the title feat: basic auth reset password ui/user: Allow setting/changing basic auth passwords in UI Jun 1, 2023
mastercactapus
mastercactapus previously approved these changes Jun 7, 2023
@andrewbenington
Copy link
Contributor

Screenshot 2023-06-08 at 10 51 27 AM
What is the Username field used for? it doesn't seem to correspond to the display name or email

@allending313
Copy link
Contributor Author

allending313 commented Jun 8, 2023

Screenshot 2023-06-08 at 10 51 27 AM What is the Username field used for? it doesn't seem to correspond to the display name or email

The Username field is there for admins to create a basic auth AuthSubject for a user. Normally when a user already has basic auth as an AuthSubject, the pw reset feature would just update that basic auth password. However, if they don't already have an existing basic auth AuthSubject we want admins to have the ability to create one for the user which requires a username for login.

@allending313 allending313 merged commit a3d9a02 into master Jun 8, 2023
@allending313 allending313 deleted the feat/2667-reset-password branch June 8, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Self reset password function for user
6 participants