Merge pull request #18 from opf/claim-validation

Fix validation of essential claims

Author: NobodysNightmare

lib/omniauth/openid_connect/version.rb

@@ -1,5 +1,5 @@
 module OmniAuth
   module OpenIDConnect
-    VERSION = '0.4.2'
+    VERSION = '0.5.0'
   end
 end

lib/omniauth/strategies/openid_connect.rb

@@ -106,6 +106,8 @@ def callback_phase
         fail!(:timeout, e)
       rescue ::SocketError => e
         fail!(:failed_to_connect, e)
+      rescue OmniAuth::Strategies::OpenIDConnect::Claims::InvalidClaims => e
+        fail!(:claim_validation_error, e)
       end
 
       def other_phase

lib/omniauth/strategies/openid_connect/claims.rb

@@ -2,6 +2,8 @@ module OmniAuth
   module Strategies
     class OpenIDConnect
       module Claims
+        class InvalidClaims < Error; end
+
         def self.prepended(base)
           base.class_exec do
             # Additional, specific claims to be requested on top of those requested via scopes.
@@ -28,7 +30,7 @@ def self.prepended(base)
         def validate_access_token!(access_token)
           super
 
-          verify_id_token_claims! access_token
+          verify_id_token_claims!(access_token)
         end
 
         def authorize_options
@@ -73,41 +75,32 @@ def essential_claims(context)
         def verify_id_token_claims!(access_token)
           return unless claims?
 
-          id_token = decode_id_token access_token.id_token
+          id_token = decode_id_token(access_token.id_token)
 
           essential_claims(:id_token).each do |claim, request|
-            require_essential_claim! claim, Hash(request), id_token.send(claim) if id_token.respond_to? claim
+            fail_missing_claim!(claim) unless id_token.respond_to?(claim)
+
+            validate_essential_claim_value!(claim, request, id_token.public_send(claim))
           end
         end
 
-        def require_essential_claim!(claim, request, response)
-          expected_values = [request["value"]].select(&:present?).presence || claim_values(request["values"].presence)
-
-          return unless expected_values.present?
-
-          actual_values = claim_values response
-
-          return if expected_values.any? { |value| actual_values.include? value }
+        def fail_missing_claim!(name)
+          raise InvalidClaims, "Expected #{name} claim, but it was missing"
+        end
 
-          expected = expected_values.map { |v| "'#{v}'" }.join(", ")
-          actual = actual_values.map { |v| "'#{v}'" }.join(", ")
+        def validate_essential_claim_value!(claim, request, actual)
+          requested_values = requested_values(request)
+          return if requested_values.nil?
+          return if requested_values.include?(actual)
 
-          raise(
-            ::OpenIDConnect::ResponseObject::IdToken::InvalidToken,
-            "Expected one of #{claim.to_s.upcase} values [#{expected}] in [#{actual}]"
-          )
+          raise InvalidClaims, "Expected one of #{claim} values #{requested_values.inspect}, got #{actual.inspect}"
         end
 
-        ##
-        # Makes sure the given ACR values are parsed correctly as an array.
-        # They are supposed to be given as an array but in other places such as the `acr_values`
-        # request parameter they are just a string of space-separated values.
-        #
-        # @param input [String, Array<String>] ACR values either directly as an array or as a space-separated string.
-        #
-        # @return [Array<String>] An array of ACR values.
-        def claim_values(input)
-          Array(input).flat_map { |value| String(value).split(" ") }
+        def requested_values(request)
+          return [request["value"]] if request.key?("value")
+          return request["values"] if request.key?("values")
+
+          nil
         end
       end
     end

test/lib/omniauth/strategies/openid_connect_test.rb

@@ -212,18 +212,73 @@ def test_callback_phase_with_missing_claims
           }
         }
 
+        stub_id_token = lambda do |id_token|
+          id_token.stubs(:abc).returns("def")
+        end
+
+        catching_failures do
+          test_callback_phase(options: options, userinfo: false, stub_id_token: stub_id_token)
+          expect_authentication_error(
+            :claim_validation_error,
+            exception_class: OmniAuth::Strategies::OpenIDConnect::Claims::InvalidClaims,
+            message: "Expected acr claim, but it was missing"
+          )
+        end
+      end
+
+      def test_callback_phase_with_missing_claim_values
+        options = {
+          claims: {
+            id_token: {
+              acr: {
+                essential: true,
+                values: ["phr", "phrh"]
+              }
+            }
+          }
+        }
+
         stub_id_token = lambda do |id_token|
           id_token.stubs(:acr).returns([])
         end
 
-        ex = assert_raises ::OpenIDConnect::ResponseObject::IdToken::InvalidToken do
+        catching_failures do
           test_callback_phase(options: options, userinfo: false, stub_id_token: stub_id_token)
+          expect_authentication_error(
+            :claim_validation_error,
+            exception_class: OmniAuth::Strategies::OpenIDConnect::Claims::InvalidClaims,
+            message: 'Expected one of acr values ["phr", "phrh"], got []'
+          )
+        end
+      end
+
+      def test_callback_phase_with_wrong_claim_values
+        options = {
+          claims: {
+            id_token: {
+              acr: {
+                essential: true,
+                values: ["phr", "phrh"]
+              }
+            }
+          }
+        }
+
+        stub_id_token = lambda do |id_token|
+          id_token.stubs(:acr).returns("abc")
         end
 
-        assert_equal ex.message, "Expected one of ACR values ['phr', 'phrh'] in []"
+        catching_failures do
+          test_callback_phase(options: options, userinfo: false, stub_id_token: stub_id_token)
+          expect_authentication_error(
+            :claim_validation_error,
+            exception_class: OmniAuth::Strategies::OpenIDConnect::Claims::InvalidClaims,
+            message: 'Expected one of acr values ["phr", "phrh"], got "abc"'
+          )
+        end
       end
 
-      def test_callback_phase_with_returned_claims
+      def test_callback_phase_with_array_result
         options = {
           claims: {
             id_token: {
@@ -239,7 +294,84 @@ def test_callback_phase_with_returned_claims
           id_token.stubs(:acr).returns(["phr"])
         end
 
-        test_callback_phase(options: options, userinfo: true, stub_id_token: stub_id_token)
+        catching_failures do
+          # This is a regression test, we used to accept a claim response that was an array with one of the requested
+          # values, when the specification merely asks to validate that the actual value EQUALS one of the requested values
+          test_callback_phase(options: options, userinfo: false, stub_id_token: stub_id_token)
+          expect_authentication_error(
+            :claim_validation_error,
+            exception_class: OmniAuth::Strategies::OpenIDConnect::Claims::InvalidClaims,
+            message: 'Expected one of acr values ["phr", "phrh"], got ["phr"]'
+          )
+        end
+      end
+
+      def test_callback_phase_with_expected_claim_value
+        options = {
+          claims: {
+            id_token: {
+              acr: {
+                essential: true,
+                values: ["phr", "phrh"]
+              }
+            }
+          }
+        }
+
+        stub_id_token = lambda do |id_token|
+          id_token.stubs(:acr).returns("phr")
+        end
+
+        catching_failures do
+          test_callback_phase(options: options, userinfo: true, stub_id_token: stub_id_token)
+          expect_no_authentication_error
+        end
+      end
+
+      def test_callback_phase_with_missing_essential_claims
+        options = {
+          claims: {
+            id_token: {
+              abc: {
+                essential: true
+              }
+            }
+          }
+        }
+
+        stub_id_token = lambda do |id_token|
+          id_token.stubs(:def).returns("ghi")
+        end
+
+        catching_failures do
+          test_callback_phase(options: options, userinfo: false, stub_id_token: stub_id_token)
+          expect_authentication_error(
+            :claim_validation_error,
+            exception_class: OmniAuth::Strategies::OpenIDConnect::Claims::InvalidClaims,
+            message: "Expected abc claim, but it was missing"
+          )
+        end
+      end
+
+      def test_callback_phase_with_present_essential_claims
+        options = {
+          claims: {
+            id_token: {
+              abc: {
+                essential: true
+              }
+            }
+          }
+        }
+
+        stub_id_token = lambda do |id_token|
+          id_token.stubs(:abc).returns("def")
+        end
+
+        catching_failures do
+          test_callback_phase(options: options, userinfo: true, stub_id_token: stub_id_token)
+          expect_no_authentication_error
+        end
       end
 
       def test_callback_phase_with_discovery

test/strategy_test_case.rb

@@ -51,4 +51,24 @@ def strategy
       strategy.stubs(:user_info).returns(user_info)
     end
   end
+
+  # Allows to test that properly handled authentication errors occured within the scope of the passed block
+  def catching_failures
+    original_failure_handler = OmniAuth.config.on_failure
+    @failure_env = nil
+    OmniAuth.config.on_failure = ->(env) { @failure_env = env; [400, {}, "sad sad"] }
+    yield
+  ensure
+    OmniAuth.config.on_failure = original_failure_handler
+  end
+
+  def expect_authentication_error(type, exception_class: nil, message: nil)
+    assert_equal(@failure_env["omniauth.error.type"], type)
+    assert_equal(@failure_env["omniauth.error"].class, exception_class) if exception_class
+    assert_equal(@failure_env["omniauth.error"]&.message, message) if message
+  end
+
+  def expect_no_authentication_error
+    assert(@failure_env.nil?, "expected no authentication errors")
+  end
 end