Remove email addresses from UPN query in KeyVault Access Policy CmdLets. #4102

RandalliLama · 2017-06-09T05:25:56Z

Description

Before this change, when a UPN argument is passed to one of the vault policy cmdlets the query for object id would include email addresses as well. After this change UPN will mean UPN and there will be a seperate email argument if the user wants to look up the object id by email address.

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

Before this change, when a UPN argument is passed to one of the vault policy cmdlets the query for object id would include email addresses as well. After this change UPN will mean UPN and there will be a seperate email argument if the user wants to look up the object id by email address.

msftclas · 2017-06-09T05:26:00Z

@RandalliLama,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

azuresdkci · 2017-06-09T05:30:18Z

Can one of the admins verify this patch?

RandalliLama · 2017-06-09T05:31:35Z

Please remove @pomortaz and @adarce as the default reviewers and replace with @RandalliLama , @schaabs , and @dragav . @pomortaz has moved off of SDK and PowerShell work and Scott, Dragos and myself are now responsible for Key Vault SDK and PowerShell work.

cormacpayne · 2017-06-09T16:50:25Z

@azuresdkci add to whitelist

cormacpayne · 2017-06-09T20:34:16Z

@RandalliLama it looks like the default reviewers (who are just suggestions) are set by GitHub: "Suggested reviewers are based on git blame data"

Feel free to request a review from those who you have mentioned previously.

cormacpayne · 2017-06-09T20:50:43Z

src/ResourceManager/KeyVault/Commands.KeyVault.Test/Scripts/RunKeyVaultTests.ps1

+
+    <#
+    This test is disabled because it requires a user with an email address that matches their UPN.
+    Run-TestProtected { Run-VaultTest { Test_SetRemoveAccessPolicyByEmail } "Test_SetRemoveAccessPolicyByEmail" } "Test_SetRemoveAccessPolicyByEmail"


@RandalliLama is there any way to enable this test so we can (1) ensure that the functionality added does work as intended, (2) ensure that there are no regressions in the future?

These tests are currently not run under the recording framework. I am still investigating why that is, and what I can do about it. As far as I can tell these tests are run manually by devs before checkin. Since they are not recorded, if this test was enabled by default, the developer would need to ensure that they have a test environment that has a user with an email address, and further, that the email address matched their UPN. That's not easy to set up if you don't already have one. Setting up a user with an email address requires having a user licensed for Exchange, or doing manual REST calls to AAD graph.

If these tests were being run under the recording framework then only one person would need to have the necessary test setup and all other runs would be on recorded data. Alternatively, we could set up a permanent test environment with users that have the necessary pre-reqs. It's also possible to automate this kind of user creation in our current, manual, test infrastructure.

The right long term answer is to get the tests running in the recording framework. However, it is non-trivial to do so. I need some time to investigate what it takes to get us completely back in to the recording framework. I'd rather not invest significant effort in a temporary solution. I plan on looking in to fixing this the right way in Calcium.

Without this fix it's possible for a user creating a new vault to inadvertently ACL it for someone other than the person running the CmdLet. While we show the policy created by the CmdLet as part of the output, if the person running it isn't paying attention they may end up with a policy that grants access to the vault that they didn't intend. This should be a very rare occurrence, but I would still like to get this fix out soon. Blocking the release of this fix on test infrastructure investments could significantly delay it.

The risk of having this test disabled is small. The code that does the lookup by email is factored out in to it's own method that is not shared by any other code path.

I may have missed something, and of course I am open to other suggestions.

@cormacpayne ^

By the way, I ran the test before disabling it. I have verified that the feature works as intended.

cormacpayne

@RandalliLama some minor comments

cormacpayne · 2017-06-15T22:32:02Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RemoveAzureKeyVaultAccessPolicy.cs

+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "Specifies the email address of the user in Azure Active Directory for which to grant permissions.")]
+        [ValidateNotNullOrEmpty()]
+        public string Mail { get; set; }


@RandalliLama can we rename this parameter to EmailAddress? This seems to be the pattern for a few other KeyVault cmdlets that have a parameter for email address

cormacpayne · 2017-06-15T22:32:12Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/SetAzureKeyVaultAccessPolicy.cs

+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "Specifies the email address of the user in Azure Active Directory for which to grant permissions.")]
+        [ValidateNotNullOrEmpty()]
+        public string Mail { get; set; }


@RandalliLama same comment

cormacpayne · 2017-06-15T22:34:10Z

src/ResourceManager/KeyVault/Commands.KeyVault/help/Remove-AzureRmKeyVaultAccessPolicy.md

@@ -230,6 +236,20 @@ Accept pipeline input: True (ByPropertyName)
 Accept wildcard characters: False
 ```

+### -Mail


@RandalliLama if you do change this parameter to EmailAddress, the markdown documentation will need to be updated

cormacpayne · 2017-06-15T22:36:10Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/SetAzureKeyVaultAccessPolicy.cs

+        [Parameter(Mandatory = false,
+            ParameterSetName = ByEmail,
+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "Specifies key operation permissions to grant to a user or service principal.")]


@RandalliLama "Specifies secret operation permissions..."

cormacpayne · 2017-06-15T22:36:26Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/SetAzureKeyVaultAccessPolicy.cs

+        [Parameter(Mandatory = false,
+            ParameterSetName = ByEmail,
+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "Specifies key operation permissions to grant to a user or service principal.")]


@RandalliLama "Specifies certificate operation permissions..."

adarce · 2017-06-15T23:51:43Z

src/ResourceManager/KeyVault/Commands.KeyVault/help/Set-AzureRmKeyVaultAccessPolicy.md

+ -Mail <String> [-PermissionsToKeys <String[]>] [-PermissionsToSecrets <String[]>]
+ [-PermissionsToCertificates <String[]>] [-PermissionsToStorage <String[]>] [-PassThru] [-WhatIf] [-Confirm]
+ [<CommonParameters>]
+


You need to close the triple backtick here:

```

adarce · 2017-06-15T23:57:10Z

src/ResourceManager/KeyVault/Commands.KeyVault/Properties/Resources.resx

@@ -414,4 +414,7 @@ You can find the object ID using Azure Active Directory Module for Windows Power
  <data name="InvalidSasPermission" xml:space="preserve">
    <value>Invalid Sas permission '{0}'.</value>
  </data>
+  <data name="ADObjectAmbiguous" xml:space="preserve">
+    <value>The Email argument specified, &apos;{1}&apos;, matches multiple objects in the Azure Active Directory tenant &apos;{2}&apos;. Please use -UserPrincipalName to narrow down the the filter to a single object. The TenantID displayed by the cmdlet 'Get-AzureRmContext' is the current subscription's Azure Active directory.</value>


"narrow down the the filter" -> "narrow down the filter"

Consider ensuring that the casing of AAD is consistent.
First it's Azure Active _D_irectory, then it's Azure Active __d__irectory.

RandalliLama · 2017-06-17T01:12:31Z

@cormacpayne I've addressed all your comments. I've also enabled the new test by default.

RandalliLama · 2017-06-17T01:15:15Z

@adarce Thanks for the review. I've addressed your comments as well.

adarce · 2017-06-17T07:19:30Z

src/ResourceManager/KeyVault/Commands.KeyVault/help/Set-AzureRmKeyVaultAccessPolicy.md

+ -EmailAddress <String> [-PermissionsToKeys <String[]>] [-PermissionsToSecrets <String[]>]
+ [-PermissionsToCertificates <String[]>] [-PermissionsToStorage <String[]>] [-PassThru] [-WhatIf] [-Confirm]
+ [<CommonParameters>]
+


This is still missing some closing triple backticks. View the rendered markdown here:

https://github.com/RandalliLama/azure-powershell/blob/c741b45a190943b364685f6edebc982dd0517d23/src/ResourceManager/KeyVault/Commands.KeyVault/help/Set-AzureRmKeyVaultAccessPolicy.md#byuseremailaddress

@adarce I could only find a single instance of missing back ticks. Are there more?

@RandalliLama That's it as far as what you introduced in to this PR. However, it looks like there's some more broken markdown in the same file that got introduced when PermissionsToStorage was added.

There needs to be a couple of newlines after "service principal." and before the triple backticks here:
https://github.com/RandalliLama/azure-powershell/blame/8a0fc06eadee571cc013612cd6d90f1ff0248998/src/ResourceManager/KeyVault/Commands.KeyVault/help/Set-AzureRmKeyVaultAccessPolicy.md#L370

Markdown: https://github.com/RandalliLama/azure-powershell/blob/8a0fc06eadee571cc013612cd6d90f1ff0248998/src/ResourceManager/KeyVault/Commands.KeyVault/help/Set-AzureRmKeyVaultAccessPolicy.md#-permissionstostorage

cormacpayne · 2017-06-19T16:45:30Z

@RandalliLama this is good to merge from our side; just need sign-off from your team.

Once this gets merged, let's open an issue to track enabling the KeyVault tests.

cormacpayne

@RandalliLama would you mind updating the change log with a brief description of what was changed in this PR under the Current Release header?

cormacpayne

@RandalliLama we bump the versions during the release process, so you can revert the changes you made to the version numbers in the three files mentioned. Other than that, LGTM

cormacpayne · 2017-06-20T20:18:47Z

src/ResourceManager/KeyVault/AzureRM.KeyVault.psd1

@@ -12,7 +12,7 @@
 # RootModule = ''

 # Version number of this module.
-ModuleVersion = '3.1.0'
+ModuleVersion = '3.2.0'


@RandalliLama we will bump the versions during the release, so you can revert this change

cormacpayne · 2017-06-20T20:19:02Z

src/ResourceManager/KeyVault/Commands.KeyVault.Test/Properties/AssemblyInfo.cs

 // by using the '*' as shown below:

 [assembly: AssemblyVersion( "3.0.0.0" )]
-[assembly: AssemblyFileVersion("3.1.0")]
+[assembly: AssemblyFileVersion("3.2.0")]


@RandalliLama please revert this change to the version

cormacpayne · 2017-06-20T20:19:24Z

src/ResourceManager/KeyVault/Commands.KeyVault/Properties/AssemblyInfo.cs

-[assembly: AssemblyVersion("3.1.0")]
-[assembly: AssemblyFileVersion("3.1.0")]
+[assembly: AssemblyVersion("3.2.0")]
+[assembly: AssemblyFileVersion("3.2.0")]


@RandalliLama please revert the changes made to version numbers in this file

msftclas added the cla-already-signed label Jun 9, 2017

azuresdkci added the needs-review label Jun 9, 2017

azuresdkci assigned cormacpayne Jun 9, 2017

RandalliLama changed the title ~~Remove email addresses from UPN query.~~ Remove email addresses from UPN query in KeyVault Access Policy CmdLets. Jun 9, 2017

cormacpayne suggested changes Jun 9, 2017

View reviewed changes

cormacpayne assigned RandalliLama and unassigned cormacpayne Jun 9, 2017

cormacpayne added needs-revision and removed needs-review labels Jun 9, 2017

RandalliLama requested review from schaabs and dragav June 9, 2017 23:18

cormacpayne suggested changes Jun 15, 2017

View reviewed changes

adarce reviewed Jun 15, 2017

View reviewed changes

adarce previously approved these changes Jun 16, 2017

View reviewed changes

Rename KeyVault Mail parameter to EmailAddress

9f55160

RandalliLama dismissed adarce’s stale review via 9f55160 June 17, 2017 01:11

Correct grammar in resources.

c741b45

adarce reviewed Jun 17, 2017

View reviewed changes

Fix broken markdown in Key Vault powershell

8a0fc06

cormacpayne approved these changes Jun 19, 2017

View reviewed changes

cormacpayne removed the needs-revision label Jun 19, 2017

cormacpayne unassigned RandalliLama Jun 19, 2017

cormacpayne previously approved these changes Jun 19, 2017

View reviewed changes

adarce previously approved these changes Jun 19, 2017

View reviewed changes

Update version numbers and changelog

cd2091a

RandalliLama dismissed stale reviews from adarce and cormacpayne via cd2091a June 19, 2017 23:15

Small changelog formatting change

01f3c9e

adarce previously approved these changes Jun 19, 2017

View reviewed changes

cormacpayne suggested changes Jun 20, 2017

View reviewed changes

cormacpayne assigned RandalliLama Jun 20, 2017

cormacpayne added the needs-revision label Jun 20, 2017

Revert version number changes in Key Vault Cmdlets

52cedeb

RandalliLama dismissed adarce’s stale review via 52cedeb June 20, 2017 22:14

cormacpayne approved these changes Jun 21, 2017

View reviewed changes

cormacpayne merged commit bbc5924 into Azure:preview Jun 21, 2017

RandalliLama mentioned this pull request Mar 28, 2018

New-AzureRmKeyVault gives the wrong user administrative access upon creation #2548

Closed

Remove email addresses from UPN query in KeyVault Access Policy CmdLets. #4102

Remove email addresses from UPN query in KeyVault Access Policy CmdLets. #4102

Uh oh!

Conversation

RandalliLama commented Jun 9, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

msftclas commented Jun 9, 2017

Uh oh!

azuresdkci commented Jun 9, 2017

Uh oh!

RandalliLama commented Jun 9, 2017

Uh oh!

cormacpayne commented Jun 9, 2017

Uh oh!

cormacpayne commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RandalliLama commented Jun 17, 2017

Uh oh!

RandalliLama commented Jun 17, 2017

Uh oh!

adarce Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne commented Jun 19, 2017

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RandalliLama commented Jun 9, 2017 •

edited by azuresdkci

Loading

adarce Jun 17, 2017 •

edited

Loading