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

alternative group claim #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

alternative group claim #5

wants to merge 1 commit into from

Conversation

dabloem
Copy link

@dabloem dabloem commented Jan 18, 2021

@rmannibucau please review and feedback

@@ -132,6 +139,20 @@ public Principal getUserPrincipal() {

@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