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

feat: CLI allows and requires creating a user with a password DET-10184 #9112

Conversation

jesse-amano-hpe
Copy link
Contributor

@jesse-amano-hpe jesse-amano-hpe commented Apr 5, 2024

Description

BREAKING CHANGE: DET-10184 The command det user create allows passing a --password flag; if neither --password nor --remote is specified, the command will interactively prompt twice for a password, as though setting a password via det user change-password. Additionally, whenever a password is changed by any CLI command or SDK method, it is checked to ensure the following criteria are met:

  • password cannot be blank
  • password must be at least 8 characters
  • password must contain at least one upper-case letter
  • password must contain at least one lower-case letter
  • password must contain at least one number

Test Plan

Several e2e and integration tests have been modified to accommodate and cover this change.

The CLI should also be manually tested by exercising the det user create command in the various possible modes:

  • --password <password> should create a user with the provided password (assuming it fits the rules above).
  • --remote should create a "remote" user with no login option until SSO is configured for them (even if --password is also given).
  • if neither --remote nor --password is provided, the CLI should prompt for a password; if no input pipe is available, this should produce an error.

Commentary

This matches the requirements of passwords set via the Web UI.

This is my eighth week at HPE/Determined. There's no need to hold back or be overly delicate in critique, but I probably have not thought through every possible way a user could interact with the SDK or CLI. It's my understanding that we're basically aligned on the security needs taking precedence over disruption to any automated flows or scripts or whatever else may have previously depended on behaviors this change breaks, however, please try to think of whether there might be any other features that will outright stop working if/when this change lands; I'm not familiar enough with the product yet to be confident I'll have good intuition about that.

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.

Ticket

DET-10184

@jesse-amano-hpe jesse-amano-hpe added User-facing API Change python Pull requests that update Python code labels Apr 5, 2024
@jesse-amano-hpe jesse-amano-hpe requested review from a team as code owners April 5, 2024 05:15
@cla-bot cla-bot bot added the cla-signed label Apr 5, 2024
@determined-ci determined-ci requested a review from a team April 5, 2024 05:16
@determined-ci determined-ci added the documentation Improvements or additions to documentation label Apr 5, 2024
Copy link

netlify bot commented Apr 5, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 0b0f5ee
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6626a7d5bc0621000893af2d
😎 Deploy Preview https://deploy-preview-9112--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@jesse-amano-hpe jesse-amano-hpe requested review from a team, amandavialva01 and eecsliu and removed request for a team and amandavialva01 April 5, 2024 05:16
@jesse-amano-hpe jesse-amano-hpe force-pushed the jta/DET-10184/feat-CLI-allows-creating-a-user-with-a-password branch from 7a7f601 to 3ddb7c3 Compare April 5, 2024 05:18
@determined-ci determined-ci requested a review from a team April 5, 2024 05:18
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

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

Project coverage is 44.68%. Comparing base (8caf3cb) to head (0b0f5ee).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9112      +/-   ##
==========================================
+ Coverage   44.64%   44.68%   +0.03%     
==========================================
  Files        1270     1270              
  Lines      155045   155132      +87     
  Branches     2443     2443              
==========================================
+ Hits        69227    69324      +97     
+ Misses      85582    85572      -10     
  Partials      236      236              
Flag Coverage Δ
backend 41.51% <ø> (-0.06%) ⬇️
harness 64.31% <98.92%> (+0.17%) ⬆️
web 35.23% <ø> (ø)

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

Files Coverage Δ
harness/determined/common/api/authentication.py 87.19% <100.00%> (+1.75%) ⬆️
...rness/determined/common/experimental/determined.py 46.70% <100.00%> (+5.49%) ⬆️
harness/determined/common/experimental/user.py 64.51% <100.00%> (+0.58%) ⬆️
harness/determined/experimental/client.py 55.26% <ø> (ø)
harness/tests/cli/test_user.py 100.00% <100.00%> (ø)
harness/tests/common/api/test_authentication.py 100.00% <100.00%> (ø)
harness/determined/cli/user.py 54.81% <85.71%> (+5.97%) ⬆️

... and 7 files with indirect coverage changes

@determined-ci determined-ci requested a review from a team April 5, 2024 14:30
Copy link
Member

@tara-det-ai tara-det-ai left a comment

Choose a reason for hiding this comment

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

suggested edits

@determined-ci determined-ci requested a review from a team April 5, 2024 14:43
@jesse-amano-hpe jesse-amano-hpe requested review from wes-turner and removed request for wes-turner and jgongd April 8, 2024 19:48
@jesse-amano-hpe
Copy link
Contributor Author

@wes-turner Jerry brought up that I should probably get you involved since this is mostly a CLI design/UX concern. Happy to discuss in here or on Slack, whatever makes sense to you :)

@jesse-amano-hpe jesse-amano-hpe force-pushed the jta/DET-10184/feat-CLI-allows-creating-a-user-with-a-password branch from 0e06f43 to 0b0f5ee Compare April 22, 2024 18:09
@jesse-amano-hpe jesse-amano-hpe merged commit da8a040 into main Apr 22, 2024
83 of 97 checks passed
@jesse-amano-hpe jesse-amano-hpe deleted the jta/DET-10184/feat-CLI-allows-creating-a-user-with-a-password branch April 22, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed documentation Improvements or additions to documentation python Pull requests that update Python code User-facing API Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants