CXH-1928: add group membership provisioning - #35
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| err = b.client.AddUserToGroup(ctx, directoryID, groupID, accountID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("baton-atlassian: failed to add user to group: %w", err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Revoke treats not-found as success via client.IsNotFound, but Grant has no matching already-exists handling. Re-granting membership a user already has (a common retry/idempotency path for C1) will surface a hard error instead of succeeding. Consider adding an IsAlreadyExists-style check next to IsNotFound in pkg/client/helper.go and returning annotations.New(&v2.GrantAlreadyExists{}), nil when Atlassian reports the membership already exists (409 maps to codes.Unknown via uhttp, so this may need to inspect the parsed APIError code rather than the gRPC status).
| return "", err | ||
| } | ||
|
|
||
| return groupResponse.Data.DirectoryId, nil |
There was a problem hiding this comment.
🟡 Suggestion: this returns Data.DirectoryId without checking it is non-empty. If the group-detail response omits directoryId (or data is absent), the caller gets "" and builds v2/orgs/{org}/directories//groups/{id}/memberships; url.JoinPath cleans the empty segment away, so the request silently goes to .../directories/groups/{id}/memberships — a wrong path rather than a clear failure. Returning an explicit error when DirectoryId == "" would make the failure diagnosable.
| _, err = c.doRequest(ctx, | ||
| http.MethodDelete, | ||
| requestURL, | ||
| nil, | ||
| nil, | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: this is the first http.MethodDelete caller in the client, and the MethodDelete branch of doRequest (line 565-569) calls c.wrapper.Do(req) with no uhttp.WithErrorResponse(&apiErr). Revoke failures will therefore lose the Atlassian error detail that APIError.Message() exists to surface. The gRPC code mapping still works (so IsNotFound is fine), but adding the error-response option to the DELETE branch would make failures debuggable.
Connector PR Review: CXH-1928: add group membership provisioningBlocking Issues: 0 | Suggestions: 5 | Threads Resolved: 0 Review SummaryReviewed the full PR diff (client endpoints, group Grant/Revoke, capabilities JSON, README, docs) for security and correctness. Provisioning entity sources are correct - Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Ran a deep review on this — mostly good, a couple idempotency gaps worth fixing before merge (see inline comments). Also small heads up: docs/docs-info.md still says only User Roles are provisionable, might want to update that too even though it is not part of this diff.
|
|
||
| err = b.client.AddUserToGroup(ctx, directoryID, groupID, accountID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("baton-atlassian: failed to add user to group: %w", err) |
There was a problem hiding this comment.
[High] Grant() does not treat "already a member" as a no-op. Revoke() right below checks client.IsNotFound and returns GrantAlreadyRevoked on 404 — Grant() should do the mirror thing for a 409/AlreadyExists so a retry does not hard-fail on a grant that already went through. uhttp already maps 409 -> codes.AlreadyExists the same way it maps 404 -> codes.NotFound, so this is basically the same fix pattern:
if client.IsAlreadyExists(err) {
return annotations.New(&v2.GrantAlreadyExists{}), nil
}|
|
||
| directoryID, err := b.client.GetGroupDirectoryID(ctx, groupID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("baton-atlassian: failed to resolve group directory: %w", err) |
There was a problem hiding this comment.
[Medium] Smaller version of the same idempotency thing: if the group itself got deleted upstream, this GetGroupDirectoryID call 404s and we return a hard error here instead of GrantAlreadyRevoked — even though "group is gone" means the membership is gone too. Might be worth wrapping this err check with client.IsNotFound as well, same as the one a few lines down.
| groupID := entitlement.Resource.Id.Resource | ||
| accountID := principal.Id.Resource | ||
|
|
||
| directoryID, err := b.client.GetGroupDirectoryID(ctx, groupID) |
There was a problem hiding this comment.
[Medium] ListGroups already returns DirectoryId on every group during sync (see client.Group), but parseIntoGroupResource drops it. So every single Grant/Revoke call pays for an extra GET here just to look it back up again — doubles the API calls for every provisioning op (same thing happens in Revoke() below). Might be worth stashing DirectoryId on the resource at sync time instead.
3 participants
Users can now be added to and removed from Atlassian groups through C1 access requests, instead of managing group membership by hand in the Atlassian admin console.