docs: Correct what _validate_token claims to do - #6689
Conversation
The method is named validate, its docstring says it validates the token against the OAuth2 server, and the caller logs 'Token successfully validated' afterwards. None of that holds: the bearer scheme only parses an Authorization header, and the method builds that header itself, so every token value passes including the empty string. The token is really verified in _decode_token. Anyone auditing the auth path is misled into thinking a verification step happens here. Describe what the call actually checks (that the discovery document exposes the OAuth2 endpoints) and log that instead. No behavior change: docstring and log message only. Whether the call should exist at all is asked separately in feast-dev#6688. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6689 +/- ##
==========================================
- Coverage 46.77% 46.76% -0.01%
==========================================
Files 414 414
Lines 50191 50189 -2
Branches 7181 7181
==========================================
- Hits 23475 23473 -2
Misses 25077 25077
Partials 1639 1639
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@ntkathole @franciscojavierarceo Could one of you take a look, or route it to whoever owns this area? It also needs a |
| async def _validate_token(self, access_token: str): | ||
| """ | ||
| Validate the token extracted from the header of the user request against the OAuth2 server. | ||
| """Check that the provider's discovery document exposes the OAuth2 endpoints. |
There was a problem hiding this comment.
isn't the function name misleading then ?
The method never verified the token it takes: it builds a bearer scheme whose header it supplies itself, so any token value passes. What it does check is that the discovery document exposes the OAuth2 endpoints, which is what the docstring already said after the previous commit. Leaving the old name meant the docstring had to spend its opening correcting the name. Private method, single caller in the same file, so the rename is contained. The access_token parameter stays: the bearer scheme requires a well-formed Authorization header, and the caller has the token to hand. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
|
@ntkathole It's private with a single caller in the same file, so the rename is contained. I kept the 317 permissions tests pass. |
What this PR does / why we need it
OidcTokenParser._validate_tokenis named validate, its docstring says it validates the token against the OAuth2 server, and the caller logs "Token successfully validated" afterwards. None of that is accurate. The bearer scheme only parses anAuthorizationheader, and the method constructs that header itself from the token it was handed, so every value passes. Verified against fastapi 0.139.2: the empty string, whitespace, and arbitrary garbage are all accepted.The token is genuinely verified a few lines later in
_decode_token, which fetches the JWKS signing key and validates the signature and claims. So authentication is sound. The problem is that anyone auditing this path reads a verification step that does not exist, and a debug log that confirms it.This changes the docstring to describe what the call actually checks (that the discovery document exposes the OAuth2 endpoints, which is the only thing that can make it fail) and changes the log line to match.
No behavior change: docstring and one log string only. Existing tests pass unmodified.
Whether the call should exist at all is a separate question, asked in #6688, since removing it would change behavior for providers whose discovery document omits a token endpoint. This PR deliberately does not pre-empt that answer.
Which issue(s) this PR fixes
Relates to #6688