Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Throw an ArgumentException if tenant is 'common' or 'organizations' for acquire token for app scenarios #795

Merged
merged 5 commits into from
Dec 2, 2020
Merged

Throw an ArgumentException if tenant is 'common' or 'organizations' for acquire token for app scenarios #795

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d8f00f
Add support for tenant selection when using AppOnly Microsoft Graph.
hajekj Nov 27, 2020
3b3698f
Merge pull request #1 from AzureAD/master
hajekj Dec 1, 2020
8b61888
Add Common tenant to meta tenant identifiers and check for TenantId n…
hajekj Dec 1, 2020
76f11e8
Allow client_credentials to function with consumers endpoint.
hajekj Dec 1, 2020
9e705ba
Explicitly provide tenantId from configuration if not specified when …
hajekj Dec 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Microsoft.Identity.Web/TokenAcquisition.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public TokenAcquisition(
private readonly ISet<string> _metaTenantIdentifiers = new HashSet<string>(
new[]
{
Constants.Common,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the only fix that is needed. Can you add a test as well? or modify an existing test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Will add it tomorrow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hajekj Actually, nevermind on the tests, you'll need access to our KeyVault to run the tests that would cover this. If you can take a look at the comment above, and then once we merge this, i will add the test(s). If you can test manually though and share the results (assuming it works), that would be helpful.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a branch with some additional tests for the work you have here. will merge after we merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Constants.Organizations,
Constants.Consumers,
},
Expand Down Expand Up @@ -276,6 +277,11 @@ public async Task<string> GetAccessTokenForAppAsync(
throw new ArgumentException(IDWebErrorMessage.ClientCredentialTenantShouldBeTenanted, nameof(tenant));
}

if (!string.IsNullOrEmpty(_microsoftIdentityOptions.TenantId) && _metaTenantIdentifiers.Contains(_microsoftIdentityOptions.TenantId))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed. We already are checking the tenant in the lines above (starting line 274), and the developer needs to specifically pass in the tenant in this method, so it doesn't matter what the tenant is with the options. They could even be different tenants, we just care about the one passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure about that, since it either takes the provided tenant or uses the Authority (https://github.com/AzureAD/microsoft-identity-web/blob/master/src/Microsoft.Identity.Web/TokenAcquisition.cs#L703) passed from within the options passed to MSAL, that's why I added this part.

The reason is to cover a scenario, where someone sets up the authority in settings as common for example, and would then call GetAccessTokenForAppAsync(scope) without explicitly provided tenant which won't work and AAD will return an error, so this check prevents the request reaching AAD at all.

I am perfectly fine with removing it of course. Please let me know, what makes more sense.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hajekj I see. thanks for the explanation. I would propose something like this starting on line 274:

if (string.IsNullOrEmpty(tenant))
{
        tenant = _applicationOptions.TenantId ?? _microsoftIdentityOptions.TenantId;
}

if (!string.IsNullOrEmpty(tenant) && _metaTenantIdentifiers.Contains(tenant))
{
       throw new ArgumentException(IDWebErrorMessage.ClientCredentialTenantShouldBeTenanted, nameof(tenant));
}

then we'll always have a value for the tenant, or not, and it will be handled before we create the authority. and the authority will have the tenanted value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS...i haven't tested it, just a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I have added the change and tested locally with very limited scenario. Seems to work fine, but would be great to hook it up with some more tests, especially when the application is configured as multi-tenant with TenantId set to common or organizations in configuration.

{
throw new ArgumentException(IDWebErrorMessage.ClientCredentialTenantShouldBeTenanted, nameof(_microsoftIdentityOptions.TenantId));
}

// Use MSAL to get the right token to call the API
_application = await GetOrBuildConfidentialClientApplicationAsync().ConfigureAwait(false);
string authority = CreateAuthorityBasedOnTenantIfProvided(_application, tenant);
Expand Down