OAuth cache improvements #435
Conversation
hovaesco
force-pushed
the
hovaesco/oauth-cache
branch
from
5632003
to
292bd0f
Compare
trino/auth.py
Outdated
|
|
||
| @staticmethod | ||
| def _construct_cache_key(host: Optional[str], user: Optional[str]) -> str: | ||
| return f"{host}@{user}" |
There was a problem hiding this comment.
usually people expect stuff to be user@host but it doesn't matter here since it's an internal cache key (unless we log it somewhere)
|
If there is no backend available then keyring backend is being initialized as |
|
|
||
| @staticmethod | ||
| def _determine_host(url: Optional[str]) -> Any: | ||
| return urlparse(url).hostname | ||
|
|
||
| @staticmethod | ||
| def _determine_user(headers: Mapping[Any, Any]) -> Optional[Any]: | ||
| return headers.get(HEADER_USER) |
There was a problem hiding this comment.
X-Trino-User header is optional
There was a problem hiding this comment.
You're right, is there any other way to get username?
There was a problem hiding this comment.
Since the actual user might come from user mapping then getting it from the server is probably the only option.
There was a problem hiding this comment.
maybe we can use the client provided principal/user instead of user which it gets resolved to on the server to bypass this problem entirely?
hovaesco
force-pushed
the
hovaesco/oauth-cache
branch
from
292bd0f
to
8c4c75f
Compare
| """ | ||
|
|
||
| def __init__(self) -> None: | ||
| self._cache: Dict[Optional[str], str] = {} | ||
|
|
||
| def get_token_from_cache(self, host: Optional[str]) -> Optional[str]: | ||
| return self._cache.get(host) | ||
| def get_token_from_cache(self, key: Optional[str]) -> Optional[str]: |
There was a problem hiding this comment.
if you extract the host -> key rename to it's own commit functional changes will be more visible and easier to see.
hovaesco
force-pushed
the
hovaesco/oauth-cache
branch
from
8c4c75f
to
9064581
Compare
There was a problem hiding this comment.
LGTM.
@lukasz-walkiewicz do you want to take a look?
We now do like JDBC driver, if username was specified we use host+user cache key otherwise we use only host as the cache key and tokens get shared (same as JDBC driver).
Description
Closes #430 and #431
Non-technical explanation
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: