waiting-for-dev / warden-jwt_auth

JWT token authentication with warden
MIT License
113 stars 56 forks source link

Support Issuer claim #56

Closed OleMchls closed 4 months ago

OleMchls commented 4 months ago

Summary

When an application needs to handle multiple JWT tokens issued by multiple issuers, this middleware gets in the way as it activates the warden strategy whenever a token is present in the authentication header.

This PR adds support for the Issuer claim as specified in RFC 7519 and when configured verified that the iss claim matches before using the warden middleware.

With this, other warden strategies or middleware can authorize the JWT passed in the Authorization header.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed (~cross them out~ if they are not):

waiting-for-dev commented 4 months ago

Thanks for your contribution. It looks great. I'll review it in detail in the following days πŸ™

OleMchls commented 4 months ago

Thanks for the review πŸ™Œ I appreciate your fast feedback πŸ™

To get a feel for the suggestion on separating the parsing I've sketched it out locally, here is what I ended up with:

diff --git a/lib/warden/jwt_auth/hooks.rb b/lib/warden/jwt_auth/hooks.rb
index 559de10..2ebfb0d 100644
--- a/lib/warden/jwt_auth/hooks.rb
+++ b/lib/warden/jwt_auth/hooks.rb
@@ -36,7 +36,8 @@ module Warden

       def add_token_to_env(user, scope, env)
         aud = EnvHelper.aud_header(env)
-        token, payload = UserEncoder.new.call(user, scope, aud)
+        payload = UserEncoder.new.call(user, scope, aud)
+        token = TokenEncoder.new.call(payload)
         user.on_jwt_dispatch(token, payload) if user.respond_to?(:on_jwt_dispatch)
         env[PREPARED_TOKEN_ENV_KEY] = token
       end
diff --git a/lib/warden/jwt_auth/strategy.rb b/lib/warden/jwt_auth/strategy.rb
index 9ad6ce9..06a06f7 100644
--- a/lib/warden/jwt_auth/strategy.rb
+++ b/lib/warden/jwt_auth/strategy.rb
@@ -7,6 +7,8 @@ module Warden
     # Warden strategy to authenticate an user through a JWT token in the
     # `Authorization` request header
     class Strategy < Warden::Strategies::Base
+      attr_reader :payload
+
       def valid?
         token_exists? && issuer_claim_valid?
       end
@@ -17,7 +19,7 @@ module Warden

       def authenticate!
         aud = EnvHelper.aud_header(env)
-        user = UserDecoder.new.call(token, scope, aud)
+        user = UserDecoder.new.call(payload)
         success!(user)
       rescue JWT::DecodeError => e
         fail!(e.message)
@@ -29,7 +31,7 @@ module Warden
         configured_issuer = Warden::JWTAuth.config.issuer
         return true if configured_issuer.nil?

-        payload = TokenDecoder.new.call(token)
+        @payload = TokenDecoder.new.call(token)
         PayloadUserHelper.issuer_matches?(payload, configured_issuer)
       rescue JWT::DecodeError
         true
diff --git a/lib/warden/jwt_auth/user_decoder.rb b/lib/warden/jwt_auth/user_decoder.rb
index f6eb25c..6afc42e 100644
--- a/lib/warden/jwt_auth/user_decoder.rb
+++ b/lib/warden/jwt_auth/user_decoder.rb
@@ -29,8 +29,7 @@ module Warden
       # @raise [Errors::WrongScope] when encoded scope does not match with scope
       # @raise [Errors::WrongAud] when encoded aud does not match with aud
       # argument
-      def call(token, scope, aud)
-        payload = TokenDecoder.new.call(token)
+      def call(payload, scope, aud)
         check_valid_claims(payload, scope, aud)
         user = helper.find_user(payload)
         check_valid_user(payload, user, scope)

My main concern is that it's creating a dependency between valid? and authenticate!, which could be solved with lazy evaluation, but I doubt the added complexity is worth it. Given the scope of the changes and the API change I would not see it part on this PR. WDYT?

waiting-for-dev commented 4 months ago

Looks god as it is @OleMchls. Thanks for your collaboration!

waiting-for-dev commented 4 months ago

Available in v0.9.0