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

remove http_signature_test from samples when not supported #15221

Conversation

tiffmaelite
Copy link
Contributor

@tiffmaelite tiffmaelite commented Apr 14, 2023

For some samples, the openapi.yaml file contains endpoints with http signature authentication. But not all generators support this type of authentication. In some cases (addressed by #15220), this leads to invalid API being generated, because the signature authentication will be incorrectly considered to be a basic or bearer authentication.

The signature scheme was initially added in petstore-with-fake-endpoints-models-for-testing by #6058 for jersey2-experimental (now jersey2-java8) only. After the creation of new samples using the same input file, this small "localized" change ended up affecting almost all samples.
Furthermore, this change was not removed when another special yaml file with http-signature-test (#4993) was introduced, and the current PR aims at fixing this omission.

The goal of this PR is to clean up the samples to ensure that they do not contain and show invalid API code.

Once this PR and #15220 are merged, I will be able to submit another PR which explicitly verifies API behavior (e.g. exceptions) in case the openapi.yaml file contains authentication methods not supported by the generator.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

Regenerated samples for several modules, so here are mentions of concerned technical committee members for review and approval:

@tiffmaelite tiffmaelite force-pushed the fix/remove_signature_auth_from_samples_when_not_supported branch 5 times, most recently from d6e1430 to c1efe46 Compare April 14, 2023 15:32
@tiffmaelite tiffmaelite marked this pull request as draft April 14, 2023 19:48
@tiffmaelite tiffmaelite force-pushed the fix/remove_signature_auth_from_samples_when_not_supported branch 8 times, most recently from a8b5ef9 to 61385a4 Compare April 17, 2023 11:55
@tiffmaelite tiffmaelite marked this pull request as ready for review April 17, 2023 12:45
@mostafa
Copy link
Contributor

mostafa commented Apr 17, 2023

LGTM!

@mrmstn
Copy link
Contributor

mrmstn commented Apr 17, 2023

LGTM for elixir

@mrmstn
Copy link
Contributor

mrmstn commented Apr 17, 2023

@tiffmaelite Looks like the CI complains about some compile errors. I'm not quite sure if you have access to the reports but it would be great if you could have a look

I also want to thank you for your contribution and the time you've already invested 😁!

@tiffmaelite
Copy link
Contributor Author

tiffmaelite commented Apr 18, 2023

@mrmstn Thank you for the notice. I did not see these yesterday. Of course, I forgot to push the commit in which I removed the non-generated test methods which are no longer relevant. Now done.

@tiffmaelite
Copy link
Contributor Author

tiffmaelite commented Apr 20, 2023

Note: I aimed this PR at 7.x due to its relation to #15220 (which can be seen as a breaking change without fallback), but I'd be happy to move the current PR to master if the core team thinks it would be a better target for scope it covers

Copy link
Contributor

@muttleyxd muttleyxd left a comment

Choose a reason for hiding this comment

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

Changes in C++ code look good to me

@@ -1272,9 +1246,6 @@ components:
type: http
scheme: bearer
bearerFormat: JWT
http_signature_test:
type: http
scheme: signature
Copy link
Member

Choose a reason for hiding this comment

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

@tiffmaelite thanks for the PR but why not still include the test anyway as it shouldn't break the client/server, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason is that it is misleading to have sample openapi.yaml files suggest that the HTTP signature security scheme is supported by many more generators than those actually supporting it.
However, testing and showing in samples the behavior of all generators when faced with unsupported schemes is important. In particular for the ones throwing exceptions when faced with unsupported security schemes.

As a result, I plan to submit a new PR once this one is merged which explicitly includes an invalid scheme (named "unsupported_test" rather than "http_signature_test", for documentation purposes) and a few additional tests in order to avoid that the issue described in #14876 occurs again.
I can also submit both set of changes in one go, but that will possibly be harder to read and review.

Copy link
Member

Choose a reason for hiding this comment

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

can you please PM me via Slack to further discuss this PR when you've time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wing328 will do somewhen tomorrow or early next week.

I also wanted to add a last argument here: what is the point of having files named petstore-with-fake-endpoints-models-for-testing.yaml and petstore-with-fake-endpoints-models-for-testing-with-http-signature.yaml if both contain the http-signature scheme?

But maybe it does not make sense to have both of them side by side, because they are quite hard to maintain (and actually one can see that their content on some endpoints diverges already for no other reason than "someone forgot about the other file")

@tiffmaelite tiffmaelite force-pushed the fix/remove_signature_auth_from_samples_when_not_supported branch from 90b0645 to f7cccb6 Compare May 11, 2023 06:49
@tiffmaelite tiffmaelite changed the base branch from 7.0.x to master May 11, 2023 06:50
+ remove http signature from test input yaml when not supported
+ use http-signature file for python-nextgen and python-legacy instead of standard test input file

the signature scheme was added by OpenAPITools#6058 for jersey2-experimental (now jersey2-java8) only but this change affected all samples; and it was not removed when there is another special yaml file with http-signature-test (OpenAPITools#4993) was introduced
@tiffmaelite tiffmaelite force-pushed the fix/remove_signature_auth_from_samples_when_not_supported branch from f7cccb6 to 032efcb Compare May 22, 2023 06:51
@tiffmaelite
Copy link
Contributor Author

tiffmaelite commented May 22, 2023

This PR is likely to become irrelevant once #15523 and #15220 are merged, because samples will no longer contain invalid code or documentation at all

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 this pull request may close these issues.

None yet

5 participants