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 custom claim types in quarkus-test-security-jwt and quarkus-test-security-oidc #34170

Closed

Conversation

sberyozkin
Copy link
Member

Fixes #34133.
Fixes #30442.

quarkus-test-security-jwt supports testing endpoints expecting JsonWebToken, and quarkus-test-security-oidc supports testing endpoints expecting JsonWebToken, both as access and ID tokens.

This PR goes ahead and adds support for custom claim types. Note it is only required to support the unit testing, when no quarkus-smallrye-jwt or quarkus-oidc is configured.

Long, Integer, Boolean, String, jakarta.json.JsonArray and jakarta.json.JsonObjects claim type hints are supported. Some well known claims like exp (expiry, see #34133) does not need a token hint.

Note JsonWebToken API is not typed, example, it has Object getClaim(String name), so smallrye-jwt for some non-standard claims will convert their value to JsonNumber/JsonValue, which is why in tests, you'd see for example some Long claim values cast to Long, and for customlong to JsonNumber.

* Claim value type, the value will be converted to String if this type is set to Object.class.
* Supported types: String, Integer, int, Long, long, Boolean, boolean, jakarta.json.JsonArray, jakarta.json.JsonObject.
*/
Class<?> type() default Object.class;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using an enum for the valid types?

Copy link
Contributor

Choose a reason for hiding this comment

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

Asking because UX-wise that would be easier to configure, but not sure if it makes sense to be that restricted

Copy link
Contributor

Choose a reason for hiding this comment

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

Also because any other class than the existing list used here will not work

Copy link
Member Author

@sberyozkin sberyozkin Jun 21, 2023

Choose a reason for hiding this comment

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

@gastaldi Hi,

Also because any other class than the existing list used here will not work

This is expected though, right now, the test type converters can not be provided by users for random types. This list supports typical types. most claims would be strings, sometimes long, int, complex claims can be arrays or objects, rarely enough boolean and that is pretty much it. We've been thinking with Roberto for a while how to get an arbitrary number of converters supported - but if we will eventually support it then they'd be supported at the smallrye-jwt level and then these test extensions will pick them up somehow, which is also why Converter interface is private.

This test extensions should be aligned with how smallrye-jwt can let users get the claim values, otherwise it will not be same with the test extensions as with the prod :-)

I'm going to change to using an enum in any case

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @gastaldi @geoand I've introduced a enum instead, note I still need to use a default value such as Type.UNKNOWN, because the idea is that for the well known claim such as exp (expiry), which, in JsonWebToken API is returned as long, users should not be required to add a Type.LONG hint. And if I set the default value to Type.String then it would be a bit strange to still convert exp to long...

Does the way I did it look reasonable enough now ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the PR needs to be rebased. It has 301 commits?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gastaldi Sorry, I have no idea how it happened, I'll create a new one

* Claim value type, the value will be converted to String if this type is set to Object.class.
* Supported types: String, Integer, int, Long, long, Boolean, boolean, jakarta.json.JsonArray, jakarta.json.JsonObject.
*/
Class<?> type() default Object.class;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@@ -28,6 +28,10 @@
<artifactId>junit-jupiter</artifactId>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-jsonp</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal because it's for testing, but I wonder if this dependency can be made optional

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 20, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@sberyozkin
Copy link
Member Author

Thanks @gastaldi @geoand, let me look at the comments a bit later today

geoand and others added 21 commits June 21, 2023 14:52
The remaining scripts have been moved to the quarkus-updates repository.

See https://github.com/quarkusio/quarkus-updates/tree/main/tools/3.0
integration testing

finish quarkus:run and integration tests

quarkus:run gradle

cli run command

implement deploy

afk rebase'

azf deploy

azf deploy working

azf more fixes

azf service-principal

azf fix codestarts and path config

azf fix codestarts and path config

azf gradle and codestart

azf new docs

fix pom

azf docs and imports

fix gradle

quarkusRun
Bumps [proto-google-common-protos](https://github.com/googleapis/sdk-platform-java) from 2.16.0 to 2.19.1.
- [Release notes](https://github.com/googleapis/sdk-platform-java/releases)
- [Changelog](https://github.com/googleapis/sdk-platform-java/blob/main/CHANGELOG.md)
- [Commits](https://github.com/googleapis/sdk-platform-java/commits)

---
updated-dependencies:
- dependency-name: com.google.api.grpc:proto-google-common-protos
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
This method can be called by filters to override
the entity being sent by the JAX-RS client and
before this commit we were not properly handling
the case where either the entity was set to null
or only the body of the entity was being set
(and not the media type).

Fixes: quarkusio#33738
fixed a missing configuration key in example
@sberyozkin
Copy link
Member Author

Ouch, what happened, strange

@github-actions
Copy link

🙈 The PR is closed and the preview is expired.

@rsvoboda
Copy link
Member

Removed the noteworthy label as the PR is closed and replaced by another one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done