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

[hajekj]

• Added common tenant to meta tenant identifiers since client_credentials flow cannot be used without explicit tenant.
• Check TenantId value against the above list as well, like with explicitly passed tenant.

Fixes #793

[jmprieur]

Thanks for your PR, @hajekj
actually, I believe that Consumers should be allowed (for some first party scenarios)

[hajekj]

So what would be the best way to approach this? It's the first time I hear, that it's possible with consumers endpoint. So you think it would be the best to skip the validation for Consumers endpoint? Because right now, the check is being done against Consumers and Organizations and Common was skipped out (https://github.com/AzureAD/microsoft-identity-web/blob/master/src/Microsoft.Identity.Web/TokenAcquisition.cs#L86).

[bgavrilMS]

consumers is essentially a single (huge) tenant, while organizations and common are not tenants, they are descriptors to tell AAD "I don't know what tenant I want, you figure it out for me". These 2 descriptors should not be used in conjunction with client_creds grant.

[hajekj]

Gotcha, makes sense! I have updated the code to omit the consumers endpoint from validation. Would that be okay now?

[jmprieur]

Thanks @hajekj. I think this is ok.
Let's have @jennyf19 review and merge.

[jennyf19]

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.

[hajekj]

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.

[jennyf19]

@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.

[jennyf19]

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

[hajekj]

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.

[jennyf19]

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

[hajekj]

Sure! Will add it tomorrow.

[jennyf19]

@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.

[jennyf19]

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

[hajekj]

Looks good to me!

[jennyf19]

thank you so much for all your work in this area @hajekj much appreciated.

[jennyf19]

@hajekj nice blog post! mind if we link to it from our wiki?

[jennyf19]

@hajekj also, in regards to this...

if you got it to work, we'll help support it (what a great surprise for us). just open issues here if you need help w/anything. we'll probably try it out in the coming weeks. thanks again!

[hajekj]

@jennyf19 Feel free to link or re-use in any way you want! I will update the blog post later today to update the information about the support!

Update: Article updated. Thank you for this. I have put this repo on my watch list, so if there are any issues, I will try to take a look into those as well!