[sergiocorral-conductorone]

INTENT

• Grant and Revoke read the current role without the cache before writing.
• No write happens when the user is already in the requested state, and the call says so through an annotation.
• Admin roles are not overwritten without an explicit decision recorded in the docs.
• Tests use a single client (grant then revoke, and a role changed between two calls) and fail if the no-cache option is removed.
• README and docs/connector.mdx describe the resulting behavior.

Fixes CXH-3520

What and why

• Read the current site role without the response cache before Grant/Revoke. Exact-role Grant is a no-op (GrantAlreadyExists); missing-user, stale-role and held-Unlicensed Revoke are no-ops (GrantAlreadyRevoked), with no PUT.
• Replace a different requested Site Role exactly, including explicit SiteAdministrator changes, and return GrantReplaced for the prior site-role grant only after a successful write. A held licensed role is revoked to Unlicensed.
• Refuse site-role writes that would remove ServerAdministrator (FailedPrecondition, no PUT). The connector does not assign that server-wide role; this is an explicit protection policy, not a claim that the API itself rejects such changes.
• Add a per-site WithExclusionGroup to Site Role entitlements. Entitlement/grant IDs are unchanged; this does not assert automatic cleanup of historical C1 assignments.
• Document the agreed rules in README and connector docs. License behavior is unchanged.

Dependency / review scope

Stacked on #55 at 42f1601b: it supplies the shared uncached GetUser, IsNotFoundError and annotation-merging helper. This diff contains only the Site Role follow-up (5 files). The stack is staging → #55 → #56; #55 targets staging. Rebased the four Site Role commits onto the updated #55 head; no license commits were replayed, and the Site Role patch is unchanged.

How tested

• Rebased head 221aaf01: go build ./..., go test ./... -count=1 and go vet ./... pass. Local golangci-lint is unavailable; CI lint passes. All 5 executed checks pass (lint, tests, docs and metadata); 3 jobs are skipped. The added/removed lines match the previous Site Role-only patch exactly.

• Before rebase, head 3398f7e5: build, full unit suite, vet and golangci-lint v2.14.0 passed. Includes [CXH-2368] - Fix license grant rewriting the site role of a user who already holds the tier #55's license materialization regression. The Site Role update is comments/docs only; affected README text was reduced from 228 to 198 words, the Site roles docs section from 244 to 212, with existing policies retained. No new live provisioning run.

• go build ./..., go vet ./..., go test ./... -count=1: pass. Capabilities regenerated from the binary and unchanged.

• Stateful mock tests cover exact no-ops, replacements, administrative guards, deleted users, read/write failures and one-client sequences with external role changes. Removing WithNoCache makes the cache-sensitive regression tests fail; restored source passes.

• Live on a non-production Tableau Cloud trial, commit 43cf3e4d: 12 Grant/Revoke scenarios pass, with independent REST role read-back after every action (Creator, Unlicensed and SiteAdministratorCreator, including stale revokes and deleted users).

• Two baseline syncs pass (28 resources / 269 entitlements / 534 grants); all 9 Site Role entitlements have the exclusion annotation; site entitlement/grant IDs are stable.

• One temporary reserved-domain fixture was created, restored to Unlicensed and deleted. Final tenant state matches baseline: one operator user still SiteAdministratorCreator; operator account and license configuration untouched.

• Containerization audit: platform confirms deployment support, not excluded, field mapping and admin registration present; already containerized. No containerization changes included.

Verification limits

• The old local linter could not read Go 1.27 export data. Using CI's golangci-lint v2.14.0 resolved that tooling limitation; the three G705 mock-JSON findings were fixed in 453b91b1, and lint/tests/docs/metadata passed on that revision. No module downgrade or global config changes. Integration/regression were skipped; version checks and automated review did not run against the stacked base.
• Viewer/Explorer have no seats in the trial; those roles are covered by unit tests only. ServerAdministrator exists on Tableau Server, not this Cloud trial; protection is covered by mocks and official API documentation.
• One-shot live calls do not expose response annotations or prove same-client cache behavior; those assertions are unit-tested, including mutation sensitivity. The live matrix used the connector CLI flags directly rather than baton-test (CLI/bundle compatibility limitation).
• C1 backend migration behavior for existing assignments receiving the new exclusion annotation was not tested.

🤖 Generated with AI

[linear-code]

CXH-3520

[johnallers]

[Minor] Site-role writes do not update the license grant

pkg/connector/site.go:215-229, pkg/connector/license.go:28

A site-role write changes the license tier, but no license grant is updated. A site-role Grant of Creator to a user with a Viewer license emits GrantReplaced only for site:…:viewer. C1 keeps the stale license Viewer grant until the next sync. C1 does not see the new Creator license grant until then. A Revoke to Unlicensed has the same gap. License writes in the reverse direction also have this gap.

The two exclusion groups (site-role:<siteID> and tableau-license) model the same Tableau siteRole field. Neither group stops a concurrent "license X" request and "site role Y" request from overwriting each other. The fresh read keeps revokes safe. Only concurrent grants can conflict. This matches "License behavior is unchanged" in the PR body.

Suggestion: state this as a known limitation in the README and connector docs. Alternatively, emit the matching license GrantReplaced in a follow-up.

[johnallers]

[Minor] docs/docs-info.md is stale for site-role provisioning

docs/docs-info.md:122-124

The text still says "Grant: Update a user's site role / Revoke: Set a user's site role to Unlicensed". It omits the uncached read, the no-op annotations, the ServerAdministrator refusal, and the exclusion group. PR #55 updated this file for licenses. The three doc surfaces now disagree for site roles.

Suggestion: update this section to match the README and connector.mdx.

[johnallers]

[Minor] No guard against self-lockout of the connector account

pkg/connector/site.go:213, pkg/connector/site.go:273

Nothing stops the connector from removing its own admin rights. Assume the PAT owner is SiteAdministratorCreator. A site-role Grant of Viewer, or a Revoke of their admin role, then succeeds. The connector loses admin rights, and later syncs fail with 403. Login returns Credentials.User.ID (pkg/client/client.go:136-183), but the client does not keep it.

Suggestion: refuse a demotion of the authenticated user with FailedPrecondition, or document the risk. This is a policy choice.

[johnallers]

[Minor] Exclusion group has no default and no order

pkg/connector/site.go:100

Revoke always writes Unlicensed, but C1 does not know that Unlicensed is the fallback. C1 can show a user with no site role. WithExclusionGroupDefault on the Unlicensed entitlement would match the real behavior.

Suggestion: if the team wants this, add it now. A later addition is another C1 migration.

[johnallers]

[Minor] PUT rate-limit annotations are dropped on error

pkg/client/client.go:313-315, pkg/client/client.go:327-329

UpdateUserSiteRole and RemoveUserFromSite return nil, err on failure. A 429 on the PUT then carries only the rate-limit data from the earlier GET. The gRPC code stays Unavailable, so retries still occur. This code predates #56, but #56 depends on it.

Suggestion: return annos, err.

[luisina-santos]

docs/docs-info.md still describes the old behavior (Provisioning section: "Revoke: Set a user's site role to Unlicensed") and doesn't mention the no-op cases or the ServerAdministrator guard. Please update it to match the new Site roles section in connector.mdx.

[sergiocorral-conductorone]

Thanks @johnallers and @luisina-santos. Addressed in this round:

• docs-info site-role section (both): rewritten in 0f8b0ea to match connector.mdx: uncached read, no-ops, ServerAdministrator handling, exclusion group and default, self-lockout guard.
• Site-role writes don't update the license grant: documented as a known limitation in the README, connector.mdx and docs-info (0f8b0ea). C1 only updates the grant of the entitlement that was written, and the other one is corrected on the next sync.
• Self-lockout: the client now keeps the signed-in user ID from Login (d35699f). A license or site-role Grant/Revoke that would write a role change for that user is refused with FailedPrecondition and no write; no-op outcomes stay no-ops (50bf67f).
• Exclusion group default: Unlicensed is now the default (order 0) of both the site-role and the license groups (9fee2df). License entitlement IDs are unchanged (license:<tier>:member).
• PUT annotations on error: UpdateUserSiteRole and RemoveUserFromSite now return annos, err (d35699f).

[mateoHernandez123]

WithGrantableTo(resourceTypeUser) is applied to all nine roles, so server administrator is advertised as a grantable entitlement — and this PR now also puts it in the mandatory site-role:<siteID> group. But Grant rejects it unconditionally at line 186 (nonGrantableRoles), so every access request for "Site Server Administrator" in C1 fails, and nothing self-heals on the next sync.

The grantable side predates this PR; it becomes worth fixing here because these are the lines that add the entitlement to the exclusion group.

Suggestion

Drop ServerAdministrator from the grantable set and mark its sync grants immutable instead, so C1 never offers the operation:

if nonGrantableRoles[apiRole] {
    grantOpts = append(grantOpts, grant.WithAnnotation(&v2.GrantImmutable{}))
}

See also

• This repo already does exactly this for the "All Users" group in pkg/connector/group.go:126-131
• Sibling: baton-box role.go#L120-L124 — same pattern for a role the connector will not provision
• GrantImmutable would also stop C1 dispatching a Revoke of a held ServerAdministrator; I am not reopening that policy, which you documented, but it is worth weighing while you are here.