Adding flexibility for the authority and audience to support Azure AD v2.0 #7913
Conversation
jmprieur
changed the title
|
looking at the test failures |
| /// <summary> | ||
| /// Azure Active Directory Authority | ||
| /// </summary> | ||
| public string Authority { get; set; } = "{Instance}{TenantId}"; |
There was a problem hiding this comment.
These seem to be more like string templates and not default values. It should then be the responsability of *Configuratgion.cs to create default values if the user hasn't set any.
i.e. Just tell the user that the default value will be {Instance}{TenantId} if he does not set anything.
There was a problem hiding this comment.
well this is {Instance}{TenantId}/v2.0 in the case of v2.0.
See also the templates.
| //#endif | ||
| // }, | ||
| //#endif | ||
| ////#if (IndividualB2CAuth) |
There was a problem hiding this comment.
Was this just a whitespace change? If so revert.
|
|
||
| /// <summary> | ||
| /// Gets or sets the Azure Active Directory instance. | ||
| /// Typical values are: |
| ////#elseif (OrganizationalAuth) | ||
| // "AzureAd": { | ||
| // "Instance": "https:////login.microsoftonline.com/", | ||
| // "Autority": "{Instance}{TenantId}/v2.0", |
There was a problem hiding this comment.
Use the same order in both templates.
There was a problem hiding this comment.
@Tratcher : the templates are in a different order already today (I was surprised too). I did not change the order.
|
|
||
| options.Audience = azureADOptions.ClientId; | ||
| options.Authority = new Uri(new Uri(azureADOptions.Instance), azureADOptions.TenantId).ToString(); | ||
| options.Audience = string.Format(azureADOptions.Audience?.Replace("{ClientId}", "{0}"), |
There was a problem hiding this comment.
This has several problems.
- What happens if azureAdOptions.Audience is null? (It's not ok to show explicitly on the template config file).
- We don't do replacements this way.
azureADOptions.Audience?.Replace("{ClientId}", "{0}"- Normally we use a string interpolation.
- Same for authority below.
There was a problem hiding this comment.
@javiercn : how can we use string interpollation for a string which is not in the code, but in a config file? could you please show me an example on how to do it?
There was a problem hiding this comment.
@javiercn is there a way to enforce some type of validation, For example, azureADOptions.Audience must be populated correctly from config?
| // "Instance": "https:////login.microsoftonline.com/", | ||
| // "Autority": "{Instance}{TenantId}/v2.0", | ||
| //#if (MultiOrgAuth) | ||
| // "TenantId": "organizations", |
There was a problem hiding this comment.
Why do we need this now? MultiOrg is kind of it's own beast and it used to point to https:////login.microsoftonline.com/common would this now point to https:////login.microsoftonline.com/organizations?
Asa general principle we don't introduce concepts in the template if we don't absolutely need to, so could this go back to the way it was before?
There was a problem hiding this comment.
So in Azure AD v2.0, common means AAD+MSA whereas in Azure AD v1.0 it used to mean "AAD multi-org.
To say AAD multi-org in "Azure AD v2.0", one needs to use organizations. I proposed this one, in order to not change the behavior of the template (by using organizations with a v2.0 template, it behaves as common for a v1.0 template).
| ////#elseif (OrganizationalAuth) | ||
| // "AzureAd": { | ||
| // "Instance": "https:////login.microsoftonline.com/", | ||
| // "Autority": "{Instance}{TenantId}/v2.0", |
|
Thanks for the contribution!
Based on what I can see from looking at the code I think we can do this without explicitly introducing Audience and Authority explicitly on the Options type and on the templates. Startup wise there are several ways we can do this, including not doing anything at all. Implicit version on the method name. services.AddAuthentication().AddAzureADV2Explicit as a parameter services.AddAuthentication().AddAzureADV2(
AzureADEndpointVersion.2_0,
options => Configuration.Bind(options));As part of the options type services.AddAuthentication().AddAzureAD(options => { Configuration.Bind(options); options.EndpointVersion = AzureADEndpointVersion.2_0 });We migrate users forward (if endpoints are forward compatible) services.AddAuthentication().AddAzureAD(options => Configuration.Bind(options));With an additional extension method (if endpoints are forward compatible) services
.AddAuthentication()
.AddAzureAD(options => Configuration.Bind(options))
.(Enable/Use/Configure/Other/)AzureADEndpointVersion(AzureADEndpointVersion.Latest);We just patch the OIDC and JwtBearer options respectively on the template: services
.AddAuthentication()
.AddAzureAD(options => Configuration.Bind(options))
.(Enable/Use/Configure/Other/)AzureADEndpointVersion(AzureADEndpointVersion.Latest);services.Configure<OpenIdConnectOptions>(options => {
options.Audience = $"api://{options.ClientId}",
options.Authority = $"{options.Authority}/v2.0"
});No matter what, this needs input from our PM team as it changes the templates and introduces new concepts in the users faces, either in config or in startup. For the rest of the ASP.NET Team, we would need to explicitly test the templates for this change before merging. |
|
@javiercn : thanks for your review and feedback. To answer your questions:
The Microsoft identity platform (fomerly AAD v2.0) supports the notion of authority in the following way. For the AzureAD.UI components, we don't need to bother here about B2C and ADFS, but giving the big picture.
This might not be completely natural, but you provided good alternative ideas here. (See at the end of this comment)
That's a good question which I had a bit overlooked: there are actually changes in the claims as, if going to the v1.0 authorize endpoint, Web apps will receive a v1.0 token, whereas when going to the v2.0 authorize endpoint, they will receive a v2.0 tokens. More about the tokens differences here: IDToken and AccessToken. In particular the name is, in v2.0 prefered_username @javiercn : overall thanks for your feedback on the alternatives. Interestingly some are quite close to some of our implementation in the Web App tutorial. Could you please have a look at the following code and give your feedback (it's very small). I'm starting to think that this might be what we to do, and not change the templates |
|
@Eilon I think if we are willing to take a breaking change here, we can update the implementation to point to the 2.0 endpoints, and 3.0 is a good timeframe to do so. We don't need to add new concepts, simply change the implementation. We would have to take a look at the claim-set difference between 1.0 endpoints and 2.0 endpoints, but I don't expect it to be an issue. From the sample @jmprieur provided seems that there might be smaller tweaks we can do to improve the E2E experience (like around account selection) so I think there's some value in moving to the 2.0 endpoints. That said, we need a PM on our team to chime in. @danroth27 or @blowdart should be the ones to say something. I expect this to be an M sized workitem. My opinion (in case it's not clear) is that we don't add additional methods or support both scenarios side -by-side (v1 and v2 endpoints) with the same package, but that we break and update. In the end, you can always implement this by yourself, our package just makes it convenient. |
|
The token differences concern me, everything must map the same to the resulting identity, no matter what version of the endpoint is used. That needs to be guaranteed :) |
|
@blowdart not necessarily. If AAD versioned the endpoints I imagine is because they wanted to do breaking changes. We can take the 3.0 opportunity to update. I’m not concerned about the fundamental claims of the token changing. But whatever you guys feel like. |
|
The change of the email address claim to a very non-standard "preferred_username" would need addressing, otherwise it's never going to work for people upgrading and who are using email as their default key, rather than sub (because sub isn't obviously the right one to use). The sample tokens also show "given name" / "family name" changing to "name". So, I'd need to see token contents for exactly the same user before decided if we can afford the change in 3.0, given the possibility of extra work, because it doesn't look like it's just pointing to the new endpoint. |
|
@blowdart You can get the email on the email claim by asking for the email scope (which we can ask for by default and is more standard compliant). This is the V1 token
This is the V2 token
We can likely configure the V2 default set of claims to include the email and other claims based on open id. Overall I think it's worth pointing the size reduction of the token and the standarization. |
|
As long as the claims that the eventual identity is constructed from will match from one version to the next, so there's no unexpected loss then I'm ok with it. |
|
@jmprieur Do you think this can be achieved? Or is there any claim that is gone forever (that matters) that we can't bring back by requesting some other scopes by default. |
|
@javiercn @blowdart - just to note that Jean-Marc is OOF until 30th, please expect delays in his answers. You can have a look at the optional claims you can request for v1 and v2 endpoints here: https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-optional-claims |
|
@blowdart if the tokens contain different claims, the ClaimsIdentity will not be the same. |
| /// Gets or sets the audience for a Web API (This audience needs | ||
| /// to match the audience of the tokens sent to access this application) | ||
| /// </summary> | ||
| public string Audience { get; set; } = "{ClientId}"; |
There was a problem hiding this comment.
Should we allow the setters to accept null or empty strings?
Similar for all these setters.
| /// </list> | ||
| /// </summary> | ||
| public string Instance { get; set; } | ||
| public string Instance { get; set; } = "https://login.microsoftonline.com/"; |
There was a problem hiding this comment.
Is there ever a time when these can be empty or null?
| azureADOptions.Instance, azureADOptions.TenantId); | ||
| } | ||
|
|
||
| public void Configure(JwtBearerOptions options) |
There was a problem hiding this comment.
This method doesn't seem to do anything.
There was a problem hiding this comment.
It implements the interface https://github.com/aspnet/Extensions/blob/master/src/Options/Options/src/IConfigureOptions.cs#L11
Though I'm not sure if this is correct - should it be doing something?
There was a problem hiding this comment.
This is fine, as this class only targets named configuration instances.
There was a problem hiding this comment.
A code comment might be useful, then.
|
|
||
| options.Audience = azureADOptions.ClientId; | ||
| options.Authority = new Uri(new Uri(azureADOptions.Instance), azureADOptions.TenantId).ToString(); | ||
| options.Audience = string.Format(azureADOptions.Audience?.Replace("{ClientId}", "{0}"), |
There was a problem hiding this comment.
Should add some parameter checks for nulls.
|
We're not going to be able to take this in 3.0. We can revisit in 3.1. |
10 participants
Adding flexibility for the authority and audience (to enable the Azur…e AD v2.0 endpoint)
Summary of the changes (Less than 80 chars)
Authoritywhich is now computed from theInstanceandClientId, so that AzureAD v2.0 can be supported (Authority would therefore behttps://{Instance}/{TenantId}/v2.0)Audiencefor Web APIs so that it can be set (for instance toapi://{tenantId}in v2.0)Addresses requests for supporting Azure AD v2.0 (Microsoft identity platform for developers)
This is similar to aspnet/AADIntegration#49, but without breaking changes. This will enable supporting Azure AD v2.0 naturally (from the templates)