Proposes more flexible AzureADOptions, and defaulting to AzureAD v2.0 by jmprieur · Pull Request #49 · aspnet/AADIntegration

jmprieur · 2018-10-31T14:01:39Z

Proposes improvements to the AzureADOptions:

make it possible to specify the Audience of the tokens that will be accepted (with a default which is consistent with the Azure AD experience). For the moment it's hardcoded to {ClientId}
make it possible to specity the Authority (for the moment it's hardcoded to {Instance}/{TenantId}. This is needed to make v2.0 primary without having users to have the change app generated by dotnet new mvc
Proposing to remove Domain, and rename TenantId in Tenant as the tenant identification can be specified as a TenantID (a Guid), or a domain, or some well known "meta-tenants" (common, organizations and consumers)

Proposing that Web applications are now v2.0 apps by default.

Proposing improvements to the XML documentation

Note that
This PR is essentially to start the discussion. It's not mean to be final. In particular, no special effort was made for the tests yet

- make it possible to specify the Audience of the tokens that will be accepted (with a default which is consistent with the Azure AD experience). For the moment it's hardcoded to {ClientId} - make it possible tp specity the Authority (for the moment it's hardcoded to {Instance}/{TenantId}. This is needed to make v2.0 primary without having users to have the change app generated by dotnet new mvc - Proposing to remove Domain, and rename TenantId in Tenant as the tenant identification can be specified as a TenantID (a Guid), or a domain, or some well known metatenant (common, organizations and consumers) Proposing that Web applications are now v2.0 apps by default.

dnfclas · 2018-10-31T14:01:54Z

All CLA requirements met.

jmprieur · 2018-10-31T14:11:13Z

@Tratcher @brentschmaltz FYI

Tratcher · 2018-10-31T15:08:42Z

@blowdart @BillHiebert This would need to be accompanied with template and tooling changes to get the right fields populated in config.

blowdart · 2018-10-31T15:15:14Z

Without tooling changes this can't be merged, the VS UI would get everything wrong. So it's not as simple as this PR :)

jmprieur · 2018-10-31T15:18:42Z

I'm well aware of this, @blowdart : this is a proposal to help the discussion that @Tratcher and I have been having offline. Some of the things we'd want to consider is the breaking changes:

are we happy with renaming TenantId to Tenant? or not. Note that we could do with keeping TenantId (which would be one less breaking change) and use it for tenant information in general: not necessarily a guid. That's the case today, even if the meaning is not very clear
are we happy to remove Domain (which is not used anywhere btw)

Then as far as template and tooling changes, could you please send me the location of the repos, so that we propose PRs?

blowdart · 2018-10-31T15:32:48Z

I have no real feelings about the changes, if they matched the Azure Portal wording for these details that would be best (assuming this won't change of course)

Tratcher

This looks fine for 3.0 with a little code cleanup.

Tratcher · 2018-10-31T15:58:13Z

src/Microsoft.AspNetCore.Authentication.AzureAD.UI/JwtBearerOptionsConfiguration.cs

-            options.Audience = azureADOptions.ClientId;
-            options.Authority = new Uri(new Uri(azureADOptions.Instance), azureADOptions.TenantId).ToString();
+            string audienceFormat = azureADOptions.Authority.Replace("{ClientId}", "{0}");
+            options.Audience = string.Format(audienceFormat, azureADOptions.ClientId);


This can be a single operation everywhere you do it.

Suggested change

options.Audience = string.Format(audienceFormat, azureADOptions.ClientId);

options.Audience = azureADOptions.Authority.Replace("{ClientId}", azureADOptions.ClientId);

We don't do things like this in the framework. Audience should be initialized here, not in the property, and it's also better to just use interpolated strings directly. $"api://{azureAdOptions.ClientId}"

Tratcher · 2018-10-31T16:00:10Z

test/Microsoft.AspNetCore.Authentication.AzureAD.FunctionalTests/ApiAuthenticationTests.cs

                        {
                            o.Instance = "https://login.microsoftonline.com/";
-                            o.Domain = "test.onmicrosoft.com";
+                            o.Tenant= "test.onmicrosoft.com";


Tratcher · 2018-10-31T16:02:37Z

@phenning for tooling.

Tratcher · 2018-10-31T16:03:31Z

Template code: https://github.com/aspnet/Templating/blob/master/src/Microsoft.DotNet.Web.ProjectTemplates/content/StarterWeb-CSharp/appsettings.json

Tratcher · 2018-10-31T16:13:46Z

#50

javiercn

I have some comments, but it's a good starting point for a discussion. As @blowdart said this will require coordination with templates and tooling.

javiercn · 2018-10-31T17:23:56Z

src/Microsoft.AspNetCore.Authentication.AzureAD.UI/JwtBearerOptionsConfiguration.cs

-            options.Audience = azureADOptions.ClientId;
-            options.Authority = new Uri(new Uri(azureADOptions.Instance), azureADOptions.TenantId).ToString();
+            string audienceFormat = azureADOptions.Authority.Replace("{ClientId}", "{0}");
+            options.Audience = string.Format(audienceFormat, azureADOptions.ClientId);


We don't do things like this in the framework. Audience should be initialized here, not in the property, and it's also better to just use interpolated strings directly. $"api://{azureAdOptions.ClientId}"

javiercn · 2018-10-31T17:24:38Z

src/Microsoft.AspNetCore.Authentication.AzureAD.UI/OpenIdConnectOptionsConfiguration.cs

            options.ClientId = azureADOptions.ClientId;
            options.ClientSecret = azureADOptions.ClientSecret;
-            options.Authority = new Uri(new Uri(azureADOptions.Instance), azureADOptions.TenantId).ToString();
+            string authorityFormat = azureADOptions.Authority.Replace("{Instance}", "{0}").Replace("{Tenant}", "{1}");


Same comment as for the audience

javiercn · 2018-10-31T17:27:25Z

src/Microsoft.AspNetCore.Authentication.AzureAD.UI/AzureADOptions.cs

+        /// </list>
        /// </summary>
-        public string TenantId { get; set; }
+        public string Tenant { get; set; } = "common";


This is a breaking change. It'll have to be discussed

I don't think we should default to the common tenant here as just using the common tenant without additional per-app restriction is not recommended.

javiercn · 2018-10-31T17:31:09Z

src/Microsoft.AspNetCore.Authentication.AzureAD.UI/AzureADOptions.cs

+        /// Gets or sets the audience for this Web Application or Web API (This audience needs
+        /// to match the audience of the tokens sent to access this application)
        /// </summary>
+        public string Audience { get; set; } = "api://{ClientId}";


Can the audience be completely different from the client id or is always derived from the client id though some transformation.

Tratcher requested a review from javiercn October 31, 2018 15:55

Tratcher reviewed Oct 31, 2018

View reviewed changes

javiercn suggested changes Oct 31, 2018

View reviewed changes

jmprieur mentioned this pull request Feb 25, 2019

Adding flexibility for the authority and audience to support Azure AD v2.0 dotnet/aspnetcore#7913

Closed

	options.Audience = string.Format(audienceFormat, azureADOptions.ClientId);
	options.Audience = azureADOptions.Authority.Replace("{ClientId}", azureADOptions.ClientId);

Conversation

jmprieur commented Oct 31, 2018

Uh oh!

dnfclas commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmprieur commented Oct 31, 2018

Uh oh!

Tratcher commented Oct 31, 2018

Uh oh!

blowdart commented Oct 31, 2018

Uh oh!

jmprieur commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blowdart commented Oct 31, 2018

Uh oh!

Tratcher left a comment

Choose a reason for hiding this comment

Uh oh!

Tratcher Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

Tratcher Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Oct 31, 2018

Uh oh!

Tratcher commented Oct 31, 2018

Uh oh!

Tratcher commented Oct 31, 2018

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

javiercn Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

javiercn Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

javiercn Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

javiercn Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

javiercn Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dnfclas commented Oct 31, 2018 •

edited

Loading

jmprieur commented Oct 31, 2018 •

edited

Loading

Tratcher Oct 31, 2018 •

edited

Loading