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] Add OAuth2 Preauthorize annotations based on scope #6358

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

nhomble
Copy link
Contributor

@nhomble nhomble commented May 19, 2020

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.

FYI @wing328
And to copy other members you cc'd in the other pr
cc @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10) @bkabrda (2020/01)

@patrick-zinner I didn't lose your fix from the previous pr. You will see it incorporated here.

Fixes #1975

@nhomble nhomble changed the title [wip] preauthorize [Java][Spring] Add OAuth2 Preauthorize annotations based on scope May 19, 2020
@nhomble
Copy link
Contributor Author

nhomble commented May 20, 2020

@wing328 let's ignore how long it took me to fix my pr 😄

@nhomble
Copy link
Contributor Author

nhomble commented May 21, 2020

Let me know if there's anything else I should do!

@nhomble
Copy link
Contributor Author

nhomble commented May 22, 2020

Based off the sample, I created https://github.com/nhomble/spring-security-openapi-demo to try things out more extensively. Two things I observed which should get resolved:

  1. security.api-key: [] isn't handled well and maps to @PreAuthorize("") which breaks spring's expression parser
  2. I needed to use newer versions of spring security to discover the annotations. What's the process to upgrade spring dependencies?

@Walliee
Copy link

Walliee commented Jun 10, 2020

@wing328 @diyfr @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10) @bkabrda (2020/01)

Appreciate we can get some eyes on this - we are trying to adopt openapi-generator and are currently blocked due to the lack of @Preauthorize support. Thanks

@wing328
Copy link
Member

wing328 commented Jun 16, 2020

@nhomble thanks for the PR. Can you please resolve the merge conflicts when you've time?

…rator into OpenAPIToolsgh-1975-VI

� Conflicts:
�	bin/spring-all-petstore.sh
�	modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java
@nhomble
Copy link
Contributor Author

nhomble commented Jun 16, 2020

no problem @wing328 - I'll resolve the conflicts tomorrow and then on a separate commit I'll propose changes for the two points I raised earlier about spring dependencies and the bug.

break;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @OpenAPITools/generator-core-team for review

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy to move this to a separate changeset as well so that this pr is focused on preauthorize being a capability on eligible authMethods and we can consider bearerAuth being another scheme later

@nhomble
Copy link
Contributor Author

nhomble commented Jun 25, 2020

travis error seems unrelated

$ phpenv global 7.3

rbenv: version `7.3' not installed

The command "phpenv global 7.3" failed and exited with 1 during .

@nhomble
Copy link
Contributor Author

nhomble commented Jun 29, 2020

@wing328 is there a way to retrigger the ci manually? otherwise I can push a dummy commit since I don't think the error applies to this changeset

@wing328
Copy link
Member

wing328 commented Jun 29, 2020

We've addressed that issue in the latest master. Please merge the latest master into your branch to fix it.

@nhomble
Copy link
Contributor Author

nhomble commented Jun 29, 2020

Pulled from master and there's an auth issue:

Access denied to: https://repo.gradle.org/gradle/libs-releases-local/org/gradle/gradle-tooling-api/5.6.4/gradle-tooling-api-5.6.4.pom , ReasonPhrase:Forbidden

happy to help but I am not sure what I can do from my side othewise I can pull if there is another fix pending on master

@nhomble
Copy link
Contributor Author

nhomble commented Jul 4, 2020

We're back in business boys! @wing328 @Walliee

@bogdantudor74
Copy link

As @Walliee mentioned previously, I am doing some workarounds in order to have OpenAPI working and this is a major one.

@addynaikvh
Copy link
Contributor

Any update on when this can be merged into the releases?

@bogdantudor74 Can you please describe the workaround you have in place? We tried a few things without luck.

@@ -123,6 +126,11 @@ public interface {{classname}} {
{{/headerParams}}
})
{{/implicitHeaders}}
{{#hasAuthMethods}}
{{#useSpringSecurity}}
@PreAuthorize("{{#authMethods}}{{#isOAuth}}({{#scopes}}hasAuthority('{{scope}}'){{#hasMore}} and {{/hasMore}}{{/scopes}}){{/isOAuth}}{{#isBasicBearer}}({{#scopes}}hasAuthority('{{scope}}'){{#hasMore}} and {{/hasMore}}{{/scopes}}){{/isBasicBearer}}{{#hasMore}} or {{/hasMore}}{{/authMethods}}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will basicbearer auth method have scopes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind.. looks like the PR is adding the capability to do this.

@bogdantudor74
Copy link

Any update on when this can be merged into the releases?

@bogdantudor74 Can you please describe the workaround you have in place? We tried a few things without luck.

I have written for now a custom annotation which parses the generated @authorization annotations that are present in the Api interface.

@felixklauke
Copy link

Hey, how is the progress in here? Do you need any help?

@bilak
Copy link
Contributor

bilak commented Nov 12, 2020

@felixklauke I think this is related to this issue. So at first there must be way how to create structure for securities where we will be able to decide whether it's AND or OR. See Using Multiple Authentication Types section.

Proposal about which I was chatting with @jimschubert is to create another structure in CodegenConfig that will be capable of holding informations about securities and their AND OR combinations. Currently there is method something like List<CodegenSecurity> fromSecurity(Map<String, SecurityScheme> schemas) with which this is not possible. So there should be another method that will handle this. Would you be able to perpare PR for this? Then we can probably easily enhance spring templates with security.

@wing328 wing328 modified the milestones: 5.0.0, 5.0.1 Dec 21, 2020
@wing328 wing328 modified the milestones: 5.0.1, 5.1.0 Feb 8, 2021
@wing328 wing328 removed this from the 5.1.0 milestone Mar 19, 2021
@ghost
Copy link

ghost commented Jun 8, 2021

I have written for now a custom annotation which parses the generated @authorization annotations that are present in the Api interface.

@bogdantudor74 can you provide a sample for this custom annotation? I'm interested on this.

@addynaik
Copy link

I have written for now a custom annotation which parses the generated @authorization annotations that are present in the Api interface.

@bogdantudor74 can you provide a sample for this custom annotation? I'm interested on this.

here is what we did - this is how our swagger doc looks like - x-custom-role

....
      summary: Create record
      description: ''
      operationId: Create Provider
      x-custom-role: hasRole('ROLE_ADMIN')
      responses:
        '201':
          description: created
          content:
            application/json:
              schema:
....

and then we created a custom template for api.mustache

...
        @RequestMapping(value = "{{{path}}}",{{#singleContentTypes}}{{#hasProduces}}
        produces = "{{{vendorExtensions.x-accepts}}}", {{/hasProduces}}{{#hasConsumes}}
        consumes = "{{{vendorExtensions.x-contentType}}}",{{/hasConsumes}}{{/singleContentTypes}}{{^singleContentTypes}}{{#hasProduces}}
        produces = { {{#produces}}"{{{mediaType}}}"{{#hasMore}}, {{/hasMore}}{{/produces}} }, {{/hasProduces}}{{#hasConsumes}}
        consumes = { {{#consumes}}"{{{mediaType}}}"{{#hasMore}}, {{/hasMore}}{{/consumes}} },{{/hasConsumes}}{{/singleContentTypes}}
        method = RequestMethod.{{httpMethod}}){{#vendorExtensions.x-custom-role}}{{#hasAuthMethods}}
    @PreAuthorize("{{{.}}}"){{/hasAuthMethods}}{{/vendorExtensions.x-custom-role}}
    {{#jdk8-default-interface}}default {{/jdk8-default-interface}}{{#responseWrapper}}{{.}}<{{/responseWrapper}}ResponseEntity<{{>returnTypes}}>{{#responseWrapper}}>{{/responseWrapper}} {{#delegate-method}}_{{/delegate-method}}{{operationId}}({{#allParams}}{{>queryParams}}{{>pathParams}}{{>headerParams}}{{>bodyParams}}{{>formParams}}{{#hasMore}},{{/hasMore}}{{^hasMore}}{{#reactive}}, {{/reactive}}{{/hasMore}}{{/allParams}}{{#reactive}}ServerWebExchange exchange{{/reactive}}){{^jdk8-default-interface}};{{/jdk8-default-interface}}{{#jdk8-default-interface}}{{#unhandledException}} throws 
...

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
8 participants