search

mealie-recipes / mealie

Mealie is a self hosted recipe manager and meal planner with a RestAPI backend and a reactive frontend application built in Vue for a pleasant user experience for the whole family. Easily add recipes into your database by providing the url and mealie will automatically import the relevant data or add a family recipe with the UI editor
https://docs.mealie.io
GNU Affero General Public License v3.0
6.78k stars 697 forks source link

Add time-based caching for JWKS fetching #3586

Closed cmintey closed 4 months ago

cmintey commented 4 months ago

What type of PR is this?

What this PR does / why we need it:

We currently cache the output from getting the JSON Web Key Set (JWKS) from a user's OIDC configuration indefinitely. This causes login issues when the key set is rotated and the key set that is cached is no longer valid. This PR just removed the caching on the method, which should be fine since the requests shouldn't be very heavy or that frequent.

Which issue(s) this PR fixes:

Fixes #3562

Special notes for your reviewer:

I considered switching the lru cache to a TTL cache from cachetools, but didn't see one built in and didn't want to add another dependency to the project

Testing

Manually and E2E

michael-genson commented 4 months ago

I considered switching the lru cache to a TTL cache from cachetools

I wouldn't mind adding this dependency, I've used it in other projects and it's well-maintained. However, it's also reasonably simple to implement a TTL cache with lru_cache, from https://stackoverflow.com/a/55900800:

def get_ttl_hash(seconds=3600):
    return time.time() // seconds

@lru_cache
@staticmethod
def get_jwks(ttl_hash=None) -> KeySet | None:
    """Get the key set from the open id configuration"""
    del ttl_hash  # this is unused, its purpose is to bust the lru cache
    ...

jwks = get_jwks(ttl_hash=get_ttl_hash())

Or, if you don't think it's that important to cache, I'm happy with the PR as-is. Just wanted to provide some options.

cmintey commented 4 months ago

Yeah I saw that SO post as well and considered it. Seemed like a bit of a hack, but I don't mind hacks :)

Yeah I'll implement that and have a short cache period

michael-genson commented 4 months ago

Hacky for sure, but I feel like it's easy enough to read as long as there's one or two comments in there