Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adds optional OAuth2 (client credentials) authentication support to the ticket validation/refresh API calls, with token caching and new tests/docs to support the rollout of an authenticated validation endpoint.
Changes:
- Add OAuth2 client-credentials token fetching + caching and include
Authorization: Bearer …on ticket API calls when configured. - Skip ticket refresh/validation calls when OAuth2 is configured but a token cannot be obtained.
- Add pytest coverage for token caching and header injection, plus document new
.secretsvariables.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
discord_bot/helpers/ticket_connector.py |
Implements OAuth2 token retrieval/caching and applies bearer auth headers to ticket API requests. |
discord_bot/configuration.py |
Adds optional OAuth2 settings to the config loader. |
discord_bot/config.toml |
Introduces optional OAuth2 config keys (disabled by default). |
tests/test_ticket_connector_oauth2.py |
Adds async tests for OAuth2 token caching and request header behavior. |
README.md |
Documents the new optional OAuth2 environment variables in .secrets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._oauth2_token: str | None = None | ||
| self._oauth2_token_expires_at: float = 0.0 | ||
| self._oauth2_token_lock = asyncio.Lock() |
There was a problem hiding this comment.
TicketOrder is instantiated at import time in discord_bot/bot.py and discord_bot/cogs/registration.py. Creating asyncio.Lock() in __init__ can bind the lock to a different (non-running) event loop than the one used by asyncio.run()/discord, which can later raise "bound to a different event loop" errors when acquiring the lock. Consider lazily initializing the lock inside _get_oauth2_token (when a running loop exists) or otherwise ensuring the lock is created in the same running loop that will use it.
| data = await response.json() | ||
| except (aiohttp.ClientError, ValueError): | ||
| _logger.exception("Error occurred while fetching OAuth2 token from %r", self.TICKETS_OAUTH2_TOKEN_URL) | ||
| return None | ||
|
|
||
| access_token = data.get("access_token") | ||
| if not access_token: | ||
| _logger.error("OAuth2 token response does not contain an access token") | ||
| return None |
There was a problem hiding this comment.
_fetch_oauth2_token assumes await response.json() returns a dict and immediately calls data.get(...). If the IdP returns valid JSON that isn't an object (e.g., a list or string), this will raise AttributeError and bypass the current exception handling. Add an explicit isinstance(data, dict) check (and log/return None if not) before accessing .get.
| async def _update_tickets(self, url: str) -> bool: | ||
| async with aiohttp.ClientSession() as session, session.get(url, headers=self.HEADERS) as response: | ||
| headers = await self._build_headers() | ||
| if self._oauth2_is_enabled() and "Authorization" not in headers: | ||
| _logger.error("Skipping ticket refresh because OAuth2 token could not be obtained") | ||
| return False | ||
|
|
There was a problem hiding this comment.
New behavior in _update_tickets skips refresh calls when OAuth2 is enabled but the token cannot be obtained. There are tests for get_ticket_type OAuth2 behavior, but no test covering this refresh-path skip; consider adding a similar test asserting aiohttp.ClientSession is not called (or that _update_tickets returns False) when _get_oauth2_token returns None.
| # Optional: required when ticket validation API enforces OAuth2 | ||
| TICKETS_OAUTH2_CLIENT_ID=<OAuth2ClientId> | ||
| TICKETS_OAUTH2_CLIENT_SECRET=<OAuth2ClientSecret> | ||
| TICKETS_OAUTH2_TOKEN_URL=<https://your-idp.example.com/realms/<realm>/protocol/openid-connect/token> |
There was a problem hiding this comment.
The .secrets example for TICKETS_OAUTH2_TOKEN_URL uses nested angle brackets (<https://.../<realm>/...>), which is hard to copy/paste and ambiguous as a placeholder. Consider using a plain URL with a single placeholder style (e.g., .../realms/{realm}/...) and avoid wrapping the whole value in <...>.
| TICKETS_OAUTH2_TOKEN_URL=<https://your-idp.example.com/realms/<realm>/protocol/openid-connect/token> | |
| TICKETS_OAUTH2_TOKEN_URL=https://your-idp.example.com/realms/{realm}/protocol/openid-connect/token |
- Reject non-object JSON token responses instead of raising AttributeError - Add tests for the refresh-path auth skip, bearer header on refresh, and non-dict token responses - Use an unambiguous, copy-pasteable OAuth2 token URL example in README
|
Reviewed against the actual validation API (
Also added a test for the non-dict token response. Full suite is green (82 passed) and Operational note (Keycloak): the API validates the token |
The validation API requires authentication now.