Fixed the condition of AAD Query. ActiveDirectoryClient.FilterUsers does not support SPN. Here, UPN is correct.

[takekazuomi]

Description

If there are multiple users in the directory, UserPrincipalName will always result in 'Sequence contains more than one element' error.
See #5201 for the reproduction procedure.

This is caused by querying ActiveDirectoryClient.FilterUsers with 'SPN = upn'. 'UPN = upn' is correct.

---

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.

[markcowl]

@darshanhs90 Can you look to see if this change is corect? Also, can one of @takekazuomi or @darshanhs90 add a test to protect this scenario from regressions in the near future?

[darshanhs90]

@takekazuomi Can you add a test case for this

[takekazuomi]

@darshanhs90
I can do it. I need two or three days. I am busy now.

I think there are other problems with this cmdlet. EmailAddress option does not search email with AAD Query. Search by UPN.

In AAD, UPN and Email are not the same. Especially for MSA account UPN does not match email.
Also, with Get-AzureRMADUser, since e-mail is not displayed, it is difficult to confirm.

Then, the EmailAddress option of Get-AzureRMADUser similarly searches for UPN.
If the e-mail is UPN's Alias this behavior will be accepted. However, I think that it is the source of confusion at the present time.

What do you think about?

[maddieclayton]

@takekazuomi @darshanhs90 Ping on this issue.

[takekazuomi]

@maddieclayton I'm still working for unit test code.

Workaround
#5201 (comment)

It's not so smart because it depends confused specification. But it works.

[RandalliLama]

from: @seanbamsft

"That’s one of the bugs. The other mentioned below is that it removed our query for “/me” when creating vaults (the change on 207). Also, line 253 is wrong as it only checks the primary email address and the previous code also checked OtherMails (this one prevents me from adding myself to access policies by my normal email address)."

[takekazuomi]

@RandalliLama
In "ActiveDirectoryClient.FilterUsers", options.UPN and options.Mail are the same.

https://github.com/Azure/azure-powershell/blob/preview/src/Common/Commands.Common.Graph.RBAC/ActiveDirectory/ActiveDirectoryClient.cs#L190

I think this is a problem.

[RandalliLama]

@takekazuomi I agree this is a problem. However, it appears that their might be a work around. It looks like the API returns a list of all the users found that have a match on either UPN or Mail. Can we filter that list to only those that have a match on UPN?

[takekazuomi]

@RandalliLama
I feel I do not understand it completely yet. I will summarize my understanding of now. First of all, when I make a query with UPN, the maximum number of results is 1. The UPN is unique within the directory.

If I queried by email, the result may be more than one case. Email is not guaranteed uniqueness in the directory. When multiple results are obtained, I think Cmdlet should return an error. The current error is hard to understand. This is better to fix the error message.

There is another problem when searching by Email. Currently, Get-AzureRmADUser does not return Email. Therefore, a user is no way to check if the corresponding email is not in the directory. If Get - AzureRmADUser returns Email, the users will be able to solve the problem themselves.

In an organization account, UPN is often an email, but it is not mandatory. In MSA, UPN and Email are different.

Is this correct in such understanding?

[RandalliLama]

@takekazuomi Your understanding is correct. A users email address and UPN do not need to match. I believe that there can be multiple email addresses. In practice however, one of the users email addresses must match the UPN. Many other systems treat them as interchangeable. We can't count on that though.

Your last point about MSA probably explains why someone changed the code to include Mail in the UPN search. MSA guest accounts in AAD do not have a UPN as MSA has no concept of UPN. Thus, the only way to look them up is via Email. However, the cmdlet should not second guess the user in my opinion. If the user asked for a UPN search, then that is exactly what the cmdlet should do. If they want to search by Email then let them be explicit about that and only search for Email. If it is useful and deterministic for them to search both fields at the same time, then give them an explicit option to do that. This is why, for example, the Set-AzureKeyVaultAccessPolicy cmdlet has an EmailAddress parameter, and it uses the GetObjectIdByEmail method to find the user.

[markcowl]

Approving as this improves the functionality

[cormacpayne]

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/378/

[RandalliLama]

@markcowl Can you expand on why you are approving? Is @takekazuomi going to open another PR with the rest of the regression fixes?

[markcowl]

@RandalliLama We are approving it because it partially fixes the immediate issue, so it is better than the code that is currently shipping. Since you guys didn't have the resources to fix this, we will have to take up the rest of the fix in the next sprint on our team.

[RandalliLama]

@markcowl If we don't have time to do this correctly, then please just revert the change that broke this in the first place.

[RandalliLama]

Needs a complete fix. Alternatively, the commit that broke the code should be reverted.

[takekazuomi]

I have two problems here. That is a problem with EmailAddress option and missing Unit Test.

1. In KeyVault Cmdlet, many Unit tests are marked as skipped. I need more time to understand.

2. I think that about "-EmailAddress" should make another Issue. AAD OtherMails property is just only "alternate mail address". I do not know if it makes sense to search by this. we need more discussion is necessary, I think.
   Set-AzureRmKeyVaultAccessPolicy with EmailAddress dosen't search email. #5332

[RandalliLama]

I agree that more discussion is necessary. I don't understand exactly why the changes were made that broke this, and so I am not really able to review this properly. I think we should just revert the change that introduced the regressions and then we can review those changes in more depth before applying them. We should not be regressing existing customers for new functionality without wider agreement between our teams.

[takekazuomi]

@RandalliLama KeyVault Cmdlet still a problem, but I think that this PR just fixed a simple mistake.
The mistake seems to be this commitment.
9cf77ac#diff-4a51df99c6d409e7dbea11a8622dd347L232

[markcowl]

@RandalliLama As we have discussed, reverting the netcore change is not possible. The choice now is continuing to ship with the current code, or shipping this obvious partial fix. If you think the current code is preferrable, then that's what we will ship.

[maddieclayton]

Closing in favor of: #5334, which makes the same change as well as other KeyVault changes.