CXH-1928: add group membership provisioning - #35
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: CXH-1928: add group membership provisioningBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryReviewed the full PR diff (8 files: client, helper, model, Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone new. Prior open suggestion: |
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.
Address review feedback on PR #35: - Grant now treats an already-existing membership (HTTP 409 -> codes.AlreadyExists) as success, mirroring Revoke's not-found handling. - Revoke now treats a 404 from GetGroupDirectoryID (group deleted upstream) as GrantAlreadyRevoked, since a missing group means the membership is gone. - docs-info.md now lists Group Memberships as a provisionable resource.
Address review-bot feedback on PR #35: - GetGroupDirectoryID now returns codes.Internal when a 200 response omits directoryId, instead of silently building a wrong membership URL from an empty path segment. - The DELETE branch of doRequest now passes uhttp.WithErrorResponse, so revoke failures keep the Atlassian error detail. IsNotFound still resolves through the resulting joined error, so revoke idempotency is unchanged.
mateoHernandez123
left a comment
mateoHernandez123
left a comment
There was a problem hiding this comment.
Verified the group-membership provisioning end to end. The DELETE endpoint matches Atlassian's v2 org-directory API (/v2/orgs/{orgId}/directories/{directoryId}/groups/{groupId}/memberships/{accountId}, keyed on accountId), the directoryId is resolved up front, and idempotency is symmetric — Grant returns GrantAlreadyExists on a 409, and Revoke returns GrantAlreadyRevoked on a 404 for both the directory lookup and the membership delete. Error wrapping and the DELETE WithErrorResponse are correct, there are no Warn logs, and the extra per-op GET to resolve the directory is a fair tradeoff given the service-mode re-keying constraint you called out (a profile-cached directoryId would be empty on the provisioning path, and encoding it into the resource ID would re-key existing grants). LGTM.
Two non-blocking follow-ups:
- The new client methods and Grant/Revoke ship without unit coverage — worth adding when this connector grows a test harness.
- #33 is a duplicate of this PR for the same ticket and should be closed as superseded.
5 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.