feat(jira): add Cloud OAuth 2.0 client-credentials auth - #152
Conversation
Service-account 2LO can mint 60-minute Bearer tokens and call the Atlassian API gateway, so Jira Cloud connections no longer require a user API token. Upstream-Status: Pending Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 2:36 PM UTC · Completed 2:59 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.97 |
|
Risk Assessment: elevated (3/5) DetailsTier 1 signals unchanged from prior assessment (Tier1=2.25); re-review anchoring preserves score 3 — large blast radius, 3 security-sensitive OAuth/token files, and ~1700 lines of new authentication code across dormant core connection files remain the primary risk drivers. Previous runRisk Assessment: elevated (3/5) DetailsTier 1 signals unchanged from prior assessment (Tier1=2.25); re-review anchoring preserves score 3 — the large blast radius, 3 security-sensitive OAuth/token files, and near-2000 lines of new authentication code across dormant core connection files remain the primary risk drivers. Previous run (2)Risk Assessment: elevated (3/5) DetailsTier 1 and Tier 2 signals unchanged from prior assessment (Tier1=2.25, Tier2=2.71); large blast radius, 3 security-sensitive OAuth/token files, and high historical fix rates on core connection and auth files that have been dormant 2-3 years. Previous run (3)Risk Assessment: elevated (3/5) DetailsThis PR adds OAuth 2.0 client-credentials authentication to the Jira plugin, touching 3 security-sensitive files across a large blast radius (1629 lines, 18 files) while modifying core connection and API client files that have been dormant for 2-3 years, producing a composite elevated score of 3 (Tier1=62%x2.25 + Tier2=38%x2.75 = 2.54 rounded to 3). Previous run (4)Risk Assessment: elevated (3/5) DetailsScore 3 (elevated) driven by large blast radius (1629 lines across 18 files spanning backend Go and frontend TypeScript), 3 security-sensitive files introducing OAuth2 client-credential storage and Bearer-token minting, and only 22% test file ratio; moderated by an experienced author, stable low-churn codebase, no CI or dependency changes, and no protected-path touches. Previous run (5)Risk Assessment: moderate (2/5) DetailsLarge-blast-radius OAuth2 feature (18 files, 1519 lines, 3 security-sensitive paths) is offset by strong test coverage, no churned or contested files in git history, no CI or dependency changes, and a known non-first-time author, yielding a moderate composite score of 2. Previous run (6)Risk Assessment: moderate (2/5) DetailsModerate risk: PR introduces OAuth 2.0 client-credentials auth across 18 files/1363 lines with large blast radius and 3 security-sensitive touch points, but structural mitigators (no protected paths, no CI or dependency changes, stable low-churn file history, tests present) keep the composite score at 2, consistent with the prior assessment. Previous run (7)Risk Assessment: moderate (2/5) DetailsModerate risk: the PR introduces OAuth 2.0 client-credentials auth across 1200 lines with a large blast radius and 3 security-sensitive touch points, but structural risk mitigators (no protected paths, no CI or dependency changes, stable file history) keep the composite score at 2. |
ReviewFindingsHigh
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (4)ReviewFindingsMedium
Low
Next steps:
Previous run (5)ReviewFindingsMedium
Low
Next steps:
Previous run (6)ReviewFindingsMedium
Low
Next steps:
Previous run (7)ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Credential 401s were forwarded as HTTP 401, so Config UI treated them as session expiry. Map them to 400, skip the login interceptor for plugin tests, and include Jira's response body (e.g. scope does not match). Upstream-Status: Pending Co-Authored-By: Cursor Grok 4.6 <noreply@example.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 11:21 AM UTC · Completed 11:46 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.34 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Clear authFunc when the refresh round tripper is installed so collection workers do not race on oauthToken. Validate CloudId, restore request bodies on 401 retry, and stop echoing raw token-endpoint bodies. Upstream-Status: Pending Co-Authored-By: Cursor Grok 4.6 <noreply@example.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 5:52 PM UTC · Completed 6:18 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.81 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Snapshot the original body when GetBody is missing so a reminted token retry does not send an empty POST. Also drop the redundant gateway and endpoint checks from Test Connection validation. Upstream-Status: Pending Co-Authored-By: Cursor Grok 4.6 <noreply@example.com> Co-authored-by: Cursor <cursoragent@cursor.com>
A 4KiB cap truncated Atlassian token JSON when many scopes were granted, so Test Connection failed with unexpected end of JSON input. Keep a 64KiB DoS guard and fail clearly on empty or oversized bodies. Upstream-Status: Pending Co-Authored-By: Cursor Grok 4.6 <noreply@example.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 11:33 AM UTC · Completed 12:01 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.79 |
fullsend-ai-review
Bot
added
risk/elevated
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Rename AtlassianOAuthTokenURL so gosec does not treat the public auth.atlassian.com endpoint as a hardcoded credential. Upstream-Status: Pending Co-Authored-By: Cursor Grok 4.6 <noreply@example.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 12:38 PM UTC · Completed 1:07 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.53 |
| // can re-send it. http.NewRequest already sets GetBody for *bytes.Reader and | ||
| // *strings.Reader; collectors that pass an arbitrary io.Reader would otherwise | ||
| // retry with an empty body. | ||
| func ensureGetBody(req *http.Request) error { |
There was a problem hiding this comment.
+1 — agree this should move into cloneRequestWithBearer to stay clean with the RoundTripper contract.
| if req.GetBody != nil || req.Body == nil || req.Body == http.NoBody { | ||
| return nil | ||
| } | ||
| buf, err := io.ReadAll(req.Body) |
There was a problem hiding this comment.
Minor: io.ReadAll(req.Body) could use an io.LimitReader cap as a safety guard.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collection token state now lives under TokenProvider's mutex, and 401 retries snapshot the body onto clones so RoundTrip does not mutate the original request. Upstream-Status: Pending Co-Authored-By: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 9:05 AM UTC · Completed 9:35 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $11.02 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collection builds the HTTP client without FromConnection so the round tripper is the only minter and Authorization setter. Upstream-Status: Pending Co-Authored-By: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 11:52 AM UTC · Completed 12:16 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.02 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
rsoaresd
left a comment
rsoaresd
left a comment
There was a problem hiding this comment.
Amazing 🚀 I tested locally and it works pretty well!
|
🤖 Review · ❌ Terminated · Started 9:34 AM UTC · Ended 10:00 AM UTC Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #152 +/- ##
==========================================
- Coverage 48.05% 41.60% -6.45%
==========================================
Files 154 154
Lines 10501 10501
==========================================
- Hits 5046 4369 -677
- Misses 5237 6019 +782
+ Partials 218 113 -105
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
backend/plugins/jira/models/connection.go(file-level): Line 194 · [low] edge-case
When MergeFromRequest detects an auth-method change, it does not zero the now-unused OAuth2 fields (ClientId, ClientSecret, CloudId). Stale credentials remain in the database.
Suggested fix: Zero the stale OAuth2 fields when switching auth methods.
| status === 401 && | ||
| !isLoginRoute() && | ||
| !redirectingToLogin && | ||
| !isPluginConnectionTest(requestUrl) |
There was a problem hiding this comment.
[low] error-handling
The isPluginConnectionTest guard exempts all 401 responses on /plugins/.../test URLs from the login redirect. The backend already maps Jira-originated 401/403 to HTTP 400, making the frontend guard redundant for Jira. A genuine DevLake session-expiry 401 on a test endpoint would be suppressed.
Suggested fix: Consider adding a code comment explaining the belt-and-suspenders design: backend 400-mapping for Jira + frontend guard for other plugins.
| type CloudMethod = 'BasicAuth' | 'OAuth2'; | ||
|
|
||
| const gatewayEndpoint = (cloudId?: string) => | ||
| cloudId ? `https://api.atlassian.com/ex/jira/${cloudId.trim()}/rest/` : ''; |
There was a problem hiding this comment.
[low] injection
The gatewayEndpoint function interpolates user-provided cloudId into a URL template without client-side validation. The backend validates and overwrites, but client-side validation provides defense-in-depth.
Suggested fix: Add a client-side validation check matching the backend cloudIDPattern (/^[a-zA-Z0-9-]{1,64}$/).
| if tp.token == "" { | ||
| return true | ||
| } | ||
| if tp.expiresAt == nil { |
There was a problem hiding this comment.
[low] edge-case
needsRefresh returns false when expiresAt is nil but token is non-empty. The invariant that cacheToken always sets a non-nil expiresAt is implicit.
Suggested fix: Treat nil expiresAt with a non-empty token as needing refresh, or add a comment documenting the invariant.
| @@ -71,10 +71,12 @@ func testConnection(ctx context.Context, connection models.JiraConn) (*JiraTestC | |||
| return nil, errors.NotFound.New(fmt.Sprintf("Seems like an invalid Endpoint URL, please try %s", restUrl.String())) | |||
| } | |||
| if res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden { | |||
There was a problem hiding this comment.
[low] unauthorized-change
testConnection now returns errors.BadInput (HTTP 400) instead of errors.HttpStatus(401) for credential failures for ALL Jira connection types, silently altering the API contract for existing users.
Suggested fix: Document this API contract change in upstream-diffs.md. Consider whether the frontend guard makes the backend remapping unnecessary.
| return *jc | ||
| } | ||
|
|
||
| func (jc *JiraConn) IsOAuth2() bool { |
There was a problem hiding this comment.
[low] doc-style
Four exported methods lack doc comments: IsOAuth2(), OAuthAccessToken(), OAuthAccessTokenExpiresAt(), SetOAuthAccessToken().
Suggested fix: Add one-line doc comments per Go convention.
| return "_tool_jira_connections" | ||
| } | ||
|
|
||
| func (connection *JiraConnection) CustomValidate(entity interface{}, v *validator.Validate) errors.Error { |
There was a problem hiding this comment.
[low] doc-style
CustomValidate is an exported method implementing a pluginhelper interface but has no doc comment.
Suggested fix: Add a doc comment explaining the interface delegation.
|
🤖 Finished Review · ✅ Success · Started 9:34 AM UTC · Completed 10:00 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.67 |
|
@kpiwko @flacatus @psturc I think it's safe to merge this one , as it was already merged upstream as well apache#9159 |
|
/ok-to-test |
5 participants
Summary
clientId,clientSecret, andcloudId; mint a 60-minute Bearer token viagrant_type=client_credentialsagainsthttps://auth.atlassian.com/oauth/token.https://api.atlassian.com/ex/jira/{cloudId}/rest/and remint the access token near expiry or on 401. No user consent / refresh-token flow.