Setting Access Denied path to Identity.Web.UI Access Denied page

[pmaytak]

There are two ways to implement the fix. I put related comments in PR. Please give opinion. More info in #117.

Also

• Fixed warning SA1400
• Updated unit tests related to Removing a useless instruction and generating XML comments #156, [Bug] AddProtectedWebApiCallsProtectedWebApi overwrites existing OnTokenValidated event handler #154.

[pmaytak]

Option 1.
If the path is not set, we set it to default one of our choice.
Pro The setup is all in one place and the scheme we use is abstracted from the user.
Con If the user doesn't call AddMicrosoftIdentityUI and wants to use the default ASP.NET Core path (/Account/AccessDenied), we'll end up replacing it.
Con We're kind of mixing non-UI logic with UI logic.

[jmprieur]

Given that we don't impose developers to use Microsoft.Identity.Web.UI, the /MicrosoftIdentity/Account/AccessDenied page won't be accessible if they don't use it. I would think that the first Con (If the user doesn't call AddMicrosoftIdentityUI and wants to use the default ASP.NET Core path) is really a Pro, as it calling AddMicrosoftIdentityUI requires Microsoft.Identity.Web.UI to be added.

[pmaytak]

Option 2.
Set the path inside AddMicrosoftIdentityUI. We need to set the path on the scheme with the same name that we assigned to OpenIdConnectOptions.SignInScheme in AddSignIn. That's why I added a scheme parameter.
Pro UI related code is together.
Con It looks a bit weird passing a cookie scheme to this UI related method.
Con If a user uses a non-default cookie scheme name, they would have to pass it here. A bit fragile.

public static IMvcBuilder AddMicrosoftIdentityUI(this IMvcBuilder builder, string cookieScheme = CookieAuthenticationDefaults.AuthenticationScheme)
{
	if (builder == null)
	{
		throw new ArgumentNullException(nameof(builder));
	}

	builder.ConfigureApplicationPartManager(apm =>
	{
		apm.FeatureProviders.Add(new MicrosoftIdentityAccountControllerFeatureProvider());
	});

	builder.Services.Configure<CookieAuthenticationOptions>(cookieScheme, options =>
	{
		options.AccessDeniedPath = new PathString("/MicrosoftIdentity/Account/AccessDenied");
	});

	return builder;
}

[jmprieur]

That's my prefered option (even if I understand the fragility), because /MicrosoftIdentity/Account/AccessDenied is defined in Microsoft.Identity.Web.UI, which customers are not obliged to use.

We'll document the fragility

Is there a way for us to understand which CookieScheme would have been passed into AddSignIn ?

[pmaytak]

I think we could check the Services here if the cookie or OIDC options were added. But that won't work if AddSignIn is called after this method.

I also tried configuring without the scheme name, which should be the default, but that didn't work.

I did have an idea to maybe use PostConfigure somehow, which is trigged after all of the options are added. Will have to explore.

[pmaytak]

@jmprieur ConfigureAll works. No need to specify scheme name for this.

[jmprieur]

LGTM. Thanks @pmaytak
my preference is Option 2.
I don't think we shoud reference in AddSignIn something which is in a NuGet package that customers don't need to add.

[jmprieur]

Given that we don't impose developers to use Microsoft.Identity.Web.UI, the /MicrosoftIdentity/Account/AccessDenied page won't be accessible if they don't use it. I would think that the first Con (If the user doesn't call AddMicrosoftIdentityUI and wants to use the default ASP.NET Core path) is really a Pro, as it calling AddMicrosoftIdentityUI requires Microsoft.Identity.Web.UI to be added.

[jmprieur]

That's my prefered option (even if I understand the fragility), because /MicrosoftIdentity/Account/AccessDenied is defined in Microsoft.Identity.Web.UI, which customers are not obliged to use.

We'll document the fragility

Is there a way for us to understand which CookieScheme would have been passed into AddSignIn ?

[pmaytak]

@jmprieur Agree that option 2 seems better. I'll update the code to use it and see if, perhaps, there's a way to improve it by using something like PostConfigure.

[jmprieur]

LGTM.
Thanks @pmaytak