Skip to content
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

Issue with authorizations after upgrade to v6.3.2 #2459

Closed
marcominerva opened this issue Jul 15, 2022 · 13 comments · Fixed by #2460
Closed

Issue with authorizations after upgrade to v6.3.2 #2459

marcominerva opened this issue Jul 15, 2022 · 13 comments · Fixed by #2460

Comments

@marcominerva
Copy link

Using Swashbucke.AspNetCore v6.3.1, I have configured authorizations so that I have an Authorize button that, once clicked, shows up the following option (I'm using Azure AD authentication):

image

After upgrading to v6.3.2, with no changes in my code, a new authorization option now appears at bottom (Bearer (http, Bearer)):

image

Is there an issue with the last version?
Kindest regards.

@agilenut
Copy link

I'm having the same issue... and this is just a patch release

@marcominerva marcominerva changed the title Issue with authorations after upgrade to v6.3.2 Issue with authorizations after upgrade to v6.3.2 Jul 16, 2022
@domaindrivendev
Copy link
Owner

domaindrivendev commented Jul 18, 2022

@marcominerva @agilenut agree this shouldn't have gone out as a patch or even minor version release. In fact, the behavior to automatically infer authorization schemes from those configured for ASP.NET Core is still experimental. Will make this opt-in and issue a new nuget ASAP.

cc @captainsafia

@richtea
Copy link

richtea commented Jul 18, 2022

@domaindrivendev @captainsafia is there an issue that we can track these changes against (assuming that they get reverted)? The current implementation breaks my code too, and I'd appreciate the opportunity to provide feedback.

In particular, by calling GetSecuritySchemes after GetSwaggerDocument here, it bypasses all the document filters so there is no opportunity to modify the schemes derived from the IAuthenticationSchemeProvider.

It would be good to ensure that document filters are invoked later in the sequence to ensure they get a chance to override anything (for example, by excluding unreferenced security schemes from certain documents, as I am doing).

@captainsafia
Copy link
Contributor

is there an issue that we can track these changes against (assuming that they get reverted)?

We can probably use this issue to track changes in this behavior. You might also be interested in taking a look at dotnet/aspnetcore#39761 which captures some work we're hoping to do to make authentication configuration in ASP.NET apps more introspectable so its easier to automatically generate the OpenApiSecurityScheme definitions given a particular application.

There are some patterns in ASP.NET authentication that make this challenging, including one that played a role in this bug here. The scheme name is used as the unique identifier for authentication strategies registered within an app, but the scheme name doesn't necessarily reflect the scheme strategy. For example, I can use the Bearer scheme name for both an in-dev JWT-bearer based strategy and an Azure AD-auth strategy as seen here. Amongst other things, this means the information you get from IAuthenticationSchemeProvider might not be helpful enough for modifications you want to make in filters, etc.

@domaindrivendev
Copy link
Owner

@richtea - A lot of these features are being driven by the ASP.NET Core team. Going forward, I've asked them to create an issue in this repo before designing and submitting PRs. This way, I hope it will raise visibility and give more opportunity for myself and others to provide earlier feedback into the design.

Re your suggestion, I've applied it to #2460. Feel free to take a look and provide any further feedback you might have, keeping in mind that I'd like to get that out sooner rather than later. In the meantime, I will de-list the current "breaking" patch on nuget.

@pinkfloydx33
Copy link

pinkfloydx33 commented Jul 18, 2022

We have a lot of infrastructure around building our API spec the way we want it (lots of filters!). One thing we control is how security schemes are presented/generated. We generate different requirements when running behind our api gateway (which transforms headers) vs locally vs self-hosted, etc. Our applications are able to deduce the environment/setup and know what specification to generate, how to properly transform it in the PreSerialize hook (if necessary) and how to properly handle the differences in the app itself (custom authorization headers for ex.)

I just spent the last two hours trying to figure out why my app was crashing after the update in one of the scenarios. After stepping through each filter and inspecting my security schemes multiple times, they were always filled in the way I wanted but then reverted to what I'm guessing is this infered usage.

Technically all my scenarios were broken (had I tried the swagger UI) but only one was throwing errors. I'm glad it did because it surfaced a null-ref bug and ultimately drew me to the changelog that dependabot included in its PR.

This is however totally unacceptable! If I spend the cycles to customize the swagger generated document, nothing should automatically be overriding it. And such a change on a patch release is even worse.

  1. Is there an ETA on the opt-in/patch?
  2. Can I opt-out currently?
  3. Even with opt-in inference, I don't believe it should be clearing any schemes that were manually added. Inference should be an add-on, not a wipe-out
  4. How to prevent upstream "optimizations" from wiping out our carefully crafted specification automatically in the future? We are intentionally not using minimal APIs; they shouldn't be overriding the behavior of my app.

@agilenut
Copy link

agilenut commented Jul 18, 2022

@richtea - A lot of these features are being driven by the ASP.NET Core team. Going forward, I've asked them to create an issue in this repo before designing and submitting PRs.

Thanks! While this release was a breaking change. I am interested in having the security scheme automatically discovered if it can be done correctly. We have 2 filters that we use to do this.

The first is an IDocumentFilter that injects the authentication schemes and pulls out the OIDC info from the JwtBearerOptions.

The other is an IOperationFilter that injects the IAuthenticationSchemeProvider and IAuthorizationPolicyProvider to add security requirements based on the policies associated with the operations and sets the correct scopes based on a ScopeAuthorizationRequirement (which I really think should be a part of .net).

@richtea
Copy link

richtea commented Jul 18, 2022

Feel free to take a look and provide any further feedback you might have, keeping in mind that I'd like to get that out sooner rather than later.

@domaindrivendev that looks great, it addresses the key principle behind my concerns, which is to ensure that filters can get a bite at whatever the framework has provided. I'm keen on the idea of automatic discovery, but there are always likely to be corner cases that will require 'manual' intervention!

@domaindrivendev
Copy link
Owner

@marcominerva @agilenut @richtea I've merged the PR that makes this opt-in and ensure filters execute at the right time, and have published a 6.4.0 preview package to myget.org. Before I publish to nuget.org, could one or two of you pull down the latest preview package and ensure it doesn't break your existing auth stuff.

https://myget.org/feed/domaindrivendev/package/nuget/swashbuckle.aspnetcore

@marcominerva
Copy link
Author

@domaindrivendev I have just installed v6.4.0-preview in the project in which I have seen the issue and now it works as expected. Thank you very much!

@agilenut
Copy link

@domaindrivendev, I've also tested v6.4.0-preview in my project and it is working as expected.

@domaindrivendev
Copy link
Owner

OK will go ahead and publish - thx for the help in ironing this one out 👍

@richtea
Copy link

richtea commented Jul 20, 2022

Apologies, I missed this yesterday, but I can confirm that version 6.4.0 works for me, many thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants