X Tutup
Skip to content

[WIP] Add Custom Map Claim#164

Closed
gustavoamigo wants to merge 1 commit intoauth0:masterfrom
99Taxis:master
Closed

[WIP] Add Custom Map Claim#164
gustavoamigo wants to merge 1 commit intoauth0:masterfrom
99Taxis:master

Conversation

@gustavoamigo
Copy link
Copy Markdown

@gustavoamigo gustavoamigo commented Apr 13, 2017 •
edited
Loading

I need to create a claim that is a map, so added a method the JWT Builder to add custom map. Example:

{
    "user": {
        "id": "u1234",
        "accountId": "a5678",
        "firstName": "Joe",
        "lastName": "Tester",
        "email": "joe.tester@example.com",
        "paymentProviderId": null
    }
}

TODO as requested by peer review.

  • The method must add claims of type Map<String, Object> (not Map).
  • The readme states which types of claim can be set and read. Add a clear warning message
    explaining the above, telling that only primitives and collections are allowed and that custom classes are not guaranteed to be handled just fine and may throw.
  • Add the same warning to the javadoc of the withClaim() method that receives the Map.
  • Add enough tests to assert that primitives and collections are supported, but that custom classes will throw an error. Keep in mind that this should work in create and decode functions.
    See https://docs.referralsaasquatch.com/topics/json-web-tokens/

@gustavoamigo
Copy link
Copy Markdown
Author

gustavoamigo commented Apr 18, 2017

@lbalmaceda is there a channel where I can discuss this PR with you guys?

@gustavoamigo
Copy link
Copy Markdown
Author

gustavoamigo commented Apr 20, 2017 •
edited
Loading

Hi, I created a fork and published in bintray repository because I need this feature now.

@lbalmaceda
Copy link
Copy Markdown
Contributor

lbalmaceda commented Apr 20, 2017

Hi @gustavoamigo
The thing is that we can't ensure proper serialization/deserialization of the values in the Map because they are Objects. It will work for primitives and collections but it might not for other classes. The API will NOT be opened to custom serializers/deserializers as we don't want to force the user to use a specific JSON library, so a solution might be to limit the Map contents somehow. There are many closed issues regarding this. Check for example #163

We can accept an implementation if contains the following:

  • The method must add claims of type Map<String, Object> (not Map).
  • The readme states which types of claim can be set and read. Add a clear warning message explaining the above, telling that only primitives and collections are allowed and that custom classes are not guaranteed to be handled just fine and may throw.
  • Add the same warning to the javadoc of the withClaim() method that receives the Map.
  • Add enough tests to assert that primitives and collections are supported, but that custom classes will throw an error. Keep in mind that this should work in create and decode functions.

Cheers.

@gustavoamigo gustavoamigo changed the title Add Custom Map Claim [WIP] Add Custom Map Claim Apr 20, 2017
@lbalmaceda lbalmaceda mentioned this pull request May 30, 2017
Closed
@lbalmaceda lbalmaceda mentioned this pull request Jun 28, 2017
Closed
@lbalmaceda
Copy link
Copy Markdown
Contributor

lbalmaceda commented Aug 7, 2017

Closing this as it looks abandoned. Ping me if you need me to open it @gustavoamigo.

@lbalmaceda lbalmaceda closed this Aug 7, 2017
@lbalmaceda lbalmaceda mentioned this pull request Nov 8, 2017
Closed
@lbalmaceda lbalmaceda mentioned this pull request Apr 25, 2018
Closed
@AlexandreOlipe AlexandreOlipe mentioned this pull request Jun 13, 2018
Closed
@complanboy2 complanboy2 mentioned this pull request Oct 10, 2018
Closed
@complanboy2
Copy link
Copy Markdown

complanboy2 commented Oct 10, 2018

Hi @lbalmaceda ,

Your review is appreciated, #289 which is opened for this request. Thank you.

@josecolella josecolella mentioned this pull request Dec 20, 2018
Closed
@complanboy2 complanboy2 mentioned this pull request Jan 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gustavoamigo @lbalmaceda @complanboy2
X Tutup