Skip to content
This repository was archived by the owner on Dec 20, 2018. It is now read-only.

Proposes more flexible AzureADOptions, and defaulting to AzureAD v2.0 #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,29 +30,47 @@ 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 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}";
Copy link
Member

Choose a reason for hiding this comment

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

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


/// <summary>
/// Gets or sets the client secret for the application
/// </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. The tenant can have one of the following values:
/// <list type="table">
/// <item><term>a tenant ID</term><description>A GUID representing the ID of the Azure Active Directory Tenant</description></item>
/// <item><term>a domain</term><description>associated with Azure Active Directory</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; }
public string Tenant { get; set; } = "common";
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.


/// <summary>
/// Gets or sets the Azure Active Directory instance.
/// </summary>
public string Instance { get; set; }
public string Instance { get; set; } = "https://login.microsoftonline.com";

/// <summary>
/// Gets or sets the domain of the Azure Active Directory tennant.
/// Azure AD Authority.
/// </summary>
public string Domain { get; set; }
public string Authority { get; set; } = "https://{Instance}/{Tenant}/v2.0";

/// <summary>
/// Gets or sets the sign in callback path.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,12 @@ public void Configure(string name, JwtBearerOptions options)
return;
}

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);
Copy link
Member

@Tratcher Tratcher Oct 31, 2018

Choose a reason for hiding this comment

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

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);

Copy link
Member

Choose a reason for hiding this comment

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

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}"


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

}

public void Configure(JwtBearerOptions options)
Expand Down
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();
string authorityFormat = azureADOptions.Authority.Replace("{Instance}", "{0}").Replace("{Tenant}", "{1}");
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as for the audience

options.Authority = string.Format(authorityFormat, azureADOptions.Instance, azureADOptions.Tenant);
options.CallbackPath = azureADOptions.CallbackPath ?? options.CallbackPath;
options.SignedOutCallbackPath = azureADOptions.SignedOutCallbackPath ?? options.SignedOutCallbackPath;
options.SignInScheme = azureADOptions.CookieSchemeName;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,8 @@ public async Task BearerAzureAD_Challenges_UnauthorizedRequests()
.AddAzureADBearer(o =>
{
o.Instance = "https://login.microsoftonline.com/";
o.Domain = "test.onmicrosoft.com";
o.Tenant= "test.onmicrosoft.com";
Copy link
Member

Choose a reason for hiding this comment

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

Spacing

o.ClientId = "ClientId";
o.TenantId = "TenantId";
});

services.Configure<JwtBearerOptions>(AzureADDefaults.JwtBearerAuthenticationScheme, o =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,8 @@ public async Task ADEndpoints_AreAvailable_When_Authentication_IsAdded(string en
.AddAzureAD(o =>
{
o.Instance = "https://login.microsoftonline.com/";
o.Domain = "test.onmicrosoft.com";
o.Tenant = "test.onmicrosoft.com";
o.ClientId = "ClientId";
o.TenantId = "TenantId";
});

services.Configure<OpenIdConnectOptions>(AzureADDefaults.OpenIdScheme, o =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ public void AddAzureAD_ConfiguresAllOptions()
o.ClientId = "ClientId";
o.ClientSecret = "ClientSecret";
o.CallbackPath = "/signin-oidc";
o.Domain = "domain.onmicrosoft.com";
o.TenantId = "Common";
o.Tenant = "common";
});
var provider = services.BuildServiceProvider();

Expand All @@ -64,7 +63,7 @@ public void AddAzureAD_ConfiguresAllOptions()
Assert.Equal("ClientId", azureADOptions.ClientId);
Assert.Equal("ClientSecret", azureADOptions.ClientSecret);
Assert.Equal("/signin-oidc", azureADOptions.CallbackPath);
Assert.Equal("domain.onmicrosoft.com", azureADOptions.Domain);
Assert.Equal("common", azureADOptions.Tenant);

var openIdOptionsMonitor = provider.GetService<IOptionsMonitor<OpenIdConnectOptions>>();
Assert.NotNull(openIdOptionsMonitor);
Expand Down Expand Up @@ -176,8 +175,7 @@ public void AddAzureADBearer_ConfiguresAllOptions()
o.Instance = "https://login.microsoftonline.com/";
o.ClientId = "ClientId";
o.CallbackPath = "/signin-oidc";
o.Domain = "domain.onmicrosoft.com";
o.TenantId = "TenantId";
o.Tenant = "domain.onmicrosoft.com";
});
var provider = services.BuildServiceProvider();

Expand All @@ -188,7 +186,7 @@ public void AddAzureADBearer_ConfiguresAllOptions()
Assert.Equal(AzureADDefaults.JwtBearerAuthenticationScheme, options.JwtBearerSchemeName);
Assert.Equal("https://login.microsoftonline.com/", options.Instance);
Assert.Equal("ClientId", options.ClientId);
Assert.Equal("domain.onmicrosoft.com", options.Domain);
Assert.Equal("domain.onmicrosoft.com", options.Tenant);

var bearerOptionsMonitor = provider.GetService<IOptionsMonitor<JwtBearerOptions>>();
Assert.NotNull(bearerOptionsMonitor);
Expand Down