Refactor/OIDC custos #16497
Refactor/OIDC custos #16497
Conversation
|
|
||
| def _fetch_well_known_oidc_config(self, well_known_uri): | ||
| def _load_config(self,headers : dict = None, params : dict = None): | ||
| self.config.well_known_oidc_config_uri = self._get_well_known_uri_from_url(self.config.provider) |
There was a problem hiding this comment.
The sample config file says that the well known url can be overridden, but that does not appear to have been the case for a while. In fact, I'm wondering whether we need the url parameter at all, and it would be better to deprecate the url parameter in favour of the well_known_oidc_config_uri parameter, with some backward compatible code for converting the url to a well-known one.
There was a problem hiding this comment.
lib/galaxy/authnz/custos_authnz.py
Outdated
| @staticmethod | ||
| def GetCustosBasedAuthProvider (provider, oidc_config, oidc_backend_config, idphint=None): | ||
| # remove old entries (older than 15 minutes) | ||
| _CustosAuthBasedProvidersCache = filter(lambda x: datetime.now() - x.created_at < timedelta(minutes=15), |
There was a problem hiding this comment.
Is this to cache well-known info? If so, I think it changes rarely enough, if ever, that we can afford a restart if that info changes, so my personal preference would be to simplify further by removing the cache.
There was a problem hiding this comment.
Yes, this will cache the well known config for 15 minutes. If you don't want the cache to expire, just remove line 512. However having a cache in here makes galaxy 'repair' itself after 15 minutes of a oidc well-known change.
Not caching at all is IMHO a bad idea. Due to the design of the code the well-known is fetched every request to galaxy (by a user with an OIDC account). As a result every request to galaxy causes a request to the .well-known... That should cause quite some server load and also stall the transaction while the request of .well-known is in progress.
We could further refactor to only fetch the .well-known upon refresh/authenticate/callback but I believe that is a more fragile design than simply caching it.
There was a problem hiding this comment.
I'm not suggesting reloading on every request. I'm suggesting that it be loaded once permanently (using a module level variable). 15 minute caching can be removed. Afaik, OIDC well known info does not change, and on the remote chance that it does, an admin can just trigger a process level restart.
There was a problem hiding this comment.
To put it differently, if cache expiry has minimal benefit, it's better to remove it and simplify. If it does serve a purpose, it would be better to use a library like TTLCache from cachetools.
|
Please also look at linting failures. |
sorted. |
nuwang
force-pushed
the
refactor/oidc-custos
branch
2 times, most recently
from
8826b21 to
13b450a
Compare
…reate factory, build cache
nuwang
force-pushed
the
refactor/oidc-custos
branch
from
13b450a to
0ae47b7
Compare
nuwang
added
the
kind/refactoring
Refactoring of the whole OIDC code:
How to test the changes?
(Select all options that apply)
License