{VM} Fix #14897: display all requirements for vm admin password when it's invalid by houk-ms · Pull Request #14902 · Azure/azure-cli

houk-ms · 2020-08-25T06:39:54Z

Description

This PR fix issue #14897.

Before
When creating a vm with an invalid admin password, the message shows only the current rule the password violates. When user changes the password and try it again, another error may happen showing that the user violates anthoer rule.

After
This PR improves the behavior by displaying all the requirements once we found the password is invalid.

Testing Guide

Before

Admin Password:
Confirm Admin Password:
The password length must be between 12 and 123

Admin Password:
Confirm Admin Password:
Password must have the 3 of the following: 1 lower case character, 1 upper case character, 1 number and 1 special character

After

Your password is invalid for it violates Rule 1
Rule 1: The password length must be betwween 12 and 123
Rule 2: Password must have the 3 of the following: 1 lower case character, 1 upper case character, 1 number and 1 special character

src/azure-cli/azure/cli/command_modules/vm/_validators.py

yungezz · 2020-08-25T07:58:43Z

src/azure-cli/azure/cli/command_modules/vm/_validators.py

    if len(password) not in range(min_length, max_length + 1):
-        raise CLIError('The password length must be between {} and {}'.format(min_length,
-                                                                              max_length))
+        raise CLIError("Your password is invalid for it violates Rule 1\n{}".format(error_msg))


LGTM. why not make it simpler. error_msg = Password length must be between {} and {}, and must have the 3 of following....". In CLIError, just message "Your password doesn't meet requirement, pls see password constraint: {}".format(error_msg)

I think user knows where is error better when we split the error cases.

Maybe we can just show the rule that the user violates instead of printing all rules.

yungezz · 2020-08-25T07:59:18Z

thanks for quick fix!

mmyyrroonn · 2020-08-25T09:13:09Z

/azp run

azure-pipelines · 2020-08-25T09:13:22Z

Azure Pipelines successfully started running 2 pipeline(s).

qwordy · 2020-08-26T01:48:07Z

src/azure-cli/azure/cli/command_modules/vm/_validators.py

    # pylint: disable=line-too-long
    if count < 3:
-        raise CLIError('Password must have the 3 of the following: 1 lower case character, 1 upper case character, 1 number and 1 special character')
+        raise CLIError("Your password is invalid for it violates Rule 2\n{}".format(error_msg))


Maybe we can just show the rule that the user violates instead of printing all rules.

That's exactly what the issue was open for. They want to know all the rules once they failed :(

I suggest create a survey to collect users' opinions widely. That's just one user's appeal.

Our PM opened this issue, I think she knows the users well ~

qwordy · 2020-09-07T09:29:10Z

I test it in Portal. Portal will only show the rule that you violates.

This PR makes sense. But I am not sure which one is better. Both of them are OK.

chenlomis · 2020-09-07T17:49:57Z

Thank you all for the great discussion! Was informed of this new issue and thought I could leave a note to help clarify the ask:

The system logic @qwordy laid out makes sense and I like how it also aligns with portal's. If we're merely checking whether the user PWD has passed certain criterion from the CLI side then there's no fault in checking those conditions one by one.

However from the user perspective we should make clear of all the PWD requirements once they encounter the error to provide guidance and to prevent additional UX papercuts

Here's the scenario --- suppose Bob is looking to set the admin password for his new VM with a short password only to encounter the "the password length must be between 12 and 123" error. When he sees that, he'd only attempt to lengthen the password since he's unaware of any other PWD requirements. Little did he know that he will actually fail once more because the new PWD is highly likely not complicated enough.

So in this case, there's a very high chance that Bob (and hundreds others like Bob) has to spend 2x time to come up with an appropriate and acceptable password

But if we make all the PWD requirements clear from the very beginning, then Bob can take into account all of those when coming up with an appropriate password once he hits the initial error

So I'm perfectly content if we:

i) only show the "password too short" error if the password is too short but is complicated enough;
ii) only show the "password not complicated enough" error if the password is too simple but is long enough;
iii) show both "password too short" and "password not complicated enough" if the pwd violates both

The original request maps to iii) above

Hope the context makes sense...

arrownj · 2020-09-08T02:41:45Z

👍 💯 👍

arrownj · 2020-09-08T02:47:56Z

I agree with showing all the rules to user from the very beginning. Actually the user experiences are different between portal and CLI, so I think we do not need to follow portal's behavior.

However the "Rule 1, Rule 2" message seems a little imperative, suggest to combine them together and make it more friendly.

houk-ms · 2020-09-08T03:00:25Z

Thanks everyone! With these discussions, I think the behavior here is what we agree on

Admin Password: abcdef
Confirm Admin Password: abcdef
The password length must be between 12 and 123. Password must have the 3 of the following: 1 lower case character, 1 upper case character, 1 number and 1 special character

Admin Password: aA1
Confirm Admin Password: aA1
The password length must be between 12 and 123

Admin Password: 123456789012
Confirm Admin Password: 123456789012
Password must have the 3 of the following: 1 lower case character, 1 upper case character, 1 number and 1 special character

yungezz · 2020-09-08T03:22:13Z

Thanks everyone! With these discussions, I think the behavior here is what we agree on

Admin Password: abcdef
Confirm Admin Password: abcdef
The password length must be between 12 and 123. Password must have the 3 of the following: 1 lower case character, 1 upper case character, 1 number and 1 special character

Admin Password: aA1
Confirm Admin Password: aA1
The password length must be between 12 and 123

Admin Password: 123456789012
Confirm Admin Password: 123456789012
Password must have the 3 of the following: 1 lower case character, 1 upper case character, 1 number and 1 special character

what's the difference between this one and original issue? I agree with @chenlomis as a client tool it's better to show the constraint to user completely. On portal, it's very easy to show it with UI, user types something, the error shows immediately. while in Azure CLI, user need to run command repeatedly to get all rules.

qwordy · 2020-09-08T03:29:39Z

Current help message for --admin-password. Maybe we can also put the rules in help message.

--admin-password

Password for the VM if authentication type is 'Password'.

houk-ms · 2020-09-08T04:34:43Z

So, let's conclude the discussion. Whenever user inputs an invalid password, we will show the error message below.

The password length must be between 12 and 123. And the password must have the 3 of the following: 1 lower case character, 1 upper case character, 1 number and 1 special character.

improve vm password validation message

eef77c9

houk-ms requested review from arrownj and qwordy as code owners August 25, 2020 06:39

houk-ms changed the title ~~{VM} Fix issue #14897 display all requirements for vm admin password when it's invalid~~ {VM} Fix #14897: display all requirements for vm admin password when it's invalid Aug 25, 2020

houk-ms requested a review from yungezz August 25, 2020 06:42

houk-ms commented Aug 25, 2020

View reviewed changes

src/azure-cli/azure/cli/command_modules/vm/_validators.py Outdated Show resolved Hide resolved

fix typo

4da2bcb

yungezz approved these changes Aug 25, 2020

View reviewed changes

yungezz assigned qwordy Aug 25, 2020

yungezz added the Compute az vm/vmss/image/disk/snapshot label Aug 25, 2020

qwordy reviewed Aug 26, 2020

View reviewed changes

qwordy approved these changes Sep 7, 2020

View reviewed changes

adjust error messages

b7816b8

arrownj approved these changes Sep 8, 2020

View reviewed changes

houk-ms added 2 commits September 8, 2020 12:37

adjsut error message

31b8847

fix testing issues

3d108cb

houk-ms merged commit 47b0029 into Azure:dev Sep 10, 2020

Conversation

houk-ms commented Aug 25, 2020

Uh oh!

Uh oh!

yungezz Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

houk-ms Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

qwordy Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

yungezz commented Aug 25, 2020

Uh oh!

mmyyrroonn commented Aug 25, 2020

Uh oh!

azure-pipelines bot commented Aug 25, 2020

Uh oh!

qwordy Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

houk-ms Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

qwordy Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

houk-ms Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

qwordy commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenlomis commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arrownj commented Sep 8, 2020

Uh oh!

arrownj commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

houk-ms commented Sep 8, 2020

Uh oh!

yungezz commented Sep 8, 2020

Uh oh!

qwordy commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

houk-ms commented Sep 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

qwordy commented Sep 7, 2020 •

edited

Loading

chenlomis commented Sep 7, 2020 •

edited

Loading

arrownj commented Sep 8, 2020 •

edited

Loading

qwordy commented Sep 8, 2020 •

edited

Loading