CommunityToolkit / Graph-Controls

Set of Helpers and Controls for Windows development using the Microsoft Graph.
https://docs.microsoft.com/en-us/windows/communitytoolkit/graph/overview
Other
155 stars 39 forks source link

fix(msalprovider): Fix error when providing TenantId #178

Closed brekkjen closed 2 years ago

brekkjen commented 2 years ago

Fixes #170

https://github.com/CommunityToolkit/Graph-Controls/issues/170 ## PR Type What kind of change does this PR introduce?

What is the current behavior?

Providing TenantId causes both WithTenantId and WithAuthority to be specified and those are mutually exclusive resulting in error

What is the new behavior?

Providing TenantId excludes WithAuthority

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

ghost commented 2 years ago

Thanks brekkjen for opening a Pull Request! The reviewers will test the PR and highlight if there is any merge conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

net-foundation-cla[bot] commented 2 years ago

CLA assistant check
All CLA requirements met.

shweaver-MSFT commented 2 years ago

Thanks for submitting, and sorry it took me a few days to get to it. I've got one nitpick, now that the authority is only required if the tenantId is null, we should move the authority declaration into the else statement closest to where it's being used.

if (tenantId != null)
{
    clientBuilder = clientBuilder.WithTenantId(tenantId);
}
else
{
    // If the TenantId is not provided, use WithAuthority
    var authority = listWindowsWorkAndSchoolAccounts ? AadAuthorityAudience.AzureAdAndPersonalMicrosoftAccount : AadAuthorityAudience.PersonalMicrosoftAccount;
    clientBuilder = clientBuilder.WithAuthority(AzureCloudInstance.AzurePublic, authority);
}

Then we should be good to go 👍

brekkjen commented 2 years ago

Yes, i agree.