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

Support injecting OIDC WireMockServer into tests #20035

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

dimas-b
Copy link
Contributor

@dimas-b dimas-b commented Sep 9, 2021

  • Define a custom annotation: InjectWireMock

  • Override inject(TestInjector) in OidcWiremockTestResource
    to allow tests to receive the WireMockServer
    used by OidcWiremockTestResource

This is to allow (external) tests to mock
extra URLs, that are not covered by OidcWiremockTestResource


If this PR is accepted, please consider back-porting to Quarkus 2.2.x

@sberyozkin
Copy link
Member

sberyozkin commented Sep 9, 2021

Hi @dimas-b, thanks for this PR - does the test pass with -Dnative ? Also, perhaps the existing oidc client or oidc test can be enhanced a little bit to show for the users to see how it can be practically applied and for it to be documented as well ?

@stuartwdouglas Can you also have a look please ? May be instantiating TestResourceManager can be avoided somehow (unless in the PR test it is only instantiated to show the injection is working) ?

Thanks

@dimas-b
Copy link
Contributor Author

dimas-b commented Sep 9, 2021

May be instantiating TestResourceManager can be avoided somehow [...]

My understanding is that TestResourceManager is triggered from the JUnit 5 extension, which is currently not a dependency of the test-framework module and I did not want to introduce a new dependency for this PR.

If there's a better way to trigger the injection logic - by all means please let me know and I will update this PR.

@dimas-b
Copy link
Contributor Author

dimas-b commented Sep 9, 2021

does the test pass with -Dnative ?

It does, although I do not think this flag makes a real difference since the test does not actually bring a full Quarkus server up.

@dimas-b
Copy link
Contributor Author

dimas-b commented Sep 9, 2021

@sberyozkin :

perhaps the existing oidc client or oidc test can be enhanced a little bit to show for the users to see how it can be practically applied and for it to be documented as well ?

I've pushed another commit to add a test with an invalid toke to BearerTokenAuthorizationTest : 0ac955d. This is the use case that prompted me to open this PR, actually.

Is this what you meant for providing a practical example?

@sberyozkin
Copy link
Member

sberyozkin commented Sep 10, 2021

@dimas-b thanks, looks good with a new test. Can you please squash the commits ?

One other question, do you think it can make sense to drop Inject from InjectWireMock since it is implied and have only WireMock ? I don't really mind if it stays as InjectWireMock but I'm not sure we have any other marker annotation in Quarkus with the Injectin its name indicating the target has to be injected. I'll let you decide if you'd like to keep it or not...

Thanks

@dimas-b
Copy link
Contributor Author

dimas-b commented Sep 10, 2021

@sberyozkin : There's already a io.quarkus.it.hibernate.search.elasticsearch.aws.InjectWireMock for a similar use case, so I followed that pattern. I'm ok to rename though. How about OidcWireMock since it works in concert with OidcWiremockTestResource.

I'll squash as soon as we settle on a name for the annotation :)

@sberyozkin
Copy link
Member

@dimas-b Sorry, missed your last comment, I see, following the same pattern if it is already used in one other place in Quarkus makes sense, though I also like your idea of naming it as OidcWireMock as the wire mock itself is offered by the test resource factory with its name starting with Oidc - so if it works for you then please rename and squash and we'll merge, thanks

* Define a custom annotation: OidcWireMock for tests
  to indicate they want the WireMock Server injected.

* Override inject(TestInjector) in OidcWiremockTestResource
  to allow tests to receive the WireMockServer
  used by OidcWiremockTestResource

This is to allow (external) tests to mock
extra URLs, that are not covered by OidcWiremockTestResource

* Add bearer auth test for invalid token. Use a custom
  wiremock stub for generating responses for the invalid token.
@dimas-b
Copy link
Contributor Author

dimas-b commented Sep 13, 2021

Renamed, squashed and rebased

@sberyozkin sberyozkin self-requested a review September 13, 2021 17:03
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

@dimas-b thanks for your PR. I'll follow up with a doc update as I don't want to hold this PR any longer. (if you can find some time - please do a small example to https://quarkus.io/guides/security-openid-connect#integration-testing-wiremock to introduce this option)

@sberyozkin sberyozkin merged commit b79308a into quarkusio:main Sep 13, 2021
@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Sep 13, 2021
@dimas-b
Copy link
Contributor Author

dimas-b commented Sep 13, 2021

Follow-up docs PR: #20118

@dimas-b dimas-b deleted the inject-wiremock branch September 13, 2021 21:56
@dimas-b
Copy link
Contributor Author

dimas-b commented Sep 13, 2021

@sberyozkin : Would it be possible to backport this to 2.2.x?

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.

3 participants