X Tutup
Skip to content

Support to add objects (primitive and collection) as claim in token pa…#289

Closed
complanboy2 wants to merge 0 commit intoauth0:masterfrom
complanboy2:master
Closed

Support to add objects (primitive and collection) as claim in token pa…#289
complanboy2 wants to merge 0 commit intoauth0:masterfrom
complanboy2:master

Conversation

@complanboy2
Copy link
Copy Markdown

…yload

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)

@complanboy2 complanboy2 mentioned this pull request Oct 10, 2018
4 tasks
@complanboy2
Copy link
Copy Markdown
Author

complanboy2 commented Oct 15, 2018

@lbalmaceda @martinoconnor Please review at your free time.
If there is a board or group of people that I should include, please let me know. thanks.

@ghost
Copy link
Copy Markdown

ghost commented Oct 17, 2018

The added withClaim() method is not type safe, and it is not possible to overload it due to type erasure. Also, nested collections would be raw types, so there would be no guarantee as to the types they would contain.

A reasonable compromise may be to accept only primitive wrapper types, Sets and Lists and to call toString() on anything else. That would require instanceof checks at runtime which may be ugly.

@complanboy2
Copy link
Copy Markdown
Author

complanboy2 commented Oct 26, 2018

The added withClaim() method is not type safe, and it is not possible to overload it due to type erasure. Also, nested collections would be raw types, so there would be no guarantee as to the types they would contain.

A reasonable compromise may be to accept only primitive wrapper types, Sets and Lists and to call toString() on anything else. That would require instanceof checks at runtime which may be ugly.

Hi @martinoconnor , thank you for the reivew. It helped fixing it better.

Agree, type safety would be an issue if consumer passes Map of custom objects. Updated the code to warn (throwing IllegalArugment) in that case. This change supports Map and/or Collection of objects of specified type/array type only. Restricted the checks to the minimal.

Please review the updated code when you have time.

@ornom0n
Copy link
Copy Markdown

ornom0n commented Nov 1, 2018

Hello,

Adding this feature would allow me to migrate from JWT 2 to JWT 3 as one of the claims we use is a Map and that cannot be changed. Is anything pending here?

@complanboy2
Copy link
Copy Markdown
Author

complanboy2 commented Nov 3, 2018

Hello,

Adding this feature would allow me to migrate from JWT 2 to JWT 3 as one of the claims we use is a Map and that cannot be changed. Is anything pending here?

Hi @ornom0n ,

I'm waiting on review, once the review is done, we are good to merge.
If it is helpful to you, please go ahead with the use. And please have a look at checks once, though major are passed.

@complanboy2
Copy link
Copy Markdown
Author

Any hope for getting this reviewed and also please check, #297 as well.

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

X Tutup