JWT authentication #70

wilddima · 2017-06-18T17:55:09Z

Hi!
I implemented JWT authentication as module and tested it works capacity in my pet project(SPA with React). Maybe, configurations is looking complex, but i tried to give more flexibility for customization authentication. I also tried to write some documentation, but my english is not good enough for it.

Ch4s3 · 2017-12-13T04:38:11Z

@wilddima I'll try to take a look at this soon. THings got away from me over the summer, and I'm trying to catch up.

ebihara99999 · 2017-12-24T14:57:49Z

Thank you for great work! @wilddima
Would you show us a document or code snippets if the usage is wrong written bellow:

# app/sessions_controller.rb
def create
  render json: jwt_auth(params[:email], params[:password])
end

The response is like this:

{"user_data":{"id":1},"auth_token":"TOKEN"}

Then, access to a limited page with Authorization header with the value, the auth_token.

@athix and @Ch4s3: This PR seems good, having looked over this PR and RFC and jwt-ruby, and used it in a sandbox SPA rails app. The function has great advance in an API authentication if it's merged.

ebihara99999

I missed some potential improvements.
The 'no new line at end of file' must be fixed, but I would like to discuss the error response design.

ebihara99999 · 2017-12-26T09:52:50Z

spec/controllers/controller_jwt_auth_spec.rb

+      end
+    end
+  end
+end


Need to set the new line.

ebihara99999 · 2017-12-26T09:52:58Z

lib/sorcery/controller/submodules/jwt_auth.rb

+      end
+    end
+  end
+end


Need to set the new line.

ebihara99999 · 2017-12-26T10:15:30Z

lib/sorcery/controller/submodules/jwt_auth.rb

+          def jwt_not_authenticated
+            respond_to do |format|
+              format.html { not_authenticated }
+              format.json { render json: { status: 401 }, status: 401 }


IMO the message should not be the status code because it's verbose.
It would be better if the api response is like this (just an example)

format.json { render json: { "error": { "message": "Need to login first.", } }, status: 401 }

I also suggest you use status: :unauthorized to make it more explicit.

How about making this changeable, instead of hardcoding the return message

It's difficult to make an error message changeable because of the design. The cases are different when this method is called.

I use ruby-jwt error messages and I want it to be overridden if someone wants to change the message.

See this commit for details: ebihara99999@9e02135

ebihara99999 · 2017-12-26T15:16:21Z

@wilddima
Is there any configuration to set claims?
If so, we need to have discussion here which claims to be add.

Spone · 2018-01-13T18:25:35Z

I'm interested in this as well. Please let me know if I can help.

ebihara99999 · 2018-01-16T14:46:47Z

@Spone Thank you for saying that! I would appreciate it if you could review or give us feedbacks using this feature.

joshbuker · 2018-03-07T00:09:00Z

I should be able to set aside time very soon to work on JWT, I'll take a look at this when I do. @Ch4s3 if you can set aside some time for this as well, I would like to get multiple eyes on something as big as this before releasing.

ebihara99999 · 2018-03-07T00:53:37Z

I prepared for a request and system spec helper to login. Should I open a PR to the master or this branch?

Spone · 2018-03-11T22:09:38Z

lib/sorcery/controller/submodules/jwt_auth.rb

+
+          # This method creating JWT token by payload
+          def jwt_encode(payload)
+            JWT.encode(payload, Config.jwt_secret_key)


You should specify an algorithm to use, else the created JWT is unsigned!

See here: https://github.com/jwt/ruby-jwt#algorithms-and-usage

I think it should be a config option, with "HS256" as its default value.

The same algorithm parameter has to be passed to JWT.decode as well.

Spone · 2018-03-11T22:27:31Z

About the claims, I suggest adding by default:

exp (expiration date)
iat (issued at)

Also, as I understand it, the user ID could be added as the sub (subject) claim.

See https://tools.ietf.org/html/rfc7519#section-4.1 for the full list and https://github.com/jwt/ruby-jwt#add-custom-header-fields for details about the ruby-jwt implementation.

ebihara99999 · 2018-03-14T15:54:41Z

@Spone

Thanks for the review. I make it enable to configure claims in this commit: ebihara99999@4dde3c1

As for specifying an algorithm I will implement and open a PR if this PR keeps inactive.

jovanmaric · 2018-09-07T13:53:21Z

lib/sorcery/controller/submodules/jwt_auth.rb

+                acc[val] = user.public_send(val)
+              end
+              { Config.jwt_user_data_key => user_params,
+                Config.jwt_auth_token_key => jwt_encode(user_params) }


Maybe it's an idea to replace user_id with a jti (SecureRandom.uuid) and implement a whitelisting strategy to support login from multiple devices and independent logout. This implies that on login, a record is created in the whitelists table and that on logout this record gets deleted.

Also i'm missing expiration from this implementation. The generated jwt keys are valid indefinitely. The duration a key is persisted could be set from the config for instance.

@jovanmaric I don't know why user_id is here though the token includes user_id as the subject claim ( of course, it's needed to decode to acquire it ) .

I'd like to remove and only returns a token like eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJzdWIiOjQyLCJleHAiOjE1NDc3MTkyMDAsImlhdCI6MTU0NzQ2MDAwMH0.QM5mTkYiDwI-10cEOq4b_bfrwe99BRuef6pnIB-jqIk.

What do you think about it?

And I would appreciate it if @wilddima could answer why Config.jwt_user_data_key => user_params is needed.

jovanmaric · 2018-09-07T13:57:14Z

lib/sorcery/controller/submodules/jwt_auth.rb

+              JWT.decode(token, Config.jwt_secret_key)[0]
+            )
+          rescue JWT::DecodeError
+            nil


Shouldn't this be removed, considering that def jwt_require_auth catches JWT::DecodeError, and omits an unauthorized_error?

@jovanmaric
I agree with that and changed like this:
9e02135

jovanmaric · 2018-09-07T14:05:02Z

lib/sorcery/controller/submodules/jwt_auth.rb

+          def jwt_not_authenticated
+            respond_to do |format|
+              format.html { not_authenticated }
+              format.json { render json: { status: 401 }, status: 401 }


How about making this changeable, instead of hardcoding the return message

jovanmaric · 2018-09-07T14:07:27Z

sorcery.gemspec

@@ -23,6 +23,7 @@ Gem::Specification.new do |s|
  s.add_dependency 'oauth', '~> 0.4', '>= 0.4.4'
  s.add_dependency 'oauth2', '~> 1.0', '>= 0.8.0'
  s.add_dependency 'bcrypt', '~> 3.1'
+  s.add_dependency 'jwt', '~> 1.5'


The latest is 2.1.0

jovanmaric · 2018-09-07T14:10:06Z

Is there any eta on when this will be implemented? Considering that there is no defacto authentication gem which supports JWT.

andrewhood125 · 2018-09-26T23:30:04Z

I went about combing Sorcery and Knock to accomplish SPA and SSR authentication. I'd like to take a stab at this. It looks like Knock is not being very actively developed. I have a PR open against that gem to be able to support rails in API mode but it's been stale for months.

joshbuker · 2018-09-27T02:52:19Z

@andrewhood125 any help would be greatly appreciated!

joshbuker · 2018-12-31T11:42:50Z

@andrewhood125 any luck?

ebihara99999 · 2019-01-12T10:35:28Z

@athix
I work on this PR modifying to reflect the PR comments after b06aa7a. I'll open a PR again if it's done.

See the discussion here: Sorcery#70 (comment)

There is a suggestion that a message in the case not authenticated could be changeable in the discussion: Sorcery#70 (comment) But I decide to make use of a message sent from `JWT::DecodeError` because it's difficult to make the message flexible because of the design. Changes: - `#jwt_require_auth` only rescues `JWT::DecodeError` because JWT::VerificationError is a subclass. - `#jwt_decode` raises `JWT::DecodeError` or the error of the subclasses, and does not rescue in it. - `#jwt_not_authenticated` receives an argument `JWT::DecodeError` are from here: https://github.com/jwt/ruby-jwt/blob/master/lib/jwt/error.rb

ebihara99999 · 2019-01-12T15:50:33Z

@athix
I open a wip PR though not all are finished, because you can see the progress easier and I want review comments sooner. If it's OK to open as another PR, I would appreciate it if someone reviews.

`JWT::ExpiredSignature: Signature has expired` is expected when a token is expired. Review comment is: Sorcery#70 (comment)

the review comment is: Sorcery#70 (comment)

ebihara99999 · 2019-01-15T11:16:14Z

@athix @Spone @Ch4s3 @jovanmaric
Almost all review comments are reflected and open a PR from b06aa7a ( I don't have access to commit to this PR ). I would appreciate it if you could review again!

#167

privorotskii · 2019-08-26T10:57:19Z

@athix Any updates?

joshbuker · 2019-08-26T16:02:25Z

@TolichP Not at the moment unfortunately. If anyone from the community could help drive this effort, I would greatly appreciate the assistance. JWT would be a great addition, I just don't have time at the moment to drive it forward.

joshbuker · 2021-04-05T22:56:17Z

Closing this out to reduce noise, see #239 and Sorcery/sorcery-rework#9 for the latest on JWT in Sorcery.

wilddima added 14 commits June 5, 2017 17:44

create JwtAuth submodules

9e33d10

refactor jwt_auth; add some params to config

5b3fe7c

create spec

270ea1c

add some specs to jwt auth

ef86e9c

change naming for jwt auth

9bb9443

add capability not to execute user find

a70144a

add generation of secret key for jwt

edd5483

add specs to test fail login, success auth

7f12bd4

add description to jwt params in initializer file

3366df1

add description to jwt auth methods

31fcc44

add specs

9f84e58

rename methods

e6a3d4e

add readme

87de4bf

fix specs for some ruby/rails versions

b06aa7a

wilddima changed the title ~~Feature/jwt auth~~ JWT authentication Jun 18, 2017

Ch4s3 requested a review from joshbuker December 13, 2017 04:38

ebihara99999 reviewed Dec 26, 2017

View reviewed changes

Spone requested changes Mar 11, 2018

View reviewed changes

jovanmaric suggested changes Sep 7, 2018

View reviewed changes

joshbuker added the help wanted Community assistance requested label Sep 21, 2018

joshbuker added the high priority Extra attention is needed label Sep 21, 2018

Ch4s3 mentioned this pull request Jan 11, 2017

1.0 Roadmap #32

Closed

10 tasks

ebihara99999 added a commit to ebihara99999/sorcery that referenced this pull request Jan 12, 2019

Make algorithm configurable

5a5e39a

See the discussion here: Sorcery#70 (comment)

ebihara99999 mentioned this pull request Jan 12, 2019

JWT authentication ( after the commit b06aa7a ) #167

Closed

ebihara99999 added a commit to ebihara99999/sorcery that referenced this pull request Jan 14, 2019

Use sub, exp, iat as payload

373a5f3

`JWT::ExpiredSignature: Signature has expired` is expected when a token is expired. Review comment is: Sorcery#70 (comment)

ebihara99999 added a commit to ebihara99999/sorcery that referenced this pull request Jan 15, 2019

Set the newest one

5488cbe

the review comment is: Sorcery#70 (comment)

joshbuker mentioned this pull request May 1, 2020

Handling External Logins with An API #230

Closed

alexeyramazanov mentioned this pull request May 11, 2020

JWT authentication #239

Open

joshbuker added the to be implemented in v1 This issue or pull request will be resolved in the v1 rework, but has not yet been completed. label Apr 5, 2021

joshbuker closed this Apr 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JWT authentication #70

JWT authentication #70

wilddima commented Jun 18, 2017

Ch4s3 commented Dec 13, 2017

ebihara99999 commented Dec 24, 2017 •

edited

Loading

ebihara99999 left a comment

ebihara99999 Dec 26, 2017

ebihara99999 Dec 26, 2017

ebihara99999 Dec 26, 2017

Spone Mar 11, 2018

jovanmaric Sep 7, 2018

ebihara99999 Jan 12, 2019

ebihara99999 commented Dec 26, 2017

Spone commented Jan 13, 2018

ebihara99999 commented Jan 16, 2018

joshbuker commented Mar 7, 2018

ebihara99999 commented Mar 7, 2018

Spone Mar 11, 2018

Spone commented Mar 11, 2018

ebihara99999 commented Mar 14, 2018

jovanmaric Sep 7, 2018

ebihara99999 Jan 15, 2019

jovanmaric Sep 7, 2018

ebihara99999 Jan 15, 2019

jovanmaric Sep 7, 2018

jovanmaric Sep 7, 2018

jovanmaric commented Sep 7, 2018

andrewhood125 commented Sep 26, 2018

joshbuker commented Sep 27, 2018

joshbuker commented Dec 31, 2018

ebihara99999 commented Jan 12, 2019

ebihara99999 commented Jan 12, 2019

ebihara99999 commented Jan 15, 2019

privorotskii commented Aug 26, 2019

joshbuker commented Aug 26, 2019

joshbuker commented Apr 5, 2021

JWT authentication #70

JWT authentication #70

Conversation

wilddima commented Jun 18, 2017

Ch4s3 commented Dec 13, 2017

ebihara99999 commented Dec 24, 2017 • edited Loading

ebihara99999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebihara99999 commented Dec 26, 2017

Spone commented Jan 13, 2018

ebihara99999 commented Jan 16, 2018

joshbuker commented Mar 7, 2018

ebihara99999 commented Mar 7, 2018

Choose a reason for hiding this comment

Spone commented Mar 11, 2018

ebihara99999 commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jovanmaric commented Sep 7, 2018

andrewhood125 commented Sep 26, 2018

joshbuker commented Sep 27, 2018

joshbuker commented Dec 31, 2018

ebihara99999 commented Jan 12, 2019

ebihara99999 commented Jan 12, 2019

ebihara99999 commented Jan 15, 2019

privorotskii commented Aug 26, 2019

joshbuker commented Aug 26, 2019

joshbuker commented Apr 5, 2021

ebihara99999 commented Dec 24, 2017 •

edited

Loading