Repository navigation
feat: auth bearer token management (issue #110) - #118
jennifer-bowser wants to merge 9 commits into
Conversation
…ient instantiate its own
korikuzma
left a comment
korikuzma
left a comment
There was a problem hiding this comment.
This is a quick review from Codex. I haven’t reviewed the PR myself yet, but I can take a look later this week.
Please address token expiry and blocking I/O, add outbound authentication tests, and document the new client.
| self._token_expiry = datetime.now(tz=UTC) + timedelta( | ||
| seconds=data["expires_in"] | ||
| ) |
There was a problem hiding this comment.
Codex finding: Normal: Refresh tokens before their actual expiration
Adding expires_in after receiving the response extends the cached lifetime by the response delay, allowing expired tokens to be reused and requests to fail authentication. OAuth measures this lifetime from when the response was generated (section 5.1). Calculate expiry conservatively from the request start, include a refresh margin, and test delayed token responses with a controlled clock.
There was a problem hiding this comment.
Codex finding: Severity - Moderate (incorrect behavior under specific, realistic conditions); Blocking: Finish the conservative expiry calculation
The 30-second refresh margin addresses part of the original comment. Expiry is still calculated after receiving the response: if delivery takes 40 seconds after Auth0 generates the token response, the cached deadline falls 10 seconds after the token’s actual expiration. Requests’ timeout=10 limits socket inactivity, not total response duration (documentation).
Please calculate expiry conservatively, such as by anchoring it immediately before requests.post(), and add the previously requested controlled-clock test for delayed responses. This remains blocking because the current calculation can allow an expired token to be reused, causing outbound authentication failures.
| VLM_CLIENT_ID="example-client-id" | ||
| VLM_CLIENT_SECRET="example-client-secret" |
There was a problem hiding this comment.
Codex finding: Minor: Add outbound authentication documentation
These variables appear in the generated example environment page, but users have no setup or client usage instructions. Please document credential setup, the fixed Auth0 endpoint and audience, and a minimal client example under docs/, and include the new modules in the API reference. Clarify that credentials are required for outbound authentication; the existing inbound endpoints do not instantiate this provider.
There was a problem hiding this comment.
Codex finding: Severity - Moderate (incorrect behavior under specific, realistic conditions); Blocking: Correct the credential setup instructions
Credential setup instructions are now present, but the configuration table names the secret VLM_SECRET_ID; the implementation reads VLM_CLIENT_SECRET. Following the table therefore sends no configured client secret to Auth0 and prevents token retrieval. Please change the documented name to VLM_CLIENT_SECRET.
Please also distinguish these outbound credentials from the settings required to launch the existing inbound service. The table’s introductory note currently says every listed value is required for startup, although the inbound service does not instantiate TokenProvider. These are corrections needed to make the setup instructions accurate, separate from the optional documentation additions in the original comment.
There was a problem hiding this comment.
Codex finding: Severity - Low (limited impact, such as documentation or maintainability issues); Non-blocking: Add client usage guidance
The previously suggested fixed Auth0 endpoint and audience, minimal client example, and API-reference entries are still absent. Adding them would help developers construct and use the outbound client without reading its implementation. These additions are optional and can be left out; they do not need a separate issue.
There was a problem hiding this comment.
This is still Codex reviewing. Kori will perform a human review tomorrow.
Reopened the token-expiry and credential-setup threads because required fixes remain. Please address these before merging. The optional documentation improvements can be left out.
korikuzma
left a comment
korikuzma
left a comment
There was a problem hiding this comment.
Some small human review suggestions that are non-blocking
| * - ``VLM_CLIENT_ID`` | ||
| - The `client_id` provided to you by the VLM Network administrators | ||
| - ``"kjnq3iuyththofdh88347"`` | ||
| * - ``VLM_SECRET_ID`` | ||
| - The `client_secret` provided to you by the VLM Network administrators | ||
| - ``"nlk34t98nvcnjkcf78r9shvfkidsxhfabn3947tghvdb9"`` |
There was a problem hiding this comment.
Nitpick (nonblocking): could we use placeholders that are more obvious that they're placeholders?
| * - ``VLM_CLIENT_ID`` | |
| - The `client_id` provided to you by the VLM Network administrators | |
| - ``"kjnq3iuyththofdh88347"`` | |
| * - ``VLM_SECRET_ID`` | |
| - The `client_secret` provided to you by the VLM Network administrators | |
| - ``"nlk34t98nvcnjkcf78r9shvfkidsxhfabn3947tghvdb9"`` | |
| * - ``VLM_CLIENT_ID`` | |
| - The `client_id` provided to you by the VLM Network administrators | |
| - ``"example-client-id"`` | |
| * - ``VLM_SECRET_ID`` | |
| - The `client_secret` provided to you by the VLM Network administrators | |
| - ``"example-client-secret"`` |
| json={ | ||
| "client_id": os.getenv("VLM_CLIENT_ID"), | ||
| "client_secret": os.getenv("VLM_CLIENT_SECRET"), | ||
| "audience": self.AUDIENCE_URL, |
There was a problem hiding this comment.
Suggestion (non-blocking): We may consider adding a check for these env vars in the init and raise an error if either are missing
| ) | ||
|
|
||
| if response.status_code != HTTPStatus.OK: | ||
| error_message: str = "Unable to fetch bearer token from Auth0" |
There was a problem hiding this comment.
Suggestion (non-blocking): We should consider adding the status code to the error message
Closes #110