Repository navigation
feat: add server-side token storage with OBO caching - #136
kishore7snehil wants to merge 5 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return "domains_resolver_error" | ||
|
|
||
|
|
||
| class TokenStoreError(BaseAuthError): |
There was a problem hiding this comment.
TokenStoreError is in __all__ so it is a public type, but it is never actually raised anywhere.
In all the best effort paths we build it and then only log its .cause, so a consumer who writes except TokenStoreError will never catch anything from this SDK. Can we either keep it internal (drop it from __all__) or actually raise it? Also cause: Exception = None on line 182 should be Optional[Exception].
|
|
||
| claims = await api_client.verify_access_token(access_token=incoming_access_token) | ||
|
|
||
| result = await api_client.get_token_on_behalf_of( |
There was a problem hiding this comment.
We call verify_access_token just above and then call get_token_on_behalf_of without passing verified=, so claims is unused and, since a token_store is set, the token gets verified a second time inside.
Can we pass verified=VerifiedToken(access_token=incoming_access_token, claims=claims) here? That also shows the new verified entry point, which is the main thing a caller who already verified would use.
| if not isinstance(sub, str) or not sub: | ||
| logging.warning("Verified token has no usable sub claim, skipping cache") | ||
| return None | ||
| issuer = claims.get("iss") |
There was a problem hiding this comment.
Just noting here, when a caller passes verified but the claims do not carry both sub and iss, we skip caching and only log a warning. The verified docstring does not mention that iss and sub must be present, so someone passing a minimal claims mapping will silently get no caching.
Can we document the required claim keys, and maybe add a test for the iss missing case? Only the sub missing case is covered right now.
|
|
||
| async def set(self, key: str, value: TokenSet) -> None: | ||
| encrypted = self.encrypt(key, value) | ||
| ttl = max(value["expires_at"] - int(time.time()), 0) |
There was a problem hiding this comment.
Small bug in the reference Redis store. ttl can come out as 0 here, and redis.set(..., ex=0) is rejected by Redis, so a copy paste of this store will raise on writes of a token whose remaining life rounds to zero.
Writes are best effort so the call still succeeds, but caching quietly breaks for those entries. Can we clamp to max(..., 1)?
| ) | ||
| if ( | ||
| options.scope_matching == "non_strict" | ||
| and options.token_store is not None |
There was a problem hiding this comment.
This guard has and options.token_store is not None, so if someone sets scope_matching='non_strict' but forgets to pass a token_store, we neither raise nor cache, it just silently does nothing.
Can we either raise ConfigurationError for that case as well, or at least warn that caching will be inert?
| claims = verified.claims | ||
| sub = claims.get("sub") | ||
| if not isinstance(sub, str) or not sub: | ||
| logging.warning("Verified token has no usable sub claim, skipping cache") |
There was a problem hiding this comment.
Small one, all these warnings go through logging.warning(...) on the root logger, so they cannot be filtered by name.
Under a non_strict store outage the per member read path can also emit one warning per index member per lookup.
Can we switch to logging.getLogger(__name__) and maybe de dup the per member warnings?
| """ | ||
| try: | ||
| payload = get_unverified_payload(access_token) | ||
| except ValueError: |
There was a problem hiding this comment.
Nit:
This only catches ValueError. If the token payload decodes to valid JSON that is not an object, payload.get("sid") raises AttributeError, which is not caught here, so the intended sha256 fallback gets skipped. Not reachable in the normal verified token path, but an isinstance(payload, dict) check or a wider guard would be safer.
| self, identity: _OboIdentity, audience: str, scope: Optional[str] | ||
| ) -> Optional[OnBehalfOfTokenResult]: | ||
| # An unscoped request is covered by every cached token, so never reuse for one. | ||
| if not scope: |
There was a problem hiding this comment.
Nit:
if not scope is false for a value like " ", which then normalizes to an empty set and gets treated as covered by every cached token. Very unlikely input, but normalizing before this guard would close it.
| return cached["expires_at"] > now | ||
|
|
||
| @staticmethod | ||
| def _hit(cached: TokenSet) -> OnBehalfOfTokenResult: |
There was a problem hiding this comment.
Nit:
int(time.time()) is computed here separately from the usable entry check, so a token that was valid at the check can still come out with expires_in = 0 at a second boundary.
Can we compute now once and reuse it?
| """Base class for external token stores with built-in JWE encrypt and decrypt helpers.""" | ||
|
|
||
| def __init__(self, *, secret: str) -> None: | ||
| self._secret = secret |
There was a problem hiding this comment.
Nit:
Any secret is accepted here including an empty string. Might be worth a minimum length check, or at least documenting the expectation.
I didn't double check if we handle this in other SDKs. PTAL.
8f87351 to
f4b287d
Compare
42680cd to
d57457d
Compare
Add pluggable server-side token storage and use it to cache On Behalf Of exchanges. When a token_store is configured, get_token_on_behalf_of() reuses previously exchanged tokens instead of hitting the token endpoint on every call. - AbstractTokenStore ABC with JWE at-rest encryption helpers - IndexedTokenStore for non-strict scope matching - OBO cache wiring in get_token_on_behalf_of() - scope_matching option: strict (exact match) or non_strict (coverage) - TokenSet, TokenIndexMember TypedDicts and VerifiedToken dataclass - TokenStoreError (status 500) for backend failures
f4b287d to
ae99659
Compare
d57457d to
75faa2d
Compare
This branch has not been deployed
3 participants
📋 Changes
This PR adds pluggable server-side token storage to auth0-api-python and uses it to cache On Behalf Of exchanges. When a token store is configured,
get_token_on_behalf_of()(already in main from a prior PR) reuses previously exchanged tokens instead of hitting the token endpoint on every call.✨ Features
AbstractTokenStoreABC for server-side token persistence, with JWE-at-rest encrypt and decrypt helpers built in. Subclasses implementget,set, anddelete.IndexedTokenStorevariant that maintains a scope index, required for non-strict scope matching.get_token_on_behalf_of()now caches exchanged tokens whentoken_storeis set.scope_matchingoption. "strict" reuses a cached token only on an exact scope match. "non_strict" reuses a cached token when its granted scopes cover the request, and requires anIndexedTokenStore.encryption.pymodule providing the JWE helpers used by the store.TokenSetandTokenIndexMemberTypedDicts and aVerifiedTokendataclass.🔧 API Changes
token_store(AbstractTokenStore, defaultNone) toApiClientOptions. When set, OBO exchanges are cached.scope_matching(str, default"strict") toApiClientOptions. Using "non_strict" without anIndexedTokenStoreraisesConfigurationError.AbstractTokenStore,IndexedTokenStoreTokenSet(TypedDict),TokenIndexMember(TypedDict),VerifiedToken(dataclass)TokenStoreError(status 500), raised when the configured store backend failsGetTokenByExchangeProfileError📖 Documentation
docs/TokenStorage.mdcovering theAbstractTokenStorecontract, a Redis implementation example, and the scope matching modesREADME.mdwith a token storage sectionEXAMPLES.mdwith token storage and caching examples🧪 Testing
Contributor Checklist