Monadize JwtParser

Instead of relying on raised exceptions
for lots of our control flow, we are now
using a failed operation to represent these.

We are using the Failure result for all previously
considered exceptions, because all of them were kind of
expectable error conditions.

lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb

@@ -20,21 +20,14 @@ module OpenProject
           end
 
           def authenticate!
-            verified_payload, provider = ::OpenIDConnect::JwtParser.new(required_claims: ["sub"]).parse(@access_token)
-
-            user = User.find_by(identity_url: "#{provider.slug}:#{verified_payload['sub']}")
-            success!(user) if user
-          rescue JWT::ExpiredSignature
-            fail_with_header!(error: "invalid_token", error_description: "The access token expired")
-          rescue JWT::ImmatureSignature
-            # happens when nbf time is less than current
-            fail_with_header!(error: "invalid_token", error_description: "The access token is used too early")
-          rescue JWT::InvalidAudError
-            fail_with_header!(error: "invalid_token", error_description: "The access token audience claim is wrong")
-          rescue JSON::JWK::Set::KidNotFound
-            fail_with_header!(error: "invalid_token", error_description: "The access token signature kid is unknown")
-          rescue ::OpenIDConnect::JwtParser::Error => e
-            fail_with_header!(error: "invalid_token", error_description: e.message)
+            ::OpenIDConnect::JwtParser.new(required_claims: ["sub"]).parse(@access_token).either(
+              ->(payload_and_provider) do
+                payload, provider = payload_and_provider
+                user = User.find_by(identity_url: "#{provider.slug}:#{payload['sub']}")
+                success!(user) if user
+              end,
+              ->(error) { fail_with_header!(error: "invalid_token", error_description: error) }
+            )
           end
         end
       end

modules/openid_connect/app/services/openid_connect/associate_user_token.rb

@@ -48,18 +48,20 @@ module OpenIDConnect
 
       @user.oidc_user_tokens.destroy_all if clear_previous
 
-      token = @user.oidc_user_tokens.build(access_token:, refresh_token:)
-      token.audiences = merge_audiences(known_audiences, discover_audiences(access_token))
+      token = prepare_token(access_token:, refresh_token:, known_audiences:)
       token.save! if token.audiences.any?
     end
 
     private
 
+    def prepare_token(access_token:, refresh_token:, known_audiences:)
+      @user.oidc_user_tokens.build(access_token:, refresh_token:).tap do |token|
+        token.audiences = merge_audiences(known_audiences, discover_audiences(access_token).value_or([]))
+      end
+    end
+
     def discover_audiences(access_token)
-      decoded, = @jwt_parser.parse(access_token)
-      Array(decoded["aud"])
-    rescue StandardError
-      []
+      @jwt_parser.parse(access_token).fmap { |decoded, _| Array(decoded["aud"]) }
     end
 
     def merge_audiences(*args)

modules/openid_connect/app/services/openid_connect/jwt_parser.rb

@@ -30,7 +30,7 @@
 
 module OpenIDConnect
   class JwtParser
-    class Error < StandardError; end
+    include Dry::Monads[:result]
 
     SUPPORTED_JWT_ALGORITHMS = %w[
       RS256
@@ -44,41 +44,41 @@ module OpenIDConnect
     end
 
     def parse(token)
-      issuer, alg, kid = parse_unverified_iss_alg_kid(token)
-      raise Error, "Token signature algorithm #{alg} is not supported" if SUPPORTED_JWT_ALGORITHMS.exclude?(alg)
+      parse_unverified_iss_alg_kid(token).bind do |issuer, alg, kid|
+        return Failure("Token signature algorithm #{alg} is not supported") if SUPPORTED_JWT_ALGORITHMS.exclude?(alg)
 
-      provider = fetch_provider(issuer)
-      raise Error, "The access token issuer is unknown" if provider.blank?
+        provider = fetch_provider(issuer)
+        return Failure("The access token issuer is unknown") if provider.blank?
 
-      jwks_uri = provider.jwks_uri
-      key = JSON::JWK::Set::Fetcher.fetch(jwks_uri, kid:).to_key
+        verified_payload, = JWT.decode(
+          token,
+          fetch_key(provider:, kid:),
+          true,
+          {
+            algorithm: alg,
+            verify_aud: @verify_audience,
+            aud: provider.client_id,
+            required_claims: all_required_claims
+          }
+        )
 
-      verified_payload, = JWT.decode(
-        token,
-        key,
-        true,
-        {
-          algorithm: alg,
-          verify_aud: @verify_audience,
-          aud: provider.client_id,
-          required_claims: all_required_claims
-        }
-      )
-
-      [verified_payload, provider]
+        Success([verified_payload, provider])
+      rescue JWT::DecodeError => e
+        Failure(e.message)
+      rescue JSON::JWK::Set::KidNotFound
+        Failure("The signature key ID is unknown")
+      end
     end
 
     private
 
     def parse_unverified_iss_alg_kid(token)
       unverified_payload, unverified_header = JWT.decode(token, nil, false)
-      raise Error, "The token's Key Identifier (kid) is missing" unless unverified_header.key?("kid")
+      return Failure("The token's Key Identifier (kid) is missing") unless unverified_header.key?("kid")
 
-      [
-        unverified_payload["iss"],
-        unverified_header.fetch("alg"),
-        unverified_header.fetch("kid")
-      ]
+      Success([unverified_payload["iss"], unverified_header.fetch("alg"), unverified_header.fetch("kid")])
+    rescue JWT::DecodeError => e
+      Failure(e.message)
     end
 
     def fetch_provider(issuer)
@@ -87,6 +87,10 @@ module OpenIDConnect
       OpenIDConnect::Provider.where(available: true).where("options->>'issuer' = ?", issuer).first
     end
 
+    def fetch_key(provider:, kid:)
+      JSON::JWK::Set::Fetcher.fetch(provider.jwks_uri, kid:).to_key
+    end
+
     def all_required_claims
       claims = ["iss"] + @required_claims
       claims << "aud" if @verify_audience

modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb

@@ -39,7 +39,7 @@ RSpec.describe OpenIDConnect::AssociateUserToken do
   let(:access_token) { "access-token-foo" }
   let(:refresh_token) { "refresh-token-bar" }
 
-  let(:parser) { instance_double(OpenIDConnect::JwtParser, parse: [parsed_jwt, nil]) }
+  let(:parser) { instance_double(OpenIDConnect::JwtParser, parse: Success([parsed_jwt, nil])) }
   let(:parsed_jwt) { { "aud" => ["aud1", "aud2"] } }
 
   before do
@@ -80,9 +80,7 @@ RSpec.describe OpenIDConnect::AssociateUserToken do
   end
 
   context "when the access token is not a valid JWT" do
-    before do
-      allow(parser).to receive(:parse).and_raise("Oops, not a JWT!")
-    end
+    let(:parser) { instance_double(OpenIDConnect::JwtParser, parse: Failure("Oops, not a JWT!")) }
 
     it "creates a correct user token", :aggregate_failures do
       expect { subject }.to change(OpenIDConnect::UserToken, :count).by(1)

modules/openid_connect/spec/services/openid_connect/jwt_parser_spec.rb

@@ -51,13 +51,15 @@ RSpec.describe OpenIDConnect::JwtParser do
     provider.save!
   end
 
+  it { is_expected.to be_success }
+
   it "parses the token" do
-    parsed, = parse
+    parsed, = parse.value!
     expect(parsed).to eq payload
   end
 
   it "returns the provider configuration for the associated provider" do
-    _, p = parse
+    _, p = parse.value!
     expect(p).to eq provider
   end
 
@@ -70,8 +72,10 @@ RSpec.describe OpenIDConnect::JwtParser do
   context "when the provider signing the token is not known" do
     let(:known_issuer) { "Lunar Gateway" }
 
-    it "raises an error" do
-      expect { parse }.to raise_error(OpenIDConnect::JwtParser::Error, /issuer is unknown/)
+    it { is_expected.to be_failure }
+
+    it "indicates the problem" do
+      expect(parse.failure).to match(/issuer is unknown/)
     end
   end
 
@@ -80,24 +84,26 @@ RSpec.describe OpenIDConnect::JwtParser do
       provider.update!(available: false)
     end
 
-    it "raises an error" do
-      expect { parse }.to raise_error(OpenIDConnect::JwtParser::Error, /issuer is unknown/)
+    it { is_expected.to be_failure }
+
+    it "indicates the problem" do
+      expect(parse.failure).to match(/issuer is unknown/)
     end
   end
 
   context "when the token is not a valid JWT" do
     let(:token) { Base64.encode64("banana").strip }
 
-    it "raises an error" do
-      expect { parse }.to raise_error(JWT::DecodeError)
-    end
+    it { is_expected.to be_failure }
   end
 
   context "when the token is signed using an unsupported signature" do
     let(:token) { JWT.encode(payload, "secret", "HS256", { kid: "key-identifier" }) }
 
-    it "raises an error" do
-      expect { parse }.to raise_error(OpenIDConnect::JwtParser::Error, /HS256 is not supported/)
+    it { is_expected.to be_failure }
+
+    it "indicates the problem" do
+      expect(parse.failure).to match(/HS256 is not supported/)
     end
   end
 
@@ -106,44 +112,36 @@ RSpec.describe OpenIDConnect::JwtParser do
       payload["aud"] = "Alice"
     end
 
-    it "raises an error" do
-      expect { parse }.to raise_error(JWT::InvalidAudError)
-    end
+    it { is_expected.to be_failure }
 
     context "and the audience shall not be verified" do
       subject(:parse) { described_class.new(verify_audience: false).parse(token) }
 
-      it "parses the token" do
-        parsed, = parse
-        expect(parsed).to eq payload
-      end
+      it { is_expected.to be_success }
     end
   end
 
   context "when the token does not indicate a Key Identifier" do
     let(:token) { JWT.encode(payload, private_key, "RS256") }
 
-    it "raises an error" do
-      expect { parse }.to raise_error(OpenIDConnect::JwtParser::Error, /Key Identifier .+ is missing/)
+    it { is_expected.to be_failure }
+
+    it "indicates the problem" do
+      expect(parse.failure).to match(/Key Identifier .+ is missing/)
     end
   end
 
   context "when requiring a specific claim" do
     subject(:parse) { described_class.new(required_claims: ["sub"]).parse(token) }
 
-    it "parses the token" do
-      parsed, = parse
-      expect(parsed).to eq payload
-    end
+    it { is_expected.to be_success }
 
     context "and when the required claim is missing" do
       before do
         payload.delete("sub")
       end
 
-      it "raises an error" do
-        expect { parse }.to raise_error(JWT::MissingRequiredClaim)
-      end
+      it { is_expected.to be_failure }
     end
   end
 end

spec/requests/api/v3/authentication_spec.rb

@@ -467,13 +467,13 @@ RSpec.describe "API V3 Authentication" do
 
         context "when access token has not expired yet" do
           context "when aud does not contain client_id" do
-            let(:token_aud) { ["master-realm", "account"] }
+            let(:token_aud) { ["Lisa", "Bart"] }
 
             it do
               get resource
 
               expect(last_response).to have_http_status :unauthorized
-              error = "The access token audience claim is wrong"
+              error = 'Invalid audience. Expected https://openproject.local, received ["Lisa", "Bart"]'
               expect(last_response.header["WWW-Authenticate"])
                 .to eq(%{Bearer realm="OpenProject API", error="invalid_token", error_description="#{error}"})
               expect(JSON.parse(last_response.body)).to eq(error_response_body)
@@ -497,7 +497,7 @@ RSpec.describe "API V3 Authentication" do
 
             expect(last_response).to have_http_status :unauthorized
             expect(last_response.header["WWW-Authenticate"])
-              .to eq(%{Bearer realm="OpenProject API", error="invalid_token", error_description="The access token expired"})
+              .to eq(%{Bearer realm="OpenProject API", error="invalid_token", error_description="Signature has expired"})
             expect(JSON.parse(last_response.body)).to eq(error_response_body)
           end
 
@@ -531,7 +531,7 @@ RSpec.describe "API V3 Authentication" do
           get resource
           expect(last_response).to have_http_status :unauthorized
           expect(JSON.parse(last_response.body)).to eq(error_response_body)
-          error = "The access token signature kid is unknown"
+          error = "The signature key ID is unknown"
           expect(last_response.header["WWW-Authenticate"])
             .to eq(%{Bearer realm="OpenProject API", error="invalid_token", error_description="#{error}"})
         end

spec/spec_helper.rb

@@ -54,4 +54,5 @@ RSpec.configure do |config|
   # Have the FactoryBot methods like #create and #build_stubbed without
   # having to call it on FactoryBot.
   config.include FactoryBot::Syntax::Methods
+  config.include Dry::Monads[:result]
 end