Support to add objects (primitive and collection) as claims in token … #318

complanboy2 · 2019-01-23T10:44:19Z

…payload

As requested @gustavoamigo we are giving support to add objects if primitive and collection types as claim in token payload.

Followed the suggestion by @lbalmaceda to make this change.
#164 (comment)

This PR is in continution to the old PR #289 which was accidentally closed. Please continue the review, thank you.

…payload

complanboy2 · 2019-01-24T08:30:44Z

@lbalmaceda can you please review this request ! We've a requirement for this and I guess many also have. Thank you.

mockmotor · 2019-01-25T01:38:11Z

Same here. Need to claim an array and an object.

mockmotor · 2019-01-25T03:25:40Z

Hey @complanboy2 - a method that allows to add an array of objects is missing.

I.e. jwt.withArrayClaim(name, arrayOfMaps);

I have

"identities": [
   { ... },
   { ... },
   { ... }
]

in an existing system I need to check interoperability with.

SORRY MY BAD, the following works just fine:

jwt.withClaim(key, (List) val);

Then, with the modification of adding Map.class that I described on the review page, I was able to generate and decode the following test payload:

  "payload":    {
      "dummy": {"mask": 1},
      "dummy2":       {
         "nested":          {
            "a": "b",
            "c": 100,
            "d": 1.5,
            "e": 1548388732000,
            "f":             [
               1,
               5,
               6
            ]
         },
         "mask": 2
      },
      "identities":       [
                  {
            "provider": "sap",
            "id": "k887421az"
         },
                  {
            "provider": "ldap",
            "id": "b10000272"
         }
      ],
      "exp": 1548388742,
      "login": "john.doe"
   }

mockmotor · 2019-01-25T03:52:25Z

lib/src/main/java/com/auth0/jwt/JWTCreator.java

+        static {
+            SUPPORTED_ARRAY_TYPES.addAll(Arrays.asList(Integer[].class, Long[].class, String[].class));
+            SUPPORTED_TYPES.addAll(
+                    Arrays.asList(String.class, Integer.class, Double.class, Long.class, Date.class, Boolean.class));


To support objects of objects in claims, I suggest to add Map.class to SUPPORTED_TYPES.

Then below, in the isValidClaimsValue, you'd need to modify the check as

for(Class typ: SUPPORTED_TYPES) { if( typ.isAssignableFrom(clazz) ) return true; } return false;

instead of just

return SUPPORTED_TYPES.contains(clazz);

That works, I have tested it for both generation and decoding.

You are right, it works with Object of Objects if we add Map.class. I'm not sure second level objects of non-primitive (other than mentioned supported types) will be allowed in this lib.

I agree that it will greatly help to construt complex/custom JWT's with that support. For now, I try to keep this PR as it is unless someone from library second this addition. Thank you.

Sure. For the time being, I'm using a modified lib built from your branch because I need to deal with nested objects coming from outside authorization service - today. When your code is merged, I'll try and push for nested objects, with a separate merge request.

mockmotor · 2019-01-25T04:37:35Z

lib/src/main/java/com/auth0/jwt/JWTCreator.java

+        }
+
+        private static boolean validateClaim(Collection<Object> values) {
+            if (values == null || values.isEmpty()) {


values.isEmpty() - does it mean I cannot have a claim with an empty object, like this:

"warnings": {}

I don't think it is right then. This is a totally valid claim.

Same for empty array claim:

"alternative_identities": []

Agree, removed the empty check and keeping the null check as the library also enforces it.

mockmotor · 2019-01-25T04:39:51Z

lib/src/main/java/com/auth0/jwt/JWTCreator.java

+                return true;
+            }
+            return SUPPORTED_TYPES.contains(clazz);
+        }


I suggest, instead of returning boolean, throw IllegalArgumentException for bad claim classes right from here.
The benefit is that you can provide the class and value of the offending claim right in the exception message, which GREATLY simplifies the troubleshooting.

Good one, I've updated the PR.
Thank you.

…or value

complanboy2 · 2019-01-29T11:45:47Z

@lbalmaceda , can you review it please ! we've a dependency and waiting for this to be merged, thank you in advance.

vm137 · 2019-02-05T09:55:31Z

Please review and merge it. We are waiting it for our project as well!
Thanks.

lbalmaceda · 2019-02-05T14:32:55Z

👋 Thanks for the contribution so far. I'm happy to review this on my free time but currently this is not a priority. As mentioned in several other related issues and PRs we do like to have this feature available sometime soon 👍
That said, I saw you @complanboy2 have followed the conditions I requested back then and that you've added the tests cases. I'll come back to this when I have a chance and leave a proper review. In the meantime, please don't be blocked since we cannot provide an ETA for it to get merged.

complanboy2 · 2019-02-08T09:04:34Z

👋 Thanks for the contribution so far. I'm happy to review this on my free time but currently this is not a priority. As mentioned in several other related issues and PRs we do like to have this feature available sometime soon 👍
That said, I saw you @complanboy2 have followed the conditions I requested back then and that you've added the tests cases. I'll come back to this when I have a chance and leave a proper review. In the meantime, please don't be blocked since we cannot provide an ETA for it to get merged.

@lbalmaceda Thank you for the note. Hope you will review soon (y)

montycheese · 2019-02-11T04:06:09Z

+1 Waiting for a review and merge

complanboy2 · 2019-04-05T10:01:54Z

@lbalmaceda Gentle reminder sir for the code review .

Gentle Reminder.. It's been so long since this PR is opened. Many are waiting.
This feature has to be supported by auth0/jwt sometime since it become a common requirement now.

Please have prioritize this review. Thank you.

jstradli · 2019-05-20T16:35:55Z

+1 Waiting for review and merge.

jswiderski · 2019-05-28T07:59:10Z

I'm also hoping this will get reviewed and merged soon 🤞

sheamunion · 2019-07-10T16:25:08Z

We're waiting for this feature as well and would greatly appreciate the time you take to do review this PR.

skjolber · 2019-07-11T17:31:36Z

@luisrudge @lbalmaceda please merge this or #282 and other claim-related pull-requests. Would it help if we opened a support case?

complanboy2 · 2019-09-27T09:21:02Z

@lbalmaceda just checking if there are plans to merge this ! thanks

lbalmaceda · 2019-09-27T18:09:02Z

Apologies for taking so long to review this. Currently the team focus is on a higher priority task and I cannot leave you the thorough review I would like to. Remember we do want to add this feature so eventually we'll come back to this PR.

complanboy2 · 2019-09-30T04:20:44Z

Apologies for taking so long to review this. Currently the team focus is on a higher priority task and I cannot leave you the thorough review I would like to. Remember we do want to add this feature so eventually we'll come back to this PR.

Thank you @lbalmaceda , Hope this will get merge sometime soon (Y)

stale · 2020-02-11T22:32:35Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

lbalmaceda · 2020-02-12T21:12:00Z

@complanboy2 we're working on reviewing and merging the other PR that adds this same feature. I'll close this one now so we can focus on that one. Thanks for the time you spent on this, though.

complanboy2 · 2020-02-13T08:33:15Z

Great @lbalmaceda , thx.

Support to add objects (primitive and collection) as claims in token …

2958bac

…payload

mockmotor reviewed Jan 25, 2019

View reviewed changes

veerasekharbab.golla added 3 commits January 26, 2019 10:01

Added IllegalArgumentException if non-supported class type recieved f…

f66a03f

…or value

Accepting empty collection claim values

aaafe3c

Fixed IllegalArgumentException related check

38adc99

lbalmaceda mentioned this pull request Feb 1, 2019

Add map as claim #322

Closed

Merge branch 'master' into master

65069af

complanboy2 requested a review from a team November 13, 2019 06:38

joshcanhelp requested review from lbalmaceda and removed request for a team November 13, 2019 22:24

stale bot added the closed:stale Issue or PR has not seen activity recently label Feb 11, 2020

lbalmaceda closed this Feb 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support to add objects (primitive and collection) as claims in token … #318

Support to add objects (primitive and collection) as claims in token … #318

complanboy2 commented Jan 23, 2019

complanboy2 commented Jan 24, 2019 •

edited

Loading

mockmotor commented Jan 25, 2019

mockmotor commented Jan 25, 2019 •

edited

Loading

mockmotor Jan 25, 2019 •

edited

Loading

complanboy2 Jan 26, 2019 •

edited

Loading

mockmotor Jan 27, 2019

mockmotor Jan 25, 2019 •

edited

Loading

complanboy2 Jan 26, 2019

mockmotor Jan 25, 2019 •

edited

Loading

complanboy2 Jan 26, 2019 •

edited

Loading

complanboy2 commented Jan 29, 2019

vm137 commented Feb 5, 2019

lbalmaceda commented Feb 5, 2019

complanboy2 commented Feb 8, 2019 •

edited

Loading

montycheese commented Feb 11, 2019

complanboy2 commented Apr 5, 2019 •

edited

Loading

jstradli commented May 20, 2019

jswiderski commented May 28, 2019

sheamunion commented Jul 10, 2019 •

edited

Loading

skjolber commented Jul 11, 2019

complanboy2 commented Sep 27, 2019

lbalmaceda commented Sep 27, 2019

complanboy2 commented Sep 30, 2019

stale bot commented Feb 11, 2020

lbalmaceda commented Feb 12, 2020

complanboy2 commented Feb 13, 2020

Support to add objects (primitive and collection) as claims in token … #318

Support to add objects (primitive and collection) as claims in token … #318

Conversation

complanboy2 commented Jan 23, 2019

complanboy2 commented Jan 24, 2019 • edited Loading

mockmotor commented Jan 25, 2019

mockmotor commented Jan 25, 2019 • edited Loading

mockmotor Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

complanboy2 Jan 26, 2019 • edited Loading

Choose a reason for hiding this comment

mockmotor Jan 27, 2019

Choose a reason for hiding this comment

mockmotor Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

complanboy2 Jan 26, 2019

Choose a reason for hiding this comment

mockmotor Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

complanboy2 Jan 26, 2019 • edited Loading

Choose a reason for hiding this comment

complanboy2 commented Jan 29, 2019

vm137 commented Feb 5, 2019

lbalmaceda commented Feb 5, 2019

complanboy2 commented Feb 8, 2019 • edited Loading

montycheese commented Feb 11, 2019

complanboy2 commented Apr 5, 2019 • edited Loading

jstradli commented May 20, 2019

jswiderski commented May 28, 2019

sheamunion commented Jul 10, 2019 • edited Loading

skjolber commented Jul 11, 2019

complanboy2 commented Sep 27, 2019

lbalmaceda commented Sep 27, 2019

complanboy2 commented Sep 30, 2019

stale bot commented Feb 11, 2020

lbalmaceda commented Feb 12, 2020

complanboy2 commented Feb 13, 2020

complanboy2 commented Jan 24, 2019 •

edited

Loading

mockmotor commented Jan 25, 2019 •

edited

Loading

mockmotor Jan 25, 2019 •

edited

Loading

complanboy2 Jan 26, 2019 •

edited

Loading

mockmotor Jan 25, 2019 •

edited

Loading

mockmotor Jan 25, 2019 •

edited

Loading

complanboy2 Jan 26, 2019 •

edited

Loading

complanboy2 commented Feb 8, 2019 •

edited

Loading

complanboy2 commented Apr 5, 2019 •

edited

Loading

sheamunion commented Jul 10, 2019 •

edited

Loading