[CXH-2285] - Return GrantAlreadyExists on folder/document no-op re-grants

[sergiocorral-conductorone]

Fixes CXH-2285

Problem

Folder and document provisioning were asymmetric on idempotency. Revoke returns GrantAlreadyRevoked on a not-found delete, but Grant returned no annotation when the user already held the requested collaborator role.

The upstream call is an idempotent PUT upsert (pkg/connector/client/mutate.go) that returns 200 (update) / 201 (create), never 409 Conflict. So re-granting a role the user already holds succeeded silently and was indistinguishable from a grant that changed upstream state — C1's audit log recorded both outcomes identically. (IsAlreadyExistsError was defined but never called.)

Fix

Because the status code alone can't distinguish a same-role re-grant from a role change, we read the user's current role before upserting, via the single-collaborator GET (GET /{folders|documents}/{id}/shares/users/{userId}, which 404s when the user is not a direct collaborator):

• current role == requested role → return annotations.New(&v2.GrantAlreadyExists{}) and skip the PUT (true no-op, symmetric with Revoke).
• user absent (404) or different role → PUT upsert and return the grant (unchanged behaviour). A view → edit change is a real state change, so it is not reported as already-exists.

Also wires IsAlreadyExistsError defensively on the upsert error path (Lucid doesn't 409 here today, but if it ever does it's treated as an idempotent success), and documents the single-slot role-replacement semantics inline (the ticket noted this was undocumented).

Scope / relation to CXH-1919

This PR intentionally does not change the grant.NewGrant argument order (the principal-vs-resource swap), which is tracked separately in CXH-1919. Both tickets touch the same two Grant() methods; the already-exists path here returns nil grants + annotation, so it does not build a grant.NewGrant and does not pre-empt or conflict with CXH-1919's fix. See the PR discussion / task report for the overlap note.

Changes

• pkg/connector/client/query.go — add GetFolderUserCollaborator / GetDocumentUserCollaborator + path constants.
• pkg/connector/folder.go, pkg/connector/document.go — pre-check current role in Grant(), return GrantAlreadyExists on a no-op; defensive IsAlreadyExistsError; inline docs.
• pkg/connector/grant_idempotency_test.go — httptest-driven tests covering the ticket's three reproduction cases (new grant, same-role re-grant, role change) for both folder and document, asserting the annotation and that the upsert is skipped on a no-op.

Validation

• go build ./..., go vet ./..., gofmt -l clean.
• golangci-lint run ./... → 0 issues.
• go test ./... passes, including the 6 new idempotency subtests.
• Full sync smoke test against cmd/test-server (the CI harness) completes with exit 0 — no regression. (The mock serves empty folder/document contents, so it does not exercise collaborator grants; the new unit tests cover that path end-to-end through the real HTTP client.)

🤖 Generated with Claude Code

[linear-code]

CXH-2285

[github-actions]

Connector PR Review: [CXH-2285] - Return GrantAlreadyExists on folder/document no-op re-grants

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 3420066d2dab.
Review mode: incremental since a8ae112
View review run

Review Summary

The new commit attaches metaRole/metaCreated metadata to the no-op re-grant returned by folderBuilder.Grant/documentBuilder.Grant (sourced from the pre-check GET's current.Role/current.Created), plus a grantMetadata test helper and parity assertions in both builder tests. The full PR diff was re-scanned for security and correctness: no secrets or PII are logged (only folder_id/document_id/user_id), the pre-check GET stays best-effort with fallthrough to the authoritative upsert, IsNotFoundError classification in Revoke is correct, and go.mod/go.sum are unchanged. The metadata keys and time.Time.String() formatting match what Grants() emits, so the no-op grant will not churn against a synced grant. Note that the previously-flagged sibling issue at folder.go:179 / document.go:194 — the upsert-success path passing the user resource to grant.NewGrant instead of entitlement.Resource — is still present and unaddressed; it is pre-existing and already has open threads, so it is not re-filed here.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• pkg/connector/folder_test.go:60 (and pkg/connector/document_test.go:60): the "identical to the upsert-success path" parity assertion compares only the GrantMetadata annotation, not the grant ID, so it does not catch the known ID divergence between the two paths.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/folder_test.go`:
- Around line 60: The comment claims the assertion proves the no-op grant is identical
  to what the upsert-success path emits, but `require.Equal(t, grantMetadata(t, upGrants[0]), noopMeta)`
  only compares the GrantMetadata annotation. The two paths currently produce different
  grant IDs: the no-op path builds `grant.NewGrant(entitlement.Resource, entitlement.Slug, resource.Id)`
  → entitlement ID `folder:9001:user/edit` (matching `Grants()`), while the success path at
  `folder.go:179` passes `resource`, which in this branch is the *user* principal →
  entitlement ID `user:100:user/edit`. Add `require.Equal(t, upGrants[0].Id, grants[0].Id)`
  (or assert the expected literal grant ID) so the parity this PR is about is actually locked in.

In `pkg/connector/document_test.go`:
- Around line 60: Same as the folder test — add a grant-ID equality assertion alongside the
  metadata comparison, expecting `document:doc-abc:user/comment:user:200`.

[github-actions]

No blocking issues found.

[github-actions]

No blocking issues found.

[github-actions]

🟡 Suggestion: this line is correct (grant.NewGrant(resource, entitlementName, principal) → entitlement ID folder:9001:user/edit, matching Grants() at line 121), but the sibling success path at line 175 is not: it passes resource — which in Grant is the user principal, not the folder — as the first arg, producing entitlement ID user:100:user/edit and grant ID user:100:user/edit:user:100. So a first-time grant and a no-op re-grant now hand C1 two different grant identities for the same membership, and only the re-grant one matches what the next sync emits. This is pre-existing (unchanged lines), but the comment above claims parity with Grants(), and the new test only asserts require.Len(t, grants, 1) for the success path — adding the same Entitlement.Resource.Id.Resource assertion there would catch it.

Separately (minor): the no-op grant omits the metaRole/metaCreated grant metadata that both Grants() and the upsert path attach. current.Role and current.Created are already in hand, so grant.WithGrantMetadata(...) would make the emitted grant fully identical to the synced one.

[github-actions]

🟡 Suggestion: same as the folder path — this line correctly builds entitlement ID document:doc-abc:user/comment (matching Grants() at line 136), but the success path at line 190 passes resource, which here is the user principal, as NewGrant's resource arg, yielding user:200:user/comment:user:200. First-time grant and no-op re-grant therefore report different grant identities for the same membership. Pre-existing and on unchanged lines, but worth fixing alongside this change; the new success-path test asserts only require.Len(t, grants, 1), so mirroring the Entitlement.Resource.Id.Resource assertion from the re-grant case would pin it.

Minor: the no-op grant also drops the metaRole/metaCreated metadata that Grants() and the upsert path attach — current.Role/current.Created are available if you want byte-identical grants.

[github-actions]

Connector PR Review: [CXH-2285] - Return GrantAlreadyExists on folder/document no-op re-grants

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 3420066d2dab.
Review mode: incremental since a8ae112c
View review run

No blocking issues found.

Review Summary

The new commit addresses @mateoHernandez123's feedback: the folder and document no-op re-grant paths now return the grant built by grant.NewGrant(entitlement.Resource, entitlement.Slug, resource.Id) alongside GrantAlreadyExists, and both tests were updated to assert the returned grant targets the folder/document rather than the principal. Verified against the vendored SDK (vendor/.../pkg/types/grant/grant.go:88-113) that this produces grant ID folder:9001:user/edit:user:100, which does match what Grants() emits. The full PR diff was also scanned for security and correctness; no security issues and no blocking correctness issues were found. The two non-blocking suggestions below concern the sibling success path in the same functions, which this fix leaves inconsistent.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• pkg/connector/folder.go:175 / pkg/connector/document.go:190 — the upsert success path passes resource (the user principal) as NewGrant's resource arg, so a first-time grant emits entitlement ID user:100:user/edit while the new no-op path emits folder:9001:user/edit; only the latter matches Grants(). Pre-existing and on unchanged lines, but the new tests assert grant identity only for the re-grant branch, so the divergence is untested.
• pkg/connector/folder.go:156 / pkg/connector/document.go:171 — the no-op grant omits the metaRole/metaCreated grant metadata that both Grants() and the upsert path attach; current.Role and current.Created are already available.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/folder.go`:
- Around line 175: grant.NewGrant(resource, folderHasUserAccessEntitlement+response.Role, userID, ...) passes `resource` as the first argument, but in Grant that parameter is the user principal, not the folder. NewGrant's first argument is the resource the entitlement lives on, so this builds entitlement ID `user:<uid>:user/<role>` and grant ID `user:<uid>:user/<role>:user:<uid>`, which does not match what Grants() (line 121) or the new no-op path (line 156) emit. Change the first argument to `entitlement.Resource` so the success path produces `folder:<folderId>:user/<role>:user:<uid>`. Then extend the "new grant returns no already-exists annotation and calls upsert" and "role change is not treated as already-exists" subtests in pkg/connector/folder_test.go to assert grants[0].Entitlement.Resource.Id.Resource equals the folder ID and grants[0].Principal.Id.Resource equals the user ID, mirroring the assertions already added to the re-grant subtest.
- Around line 156: the no-op re-grant builds its grant without grant.WithGrantMetadata(...), so it lacks the metaRole/metaCreated entries that both Grants() (line 121) and the upsert path (line 175) attach. The `current` value returned by GetFolderUserCollaborator already carries Role and Created, so pass grant.WithGrantMetadata with metaRole set to current.Role and metaCreated set to current.Created.String(), making the emitted grant identical to the one the next sync produces.

In `pkg/connector/document.go`:
- Around line 190: same issue as the folder path — grant.NewGrant(resource, documentHasUserAccessEntitlement+response.Role, userID, ...) passes the user principal where the document resource belongs, yielding entitlement ID `user:<uid>:user/<role>` instead of `document:<documentId>:user/<role>`. Change the first argument to `entitlement.Resource`, and extend the corresponding subtests in pkg/connector/document_test.go to assert the returned grant's entitlement resource is the document ID and its principal is the user ID.
- Around line 171: the no-op re-grant omits the metaRole/metaCreated grant metadata attached by Grants() (line 136) and the upsert path (line 190). Add grant.WithGrantMetadata with metaRole set to current.Role and metaCreated set to current.Created.String(), using the `current` response from GetDocumentUserCollaborator.

Note: this summary was posted as a review body rather than an update to the existing summary comment — the review harness blocked every command containing the literal review-state JSON marker, so patching that comment would have destroyed its stored state (last_reviewed_sha). The existing summary comment was left intact so the next incremental run still resolves its state correctly.

[sergiocorral-conductorone]

Addressed the two suggestions from the latest review (5044138632):

Fixed — no-op re-grant now carries grant metadata (5e61eb6)
pkg/connector/folder.go:156 / pkg/connector/document.go:171: the no-op re-grant path built its grant with grant.NewGrant(entitlement.Resource, entitlement.Slug, resource.Id) but without grant.WithGrantMetadata(...), so it lacked the metaRole/metaCreated entries that both Grants() and the upsert-success path attach. It now builds the metadata map from the current collaborator returned by the pre-check GET (which already carries .Role and .Created), mirroring the upsert path exactly:

metadata := map[string]interface{}{
    metaRole:    current.Role,
    metaCreated: current.Created.String(),
}
newGrant := grant.NewGrant(entitlement.Resource, entitlement.Slug, resource.Id, grant.WithGrantMetadata(metadata))

So the immediately-materialized no-op grant is now metadata-identical to what the next full sync produces. Added assertions in folder_test.go/document_test.go that the no-op grant's metadata (metaRole/metaCreated) equals the metadata the upsert-success path emits for the same record (via a new grantMetadata test helper). Full suite passes under -race; go build, gofmt, go vet clean.

Acknowledged, out of scope — CXH-1919 arg-swap on the upsert-success path
pkg/connector/folder.go:175 / document.go:190: the upsert-success path passes resource (the user principal) as NewGrant's first arg instead of entitlement.Resource, producing a divergent entitlement ID. That's the known CXH-1919 ticket (still open, unclaimed, pre-existing on unchanged lines) — intentionally not touched in this PR to keep the diff scoped to CXH-2285. It'll be fixed under its own ticket.

[github-actions]

🟡 Suggestion: the comment above says this "proves the no-op metadata is identical to what the upsert-success path emits", but the assertion only compares the GrantMetadata annotation — the two paths still produce different grant IDs. The no-op builds NewGrant(entitlement.Resource, ...) → folder:9001:user/edit:user:100, while the success path at folder.go:179 passes resource (the user principal) → user:100:user/edit:user:100. Adding require.Equal(t, upGrants[0].Id, grants[0].Id) here (and at document_test.go:60) would lock in the parity this PR is about and catch the sibling-path mismatch already flagged at folder.go:156 / document.go:171. Confidence: high.

[github-actions]

No blocking issues found.