Skip to content

Conversation

@dabloem
Copy link

@dabloem dabloem commented Jan 18, 2021

@rmannibucau please review and feedback


@Override
public boolean isUserInRole(final String role) {
if (tokenExtractor.get().containsClaim(groupsName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets dont call get twice ;)

Copy link
Author

Choose a reason for hiding this comment

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

done

JsonValue jsonValue = tokenExtractor.get().getClaim(groupsName);
Set<String> groups = Collections.EMPTY_SET;
if (jsonValue.getValueType() == JsonValue.ValueType.ARRAY) {
groups = JsonArray.class.cast(jsonValue).stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

asJsonArray?

Copy link
Author

Choose a reason for hiding this comment

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

done

@rmannibucau
Copy link
Contributor

Hi, two open questions:

  1. What about jsonpatch option instead of group alias? Sounds saner to me
  2. Wat about multi key/jwt apps, shouldnt it be a map with the keyid as key (with a default)?
    Otherwise looks good

@dabloem
Copy link
Author

dabloem commented Jan 19, 2021

Hi, two open questions:

  1. What about jsonpatch option instead of group alias? Sounds saner to me
  2. Wat about multi key/jwt apps, shouldnt it be a map with the keyid as key (with a default)?
    Otherwise looks good
  1. I don't fully understand what you mean. Maybe a simple gist?
  2. I don't know. I haven't seen it in other impl's (smallrye-liberty). And, is there such a use case, that the same app, receives different jwt's, signed with the same private key?

What I do like, is the hierarchy to locatie the groups claim. The example Token1.json in the tck includes an example with customObject.

@rmannibucau
Copy link
Contributor

  1. Define a jsonpatch (JsonPatch class) and apply it on the token to get the one used after raw extraction by the runtime. Enables this mapping and more.
  2. Our impl does and spec too due to kid support so we must keep it or not add this facility (since it is already doable through other ways)

@rmannibucau
Copy link
Contributor

@dabloem not really tested but to share the idea: 2f4ab72. We can add any needed strategy in the patcher bean (like flattening groups from string or so. Note that being protected the patch impl can trivially be enhanced in any app. Hope it helps.

@dabloem
Copy link
Author

dabloem commented Jan 20, 2021

Aha, thanks.
To be honest, I didn't know about javax.json.JsonPatch class. I thought more of 'allowing' a JsonPatch, therefor I came up with the JsonPatchProcessor.class.
So, we had both the preference for JsonPatch ;-)
I will take a look, and see if I can make a test...

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.

2 participants