Adding oauth security with existing basic PAT #422
Adding oauth security with existing basic PAT #422ralphbean merged 2 commits intorelease-engineering:uat-instancefrom
Conversation
Review Summary by QodoAdd OAuth 2.0 authentication support for Jira instances
WalkthroughsDescription• Add OAuth 2.0 authentication support alongside existing PAT method • Implement token caching to avoid redundant OAuth token requests • Refactor Jira client initialization to use centralized auth builder • Add comprehensive tests for both PAT and OAuth2 authentication flows Diagramflowchart LR
A["Jira Instance Config"] --> B["build_jira_client_kwargs"]
B --> C{Auth Method}
C -->|PAT| D["Use token_auth or basic_auth"]
C -->|OAuth2| E["_get_oauth2_token"]
E --> F{Token Cached?}
F -->|Yes| G["Return Cached Token"]
F -->|No| H["_fetch_oauth2_token"]
H --> I["Request New Token"]
I --> J["Cache Token"]
J --> G
D --> K["JIRA Client"]
G --> K
File Changes1. sync2jira/jira_auth.py
|
Code Review by Qodo
1. response.json() unhandled failure
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
sync2jira/jira_auth.py
Outdated
| response.raise_for_status() | ||
| data = response.json() | ||
| access_token = data.get("access_token") | ||
| if not access_token: | ||
| raise ValueError("OAuth 2.0 token response did not contain access_token") | ||
| expires_in = int(data.get("expires_in", 3600)) | ||
| return access_token, expires_in |
There was a problem hiding this comment.
1. response.json() unhandled failure 📘 Rule violation ⛯ Reliability
_fetch_oauth2_token() calls response.json() and casts expires_in without handling invalid JSON or unexpected types, which can raise exceptions without actionable context. This reduces robustness when the token endpoint returns non-JSON or malformed responses.
Agent Prompt
## Issue description
OAuth2 token parsing is not resilient: `response.json()` and `int(...)` can raise exceptions on malformed responses, producing failures without clear, actionable context.
## Issue Context
Token endpoints sometimes return HTML/text on proxy errors, or JSON with unexpected shapes/types.
## Fix Focus Areas
- sync2jira/jira_auth.py[58-75]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
I disagree with @qodo-code-review: in order to hit a problem, the response would have to return a success status with a malformed payload; in this event, an exception from the json() call or from the int() cast would be caught at the outer level of the code and handled there. I don't think there are any actions that we would take in this code in response to a bad payload, so there is no need for exception handling here.
There was a problem hiding this comment.
Question:
I disagree with @qodo-code-review: in order to hit a problem, the response would have to return a success status with a malformed payload; in this event, an exception from the json() call or from the int() cast would be caught at the outer level of the code and handled there. I don't think there are any actions that we would take in this code in response to a bad payload, so there is no need for exception handling here.
Answer:
You can reduce (or eliminate) this kind of “add local try/except for json()/int()” suggestion by adding a repo-specific instruction to the /improve tool, telling Qodo Merge to avoid recommending local exception handling when exceptions are intentionally handled at a higher layer (and when there’s no meaningful local recovery action).
Add to your .pr_agent.toml (or wiki .pr_agent.toml) something like:
[pr_code_suggestions]
extra_instructions="""\
- Avoid suggesting local try/except blocks around parsing (e.g., response.json(), int(...) casts) when exceptions are already handled by an outer caller and there is no meaningful local recovery action.
- Only suggest adding local error handling if the exception would be uncaught, or if local context/logging materially improves diagnosability beyond existing outer-level handling.
- Prefer verifying whether upstream status checks / outer error handling exists before flagging "unhandled failure".
"""If you want this to affect the /review tool as well (since the comment looks like “review-style” reliability feedback), add the same idea here too:
[pr_reviewer]
extra_instructions="""\
Avoid flagging missing local try/except around parsing (response.json(), type casts) when the codebase intentionally relies on outer-layer exception handling and there is no local remediation step.
"""If you share which tool produced the comment in your setup (automatic /review vs /improve vs another workflow), the instructions can be narrowed to match that tool only.
Relevant Sources:
- https://docs.qodo.ai/qodo-documentation/qodo-merge/tools/improve#extra-instructions-and-best-practices
- https://docs.qodo.ai/qodo-documentation/qodo-merge/tools/improve#extra-instructions
- https://docs.qodo.ai/qodo-documentation/qodo-merge/tools/review#extra-instructions
- https://docs.qodo.ai/qodo-documentation/qodo-merge/usage-guide/configuration_options#wiki-configuration-file
There was a problem hiding this comment.
@ralphbean, perhaps this should be added to the changes in #409?
There was a problem hiding this comment.
Question:
@ralphbean, perhaps this should be added to the changes in #409?
Answer:
Yes—this is a good candidate for a repo-level configuration tweak, so future /improve (and optionally /review) runs avoid this kind of “add local try/except around parsing” noise when your codebase intentionally handles exceptions at a higher layer.
Add to the repo’s .pr_agent.toml (or the repo wiki page .pr_agent.toml):
[pr_code_suggestions]
extra_instructions="""\
- Avoid suggesting local try/except blocks around parsing (e.g., response.json(), int(...) casts) when exceptions are already handled by an outer caller and there is no meaningful local recovery action.
- Only suggest adding local error handling if the exception would be uncaught, or if adding local context/logging materially improves diagnosability beyond existing outer-level handling.
- Prefer verifying whether upstream status checks / outer error handling exists before flagging "unhandled failure".
"""If the comment you’re seeing is coming from /review (or you want the same behavior there too), add:
[pr_reviewer]
extra_instructions="""\
Avoid flagging missing local try/except around parsing (response.json(), type casts) when the codebase intentionally relies on outer-layer exception handling and there is no local remediation step.
"""If you confirm whether the bot message was generated by /improve vs /review (or an automatic run configured under [github_app].pr_commands), this can be narrowed to only the relevant tool.
Relevant Sources:
- https://docs.qodo.ai/qodo-documentation/qodo-merge/tools/improve#extra-instructions-and-best-practices
- https://docs.qodo.ai/qodo-documentation/qodo-merge/tools/review#extra-instructions
- https://docs.qodo.ai/qodo-documentation/qodo-merge/usage-guide/configuration_options#wiki-configuration-file
- https://docs.qodo.ai/qodo-documentation/qodo-merge/usage-guide/configuration_options#local-configuration-file
sync2jira/jira_auth.py
Outdated
| def _get_oauth2_token( | ||
| client_id: str, | ||
| client_secret: str, | ||
| token_url: str = DEFAULT_OAUTH2_TOKEN_URL, | ||
| ) -> str: | ||
| """Return a valid OAuth2 token, reusing cache if not expired (with refresh buffer).""" | ||
| key = (client_id, client_secret, token_url) | ||
| now = time.time() | ||
| entry = _oauth2_token_cache.get(key) | ||
| if entry: | ||
| token, expires_at = entry | ||
| if now < expires_at - OAUTH2_TOKEN_REFRESH_BUFFER_SECONDS: | ||
| return token | ||
| token, expires_in = _fetch_oauth2_token( | ||
| client_id=client_id, | ||
| client_secret=client_secret, | ||
| token_url=token_url, | ||
| ) | ||
| _oauth2_token_cache[key] = (token, now + expires_in) | ||
| return token |
There was a problem hiding this comment.
2. Retry reuses cached token 🐞 Bug ⛯ Reliability
On JIRAError, the code “refreshes” the Jira client by recreating it, but OAuth2 token retrieval is cached and may return the same (revoked/invalid) token until expiry. This makes the retry path ineffective for OAuth2 and can cause repeat failures even after a refresh attempt.
Agent Prompt
### Issue description
The Jira retry paths rebuild the Jira client on `JIRAError` expecting refreshed authentication, but for OAuth2 the access token may be returned from `_oauth2_token_cache` until expiry. If Jira rejects a token early (revoked/invalid), the retry will likely reuse the same token and fail again.
### Issue Context
- `sync_with_jira()` for both issues and PRs calls `get_jira_client()` again when retrying after `JIRAError`.
- `get_jira_client()` now uses `build_jira_client_kwargs()`, which uses `_get_oauth2_token()` and the cache.
### Fix Focus Areas
- sync2jira/jira_auth.py[77-96]
- sync2jira/jira_auth.py[118-140]
- sync2jira/downstream_issue.py[1391-1410]
- sync2jira/downstream_pr.py[188-207]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
Looks good, but it could use a polishing pass.
|
@Bala-Sakabattula, the review bot brought up a good point: there should probably be a documentation update accompanying this change, and maybe an update to the sample sync2jira.py? |
I will updated the documentation and sample part later when we will move the changes to prod |
| json=lambda t=access_token, e=expires_in: { | ||
| "access_token": t, | ||
| "expires_in": e, | ||
| }, |
There was a problem hiding this comment.
Just for future reference (i.e., I'm not actually suggesting a change, here), this approach strikes me as clever but fragile: you're counting on the fact that the json() function will be invoked with no arguments. I don't see any reason why we need to use the default argument values to inject the values -- I'm pretty sure that we can reference them directly:
| json=lambda t=access_token, e=expires_in: { | |
| "access_token": t, | |
| "expires_in": e, | |
| }, | |
| json=lambda: { | |
| "access_token": access_token, | |
| "expires_in": expires_in, | |
| }, |
I believe that the closure will capture and return the correct values.
Ha! Promises, promises.... 😉 Also, speaking of accruing technical debt, I don't think you acted on this comment: we need a tweak to |
3 participants
Adding oauth logging with existing basic PAT logging for jira.