feat: webui create user prompts for password [DET-10221] #9240

jesse-amano-hpe · 2024-04-24T20:49:01Z

Ticket

DET-10221 feat: prompt for password when creating a user in web UI

Description

BREAKING CHANGE: adds a password field to the "add user" and "edit user" modals. The password is required when creating a new user; if left blank while editing an existing user, their password will remain unchanged.

Test Plan

The unit test for this modal has been updated to ensure a password is used and the form submits properly.

To test manually, log in as an admin, and ensure that an invalid password cannot be submitted when adding a new user, and a valid one can. With an SSO provider enabled, marking a created user as Remote should remove the password prompt and requirements.

Known issues:

The "User Password required" error message may display redundantly and repeatedly, especially when deleting and retyping a password multiple times. This seems to be a bug in the React component for required fields. It also happens on the existing "change my own password" workflow in the User Settings drawer.

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

codecov · 2024-04-24T20:49:10Z

Codecov Report

Attention: Patch coverage is 62.32877% with 55 lines in your changes are missing coverage. Please review.

Project coverage is 44.61%. Comparing base (2905180) to head (df77cfc).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #9240    +/-   ##
========================================
  Coverage   44.60%   44.61%            
========================================
  Files        1273     1274     +1     
  Lines      155831   155942   +111     
  Branches     2439     2447     +8     
========================================
+ Hits        69504    69567    +63     
- Misses      86088    86136    +48     
  Partials      239      239

Flag	Coverage Δ
backend	`41.75% <ø> (-0.01%)`	⬇️
harness	`64.05% <ø> (ø)`
web	`35.07% <62.32%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
webui/react/src/components/UserSettings.tsx	`82.60% <100.00%> (-0.90%)`	⬇️
webui/react/src/constants/passwordRules.ts	`100.00% <100.00%> (ø)`
...react/src/e2e/models/components/CreateUserModal.ts	`0.00% <0.00%> (ø)`
webui/react/src/components/CreateUserModal.tsx	`88.10% <81.25%> (-2.20%)`	⬇️
webui/react/src/e2e/fixtures/user.fixture.ts	`0.00% <0.00%> (ø)`

... and 3 files with indirect coverage changes

netlify · 2024-04-24T20:49:23Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`df77cfc`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66313c66e81ce6000821991d
😎 Deploy Preview	https://deploy-preview-9240--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

keita-determined

just noticed warnings in case reviewer miss it. (im a non-reviewer :))

Screen.Recording.2024-04-24.at.2.12.06.PM.mov

djanicekpach · 2024-04-25T15:08:57Z

webui/react/src/e2e/fixtures/user.fixture.ts

@@ -51,16 +55,17 @@ export class UserFixture {
    username = safeName('test-user'),
    displayName,
    isAdmin,
+    password = 'testPassword1',


Can we pull this from an env variable in circle CI? The playwright context should probably be the context for it. We should avoid hardcoding passwords in these tests so that they are environment agnostic.

if it's required upon create, I'd suggest a new EditArgs interface with the optional password

djanicekpach · 2024-04-25T15:16:57Z

webui/react/src/e2e/fixtures/user.fixture.ts

    if (username !== undefined) {
      await this.userManagementPage.createUserModal.username.pwLocator.fill(username);
    }
    if (displayName !== undefined) {
      await this.userManagementPage.createUserModal.displayName.pwLocator.fill(displayName);
    }
+    if (password !== undefined) {


I believe tests are failing because the UI doesn't allow blank passwords on edit. Is that OK? Do users need to update their password whenever they edit? That seems like it should only be required on creation to me.

If we are sure about this behavior then the edit test needs an update.
Line ref:

determined/webui/react/src/e2e/tests/userManagement.spec.ts

Line 114 in 985daeb

test('Edit user', async ({ page, user }) => {

No, I agree, the intent was that a blank password on the edit modal should mean "keep the password the same" and it shouldn't be required if the user exists already.

djanicekpach

I see why the e2 tests fail, but I think the app behavior is a bit odd there. Let me know your thoughts.

webui/react/src/e2e/fixtures/user.fixture.ts

JComins000 · 2024-04-25T15:40:35Z

webui/react/src/e2e/models/components/CreateUserModal.ts

@@ -25,6 +25,10 @@ export class CreateUserModal extends Modal {
    parent: this.body,
    selector: '[data-testid="isRemote"] button',
  });
+  readonly password: BaseComponent = new BaseComponent({


can you include the form area so we can get PASSWORD_RULES and check for those appearing properly? it could be in another PR if you want

… around remote users not needing passwords

Co-Authored-By: justin.comins@hpe.com

…ate-user-password-prompt

Co-Authored-By: justin.comins@hpe.com

jesse-amano-hpe · 2024-04-26T21:44:41Z

All right, thanks to @JComins000 e2e tests are passing, and with his tests merged in there's pretty good coverage for what happens to a user after their password is set or changed with this new workflow as well. @djanicekpach would you mind taking another look when you have time?

JComins000

good stuff! help me out with PRs to readme.md and password validation later :-)

gt2345

Password prompts work as expected 👍 Should we mention this in release notes?

Also want to mention that usually there are two inputs of passwords, something like

I don't feel too strongly about this, it's up to you and/or design.

One thing I noticed is when rabc enabled, the password field does not show up when remote is on and off, is this expected? Shouldn't we allow setting password when remote is off?

jesse-amano-hpe · 2024-04-30T16:29:54Z

@gt2345 Thanks for the review!

Password prompts work as expected 👍 Should we mention this in release notes?

Added.

Also want to mention that usually there are two inputs of passwords[.] I don't feel too strongly about this, it's up to you and/or design.

I'd personally prefer to go without, since there's a visibility toggle on the field already; not sure who else I'd confirm design with here, but I'll double-check with my team.

One thing I noticed is when rabc enabled, the password field does not show up when remote is on and off, is this expected? Shouldn't we allow setting password when remote is off?

Great catch, I missed parentheses over a group of conditions, but on another go through it, the rbac check was redundant anyway. Should be fixed now. Thanks!

jesse-amano-hpe · 2024-04-30T17:04:38Z

I'll add the password confirmation field shortly.

…ate-user-password-prompt

gt2345

Thanks for the fix.

I noticed that with rbac enabled, when editing modal opens for remote user, the password field would show up even though it shouldn't

It also seems like we can submit password change for remote user, and backend would throw an error.

Using OSS, we can submit password changes for remote user. I'm not sure this would happen during production though.
Using rbac, we can first change a remote user to non-remote user, then submitting password change would trigger the error. I think this maybe related to the backend.

jesse-amano-hpe · 2024-04-30T18:47:26Z

Thanks for the fix.

It also seems like we can submit password change for remote user, and backend would throw an error.

Using OSS, we can submit password changes for remote user. I'm not sure this would happen during production though.

Using rbac, we can first change a remote user to non-remote user, then submitting password change would trigger the error. I think this maybe related to the backend.

@gt2345 looks like you caught another typo. df77cfc fixes it. Thank you!

gt2345

Password prompts work as expected 👍

jesse-amano-hpe added 4 commits April 24, 2024 10:45

feat: Create User modal sends password in API request

fc66611

feat: Create User modal prompts for password

67a404a

feat: show password validation in webui when creating a user

5fa697f

test: update unit test for create/edit user modal

01c7f5d

jesse-amano-hpe requested a review from a team as a code owner April 24, 2024 20:49

jesse-amano-hpe requested a review from gt2345 April 24, 2024 20:49

cla-bot bot added the cla-signed label Apr 24, 2024

keita-determined reviewed Apr 24, 2024

View reviewed changes

fix: edit modal

0883e91

jesse-amano-hpe requested a review from a team as a code owner April 25, 2024 04:32

carolinaecalderon added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Apr 25, 2024

djanicekpach approved these changes Apr 25, 2024

View reviewed changes

djanicekpach self-requested a review April 25, 2024 15:17

djanicekpach requested changes Apr 25, 2024

View reviewed changes

JComins000 reviewed Apr 25, 2024

View reviewed changes

webui/react/src/e2e/fixtures/user.fixture.ts Show resolved Hide resolved

JComins000 reviewed Apr 25, 2024

View reviewed changes

jesse-amano-hpe added 3 commits April 25, 2024 22:18

test: fix end-to-end test for edit modal

d5a0485

fix: allow edit modal to proceed with no password; also correct logic…

eb08a30

… around remote users not needing passwords

test: use env config to set passwords in tests

02be600

jesse-amano-hpe force-pushed the jta/DET-10221/web-create-user-password-prompt branch from a7e5385 to 4170c33 Compare April 26, 2024 08:11

jesse-amano-hpe marked this pull request as draft April 26, 2024 08:12

test: let test wait a couple seconds after setting a new user password

b4493a0

jesse-amano-hpe force-pushed the jta/DET-10221/web-create-user-password-prompt branch from 4170c33 to b4493a0 Compare April 26, 2024 08:38

test: slightly better arrangement of params for user edit fixtures

be61f75

jesse-amano-hpe force-pushed the jta/DET-10221/web-create-user-password-prompt branch from 6dc10d6 to be61f75 Compare April 26, 2024 16:26

jesse-amano-hpe added 3 commits April 26, 2024 10:48

revertme: troubleshoot circleci

5a7f086

test: adjust timing and ordering of args for create/edit user modal

1d0ee57

Co-Authored-By: justin.comins@hpe.com

Merge remote-tracking branch 'origin/main' into jta/DET-10221/web-cre…

c43e9df

…ate-user-password-prompt

jesse-amano-hpe added 2 commits April 26, 2024 13:18

test: created user logs in with password

f4cd088

Co-Authored-By: justin.comins@hpe.com

test: longer timeout delay for toast after creating user with password

03eb263

jesse-amano-hpe marked this pull request as ready for review April 26, 2024 21:41

jesse-amano-hpe requested a review from djanicekpach April 26, 2024 21:41

JComins000 approved these changes Apr 26, 2024

View reviewed changes

carolinaecalderon removed the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Apr 26, 2024

djanicekpach approved these changes Apr 29, 2024

View reviewed changes

jesse-amano-hpe enabled auto-merge (squash) April 29, 2024 18:33

gt2345 reviewed Apr 29, 2024

View reviewed changes

fix show/hide logic for passwords for remote users

342e1d5

determined-ci requested a review from a team April 30, 2024 16:22

determined-ci added the documentation Improvements or additions to documentation label Apr 30, 2024

jesse-amano-hpe disabled auto-merge April 30, 2024 17:00

docs: add documentation for web ui password change

e347655

jesse-amano-hpe added 2 commits April 30, 2024 10:35

feat: add Confirm Password to user create/edit modal

e1fc317

Merge remote-tracking branch 'origin/main' into jta/DET-10221/web-cre…

6004eb0

…ate-user-password-prompt

jesse-amano-hpe force-pushed the jta/DET-10221/web-create-user-password-prompt branch from c5fbd98 to 6004eb0 Compare April 30, 2024 17:36

jesse-amano-hpe requested a review from gt2345 April 30, 2024 17:36

test: fix unit test for confirm password

8a60454

gt2345 reviewed Apr 30, 2024

View reviewed changes

fix: typo

df77cfc

gt2345 approved these changes Apr 30, 2024

View reviewed changes

jesse-amano-hpe merged commit 9c068d2 into main Apr 30, 2024
85 of 100 checks passed

jesse-amano-hpe deleted the jta/DET-10221/web-create-user-password-prompt branch April 30, 2024 22:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: webui create user prompts for password [DET-10221] #9240

feat: webui create user prompts for password [DET-10221] #9240

jesse-amano-hpe commented Apr 24, 2024 •

edited

Loading

codecov bot commented Apr 24, 2024 •

edited

Loading

netlify bot commented Apr 24, 2024 •

edited

Loading

keita-determined left a comment •

edited

Loading

djanicekpach Apr 25, 2024

JComins000 Apr 25, 2024 •

edited

Loading

djanicekpach Apr 25, 2024

jesse-amano-hpe Apr 25, 2024

djanicekpach left a comment

JComins000 Apr 25, 2024

jesse-amano-hpe commented Apr 26, 2024

JComins000 left a comment

gt2345 left a comment

jesse-amano-hpe commented Apr 30, 2024

jesse-amano-hpe commented Apr 30, 2024

gt2345 left a comment

jesse-amano-hpe commented Apr 30, 2024 •

edited

Loading

gt2345 left a comment

feat: webui create user prompts for password [DET-10221] #9240

feat: webui create user prompts for password [DET-10221] #9240

Conversation

jesse-amano-hpe commented Apr 24, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

codecov bot commented Apr 24, 2024 • edited Loading

Codecov Report

netlify bot commented Apr 24, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

keita-determined left a comment • edited Loading

Choose a reason for hiding this comment

djanicekpach Apr 25, 2024

Choose a reason for hiding this comment

JComins000 Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

djanicekpach Apr 25, 2024

Choose a reason for hiding this comment

jesse-amano-hpe Apr 25, 2024

Choose a reason for hiding this comment

djanicekpach left a comment

Choose a reason for hiding this comment

JComins000 Apr 25, 2024

Choose a reason for hiding this comment

jesse-amano-hpe commented Apr 26, 2024

JComins000 left a comment

Choose a reason for hiding this comment

gt2345 left a comment

Choose a reason for hiding this comment

jesse-amano-hpe commented Apr 30, 2024

jesse-amano-hpe commented Apr 30, 2024

gt2345 left a comment

Choose a reason for hiding this comment

jesse-amano-hpe commented Apr 30, 2024 • edited Loading

gt2345 left a comment

Choose a reason for hiding this comment

jesse-amano-hpe commented Apr 24, 2024 •

edited

Loading

codecov bot commented Apr 24, 2024 •

edited

Loading

netlify bot commented Apr 24, 2024 •

edited

Loading

keita-determined left a comment •

edited

Loading

JComins000 Apr 25, 2024 •

edited

Loading

jesse-amano-hpe commented Apr 30, 2024 •

edited

Loading