Skip to content

Fix AzureAd options validation #23096

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.

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

Merged
merged 1 commit into from
Jul 15, 2020
Merged

Conversation

JunTaoLuo
Copy link
Contributor

@JunTaoLuo JunTaoLuo commented Jun 18, 2020

Addresses #20136

Description

Originally #9967 added options validation for AzureAd auth options, but it introduced a regression where it would throw for other cookie or jwt auth providers.
#13480 fixed this for jwt and cookies auth but the issue also affects OIDC auth. This PR applies the same fix to OIDC auth.

Customer Impact

Customers may not be able to use AzureAd auth combine with other kinds of auth in one app.

Regression?

Yes this is a regression in 3.1

Risk

Low, just adding a null check.

@ghost ghost added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jun 18, 2020
@JunTaoLuo JunTaoLuo requested a review from Tratcher June 18, 2020 18:22
@JunTaoLuo JunTaoLuo added this to the 5.0.0-preview7 milestone Jun 18, 2020
@JunTaoLuo JunTaoLuo changed the base branch from master to release/3.1 June 18, 2020 22:17
@JunTaoLuo JunTaoLuo changed the base branch from release/3.1 to master June 18, 2020 22:18
@JunTaoLuo JunTaoLuo changed the base branch from master to release/3.1 June 18, 2020 22:28
@JunTaoLuo JunTaoLuo force-pushed the johluo/azure-ad-validation branch from 02c2e91 to 822ec36 Compare June 18, 2020 22:28
@JunTaoLuo JunTaoLuo modified the milestones: 5.0.0-preview7, 3.1.7 Jun 18, 2020
@JunTaoLuo JunTaoLuo added the Servicing-consider Shiproom approval is required for the issue label Jun 18, 2020
@ghost
Copy link

ghost commented Jun 18, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jul 2, 2020
@AleksandrAlbert
Copy link

AleksandrAlbert commented Jul 9, 2020

Hi, I'm still having this issue with 3.1.5

services.AddAuthentication()
              .AddAzureAD(options => Configuration.Bind("AzureAD", options))
              .AddOpenIdConnect("PingOne", "PingOne ID", options =>{...});
                  
services.Configure<OpenIdConnectOptions>(AzureADDefaults.OpenIdScheme, options =>
            {
                options.Authority = options.Authority + "/v2.0/";
                options.TokenValidationParameters.ValidateIssuer = true;
                options.SignInScheme = IdentityConstants.ExternalScheme;
                options.GetClaimsFromUserInfoEndpoint = true;
            });

Throws OptionsValidationException: The 'Instance' option must be provided

My settings for ref:

"AzureAD": {
    "Instance": "https://login.microsoftonline.com/",
    "Domain": "xxx",
    "TenantId": "xxx",
    "ClientId": "xxx",
    "CallbackPath": "/signin-oidc"
  }

@JunTaoLuo
Copy link
Contributor Author

This fix has not been merged yet. It will be applied to the next 3.1 release (3.1.7).

@AleksandrAlbert
Copy link

Ok thank you, is there a scheduled release date?

@JunTaoLuo JunTaoLuo merged commit d45e6b2 into release/3.1 Jul 15, 2020
@JunTaoLuo JunTaoLuo deleted the johluo/azure-ad-validation branch July 15, 2020 00:15
@JunTaoLuo
Copy link
Contributor Author

It will be out in approximately a month. Generally we don't give out specific dates: see https://github.com/dotnet/core/blob/master/roadmap.md#upcoming-ship-dates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants