Cache key names for Identity Servers AddResourceStoreCache and AddCorsPolicyCache are badly formatted

[HitDaCa]

The private string GetKey(string key) method inside the public class RedisCache<T> : ICache<T> where T : class uses ...typeof (T).FullName... to generate part of the Cache key name.

This is problematic as Identity Server 4 (v.2.3.2) passes System.Collections.Generic.IEnumerable<T> objects for both Resources and CorsPolicies. This currently results in malformed cache key names looking like: prefix:System.Collections.Generic.IEnumerable [[IdentityServer4.Models.ApiResource, IdentityServer4.Storage, Version=2.3.1.0, Culture=neutral, PublicKeyToken=21d4F37H89d2s0F9]]:identifier

At bare minimum the PublicKeyToken will change when ever the assembly changes which prevents from building any features needing to access / modify preset keys.

Ideally this method should acknowledge that it is handling a generic type which could take any possible format. Alternatively the key name generation function could be generically injected or at least be marked as protected so it can be overridden.

I'll try and put together a fix and issue a pull request if I can find some time over the next days :-).

[AliBazzi]

Hi @HitDaCa

My take here is that if it's a cache, and an upgrade to version number happened that would still be fine, since it will be a cache miss, and the library will cache again the value with the new key.

Can you elaborate more why this is problematic ? keen to understand more about a specific scenario where you find this would introduce problem, given it's a caching layer.

Using typeof(T).Name instead of FullName would result a key that looks like: prefix:System.Collections.Generic.IEnumerable:identifier for both collections, which will have a collision, that's why FullName was necessary here.

[HitDaCa]

Hi @AliBazzi

Thanks for your response! For most general quick caching solution use cases I agree with you, however we are specifically targeting caching support for authentication/authorization systems. These most often require the ability to pretty instantly apply critical changes which lead to the requirement of cache invalidation.

Now assuming we are working on a good old monolith application, e.g. the administrative interface is directly baked into the original OIDC project structure (from a code perspective), we could easily just target and overwrite any existing cache key with a shortest possible expiry (this library implements GetKey/SetKey operations) as we are using the same assemblies. However in a more service based or microservice orientated architecture, we very well may neither have access to the original Identity Server assemblies or are likely to be using other Redis libraries to access a globally shared caching service (think public cloud).

A concrete example would be as simple as a loosely coupled administrative api service which handles CRUD operations to the underlying Identity Service data storage objects. Without reconstructable key paths such a basic service already has no way to identify the effected cache keys which need to be purged. For more clarification, <prefix>:IdentityServer4.Models.Client:<clientId> is a great example for a good and easily reconstructable key name path. Redis itself doesn't provide a complex search API so all left in the cases like AddResourceStoreCache and AddCorsPolicyCache is to get all keys and iterate over them until partial identifiers in the key-names can be matched. Redis advises against such practices in production environments (sorry, I'll have to look up the source for this...) due to the load created when retrieving all keys. This leaves the only viable alternative option to be purging the whole cache database when changing a single resource.

Don't get me wrong here, I think that using FullName is good default option, however there definitely needs to be an option to override this default functionality to provide specific naming for this part when required. There will also be many other ways of solving this, however the core issue to resolve is the requirement to have externally reconstructable cache key name. This is currently not provided as the a number of values in the actual key-name will in future change in an unpredictable manner. In summary the format should be something in the line of prefix:<never-changing-but-unique-to-the-input-type>:identifier.

[AliBazzi]

Agree with you @HitDaCa on possible use cases, myself I didn't find something useful that will not cause collision for System.Collections.Generic.IEnumerable case in the .net framework, my conclusion back then was to define a custom extension method on the Type class to exclude assembly name and version from the key name, but didn't do that and proceeded with FullName.

looking forward for your PR !

[AliBazzi]

I will close this issue until you provide a PoC as a PR.