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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core, Rust Server, ASP.NET Core] Fix Codegen Operation Scope Consistency #3495

Merged
merged 5 commits into from Nov 8, 2019

Conversation

@richardwhiuk
Copy link
Contributor

richardwhiuk commented Jul 29, 2019

PR checklist

  • Read the contribution guidelines.

  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.

  • I've run both the Rust Server changes, and the ASP.NET petstore.sh scripts.

  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.

  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

  • Filter scopes based on operation - this fixes #1610

  • Partially revert #1984 to not rely on custom attributes as to whether scopes exist, and instead use built in Mustache function. - @MBcom you may be interested in the change here.

  • Tests are added to Rust Server codegen examples to check that the generated code is correct.

Code is by @mthebridge and @richardwhiuk . Any bugs are mine, any credit is Mark's 馃槃.

- Filter scopes based on operation

- Partially revert #1984 to not rely on custom attributes as to whether scopes exist
Copy link
Contributor

bjgill left a comment

Have given it a quick review, and it looks broadly sensible. Just one thing below where I want to check the logic.

I don't know enough about ASP.NET to say anything sensible about the effect of your changes there, though.

@bjgill

This comment has been minimized.

Copy link
Contributor

bjgill commented Aug 1, 2019

Also - looks as if CI is unhappy.

@MBcom

This comment has been minimized.

Copy link
Contributor

MBcom commented Sep 10, 2019

@richardwhiuk Sorry it took so long to have a look to your pr
here a sample oas3 definition

 delete:
      summary: Remove a sth
      operationId: removeSth
      responses:
        '204':
          description: Operation successfull
        '404':
          description: not found
      security:
          - bearerAuth:
            - sth:delete

that should be translated to the following authorize annotation

        [Authorize(Roles = "sth:delete")]

but your code returns the following

        [Authorize]

so something is going wrong on creating the Authorize annotation
at first sight the template looked right - maybe you need to take a look at your security class??

@richardwhiuk

This comment has been minimized.

Copy link
Contributor Author

richardwhiuk commented Sep 22, 2019

@MBcom

I'm slightly confused by your example, as I'm not sure how you were defining the bearerAuth security scheme referenced in your example.

If you had something like the following:

components:
  securitySchemes:
    bearerAuth:
      scheme: bearer
      bearerFormat: token
      type: http
security:
  - bearerAuth:
    - "sth:access"

Then note, that according to https://swagger.io/specification/#securityRequirementObject, the security requirement may not contain a list of scope names. In this case, the code will generate [Authorize], which might not be correct, but I'm not sure what the correct answer is.

If, instead, you were defining a bearerAuth as a global level oauth2 level scheme, then this should be fixed by 664c71c

Can you let me know, so I can address any further issues here?

@richardwhiuk

This comment has been minimized.

Copy link
Contributor Author

richardwhiuk commented Nov 7, 2019

@MBcom can you comment?

If I don't hear back, I'll assume that 664c71c fixes your issue and go ahead and merge this - but I don't want to break ASP inadvertently...

@MBcom

This comment has been minimized.

Copy link
Contributor

MBcom commented Nov 8, 2019

@richardwhiuk sry for my late answer

when there is something like this

delete:
      summary: Remove a sth
      operationId: removeSth
      responses:
        '204':
          description: Operation successfull
        '404':
          description: not found
      security:
          - bearerAuth: []

you are right - there can be an [Authorize] or an [Authorize(Roles = "")] annotation

but in my example of #3495 (comment) - the specific route must be annotated including the roles/ scopes

and yes, a global security annotation is currently missing - we should open a new issue for that

@richardwhiuk richardwhiuk merged commit de162f7 into OpenAPITools:master Nov 8, 2019
5 checks passed
5 checks passed
Shippable Run 11927 status is SUCCESS.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@richardwhiuk richardwhiuk deleted the Metaswitch:scope-consistency branch Nov 8, 2019
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Nov 10, 2019
* master: (28 commits)
  [meta] Support Kotlin meta generator (OpenAPITools#4156)
  [Go][Server] minor enhancement to the template (OpenAPITools#4417)
  Replace the old ResourceSupport (OpenAPITools#4426)
  [Core, Rust Server, ASP.NET Core] Fix Codegen Operation Scope Consistency (OpenAPITools#3495)
  Add Go Server featureCORS option (OpenAPITools#4400)
  Fix treatment of nullable types in a few more places (OpenAPITools#4315)
  prefix local variable with localVar (OpenAPITools#4402)
  [kotlin][client] gson complete integration (OpenAPITools#4332)
  [kotlin] [bugfix] [maven-plugin]: prevent ClassCastException with boolean config options (OpenAPITools#4361)
  add sbt, bazel to integration (OpenAPITools#4416)
  Add a blog post tutorial about generating Java clients using OpenAPI v3 (OpenAPITools#4405)
  add freshcells to company list (OpenAPITools#4414)
  Update isSet when the object is received from callback. (OpenAPITools#4385)
  Ruby client nullable (OpenAPITools#4391)
  Fixes Kotlin client property names that include a dollar sign for template override (OpenAPITools#4351)
  [Python] [Performance] Avoid unnessacary checks inside the loop (OpenAPITools#4305)
  Add QEDIT as a company that's using OpenAPI Generator (OpenAPITools#4392)
  update cpp flag for pistache (OpenAPITools#4386)
  Feature optional emit default values (OpenAPITools#4347)
  skip the test as async call may have finished (OpenAPITools#4377)
  ...
@jasonculverhouse jasonculverhouse mentioned this pull request Nov 22, 2019
4 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.