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

Implementation of JSON Web Token #743

Merged
merged 7 commits into from
Aug 9, 2017
Merged

Conversation

semantosoph
Copy link

@semantosoph semantosoph commented Aug 9, 2017

These commits introduce the following things in order to make JSON Web Token (JWT, see https://jwt.io/) usable in takes:

  1. A new Signature interface, similar to the Codec interface. The main difference is that while Codecs can encrypt and decrypt data, a Signature can only encrypt (aka sign) data.
  2. An HMAC-SHA implementation of that interface with variations of 256, 384 and 512 bitlength. This is the standard signature algorithm for JWT.
  3. A new interface for Token and two implementations that provide a JOSE token (JWT header) and a JWT payload token.
  4. A new Pass that accepts JWT from a Bearer header and adds a new JWT to its response (obviously, that works only with JSON responses).

@0crat
Copy link
Collaborator

0crat commented Aug 9, 2017

@yegor256 please, pay attention to this pull request

@0crat
Copy link
Collaborator

0crat commented Aug 9, 2017

Job gh:yegor256/takes#743 is in scope.

@0crat
Copy link
Collaborator

0crat commented Aug 9, 2017

+15 points just awarded to @semantosoph, total is +15.

@codecov-io
Copy link

codecov-io commented Aug 9, 2017

Codecov Report

Merging #743 into master will decrease coverage by 0.82%.
The diff coverage is 40.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #743      +/-   ##
============================================
- Coverage     77.55%   76.72%   -0.83%     
- Complexity      900      912      +12     
============================================
  Files           205      208       +3     
  Lines          4245     4348     +103     
  Branches        323      330       +7     
============================================
+ Hits           3292     3336      +44     
- Misses          823      881      +58     
- Partials        130      131       +1
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/takes/facets/auth/PsToken.java 0% <0%> (ø) 0 <0> (?)
src/main/java/org/takes/facets/auth/Token.java 100% <100%> (ø) 0 <0> (?)
.../java/org/takes/facets/auth/signatures/SiHmac.java 73.33% <73.33%> (ø) 10 <10> (?)
src/main/java/org/takes/http/MainRemote.java 57.37% <0%> (+1.63%) 7% <0%> (+1%) ⬆️
src/main/java/org/takes/misc/Base64.java 82.45% <0%> (+1.75%) 12% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5888370...9448788. Read the comment docs.

/**
* JSON Web Token.
*/
@SuppressWarnings
Copy link
Owner

Choose a reason for hiding this comment

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

@semantosoph you can remove that and just initialize all attributes in the constructor, it's a good practice.

Copy link
Author

Choose a reason for hiding this comment

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

@yegor256 Yep, will do.

try {
bytes = new Base64().encode(this.jwto.toString());
} catch (final IOException ignore) {
bytes = new byte[] {};
Copy link
Owner

Choose a reason for hiding this comment

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

@semantosoph this looks a bit weird. Why so? Can't you just throw an unchecked exception here?

Copy link
Author

Choose a reason for hiding this comment

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

@yegor256 Yeah, I don't like declaring throws on interface methods. But as of now, it looks like there will always be a reason to throw an IOException, so we can predeclare it anyway.

* @throws IOException
* for all unexpected exceptions
*/
@SuppressWarnings("resource")
Copy link
Owner

Choose a reason for hiding this comment

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

@semantosoph what's the point of this suppression? Can't you do what Java wants you to do?

Copy link
Author

Choose a reason for hiding this comment

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

@yegor256 This is kinda silly. Constructing the Formatter without parameters forces it to use an internal StringBuilder which is totally OK for our use. But that StringBuilder is not closable, so calling close() on the Formatter is useless in terms of saving resources. Eclipse doesn't know this and warns anyway. That's why I suppressed it. I'll close it, if you like it better that way.

@yegor256
Copy link
Owner

yegor256 commented Aug 9, 2017

@semantosoph looks cool, thanks for your contribution! I made a few minor comments above

@semantosoph
Copy link
Author

@yegor256 There you go, I made the requested changes.

I would appreciate it, if you could add me to the contributors list this time.

final String jwt = head.trim().split(" ", 2)[1].trim();
final byte[] jwtheader = jwt.split(dot)[0].getBytes();
final byte[] jwtpayload = jwt.split(dot)[1].getBytes();
final byte[] jwtsign = jwt.split(dot)[2].getBytes();
Copy link
Owner

Choose a reason for hiding this comment

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

@semantosoph this code looks a bit weird. Why can't you do:

final String[] parts = jwt.split(dot);
final byte[] jwtheader = parts[0].getBytes();
final byte[] jwtpayload = parts[1].getBytes();
final byte[] jwtsign = parts[2].getBytes();

Copy link
Author

Choose a reason for hiding this comment

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

Can do.

* for all unexpected exceptions
*/
private byte[] encrypt(final byte[] bytes) throws IOException {
final byte[] encrypted;
Copy link
Owner

Choose a reason for hiding this comment

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

@semantosoph why do you need this variable? Can't you just do

return formatter.toString().getBytes();

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, I can.

@yegor256
Copy link
Owner

yegor256 commented Aug 9, 2017

@semantosoph a few more comments above. Of course, we can add you to the list, but we don't have that list now :) Feel free to add it to README, just like it's done in github.com/yegor256/cactoos

@semantosoph
Copy link
Author

@yegor256 Changes done. I'll compile the contributors list once I've got a little spare time.

@yegor256
Copy link
Owner

yegor256 commented Aug 9, 2017

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Aug 9, 2017

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 9448788 into yegor256:master Aug 9, 2017
@rultor
Copy link
Collaborator

rultor commented Aug 9, 2017

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 22min)

@semantosoph semantosoph deleted the jwt-impl branch August 10, 2017 11:03
@yegor256
Copy link
Owner

@semantosoph when you want me to release a new version, say it here, not in Twitter :)

@yegor256
Copy link
Owner

@rultor release, tag is 1.6

@rultor
Copy link
Collaborator

rultor commented Aug 10, 2017

@rultor release, tag is 1.6

@yegor256 OK, I will release it now. Please check the progress here

@rultor
Copy link
Collaborator

rultor commented Aug 10, 2017

@rultor release, tag is 1.6

@yegor256 Done! FYI, the full log is here (took me 20min)

@0crat
Copy link
Collaborator

0crat commented Aug 14, 2017

Job gh:yegor256/takes#743 assigned to @caarlos0, please go ahead (policy).

@0crat
Copy link
Collaborator

0crat commented Nov 14, 2017

@caarlos0 this job was assigned to you 8 days ago.
It will be taken away from you after 10 days from start (this is our policy).

@0crat
Copy link
Collaborator

0crat commented Nov 16, 2017

Oops! Job gh:yegor256/takes#743 is not assigned to anyone.

2 similar comments
@0crat
Copy link
Collaborator

0crat commented Nov 16, 2017

Oops! Job gh:yegor256/takes#743 is not assigned to anyone.

@0crat
Copy link
Collaborator

0crat commented Nov 16, 2017

Oops! Job gh:yegor256/takes#743 is not assigned to anyone.

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.

5 participants