Question
What is OidcTokenParser._validate_token intended to verify? As written it cannot reject any token, but it reads like a security gate and logs as though one passed, so I would rather ask than assume it is dead code.
What it does today
_validate_token builds a mock request, sets Authorization: Bearer <token> on it from the token it was handed, and awaits OAuth2AuthorizationCodeBearer. That scheme only parses the header and returns the token; it never inspects the value. Since the method supplies the header itself, the check always succeeds.
Verified against the installed fastapi (0.139.2) — every one of these is accepted, including the empty string:
'' -> ACCEPTED ''
' ' -> ACCEPTED ''
'garbage' -> ACCEPTED 'garbage'
'a.b.c' -> ACCEPTED 'a.b.c'
The caller then logs success:
await self._validate_token(access_token)
logger.debug("Token successfully validated.")
Real verification happens afterwards in _decode_token, which fetches the JWKS signing key and calls jwt.decode. So the token is genuinely validated, just not here. My concern is that the method name, the docstring ("Validate the token ... against the OAuth2 server"), and that log line together suggest a verification step that does not exist, which is the kind of thing that misleads a future reader auditing the auth path.
The one thing it does affect
Constructing the scheme reads token_endpoint and authorization_endpoint from the discovery document, so a discovery document missing token_endpoint raises a ValidationError and the request fails (an empty string passes). That is the only input that can make this method fail.
Worth noting the server-side validation path itself never needs a token endpoint — it only needs jwks_uri. So this effectively requires a discovery field that Feast's own token validation does not use. It also does not trigger anything that would otherwise be skipped: _decode_token reaches the same lazily-fetched discovery data via get_jwks_url().
Why I am asking rather than sending a patch
Removing the call would be a behavior change for anyone whose provider omits token_endpoint (they would go from failing to working), and it is the only await on this branch of the OIDC path, which matters if the blocking JWKS work is ever moved to a thread. Both seem like your call rather than mine.
Roughly the options I see:
- It is vestigial, and the call plus the misleading log line can go.
- The discovery-document shape check is wanted, in which case it would be clearer done once at parser construction rather than per request, and against the fields actually required.
- It was meant to do something else entirely (introspection against the provider?) and never did, in which case that is the real issue.
Happy to send a PR for whichever you prefer. Not urgent, and the per-request cost is small; this is about the auth path saying it does something it does not.
Question
What is
OidcTokenParser._validate_tokenintended to verify? As written it cannot reject any token, but it reads like a security gate and logs as though one passed, so I would rather ask than assume it is dead code.What it does today
_validate_tokenbuilds a mock request, setsAuthorization: Bearer <token>on it from the token it was handed, and awaitsOAuth2AuthorizationCodeBearer. That scheme only parses the header and returns the token; it never inspects the value. Since the method supplies the header itself, the check always succeeds.Verified against the installed fastapi (0.139.2) — every one of these is accepted, including the empty string:
The caller then logs success:
Real verification happens afterwards in
_decode_token, which fetches the JWKS signing key and callsjwt.decode. So the token is genuinely validated, just not here. My concern is that the method name, the docstring ("Validate the token ... against the OAuth2 server"), and that log line together suggest a verification step that does not exist, which is the kind of thing that misleads a future reader auditing the auth path.The one thing it does affect
Constructing the scheme reads
token_endpointandauthorization_endpointfrom the discovery document, so a discovery document missingtoken_endpointraises aValidationErrorand the request fails (an empty string passes). That is the only input that can make this method fail.Worth noting the server-side validation path itself never needs a token endpoint — it only needs
jwks_uri. So this effectively requires a discovery field that Feast's own token validation does not use. It also does not trigger anything that would otherwise be skipped:_decode_tokenreaches the same lazily-fetched discovery data viaget_jwks_url().Why I am asking rather than sending a patch
Removing the call would be a behavior change for anyone whose provider omits
token_endpoint(they would go from failing to working), and it is the onlyawaiton this branch of the OIDC path, which matters if the blocking JWKS work is ever moved to a thread. Both seem like your call rather than mine.Roughly the options I see:
Happy to send a PR for whichever you prefer. Not urgent, and the per-request cost is small; this is about the auth path saying it does something it does not.