updates to id web to support authentication handlers than JwtBearer #1493
Conversation
* initial commit to move aadIssuerValidator * avoid breaking changes * add xml * update tests * fix test warning * PR feedback
|
I did a release build last night which we've already been testing with, so not sure if another one is needed at this point. and yes, i would like to merge to master if this is the direction we want to go. In reply to: 782580128 |
src/Microsoft.Identity.Web.IssuerValidator/Microsoft.Identity.Web.IssuerValidator.csproj
Outdated
Show resolved
Hide resolved
|
SonarCloud Quality Gate failed. |
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>netcoreapp3.1; net462; net472; net5.0; netstandard2.0</TargetFrameworks> |
There was a problem hiding this comment.
Can we support net45 and net461 with this package?
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/d8ad857088d961ccccf69058c7fd7eb4f640607c/build/targets.props#L3
There was a problem hiding this comment.
I guess net45 is out of the picture because of the System.Text.Json dependency.
There was a problem hiding this comment.
@GeoK: Do we need to support net45? would we want to leverage Microsoft.IdentityModel.Json for net45 ? or even for all the target framework?
There was a problem hiding this comment.
Depending on the assembly that will host this model. Microsoft.IdentityModel.Json is internal and would have to grant internals to IdWeb which is less than ideal.
| using System.Runtime.CompilerServices; | ||
|
|
||
| // Allow this assembly to be serviced when run on desktop CLR | ||
| [assembly: InternalsVisibleTo("Microsoft.Identity.Web.IssuerValidator.Test, PublicKey=00240000048000009400000006020000002400005253413100040000010001002D96616729B54F6D013D71559A017F50AA4861487226C523959D1579B93F3FDF71C08B980FD3130062B03D3DE115C4B84E7AC46AEF5E192A40E7457D5F3A08F66CEAB71143807F2C3CB0DA5E23B38F0559769978406F6E5D30CEADD7985FC73A5A609A8B74A1DF0A29399074A003A226C943D480FEC96DBEC7106A87896539AD")] |
| actualIssuer)); | ||
| } | ||
|
|
||
| private string CreateV1Authority() | ||
| { | ||
| if (AadAuthority.Contains(Constants.Organizations, StringComparison.OrdinalIgnoreCase)) | ||
| #if DOTNET_STANDARD_20 || DOTNET_462 || DOTNET_472 | ||
| if (AadAuthority.Contains(IssuerValidatorConstants.Organizations)) |
There was a problem hiding this comment.
Would it be fair to say that if AadAuthority is not 'common'/'organizations', extracting a tenantId and creating & comparing URIs is not necessary? Thinking if we could cut these cycles as issuer validation is on the hot-path.
There was a problem hiding this comment.
Makes sense, great point.
| @@ -132,36 +129,46 @@ public class AadIssuerValidator | |||
| throw new SecurityTokenInvalidIssuerException( | |||
| string.Format( | |||
| CultureInfo.InvariantCulture, | |||
| IDWebErrorMessage.IssuerDoesNotMatchValidIssuers, | |||
| IssuerValidatorErrorMessage.IssuerDoesNotMatchValidIssuers, | |||
There was a problem hiding this comment.
Would be good to throw in the catch segment above and capture more information on what caused a potential issue.
There was a problem hiding this comment.
good idea. will update.
| AadAuthority = aadAuthority.TrimEnd('/'); | ||
| } | ||
|
|
||
| private IOptions<AadIssuerValidatorOptions> AadIssuerValidatorOptions { get; } | ||
| private IHttpClientFactory HttpClientFactory { get; } | ||
| private HttpClient? HttpClient { get; } |
There was a problem hiding this comment.
If TVP.ValidIssuer(s) is empty, AadIssuerV1/2 will be populated only once (no refresh). Is this an issue?
There was a problem hiding this comment.
are you suggesting a refresh after a certain amount of time to check if the metadata has changed?
There was a problem hiding this comment.
Yes. Not sure if the lack of refresh could be the issue (not expecting issuer to change). Code might be broken if the issuer on the metadata changes so that it doesn't contain {tenantId}, and refresh wouldn't help there.
Have tested with release build, in addition to the migration samples from @jmprieur