ONYX-11330: Fix cache of signing key

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
+### Fixed
+- Fix bug of cache not working in authn jwt. [ONYX-11330](https://ca-il-jira.il.cyber-ark.com:8443/browse/ONYX-11330)
+
 ## [1.13.0] - 2021-07-29
 
 ### Added

app/domain/authentication/authn_jwt/signing_key/create_signing_key_factory.rb → app/domain/authentication/authn_jwt/signing_key/create_signing_key_provider.rb

@@ -2,7 +2,7 @@ module Authentication
   module AuthnJwt
     module SigningKey
       # Factory that returns the interface implementation of FetchSigningKey
-      CreateSigningKeyFactory ||= CommandClass.new(
+      CreateSigningKeyProvider ||= CommandClass.new(
         dependencies: {
           fetch_provider_uri_signing_key_class: Authentication::AuthnJwt::SigningKey::FetchProviderUriSigningKey,
           fetch_jwks_uri_signing_key_class: Authentication::AuthnJwt::SigningKey::FetchJwksUriSigningKey,
@@ -13,12 +13,12 @@ module SigningKey
         def call
           @logger.debug(LogMessages::Authentication::AuthnJwt::SelectingSigningKeyInterface.new)
           validate_key_configuration
-          create_signing_key
+          create_signing_key_provider
         end
 
         private
 
-        def create_signing_key
+        def create_signing_key_provider
           if provider_uri_has_valid_configuration?
             @logger.info(
               LogMessages::Authentication::AuthnJwt::SelectedSigningKeyInterface.new(PROVIDER_URI_INTERFACE_NAME)

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb

@@ -0,0 +1,25 @@
+module Authentication
+  module AuthnJwt
+    module SigningKey
+      # FetchCachedSigningKey is a wrapper of FetchSigningKeyInterface interface,
+      # in order to be able to store the signing key in our cache mechanism. If signing_key_interface don't have
+      # fetch_signing_key it is extreme case that error need to be raised so it can be investigated so reek will ignore
+      # this.
+      # :reek:InstanceVariableAssumption
+      FetchCachedSigningKey = CommandClass.new(
+        dependencies: {},
+        inputs: %i[signing_key_provider]
+      ) do
+        def call
+          fetch_signing_key
+        end
+
+        private
+
+        def fetch_signing_key
+          @signing_key_provider.fetch_signing_key
+        end
+      end
+    end
+  end
+end

app/domain/authentication/authn_jwt/signing_key/fetch_jwks_uri_signing_key.rb

@@ -31,11 +31,15 @@ def valid_configuration?
           @valid_configuration = jwks_uri_resource_exists?
         end
 
-        def call
+        def fetch_signing_key
           fetch_jwks_uri
           fetch_jwks_keys
         end
 
+        def signing_key_uri
+          jwks_uri
+        end
+
         private
 
         def jwks_uri_resource_exists?

app/domain/authentication/authn_jwt/signing_key/fetch_provider_uri_signing_key.rb

@@ -25,11 +25,15 @@ def valid_configuration?
           @valid_configuration = provider_uri_resource_exists?
         end
 
-        def call
+        def fetch_signing_key
           discover_provider
           fetch_provider_keys
         end
 
+        def signing_key_uri
+          provider_uri
+        end
+
         private
 
         def provider_uri_resource_exists?

app/domain/authentication/authn_jwt/signing_key/fetch_signing_key_interface.rb

@@ -2,7 +2,7 @@ module Authentication
   module AuthnJwt
     module SigningKey
       class FetchSigningKeyInterface
-        def call; end
+        def fetch_signing_key; end
 
         def valid_configuration?; end
       end

app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb

@@ -7,12 +7,23 @@ module ValidateAndDecode
       # for the 2nd validation
       ValidateAndDecodeToken ||= CommandClass.new(
         dependencies: {
+          fetch_signing_key: ::Util::ConcurrencyLimitedCache.new(
+            ::Util::RateLimitedCache.new(
+              ::Authentication::AuthnJwt::SigningKey::FetchCachedSigningKey.new,
+              refreshes_per_interval: CACHE_REFRESHES_PER_INTERVAL,
+              rate_limit_interval: CACHE_RATE_LIMIT_INTERVAL,
+              logger: Rails.logger
+            ),
+            max_concurrent_requests: CACHE_MAX_CONCURRENT_REQUESTS,
+            logger: Rails.logger
+          ),
           verify_and_decode_token: ::Authentication::Jwt::VerifyAndDecodeToken.new,
           fetch_jwt_claims_to_validate: ::Authentication::AuthnJwt::ValidateAndDecode::FetchJwtClaimsToValidate.new,
           get_verification_option_by_jwt_claim: ::Authentication::AuthnJwt::ValidateAndDecode::GetVerificationOptionByJwtClaim.new,
+          create_signing_key_provider: ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyProvider.new,
           logger: Rails.logger
         },
-        inputs: %i[authentication_parameters fetch_signing_key]
+        inputs: %i[authentication_parameters]
       ) do
         extend(Forwardable)
         def_delegators(:@authentication_parameters, :jwt_token)
@@ -31,12 +42,22 @@ def call
 
         private
 
+        def signing_key_provider
+          @signing_key_provider ||= @create_signing_key_provider.call(
+            authentication_parameters: @authentication_parameters
+          )
+        end
+
         def validate_token_exists
           raise Errors::Authentication::AuthnJwt::MissingToken if jwt_token.blank?
         end
 
         def fetch_signing_key(force_read: false)
-          @jwks = @fetch_signing_key.call(refresh: force_read)
+          @jwks = @fetch_signing_key.call(
+            refresh: force_read,
+            cache_key: signing_key_provider.signing_key_uri,
+            signing_key_provider: signing_key_provider
+          )
           @logger.debug(LogMessages::Authentication::AuthnJwt::SigningKeysFetchedFromCache.new)
         end
 

app/domain/authentication/authn_jwt/validate_status.rb

@@ -3,7 +3,17 @@ module AuthnJwt
 
     ValidateStatus = CommandClass.new(
       dependencies: {
-        create_signing_key_interface: Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new,
+        fetch_signing_key: ::Util::ConcurrencyLimitedCache.new(
+          ::Util::RateLimitedCache.new(
+            ::Authentication::AuthnJwt::SigningKey::FetchCachedSigningKey.new,
+            refreshes_per_interval: CACHE_REFRESHES_PER_INTERVAL,
+            rate_limit_interval: CACHE_RATE_LIMIT_INTERVAL,
+            logger: Rails.logger
+          ),
+          max_concurrent_requests: CACHE_MAX_CONCURRENT_REQUESTS,
+          logger: Rails.logger
+        ),
+        create_signing_key_provider: Authentication::AuthnJwt::SigningKey::CreateSigningKeyProvider.new,
         fetch_issuer_value: Authentication::AuthnJwt::ValidateAndDecode::FetchIssuerValue.new,
         fetch_audience_value: Authentication::AuthnJwt::ValidateAndDecode::FetchAudienceValue.new,
         fetch_enforced_claims: Authentication::AuthnJwt::RestrictionValidation::FetchEnforcedClaims.new,
@@ -134,25 +144,15 @@ def webservice
       end
 
       def validate_signing_key
-        fetch_signing_key.call
-        @logger.debug(LogMessages::Authentication::AuthnJwt::ValidatedSigningKeyConfiguration.new)
-      end
-
-      def fetch_signing_key
-        @fetch_signing_key ||= ::Util::ConcurrencyLimitedCache.new(
-          ::Util::RateLimitedCache.new(
-            fetch_signing_key_interface,
-            refreshes_per_interval: ::Authentication::AuthnJwt::CACHE_REFRESHES_PER_INTERVAL,
-            rate_limit_interval: ::Authentication::AuthnJwt::CACHE_RATE_LIMIT_INTERVAL,
-            logger: @logger
-          ),
-          max_concurrent_requests: ::Authentication::AuthnJwt::CACHE_MAX_CONCURRENT_REQUESTS,
-          logger: @logger
+        @fetch_signing_key.call(
+          cache_key: signing_key_provider.signing_key_uri,
+          signing_key_provider: signing_key_provider
         )
+        @logger.debug(LogMessages::Authentication::AuthnJwt::ValidatedSigningKeyConfiguration.new)
       end
-
-      def fetch_signing_key_interface
-        @fetch_signing_key_interface ||= @create_signing_key_interface.call(
+      
+      def signing_key_provider
+        @signing_key_provider ||= @create_signing_key_provider.call(
           authentication_parameters: authentication_parameters
         )
       end

app/domain/authentication/authn_jwt/vendor_configurations/configuration_jwt_generic_vendor.rb

@@ -10,22 +10,22 @@ def initialize(
           logger: Rails.logger,
           authentication_parameters_class: Authentication::AuthnJwt::AuthenticationParameters,
           restriction_validator_class: Authentication::AuthnJwt::RestrictionValidation::ValidateRestrictionsOneToOne,
-          validate_and_decode_token_class: Authentication::AuthnJwt::ValidateAndDecode::ValidateAndDecodeToken,
           validate_resource_restrictions_class: Authentication::ResourceRestrictions::ValidateResourceRestrictions,
           extract_token_from_credentials: Authentication::AuthnJwt::InputValidation::ExtractTokenFromCredentials.new,
           create_identity_provider: Authentication::AuthnJwt::IdentityProviders::CreateIdentityProvider.new,
           create_constraints: Authentication::AuthnJwt::RestrictionValidation::CreateConstrains.new,
-          fetch_mapping_claims: Authentication::AuthnJwt::RestrictionValidation::FetchMappingClaims.new
+          fetch_mapping_claims: Authentication::AuthnJwt::RestrictionValidation::FetchMappingClaims.new,
+          validate_and_decode_token: Authentication::AuthnJwt::ValidateAndDecode::ValidateAndDecodeToken.new
         )
           @logger = logger
           @authentication_parameters_class = authentication_parameters_class
           @restriction_validator_class = restriction_validator_class
-          @validate_and_decode_token_class = validate_and_decode_token_class
           @validate_resource_restrictions_class = validate_resource_restrictions_class
           @extract_token_from_credentials = extract_token_from_credentials
           @create_identity_provider = create_identity_provider
           @create_constraints = create_constraints
           @fetch_mapping_claims = fetch_mapping_claims
+          @validate_and_decode_token = validate_and_decode_token
 
           @logger.debug(LogMessages::Authentication::AuthnJwt::CreatingAuthenticationParametersObject.new)
           @authentication_parameters = @authentication_parameters_class.new(
@@ -55,9 +55,8 @@ def validate_restrictions
         end
 
         def validate_and_decode_token
-          @authentication_parameters.decoded_token = validate_and_decode_token_instance.call(
-            authentication_parameters: @authentication_parameters,
-            fetch_signing_key: fetch_signing_key
+          @authentication_parameters.decoded_token = @validate_and_decode_token.call(
+            authentication_parameters: @authentication_parameters
           )
         end
 
@@ -83,51 +82,13 @@ def identity_provider
           )
         end
 
-        def validate_and_decode_token_instance
-          return @validate_and_decode_token_instance if @validate_and_decode_token_instance
-
-          @logger.debug(LogMessages::Authentication::AuthnJwt::CreateValidateAndDecodeTokenInstance.new)
-          @validate_and_decode_token_instance = @validate_and_decode_token_class.new(
-            fetch_jwt_claims_to_validate: fetch_jwt_claims_to_validate
-          )
-          @logger.debug(LogMessages::Authentication::AuthnJwt::CreatedValidateAndDecodeTokenInstance.new)
-          @validate_and_decode_token_instance
-        end
-
-        def fetch_signing_key
-          @fetch_signing_key ||= ::Util::ConcurrencyLimitedCache.new(
-            ::Util::RateLimitedCache.new(
-              fetch_signing_key_interface,
-              refreshes_per_interval: CACHE_REFRESHES_PER_INTERVAL,
-              rate_limit_interval: CACHE_RATE_LIMIT_INTERVAL,
-              logger: @logger
-            ),
-            max_concurrent_requests: CACHE_MAX_CONCURRENT_REQUESTS,
-            logger: @logger
-          )
-        end
-
         def create_identity_provider
           @logger.debug(LogMessages::Authentication::AuthnJwt::CreateJwtIdentityProviderInstance.new)
           @create_identity_provider ||= @create_identity_provider
           @logger.debug(LogMessages::Authentication::AuthnJwt::CreatedJwtIdentityProviderInstance.new)
           @create_identity_provider
         end
 
-        def fetch_signing_key_interface
-          @fetch_signing_key_interface ||= create_signing_key_interface.call(
-            authentication_parameters: @authentication_parameters
-          )
-        end
-
-        def create_signing_key_interface
-          @create_signing_key_interface ||= Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new
-        end
-
-        def fetch_jwt_claims_to_validate
-          @fetch_jwt_claims_to_validate ||= ::Authentication::AuthnJwt::ValidateAndDecode::FetchJwtClaimsToValidate.new
-        end
-
         def restrictions_from_annotations
           @restrictions_from_annotations ||= Authentication::ResourceRestrictions::GetServiceSpecificRestrictionFromAnnotation.new
         end

app/domain/logs.rb

@@ -712,6 +712,8 @@ module AuthnJwt
     end
   end
 
+  # This are log messages so its okay there are many
+  # :reek:TooManyConstants
   module Util
 
     RateLimitedCacheUpdated = ::Util::TrackableLogMessageClass.new(
@@ -740,5 +742,15 @@ module Util
       code: "CONJ00023D"
     )
 
+    ConcurrencyLimitedCacheKeyRetrieved = ::Util::TrackableLogMessageClass.new(
+      msg: "ConcurrencyLimitedCache retrieved cache key : '{0-cache-key}'",
+      code: "CONJ00024D"
+    )
+
+    RateLimitedCacheKeyRetrieved = ::Util::TrackableLogMessageClass.new(
+      msg: "RateLimitedCache Retrieved cache key : '{0-cache-key}'",
+      code: "CONJ00025D"
+    )
+
   end
 end

app/domain/util/concurrency_limited_cache.rb

@@ -20,15 +20,20 @@ def initialize(
     # This  method is passed exactly the same named arguments you'd pass to the
     # callable object, but you can optionally include the `refresh: true` to
     # force recalculation.
+    #
+    # In this case we should call @cache[cache_key] because setting it to variable requires treatment of case cache_key
+    # not in case before the flow runs and it can cause unexpected behaviour so reek will ignore this.
+    # reek:DuplicateMethodCall
     def call(**args)
+      cache_key = cache_key(args)
       @concurrency_mutex.synchronize do
         if @concurrent_requests >= @max_concurrent_requests
           @logger.debug(
             LogMessages::Util::ConcurrencyLimitedCacheReached.new(@max_concurrent_requests)
           )
-          raise Errors::Util::ConcurrencyLimitReachedBeforeCacheInitialization unless @cache.key?(args)
+          raise Errors::Util::ConcurrencyLimitReachedBeforeCacheInitialization unless @cache.key?(cache_key)
 
-          return @cache[args]
+          return @cache[cache_key]
         end
 
         @concurrent_requests += 1
@@ -40,15 +45,15 @@ def call(**args)
       end
 
       @semaphore.synchronize do
-        recalculate(args)
-        @cache[args]
+        recalculate(args, cache_key)
+        @cache[cache_key]
       end
     end
 
     private
 
-    def recalculate(args)
-      @cache[args] = @target.call(**args)
+    def recalculate(args, cache_key)
+      @cache[cache_key] = @target.call(**args)
       @logger.debug(LogMessages::Util::ConcurrencyLimitedCacheUpdated.new)
       decrease_concurrent_requests
     rescue => e
@@ -66,5 +71,20 @@ def decrease_concurrent_requests
         )
       end
     end
+
+    # Function returning cache key to store/retrieve in the cache
+    def cache_key(args)
+      if args.key?(:cache_key)
+        cache_key = args.fetch(:cache_key)
+        @logger.debug(
+          LogMessages::Util::ConcurrencyLimitedCacheKeyRetrieved.new(
+            cache_key
+          )
+        )
+      else
+        cache_key = args
+      end
+      cache_key
+    end
   end
 end