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

[Java][Spring][1975] Add OAuth2 Preauthorize annotations based on scope #5398

Closed

Conversation

ersinciftci
Copy link

Fixes #1975

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@ersinciftci ersinciftci force-pushed the pre-authorize-annotation branch from 07b4fb8 to c857e4d Compare February 21, 2020 14:23
@ersinciftci
Copy link
Author

@wing328 @cbornet I tried to make the PreAuthorize import conditional but failed, any help would be appreciated.

@cbornet
Copy link
Member

cbornet commented Feb 22, 2020

I think this doesn't work if there's multiple securities defined for an operation (eg. Oauth + basic).

@ersinciftci ersinciftci force-pushed the pre-authorize-annotation branch from c857e4d to 0fdafc2 Compare February 22, 2020 10:08
@ersinciftci
Copy link
Author

@cbornet I just tested with an OpenAPI spec that has both OAuth2 and Basic, and seems to be working, because it has {{#isOAuth}} so I think the extra Basic security item doesn't affect it. But I can't figure out for what use case the tests are failing.

@cbornet
Copy link
Member

cbornet commented Feb 25, 2020

What I mean is that an endpoint that has both basic and oauth should be accessible by basic OR oauth. Here I think if you try to access by basic auth, the pre-authorization will fail and you will be rejected.

@ersinciftci
Copy link
Author

Ah, got it. So it should generate @PreAuthorize only if OAuth2 is the only authentication method. I couldn't find a way to get the length of an array variable such as {{#authMethods}} in the mustache template, hence the reason I couldn't make the import statement conditional either. (tried .length and .size to no avail)

@wing328
Copy link
Member

wing328 commented Mar 1, 2020

I think this doesn't work if there's multiple securities defined for an operation (eg. Oauth + basic).

@cbornet good catch. I think such a case rarely occurs (based on my experience dealing with issues reported to this repo). We can document it as a known issue for the time being and work on it when there's a demand to fix it.

@Walliee
Copy link

Walliee commented Mar 17, 2020

I just stumbled here trying to find @PreAuthorize support in OpenAPI Generator.
I am looking to avoid custom templates in our project and I think this feature would get us there.

Anything I can do to help?

@ersinciftci
Copy link
Author

ersinciftci commented Mar 17, 2020

Hi @Walliee , sorry I didn't have much time lately, your help would be greatly appreciated.

Firstly, not sure if the tests are failing because of the use case @cbornet mentioned, so that's pending some investigation (though @wing328 mentioned that we can ignore that use case for now, but we still need to fix the tests).

One other thing is this change would fail for people that don't use Spring Security because it imports a class from there, so maybe this should be a configuration that people can turn on (like addPreAuthorize = true).

Thanks

@Walliee
Copy link

Walliee commented Mar 18, 2020

Yes; of course. The pointers you gave make sense. Thanks!

@nhomble and I looked at this briefly today and looks like we can handle bearer auth too. We’ll hopefully have a PR out tomorrow.

@nhomble
Copy link
Contributor

nhomble commented Mar 19, 2020

Awesome stuff @ersinciftci, we are still polishing a little bit (and I need to regen the samples), but here is a sneak peak

@ersinciftci
Copy link
Author

@nhomble looks great! useSpringSecurity sounds much better than addPreAuthorize :)

@nhomble
Copy link
Contributor

nhomble commented Mar 20, 2020

I would love opinions on this thanks!

@ersinciftci
Copy link
Author

@nhomble looks much better than this one, thanks! (I'm closing this one)

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

Successfully merging this pull request may close these issues.

[REQ] Spring Server generators should allow for adding OAuth2 Preauthorize annotations based on scope
5 participants