Repository navigation
[CXH-2368] - Fix license grant rewriting the site role of a user who already holds the tier - #55
Conversation
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No blocking issues found — see the full review report
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
mateoHernandez123
left a comment
mateoHernandez123
left a comment
There was a problem hiding this comment.
LGTM — the approach is right, and the inline notes are mostly nits.
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
…already holds the tier
GetUser goes through the SDK in-memory cache, so a revoke after a grant on the same client could read the pre-grant role. doRequest now accepts extra request options and GetUser passes WithNoCache; GetUsers stays cacheable. Add single-client tests for grant-then-revoke and for a role changed externally between two grants, and switch TestGetUser_Found to a literal JSON fixture.
… license docs/comments
c2aab5d to
42f1601
Compare
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
…heck, plain-language docs
…-rewriting-the-site-role-of-a
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
Superseded — see the current review report for commit
|
Connector PR Review: [CXH-2368] - Fix license grant rewriting the site role of a user who already holds the tierBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryWhat the PR does. License Grant and Revoke now read the user's current site role before writing anything:
This push. The new commit switches both Coverage. I scanned the full PR diff for security and correctness: Repo-local criteria. I read all of Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Resolved prior findings
Reviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
| // NotFound for a missing user, GrantAlreadyExists (no write) for a role already | ||
| // in the tier, FailedPrecondition for an admin role, else a write that reports | ||
| // GrantReplaced when the previous role belonged to a known tier. | ||
| func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { |
There was a problem hiding this comment.
Grant keeps the V1 signature, so the grant it creates never reaches C1. The SDK wraps a V1 provisioner in an adapter that drops them: resource_provisioner.go#L160-L167 (return nil, annos, nil). C1 reads that list to create the AppEntitlementUserBinding, so the replacement is recorded and the new binding isn't — with no error anywhere.
The signature predates this PR. What puts it in scope is that WithExclusionGroup (line 220) and the GrantReplaced on line 314 are new here, and they're the half that needs the grant to arrive.
Same shape, already solved in baton-claude-enterprise organizations.go#L201-L261: nil grants on the already-satisfied path, and AppendGrantReplaced plus []*v2.Grant{grant.NewGrant(...)} on the replace path. baton-brex titles.go#L66-L87 is the compact version.
func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) ([]*v2.Grant, annotations.Annotations, error)Return nil grants on the GrantAlreadyExists and FailedPrecondition paths, the new grant on the write path. Revoke stays as is — the SDK has no V2 variant for it.
INTENT
Fixes CXH-2368
What / why
licenseBuilder.Grantresolved the entitlement to a literal role and PUT it unconditionally, without reading the user's current site role. Because several site roles map to one tier (RolesPerLicense), granting a tier the user already held overwrote e.g.SiteAdministratorCreatorwithCreator.Revokelikewise PUTUnlicensedwithout checking what the user currently holds.How
Client.GetUser(single-user GET) andclient.IsNotFoundError.GrantAlreadyExists(the ticket's bug).SiteAdministrator,SiteAdministratorCreator,SiteAdministratorExplorer, orServerAdministrator) outside the tier: refused withFailedPrecondition, no write. A license grant must not silently remove admin rights. Use the Site Role entitlement to assign aSiteAdministrator*role; this connector does not assignServerAdministrator.GrantReplacednaming that previous license grant.NotFound.GrantAlreadyRevokedwithout writing; otherwise it setsUnlicensedas before.tableau-licenseexclusion group; entitlement and grant IDs are unchanged. This supplies the entitlement metadata required for C1 to process the existingGrantReplacedresponse when exclusion groups are enabled and ingested. No default tier, ordering, or automatic historical-assignment cleanup is introduced.docs/connector.mdx, anddocs/docs-info.mddescribe tier-aware provisioning, no-op cases, admin restrictions, and the Unlicensed floor state.Limits / open decisions
Unlicensedwhen they hold the tier (including legacySiteAdministratorandServerAdministrator, which this connector does not assign); that is the explicit meaning of revoke, it is documented in the README andconnector.mdx, and an admin guard on Revoke is a follow-up.unlicensedtier from an already-Unlicensed user is a successful no-op withGrantAlreadyRevoked, preserving read annotations and sending no PUT. Returning an error would create an unactionable offboarding failure. Unlicensed is the floor role: the user remains Unlicensed and the next sync will show the Unlicensed entitlement in C1 again. This reports the correct current state; the no-op does not remove the floor entitlement.site.go) has the same missing idempotency annotations; not changed here.GrantReplaced(the previous license grant). The same write also changes the user's site-role grant (site:<id>:<role>), which is not reported: ConductorOne reads a singleGrantReplacedper response and only resolves it against exclusion-group bindings, so a second one would be ignored. The site-role grant reconciles on the next sync.Testing
735528db: build, full unit suite, vet and golangci-lint v2.14.0 pass. An SDK sync to a temporary c1z verifies all four tier IDs and their shared exclusion group; removing the annotation makes the test fail. Same-tier, refused, and failed grants emit no replacement annotation. README, connector docs and code comments were shortened without changing provisioning policy. This revision used mocks only (no live seat/grant or C1 rollout validation).Unit (httptest, assert method/body/count): already-in-tier grant for Creator, SiteAdministratorCreator, ServerAdministrator, Explorer, SiteAdministratorExplorer, ExplorerCanPublish (no PUT,
GrantAlreadyExists); cross-tier non-admin (one PUT +GrantReplaced); cross-tier admin (FailedPrecondition, no PUT); unknown user; revoke stale / held / 404; clientGetUser200 and 404.Also covered:
GetUser500 (error, no PUT), a role outside every tier (one PUT, noGrantReplaced),ExplorerCanPublishto Creator (GrantReplacedfor the explorer grant), rate-limit annotations preserved on every return path, and revoke of the Unlicensed tier.go build ./...,go vet ./...,go test ./...pass.Live, Tableau Cloud trial (non-prod), role read back via REST after each step: bug reproduced on
main(SiteAdministratorCreator->Creator, exit 0); with the fix the role is preserved for SiteAdministratorCreator, SiteAdministratorExplorer and ExplorerCanPublish, a cross-tier grant to an admin is refused, a non-admin cross-tier grant replaces, a held revoke sets Unlicensed, and a revoke from a stale bundle (role changed out of band to Viewer) leaves Viewer untouched.Historical live evidence on earlier commit
b3a60bcb: the same run was repeated, including the old Unlicensed rejection (FailedPrecondition, role unchanged). That floor-tier behavior is superseded by the no-op decision above; this was not a live test of the latest revision.Latest Unlicensed no-op regression:
TestLicenseRevoke_UnlicensedTier/user_is_unlicensedfailed before the fix and passes after it, asserting nil error,GrantAlreadyRevoked, zero PUTs and preserved rate-limit annotations. Full unit suite, build and vet pass. The earlier local-linter toolchain limitation is resolved by using CI's v2.14.0 (see current verification above).Not verified live: the latest Unlicensed no-op behavior,
ServerAdministrator(not assignable on Cloud), and the annotations on the wire (asserted in unit tests only).🤖 Generated with AI