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

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

Merged
merged 5 commits into from
Nov 8, 2019
Merged

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

merged 5 commits into from
Nov 8, 2019

Conversation

richardwhiuk
Copy link
Contributor

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

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 bjgill left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

bjgill commented Aug 1, 2019

Also - looks as if CI is unhappy.

@MBcom
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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
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
@richardwhiuk richardwhiuk deleted the scope-consistency branch November 8, 2019 13:30
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)
  ...
@wing328 wing328 added this to the 4.2.2 milestone Dec 2, 2019
@wing328
Copy link
Member

wing328 commented Dec 2, 2019

@richardwhiuk thanks for the PR, which has been included in the v4.2.2 release: https://twitter.com/oas_generator/status/1201432648544972800

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.

Codegen operation scopes inconsistency
4 participants