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

JWTCreator for basic types #282

Merged
merged 5 commits into from
Feb 13, 2020
Merged

Conversation

skjolber
Copy link
Contributor

@skjolber skjolber commented Sep 4, 2018

Support for creating tokens using Maps / Lists which must drill down to the base types Boolean, Integer, Long, Double, String and Date.

@lbalmaceda
Copy link
Contributor

This is one of the ideas we proposed back when all this discussion started. I really like what you came up with since it's not exposing any external dependencies in the public API 🎉 Unfortunately, I won't be able to dedicate time to this repo for at least 2 more weeks. I'll come back later

@skjolber
Copy link
Contributor Author

Our use-case is that we are currently migrating to Auth0 from Keycloak and would like to use this library for validation of tokens. So then it is also very convenient to use this library for creating mock tokens in our testing libraries as well - which requires a few deeper structures in the JWT. We are running both providers in parallel so that migration can be done service per service.

@skjolber
Copy link
Contributor Author

Any hope for getting this reviewed in 2018?

@skjolber
Copy link
Contributor Author

See also #278

@skjolber skjolber requested a review from a team May 9, 2019 07:52
@skjolber skjolber force-pushed the jwtCreaterForBasicTypes branch 4 times, most recently from 508d243 to 3758bb7 Compare May 9, 2019 11:04
@skjolber
Copy link
Contributor Author

skjolber commented May 9, 2019

@lbalmaceda bump. I've added code coverage, the build now passes the codecov quality checks.

Null keys and values in maps are disallowed, but null items in lists (and arrays) are allowed - fair?

If this gets merged, it would let us remove a test dependency (your competitor's library!) from our projects.

@damieng damieng added the medium This PR may require moderate effort to action, or contains many changes to review label May 10, 2019
@luisrudge luisrudge requested review from lbalmaceda and removed request for a team June 19, 2019 17:06
@stale
Copy link

stale bot commented Jan 6, 2020

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! 🙇‍♂️

@stale stale bot added the closed:stale Issue or PR has not seen activity recently label Jan 6, 2020
@skjolber
Copy link
Contributor Author

skjolber commented Jan 6, 2020

Was any of the alternative PRs merged? If not then do not close.

@stale stale bot removed the closed:stale Issue or PR has not seen activity recently label Jan 6, 2020
@lbalmaceda
Copy link
Contributor

Hi @skjolber. Apologies for the delay here! We're planning to review this soon. If we happen to request changes, will you be able to address them?

@skjolber
Copy link
Contributor Author

skjolber commented Feb 5, 2020

@lbalmaceda yes

lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated Show resolved Hide resolved
lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated Show resolved Hide resolved
lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated Show resolved Hide resolved
lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated Show resolved Hide resolved
lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated Show resolved Hide resolved
}

private static boolean validateClaim(List<?> list) {
// accept null values in list
Copy link
Contributor

Choose a reason for hiding this comment

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

why would we want to accept null values in a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The corresponding withArrayClaim(..) does now, so basically it is just the same behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But are null values so bad? Could leave it up to the caller for value of Map as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Checking again, it makes sense the way you did it since you're keeping the behavior of the current withArrayClaim implementation that accepts null, and you're not accepting nulls for maps. Let's keep it that way for now.

lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated Show resolved Hide resolved
lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated Show resolved Hide resolved
@skjolber skjolber requested a review from a team February 11, 2020 20:43
lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated Show resolved Hide resolved
}

private static boolean validateClaim(List<?> list) {
// accept null values in list
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking again, it makes sense the way you did it since you're keeping the behavior of the current withArrayClaim implementation that accepts null, and you're not accepting nulls for maps. Let's keep it that way for now.

lib/src/test/java/com/auth0/jwt/JWTCreatorTest.java Outdated Show resolved Hide resolved
lib/src/test/java/com/auth0/jwt/JWTCreatorTest.java Outdated Show resolved Hide resolved
@lbalmaceda lbalmaceda self-requested a review February 13, 2020 18:11
@lbalmaceda lbalmaceda merged commit 0130a26 into auth0:master Feb 13, 2020
@lbalmaceda lbalmaceda added this to the v3-Next milestone Feb 13, 2020
@skjolber skjolber deleted the jwtCreaterForBasicTypes branch February 13, 2020 20:28
@lbalmaceda lbalmaceda modified the milestones: v3-Next, 3.10.0 Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Added medium This PR may require moderate effort to action, or contains many changes to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants