Skip to content

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

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
jmprieur wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
21fb647
Adding flexibility for the authority and audience (to enable the Azur…
jmprieur Feb 25, 2019
0db0dc8
fixing the Authority
jmprieur Feb 25, 2019
3d0ccfd
Avoid unnecessary changes (don't remove / for instance)
jmprieur Feb 25, 2019
efefe03
Updating the project templates according to the product change
jmprieur Feb 25, 2019
3cc8016
Updating the project templates according to the product change
jmprieur Feb 25, 2019
bf5923c
updating Authority computation
jmprieur Feb 25, 2019
723463e
Merge branch 'master' of https://github.com/aspnet/AspNetCore
jmprieur Feb 25, 2019
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: 4 additions & 2 deletions src/Azure/AzureAD/Authentication.AzureAD.UI/src/AzureADJwtBearerOptionsConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ public void Configure(string name, JwtBearerOptions options)
return;
}

options.Audience = azureADOptions.ClientId;
options.Authority = new Uri(new Uri(azureADOptions.Instance), azureADOptions.TenantId).ToString();
options.Audience = string.Format(azureADOptions.Audience?.Replace("{ClientId}", "{0}"),
Copy link
Member

@javiercn javiercn Feb 28, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jmprieur jmprieur Mar 4, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@brentschmaltz brentschmaltz May 10, 2019

Choose a reason for hiding this comment

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

@jmprieur how is azureADOptions populated?

Copy link
Contributor

@brentschmaltz brentschmaltz May 11, 2019

Choose a reason for hiding this comment

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

@javiercn is there a way to enforce some type of validation, For example, azureADOptions.Audience must be populated correctly from config?

Copy link
Contributor

@brentschmaltz brentschmaltz May 10, 2019

Choose a reason for hiding this comment

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

Should add some parameter checks for nulls.

azureADOptions.ClientId);
options.Authority = string.Format(azureADOptions.Authority.Replace("{Instance}", "{0}").Replace("{TenantId}", "{1}"),
azureADOptions.Instance, azureADOptions.TenantId);
}

public void Configure(JwtBearerOptions options)
Copy link
Contributor

@brentschmaltz brentschmaltz May 10, 2019

Choose a reason for hiding this comment

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

This method doesn't seem to do anything.

Copy link
Contributor

@Eilon Eilon May 13, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Member

@javiercn javiercn May 13, 2019

Choose a reason for hiding this comment

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

This is fine, as this class only targets named configuration instances.

Copy link
Contributor

@Eilon Eilon May 13, 2019

Choose a reason for hiding this comment

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

A code comment might be useful, then.

Expand Down
3 changes: 2 additions & 1 deletion src/Azure/AzureAD/Authentication.AzureAD.UI/src/AzureADOpenIdConnectOptionsConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public void Configure(string name, OpenIdConnectOptions options)

options.ClientId = azureADOptions.ClientId;
options.ClientSecret = azureADOptions.ClientSecret;
options.Authority = new Uri(new Uri(azureADOptions.Instance), azureADOptions.TenantId).ToString();
options.Authority = string.Format(azureADOptions.Authority.Replace("{Instance}", "{0}").Replace("{TenantId}", "{1}"),
azureADOptions.Instance, azureADOptions.TenantId);
options.CallbackPath = azureADOptions.CallbackPath ?? options.CallbackPath;
options.SignedOutCallbackPath = azureADOptions.SignedOutCallbackPath ?? options.SignedOutCallbackPath;
options.SignInScheme = azureADOptions.CookieSchemeName;
Expand Down
44 changes: 38 additions & 6 deletions src/Azure/AzureAD/Authentication.AzureAD.UI/src/AzureADOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.using Microsoft.AspNetCore.Authorization;

using Microsoft.AspNetCore.Authentication.Cookies;
Expand Down Expand Up @@ -30,30 +30,62 @@ public class AzureADOptions
public string JwtBearerSchemeName { get; internal set; }

/// <summary>
/// Gets or sets the client Id.
/// Gets or sets the client Id (Application Id) of the Azure AD application
/// </summary>
public string ClientId { get; set; }

/// <summary>
/// Gets or sets the client secret.
/// Gets or sets the client secret for the application (application password)
/// </summary>
/// <remarks>
/// The client secret is only used if the Web app or Web API
/// calls a Web API
/// </remarks>
public string ClientSecret { get; set; }

/// <summary>
/// Gets or sets the tenant Id.
/// Gets or sets the tenant id. The tenant id can have one of the following values:
/// <list type="table">
/// <item><term>a proper tenant ID</term><description>A GUID representing the ID of the Azure Active Directory Tenant (directory ID)</description></item>
/// <item><term>a domain name</term><description>associated with the Azure Active Directory tenant</description></item>
/// <item><term>common</term><description>if the <see cref="Authority"/> is Azure AD v2.0, enables to sign-in users from any
/// Work and School account or Microsoft Personal Account. If Authority is Azure AD v1.0, enables sign-in from any Work and School accounts</description></item>
/// <item><term>organizations</term><description>if the <see cref="Authority"/> is Azure AD v2.0, enables to sign-in users from any
/// Work and School account</description></item>
/// <item><term>consumers</term><description>if the <see cref="Authority"/> is Azure AD v2.0, enables to sign-in users from any
/// Microsoft personal account</description></item>
/// </list>
/// </summary>
public string TenantId { get; set; }

/// <summary>
/// Gets or sets the Azure Active Directory instance.
/// Typical values are:
Copy link
Member

@Tratcher Tratcher Feb 28, 2019

Choose a reason for hiding this comment

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

Defaults to "https://login.microsoftonline.com/"

/// <list type="table">
/// <item><term>https://login.microsoftonline.com/</term><description>For Microsoft Azure public cloud</description></item>
/// <item><term>https://login.microsoftonline.us/</term><description>For Azure US Government</description></item>
/// <item><term>https://login.partner.microsoftonline.cn/</term><description>For Azure China 21Vianet</description></item>
/// <item><term>https://login.microsoftonline.de/</term><description>For Azure Germany</description></item>
/// </list>
/// </summary>
public string Instance { get; set; }
public string Instance { get; set; } = "https://login.microsoftonline.com/";
Copy link
Contributor

@brentschmaltz brentschmaltz May 10, 2019

Choose a reason for hiding this comment

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

Is there ever a time when these can be empty or null?


/// <summary>
/// Gets or sets the domain of the Azure Active Directory tennant.
/// Gets or sets the domain associated with the Azure Active Directory tenant.
/// </summary>
public string Domain { get; set; }

/// <summary>
/// Azure Active Directory Authority
/// </summary>
public string Authority { get; set; } = "{Instance}{TenantId}/";

/// <summary>
/// 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}";
Copy link
Contributor

@brentschmaltz brentschmaltz May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Should we allow the setters to accept null or empty strings?
Similar for all these setters.


/// <summary>
/// Gets or sets the sign in callback path.
/// </summary>
Expand Down
65 changes: 33 additions & 32 deletions src/ProjectTemplates/Web.ProjectTemplates/content/RazorPagesWeb-CSharp/appsettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,37 @@
{
////#if (IndividualB2CAuth)
// "AzureAdB2C": {
// "Instance": "https:////login.microsoftonline.com/tfp/",
// "ClientId": "11111111-1111-1111-11111111111111111",
// "CallbackPath": "/signin-oidc",
// "Domain": "qualified.domain.name",
// "SignUpSignInPolicyId": "MySignUpSignInPolicyId",
// "ResetPasswordPolicyId": "MyResetPasswordPolicyId",
// "EditProfilePolicyId": "MyEditProfilePolicyId"
// },
////#elseif (OrganizationalAuth)
// "AzureAd": {
//#if (MultiOrgAuth)
// "Instance": "https:////login.microsoftonline.com/common",
//#elseif (SingleOrgAuth)
// "Instance": "https:////login.microsoftonline.com/",
// "Domain": "qualified.domain.name",
// "TenantId": "22222222-2222-2222-2222-222222222222",
//#endif
// "ClientId": "11111111-1111-1111-11111111111111111",
// "CallbackPath": "/signin-oidc"
// },
//#endif
////#if (IndividualLocalAuth)
// "ConnectionStrings": {
////#if (UseLocalDB)
// "DefaultConnection": "Server=(localdb)\\mssqllocaldb;Database=aspnet-Company.WebApplication1-53bc9b9d-9d6a-45d4-8429-2a2761773502;Trusted_Connection=True;MultipleActiveResultSets=true"
////#else
// "DefaultConnection": "DataSource=app.db"
//#endif
// },
//#endif
////#if (IndividualB2CAuth)
Copy link
Member

@Tratcher Tratcher Feb 27, 2019

Choose a reason for hiding this comment

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

Was this just a whitespace change? If so revert.

// "AzureAdB2C": {
// "Instance": "https:////login.microsoftonline.com/tfp/",
// "ClientId": "11111111-1111-1111-11111111111111111",
// "CallbackPath": "/signin-oidc",
// "Domain": "qualified.domain.name",
// "SignUpSignInPolicyId": "MySignUpSignInPolicyId",
// "ResetPasswordPolicyId": "MyResetPasswordPolicyId",
// "EditProfilePolicyId": "MyEditProfilePolicyId"
// },
////#elseif (OrganizationalAuth)
// "AzureAd": {
// "Instance": "https:////login.microsoftonline.com/",
// "Autority": "{Instance}{TenantId}/v2.0",
//#if (MultiOrgAuth)
// "TenantId": "organizations",
Copy link
Member

@javiercn javiercn Feb 28, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@jmprieur jmprieur Mar 4, 2019

Choose a reason for hiding this comment

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

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 (SingleOrgAuth)
// "Domain": "qualified.domain.name",
// "TenantId": "22222222-2222-2222-2222-222222222222",
//#endif
// "ClientId": "11111111-1111-1111-11111111111111111",
// "CallbackPath": "/signin-oidc"
// },
//#endif
////#if (IndividualLocalAuth)
// "ConnectionStrings": {
////#if (UseLocalDB)
// "DefaultConnection": "Server=(localdb)\\mssqllocaldb;Database=aspnet-Company.WebApplication1-53bc9b9d-9d6a-45d4-8429-2a2761773502;Trusted_Connection=True;MultipleActiveResultSets=true"
////#else
// "DefaultConnection": "DataSource=app.db"
//#endif
// },
//#endif
"Logging": {
"LogLevel": {
"Default": "Warning",
Expand Down
5 changes: 3 additions & 2 deletions src/ProjectTemplates/Web.ProjectTemplates/content/StarterWeb-CSharp/appsettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@
// },
////#elseif (OrganizationalAuth)
// "AzureAd": {
// "Instance": "https:////login.microsoftonline.com/",
// "Autority": "{Instance}{TenantId}/v2.0",
Copy link
Member

@Tratcher Tratcher Feb 28, 2019

Choose a reason for hiding this comment

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

Use the same order in both templates.

Copy link
Member

@javiercn javiercn Feb 28, 2019

Choose a reason for hiding this comment

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

Indentation too

Copy link
Contributor Author

@jmprieur jmprieur Mar 4, 2019

Choose a reason for hiding this comment

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

@Tratcher : the templates are in a different order already today (I was surprised too). I did not change the order.

//#if (MultiOrgAuth)
// "Instance": "https:////login.microsoftonline.com/common",
// "TenantId": "organizations",
//#elseif (SingleOrgAuth)
// "Instance": "https:////login.microsoftonline.com/",
// "Domain": "qualified.domain.name",
// "TenantId": "22222222-2222-2222-2222-222222222222",
//#endif
Expand Down
39 changes: 20 additions & 19 deletions src/ProjectTemplates/Web.ProjectTemplates/content/WebApi-CSharp/appsettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
{
////#if (IndividualB2CAuth)
// "AzureAdB2C": {
// "Instance": "https:////login.microsoftonline.com/tfp/",
// "ClientId": "11111111-1111-1111-11111111111111111",
// "Domain": "qualified.domain.name",
// "SignUpSignInPolicyId": "MySignUpSignInPolicyId"
// },
////#elseif (OrganizationalAuth)
// "AzureAd": {
//#if (!SingleOrgAuth)
// "Instance": "https:////login.microsoftonline.com/common",
//#else
// "Instance": "https:////login.microsoftonline.com/",
// "Domain": "qualified.domain.name",
// "TenantId": "22222222-2222-2222-2222-222222222222",
//#endif
// "ClientId": "11111111-1111-1111-11111111111111111"
// },
//#endif
////#if (IndividualB2CAuth)
// "AzureAdB2C": {
// "Instance": "https:////login.microsoftonline.com/tfp/",
// "ClientId": "11111111-1111-1111-11111111111111111",
// "Domain": "qualified.domain.name",
// "SignUpSignInPolicyId": "MySignUpSignInPolicyId"
// },
////#elseif (OrganizationalAuth)
// "AzureAd": {
// "Autority": "{Instance}{TenantId}/v2.0",
// "Instance": "https:////login.microsoftonline.com/",
//#if (!SingleOrgAuth)
// "TenantId": "organizations",
//#else
// "Domain": "qualified.domain.name",
// "TenantId": "22222222-2222-2222-2222-222222222222",
//#endif
// "ClientId": "11111111-1111-1111-11111111111111111"
// },
//#endif
"Logging": {
"LogLevel": {
"Default": "Warning",
Expand Down