Issue 4 - https://github.com/thenewguy/django-allauth-adfs/issues/4

[mattloomis]

Fix for Issue 4 - https://github.com/thenewguy/django-allauth-adfs/issues/4

This PR gets the signature method algorithm from the Federation Metadata XML, and sends it to the JWT signature verification method if it is required (for jwt lib > 2.x)

[mattloomis]

We should probably update the cache key to include JWT_ALGORITHM_REQUIRED since the format of the cache value changed

Hmm, in this PR, it would always get the signing method algorithm and always store a dict (with both the key bytes and any detected 'supported' algorithm) in the cache regardless of the value of JWT_ALGORITHM_REQUIRED. When verifying the JWT signature it would always get the dict back out of the cache, but conditionally send the "supported algorithms" list to the jwt.decode() when JWT_ALGORITHM_REQUIRED is true.

I could be misunderstanding your comment though. Happy to include the value of JWT_ALGORITHM_REQUIRED in the cache key string, but I don't think that being true/false has an impact on what is in the cache? Also assuming that if someone is updating the version of the jwt library (or really updating the version of this django-allauth-adfs library that uses this new approach), the entire process is being restarted and the cache is starting from empty, so there wouldn't be a concern of an old cache value (only the key bytes, not a dict) being returned to this new code?

[thenewguy]

Also assuming that if someone is updating the version of the jwt library (or really updating the version of this django-allauth-adfs library that uses this new approach), the entire process is being restarted and the cache is starting from empty, so there wouldn't be a concern of an old cache value (only the key bytes, not a dict) being returned to this new code?

Yes I am talking about upgrading. The cache is currently full of public keys. For some sites this is used on, upgrading to a new version won't necessarily purge the cache - so they would end up with the key bytes, not a dict and break.

[mattloomis]

Also assuming that if someone is updating the version of the jwt library (or really updating the version of this django-allauth-adfs library that uses this new approach), the entire process is being restarted and the cache is starting from empty, so there wouldn't be a concern of an old cache value (only the key bytes, not a dict) being returned to this new code?

Yes I am talking about upgrading. The cache is currently full of public keys. For some sites this is used on, upgrading to a new version won't necessarily purge the cache - so they would end up with the key bytes, not a dict and break.

Ah... Now I see that the cache is the Django cache.. so yeah, could be using Redis/DB/etc. gotcha. Instead of tying it to JWT_ALGORITHM_REQUIRED, maybe it is useful to introduce a general CACHE_VALUE_FORMAT_VERSION = "v1" attribute and then use that in the cache key string like:

        cache_key = ":".join([
            "allauth_adfs",
            "ADFSOAuth2Adapter",
            CACHE_VALUE_FORMAT_VERSION,
            md5(hashable_url).hexdigest(),
            "token_signature_key",
        ])

In case the cache value format changes again in a future version, this could just be incremented to CACHE_VALUE_FORMAT_VERSION = "v2", etc.?

[thenewguy]

In case the cache value format changes again in a future version, this could just be incremented to CACHE_VALUE_FORMAT_VERSION = "v2", etc.?

That works!

[thenewguy]

@mattloomis I took another look this morning and realized the "version" token that we added is unnecessary. Was busy yesterday and didn't have an opportunity to read the file as a whole.

Let's revert the CACHE_VALUE_FORMAT_VERSION change. We should change

        cache_key = ":".join([
            "allauth_adfs",
            "ADFSOAuth2Adapter",
            md5(hashable_url).hexdigest(),
            "token_signature_key",
        ])

to

        cache_key = ":".join([
            "allauth_adfs",
            "ADFSOAuth2Adapter",
            md5(hashable_url).hexdigest(),
            "token_signature",
        ])

since you changed the property from token_signature_key to token_signature and that resolves the issue.

Let me know this works for you in a production environment and I will merge. Thanks for your contribution!

[mattloomis]

since you changed the property from token_signature_key to token_signature and that resolves the issue.

Let me know this works for you in a production environment and I will merge. Thanks for your contribution!

Make sense to me. Made the update.

The code prior to these cache key string changes has been working in a production environment already.

Let me know if you need anything else from me. Thanks for starting this lib in the first place!

[mattloomis]

@thenewguy - 💯 . Do you have a cadence of when you would publish a new package version to pypi?

[thenewguy]

@mattloomis published now

[mattloomis]

@mattloomis published now

🥳