Jennyf/null ref #139
Jennyf/null ref #139
Conversation
…ither null or white space in the configuration settings.
- clean up authority helpers - add unit tests - add string.format()
jennyf19
force-pushed
the
jennyf/nullRef
branch
from
4b78b16 to
25e54bc
Compare
|
@bgavrilMS this is related to the MSAL null ref we discussed this morning. Here's a PR for MSAL .NET |
There was a problem hiding this comment.
LGTM. @jennyf19
Thanks for making the configuration more reliable
for the future, I just discovered that we could implement a IValidateOptions. See for instance https://github.com/dotnet/aspnetcore/blob/master/src/Azure/AzureAD/Authentication.AzureAD.UI/src/AzureADOptionsValidation.cs
It's then registered like this: https://github.com/dotnet/aspnetcore/blob/49b4f418d5e7351b192023515e33e06c84d25e0e/src/Azure/AzureAD/Authentication.AzureAD.UI/src/AzureADAuthenticationBuilderExtensions.cs#L63
Maybe we'd want to raise a new issue to use this pattern (which separates concerns even better)?
| var userFlow = options.DefaultUserFlow; | ||
| return new Uri(baseUri, new PathString($"{pathBase}/{domain}/{userFlow}/v2.0")).ToString(); | ||
| } | ||
| else | ||
| else // Create an AAD authority |
There was a problem hiding this comment.
If it's not B2C, it can be ADFS.
There was a problem hiding this comment.
@bgavrilMS : do you think that Microsoft.Identity.Web should support ADFS?
There was a problem hiding this comment.
I think you should at least throw a "not supported" exception or smth.
| throw; | ||
| } | ||
| } | ||
|
|
||
| internal void CheckApplicationOptionsContainClientSecret(ConfidentialClientApplicationOptions applicationOptions) |
There was a problem hiding this comment.
I think that this fix is not necessary in Identity.Web, and that it should be only in MSAL. But I can understand having a tactical fix in Identity.Web if you cannot wait for the next MSAL release (or you can hotfix MSAL).
There was a problem hiding this comment.
We want to check the other parameters anyway.
| @@ -349,6 +353,8 @@ private async Task<IConfidentialClientApplication> BuildConfidentialClientApplic | |||
|
|
|||
| try | |||
| { | |||
| CheckApplicationOptionsContainClientSecret(_applicationOptions); | |||
There was a problem hiding this comment.
TLDR: What about certificates?
There are 4 ways of specifying the secure contract between the server and AAD in a confidential client:
- client secret (should not really be used in production)
- cert (recommended)
- client assertion (which is essentially a string with cert details - we use this when MSAL can't handle wierd certs)
- cert + extra claims
ConfidentialClientApplicationOptions only allows users to pass in a secret. But you must provide mechanism for user to configure certificates.
There was a problem hiding this comment.
@bgavrilMS : yes, and we have an enhancements issue for this: (#12).
For the moment, proposing an incremental approach
| if (string.IsNullOrEmpty(applicationOptions.ClientSecret)) | ||
| { | ||
| string msg = string.Format(CultureInfo.InvariantCulture, "Client secret cannot be null or whitespace, " + | ||
| "and must be included in the configuration of the web app when calling a web API. " + |
There was a problem hiding this comment.
A certificate can be used instead of a secret.
There was a problem hiding this comment.
@bgavrilMS : yes, and we have an enhancements issue for this: (#12).
For the moment, proposing an incremental approach
| } | ||
| else | ||
| { | ||
| if (string.IsNullOrEmpty(microsoftIdentityOptions.TenantId)) |
There was a problem hiding this comment.
That's a good question @bgavrilMS
For the moment, I did not even considered that Microsoft.Identity.Web coud support ADFS, but maybe it should.
that's much easier. i just did it on a separate branch and it's easier to check web api options now. thx. will add some tests adn push that branhc. |
#66
and
#19 (comment)