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
VM/VMSS: incorporate credentials validation logic used by portal #2537
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2537 +/- ##
=========================================
+ Coverage 72.02% 72.1% +0.07%
=========================================
Files 362 362
Lines 19766 19790 +24
Branches 2913 2920 +7
=========================================
+ Hits 14236 14269 +33
+ Misses 4612 4600 -12
- Partials 918 921 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small requests, but overall LGTM!
@@ -571,6 +566,52 @@ def _validate_vm_create_auth(namespace): | |||
'/home/{}/.ssh/authorized_keys'.format(namespace.admin_username) | |||
|
|||
|
|||
def _validate_admin_username(username, is_linux): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of taking an "is_linux" bool, just accept the os_type and do the check in this method. Otherwise you run the risk of introducing bugs in the future if one invocation is changed and the other is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -571,6 +566,52 @@ def _validate_vm_create_auth(namespace): | |||
'/home/{}/.ssh/authorized_keys'.format(namespace.admin_username) | |||
|
|||
|
|||
def _validate_admin_username(username, is_linux): | |||
if not username: | |||
return "admin user name can not be empty" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of this return statement? Shouldn't it raise an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I missed this one when converted 'returning error strings' to throw. I also added a test for this
"owner", "root", "server", "sql", "support", "support_388945a0", | ||
"sys", "test2", "test3", "user4", "user5"] | ||
if username.lower() in disallowed_user_names: | ||
raise CLIError('The specified admin user name is not allowed, as it uses reserved words. Try again with a different value') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reserved words sounds like programming keywords like int, char, etc. Plus it's hard to say that "support_388945a0" is reserved. 😁 I would recommend something like "This user name "foo" meets the general requirements, but is specifically disallowed for this image. Please try a different value."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
"pass@word1", "Password!", "Password1", "Password22", "iloveyou!" | ||
] | ||
if password.lower() in disallowed_passwords: | ||
raise CLIError('The specified password is not allowed') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend a similar error as above: that the password meets the general requirements, but it specifically disallowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a second thought, let me just remove this whole section. Those old banned passwords would not meet the length requirement anyway
@@ -65,3 +67,64 @@ def test_figure_out_storage_source(self): | |||
self.assertFalse(src_disk) | |||
self.assertFalse(src_snapshot) | |||
self.assertEqual(src_blob_uri, test_data) | |||
|
|||
def test_validate_admin_username_linux(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for unit tests!
* Azure/master: (478 commits) vm live test: allow more valid power states on vmss test verifications (Azure#2564) rbac:catch more graph error (Azure#2567) appservice: support to create plan when create a webapp (Azure#2550) Update storage tests (Azure#2556) Change PEP8 check filter from whitelist to blacklist (Azure#2557) Add scenario tests documentation (Azure#2555) [ACS] Adding support for configuring a default ACS cluster (Azure#2554) [ACS] Provide a short name alias for the orchestrator type flag (Azure#2553) Sql Import/Export CLI commands and test (Azure#2538) Fix format bug. (Azure#2549) [VM/VMSS] Improved disk caching support (Azure#2522) VM/VMSS: incorporate credentials validation logic used by portal (Azure#2537) Script that creates packaged releases package archive (Azure#2508) Adding alias for defaults flag (Azure#2540) Add wait commands and --no-wait support (Azure#2524) choice list outside of named arguments (Azure#2521) Fixed test failure in test_sql_db_mgmt. (Azure#2530) core: support login using service principal with a cert (Azure#2457) Add note about being in preview (Azure#2512) vm:fix distro check mechanism used by disk encryption (Azure#2511) ...
Fix #2010. Fix #1943