fix: cli gives misleading error message when logging in with a bad password [MD-277] #8990

jgongd · 2024-03-12T20:07:56Z

Description

When a user attempts to log in with an incorrect username and/or password, CLI display a misleading error message:

Failed to log in user: Unauthenticated: Please use 'det user login <username>' for password login, or for Enterprise users logging in with an SSO provider, use 'det auth login --provider=<provider>'.

Test Plan

det user logout

Login with incorrect password and/or username

det user login

Expect "Failed to log in user: Invalid username/password combination. Please try again." as the output.

Commentary (optional)

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

MD-277

codecov · 2024-03-12T20:08:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.22%. Comparing base (3f7a396) to head (0c24f33).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8990   +/-   ##
=======================================
  Coverage   46.22%   46.22%           
=======================================
  Files        1175     1175           
  Lines      145341   145357   +16     
  Branches     2414     2414           
=======================================
+ Hits        67180    67197   +17     
+ Misses      77952    77951    -1     
  Partials      209      209

Flag	Coverage Δ
backend	`43.76% <ø> (-0.02%)`	⬇️
harness	`64.03% <100.00%> (+0.03%)`	⬆️
web	`36.77% <ø> (ø)`

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

Files	Coverage Δ
harness/determined/cli/user.py	`48.83% <100.00%> (+5.98%)`	⬆️
harness/determined/common/api/errors.py	`76.36% <100.00%> (+1.36%)`	⬆️
harness/tests/cli/test_user.py	`100.00% <100.00%> (ø)`

... and 7 files with indirect coverage changes

netlify · 2024-03-12T20:08:14Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`0c24f33`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6618095077b7f8000866b61d
😎 Deploy Preview	https://deploy-preview-8990--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.

harness/determined/common/api/errors.py

harness/determined/cli/user.py

wes-turner · 2024-04-02T22:35:26Z

harness/tests/cli/test_user.py

+    mock_die: mock.MagicMock, mock_getpass: mock.MagicMock
+) -> None:
+    mock_getpass.side_effect = lambda *_: "newpass"
+    with responses.RequestsMock(


The context manager version of this works, but since you're not doing anything complicated, the easier way to do this is to use the responses.activate decorator. It uses the default registry, which should be a little less brittle than the OrderedRegistry, anyway.

@mock.patch... @responses.activate def test_log... ... util.expect_get_info() # take a look at the code here for another example of the pattern with `responses.activate` responses.post(...)

I agree OrderedRegistry is not necessary for this test.

wes-turner · 2024-04-02T22:48:20Z

harness/tests/cli/test_user.py

@@ -47,3 +48,29 @@ def test_user_edit_no_fields(mock_die: mock.MagicMock) -> None:
        mock_die.assert_has_calls(
            [mock.call("No field provided. Use 'det user edit -h' for usage.", exit_code=1)]
        )
+
+
+@mock.patch("getpass.getpass")


It shouldn't be too dangerous, but this code can touch the actual filesystem via the TokenStore. For this reason, I recommend patching determined.common.api.authentication.TokenStore, too. I'm fine letting using the default mock class as its target (as is done when you don't pass a target to its decorator)

@mock.patch("determined.common.api.authentication.TokenStore")

But since we have a fake TokenStore, you can also do

@mock.patch("determined.common.api.authentication.TokenStore", util.MockTokenStore)

if you like. I'm not sure which I'd choose, myself. More complex or more brittle, seems like a toss-up. Including the option to help you get up to speed with Python unittest.mock.

That's a great point. It better to avoid touching the filesystem in this test (mock external dependency).

wes-turner · 2024-04-10T02:02:23Z

harness/tests/cli/test_user.py

+@mock.patch("determined.common.api.authentication.TokenStore", util.MockTokenStore(strict=False))
+@mock.patch("getpass.getpass", lambda *_: "newpass")
+@mock.patch("determined.cli.cli.die")
+def test_login_returns_invalid_credentials_error_message(mock_die: mock.MagicMock) -> None:


One comment I have left is that the naming on this method is a little wonky since login isn't isn't really "returning". "dies with" is the first thing that comes to my own mind, but I'm sure there are other good options, too. Whatever you think is best, no need to check back in.

wes-turner

Small patch, but I really like how this turned out. I'm still pleased with the InvalidCredentialsException subclass.

…ssword

jgongd requested a review from wes-turner March 12, 2024 20:07

jgongd requested review from a team as code owners March 12, 2024 20:07

jgongd requested a review from salonig23 March 12, 2024 20:07

cla-bot bot added the cla-signed label Mar 12, 2024

jgongd force-pushed the jgong/fix/misleading-login-message branch 2 times, most recently from bf900a8 to 48e4505 Compare March 12, 2024 20:28

wes-turner reviewed Mar 13, 2024

View reviewed changes

harness/determined/common/api/errors.py Outdated Show resolved Hide resolved

harness/determined/common/api/errors.py Outdated Show resolved Hide resolved

harness/determined/cli/user.py Show resolved Hide resolved

jgongd force-pushed the jgong/fix/misleading-login-message branch from d4a34c3 to d767c32 Compare April 1, 2024 21:15

jgongd requested a review from a team as a code owner April 1, 2024 21:15

jgongd requested a review from gt2345 April 1, 2024 21:15

jgongd force-pushed the jgong/fix/misleading-login-message branch 2 times, most recently from 806349d to dc136dd Compare April 1, 2024 21:24

jgongd requested review from wes-turner and removed request for a team, salonig23 and gt2345 April 1, 2024 21:25

wes-turner reviewed Apr 2, 2024

View reviewed changes

jgongd force-pushed the jgong/fix/misleading-login-message branch 3 times, most recently from 0dca7d7 to 0cd4a6e Compare April 4, 2024 15:11

jgongd requested a review from wes-turner April 4, 2024 15:13

wes-turner reviewed Apr 10, 2024

View reviewed changes

wes-turner approved these changes Apr 10, 2024

View reviewed changes

jgongd added 3 commits April 11, 2024 11:59

fix: cli gives misleading error message when logging in with a bad pa…

1645151

…ssword

lint

edd5368

Convert integration tests to a unit test

b95705f

jgongd added 5 commits April 11, 2024 11:59

lint

c8688aa

lint

cdbbafd

Address comments

4d974dd

Remove test det login calls the login endpoint

81c795f

address comments

0c24f33

jgongd force-pushed the jgong/fix/misleading-login-message branch from 6f4419b to 0c24f33 Compare April 11, 2024 16:01

jgongd merged commit bd29f1f into main Apr 11, 2024
71 of 84 checks passed

jgongd deleted the jgong/fix/misleading-login-message branch April 11, 2024 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cli gives misleading error message when logging in with a bad password [MD-277] #8990

fix: cli gives misleading error message when logging in with a bad password [MD-277] #8990

jgongd commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

netlify bot commented Mar 12, 2024 •

edited

Loading

wes-turner Apr 2, 2024

jgongd Apr 3, 2024

wes-turner Apr 2, 2024 •

edited

Loading

jgongd Apr 3, 2024 •

edited

Loading

wes-turner Apr 10, 2024

wes-turner left a comment

fix: cli gives misleading error message when logging in with a bad password [MD-277] #8990

fix: cli gives misleading error message when logging in with a bad password [MD-277] #8990

Conversation

jgongd commented Mar 12, 2024 • edited Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

netlify bot commented Mar 12, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

wes-turner Apr 2, 2024

Choose a reason for hiding this comment

jgongd Apr 3, 2024

Choose a reason for hiding this comment

wes-turner Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

jgongd Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

wes-turner Apr 10, 2024

Choose a reason for hiding this comment

wes-turner left a comment

Choose a reason for hiding this comment

jgongd commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

netlify bot commented Mar 12, 2024 •

edited

Loading

wes-turner Apr 2, 2024 •

edited

Loading

jgongd Apr 3, 2024 •

edited

Loading