From 33b2565c2ce268ec7f37129061b9e3a0abc425d0 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Fri, 16 Oct 2026 00:39:58 -0300 Subject: [PATCH 01/13] [CXH-2368] - Fix license grant rewriting the site role of a user who already holds the tier --- README.md | 1 + docs/connector.mdx | 2 + pkg/client/client.go | 25 +++- pkg/client/client_test.go | 74 ++++++++++ pkg/connector/license.go | 113 ++++++++++++++- pkg/connector/license_test.go | 262 ++++++++++++++++++++++++++++++++++ 6 files changed, 471 insertions(+), 6 deletions(-) create mode 100644 pkg/client/client_test.go diff --git a/README.md b/README.md index a1e39351..8b05cb93 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,7 @@ baton resources - **Grant expansion**: Group-based permissions on projects, workbooks, and views are supported. When a group has a permission, all members inherit it. - **"All Users" group**: Tableau automatically adds every user to the built-in "All Users" group. This group cannot be modified via the API — Grant and Revoke operations on it are not supported. The connector syncs its membership to show each user's base site role. - **License revoke constraints**: Revoking a license (setting a user to "Unlicensed") will fail if the user belongs to a group with a "Grant role on sign in" minimum site role. Remove the user from the constraining group first, then revoke the license. +- **License grant is tier-aware**: A license tier is a view of the user's single site role (e.g. `SiteAdministratorCreator` already satisfies the Creator tier), so granting a tier the user already holds is a no-op — it never overwrites their current role with the tier's base role. Granting a *different* tier to a user currently holding a `SiteAdministrator*`/`ServerAdministrator` role is refused instead of silently demoting them; use the Site Role entitlement to change an admin role explicitly. - **Idempotency**: Granting a permission that already exists returns success (not an error). Revoking a permission that is already removed also returns success. # Credentials Setup diff --git a/docs/connector.mdx b/docs/connector.mdx index 9a67d72e..94d85720 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -33,6 +33,8 @@ In Tableau, licensing is role-based: each user's **site role** determines their When your Tableau site has per-tier seat capacities configured, C1 tracks the purchased and consumed seat counts for each license tier, so you can review license utilization alongside who holds each tier. +Because a tier is a view of the user's single site role, several roles can satisfy the same tier (for example, `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op and never overwrites a more privileged role with the tier's base role. Granting a *different* tier to a user who currently holds a Site Administrator or Server Administrator role is refused rather than silently removing their admin access — use the Site Role entitlement to change an admin role explicitly. + ## Permission inheritance Tableau uses a permission inheritance model that affects how access is synced and provisioned: diff --git a/pkg/client/client.go b/pkg/client/client.go index 12c638f5..a5b551e7 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -10,7 +10,7 @@ // // Users: // - GET /api/{version}/sites/{siteId}/users - List users (paginated) -// - GET /api/{version}/sites/{siteId}/users/{userId} - Verify current user +// - GET /api/{version}/sites/{siteId}/users/{userId} - Verify current user / get a single user // - POST /api/{version}/sites/{siteId}/users - Add user to site // - PUT /api/{version}/sites/{siteId}/users/{userId} - Update user site role // - DELETE /api/{version}/sites/{siteId}/users/{userId} - Remove user from site @@ -281,6 +281,23 @@ func (c *Client) AddUserToSite(ctx context.Context, user CreateUserRequest) (*Us return res.User, annos, nil } +// GetUser returns a single user by ID. Callers can distinguish a missing +// user from other failures with IsNotFoundError. +func (c *Client) GetUser(ctx context.Context, userId string) (*User, annotations.Annotations, error) { + urlGetUser, err := c.buildSiteURL(pathUsers, userId) + if err != nil { + return nil, nil, err + } + + var res userResponse + annos, err := c.doRequest(ctx, http.MethodGet, urlGetUser, &res, nil) + if err != nil { + return nil, annos, err + } + + return &res.User, annos, nil +} + // UpdateUserSiteRole updates a user's site role. func (c *Client) UpdateUserSiteRole(ctx context.Context, userId, siteRole string) (annotations.Annotations, error) { urlUpdateUserSiteRole, err := c.buildSiteURL(pathUsers, userId) @@ -773,6 +790,12 @@ func permissionBody(granteeType, granteeID, capabilityName, capabilityMode strin } } +// IsNotFoundError reports whether err is the error doRequest returns for an +// HTTP 404 response (e.g. GetUser on a deleted/unknown user ID). +func IsNotFoundError(err error) bool { + return status.Code(err) == codes.NotFound +} + // doRequest executes an HTTP request against the Tableau API. func (c *Client) doRequest(ctx context.Context, method string, url *url.URL, res, body any) (annotations.Annotations, error) { reqOpts := []uhttp.RequestOption{ diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go new file mode 100644 index 00000000..a429ce39 --- /dev/null +++ b/pkg/client/client_test.go @@ -0,0 +1,74 @@ +package client + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/conductorone/baton-sdk/pkg/uhttp" + "github.com/stretchr/testify/require" +) + +const testSiteId = "site-1" + +// newTestClient builds a Client wired directly to srv, bypassing New's +// Login/auth-signin flow so tests only exercise the endpoint under test. +func newTestClient(t *testing.T, srv *httptest.Server) *Client { + t.Helper() + + base, err := uhttp.NewBaseHttpClientWithContext(context.Background(), srv.Client()) + require.NoError(t, err) + + return &Client{ + httpClient: base, + baseUrl: srv.URL, + authToken: "test-token", + siteId: testSiteId, + } +} + +func TestGetUser_Found(t *testing.T) { + t.Parallel() + + wantPath := "/sites/" + testSiteId + "/users/user-1" + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != wantPath { + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(userResponse{ + User: User{ID: "user-1", Email: "user1@example.com", SiteRole: "SiteAdministratorCreator"}, + }) + })) + t.Cleanup(srv.Close) + + c := newTestClient(t, srv) + + user, _, err := c.GetUser(context.Background(), "user-1") + require.NoError(t, err) + require.NotNil(t, user) + require.Equal(t, "user-1", user.ID) + require.Equal(t, "SiteAdministratorCreator", user.SiteRole) +} + +func TestGetUser_NotFound(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":{"summary":"Not Found","detail":"User not found","code":"0x1"}}`)) + })) + t.Cleanup(srv.Close) + + c := newTestClient(t, srv) + + user, _, err := c.GetUser(context.Background(), "missing-user") + require.Error(t, err) + require.Nil(t, user) + require.True(t, IsNotFoundError(err), "expected IsNotFoundError(err) to be true, got err: %v", err) +} diff --git a/pkg/connector/license.go b/pkg/connector/license.go index a8bac42b..f2606134 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -3,6 +3,7 @@ package connector import ( "context" "fmt" + "slices" "strconv" "strings" @@ -14,6 +15,8 @@ import ( "github.com/conductorone/baton-tableau/pkg/client" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) var licenses = []string{creator, explorer, viewer, unlicensed} @@ -34,6 +37,29 @@ var RolesPerLicense = map[string][]string{ unlicensed: {unlicensed}, } +// adminLicenseRoles are the site roles a cross-tier license Grant must never +// silently demote. A license tier is a derived view of a user's single site +// role, so granting a different tier is itself a role change; stripping an +// admin role as a side effect of that change would be an unannounced +// privilege reduction. See the CXH-2368 decision doc: use the Site Role +// entitlement to change an admin role explicitly instead. +var adminLicenseRoles = map[string]bool{ + siteAdministrator: true, + siteAdministratorCreator: true, + siteAdministratorExplorer: true, + serverAdministrator: true, +} + +// tierForRole returns the license tier whose RolesPerLicense set contains role. +func tierForRole(role string) (string, bool) { + for _, tier := range licenses { + if slices.Contains(RolesPerLicense[tier], role) { + return tier, true + } + } + return "", false +} + // licenseCapacity returns the raw per-tier capacity string, or nil when the site exposes none. func licenseCapacity(license string, site *client.Site) *string { if site == nil { @@ -238,23 +264,100 @@ func (l *licenseBuilder) Grants(ctx context.Context, resource *v2.Resource, opts } // Grant assigns the target license tier by setting the user's site role. +// +// A Tableau user has exactly one site role, and RolesPerLicense groups which +// roles satisfy which tier, so a license tier is a derived view of that one +// role rather than an independent attribute. Per CXH-2368, Grant therefore +// reads the user's current role before writing: +// - already in the target tier -> no write, GrantAlreadyExists (this was +// the ticket's bug: granting a held tier used to overwrite e.g. +// SiteAdministratorCreator with plain Creator). +// - current role is an admin role outside the tier -> refuse with +// FailedPrecondition; a license grant must not double as a silent +// demotion, and ServerAdministrator cannot be restored via this API. +// - otherwise -> write the tier's base role and report GrantReplaced +// naming the license grant the write displaced (tiers are exclusive, so +// a cross-tier grant is itself the requested role change). func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { - licenseName, ok := licensesMap[entitlement.Resource.Id.Resource] + log := ctxzap.Extract(ctx) + + targetTier, ok := licensesMap[entitlement.Resource.Id.Resource] if !ok { return nil, fmt.Errorf("unknown license %s", entitlement.Resource.Id.Resource) } - annos, err := l.client.UpdateUserSiteRole(ctx, principal.Id.Resource, licenseName) + user, _, err := l.client.GetUser(ctx, principal.Id.Resource) if err != nil { - return annos, fmt.Errorf("failed to grant %s license to user %s: %w", licenseName, principal.Id.Resource, err) + if client.IsNotFoundError(err) { + return nil, status.Errorf(codes.NotFound, "license: cannot grant %s: user %s not found", targetTier, principal.Id.Resource) + } + return nil, fmt.Errorf("failed to read user %s for license grant: %w", principal.Id.Resource, err) + } + + if slices.Contains(RolesPerLicense[targetTier], user.SiteRole) { + log.Debug("license grant: user already in target tier, no write needed", + zap.String("user_id", principal.Id.Resource), zap.String("tier", targetTier), zap.String("site_role", user.SiteRole)) + return annotations.New(&v2.GrantAlreadyExists{}), nil + } + + if adminLicenseRoles[user.SiteRole] { + log.Warn("license grant: refusing to move an admin role across license tiers", + zap.String("user_id", principal.Id.Resource), zap.String("tier", targetTier), zap.String("site_role", user.SiteRole)) + return nil, status.Errorf(codes.FailedPrecondition, + "license: cannot grant %s to user %s: current site role %q is an administrative role; use the Site Role entitlement to change it explicitly", + targetTier, principal.Id.Resource, user.SiteRole) + } + + annos, err := l.client.UpdateUserSiteRole(ctx, principal.Id.Resource, targetTier) + if err != nil { + return annos, fmt.Errorf("failed to grant %s license to user %s: %w", targetTier, principal.Id.Resource, err) + } + + if oldTier, ok := tierForRole(user.SiteRole); ok && oldTier != targetTier { + oldResource := &v2.Resource{ + Id: &v2.ResourceId{ + ResourceType: resourceTypeLicense.Id, + Resource: strings.ToLower(oldTier), + }, + } + oldEntitlement := ent.NewAssignmentEntitlement(oldResource, memberEntitlement) + annos.Append(&v2.GrantReplaced{ReplacedGrantId: grant.NewGrantID(principal.Id, oldEntitlement)}) } return annos, nil } // Revoke removes the license by setting the user's site role to Unlicensed. -func (l *licenseBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations.Annotations, error) { - principalID := grant.Principal.Id.Resource +// +// Per CXH-2368, Revoke reads the user's current role first: a 404, or a role +// that has already moved out of this tier (e.g. a later Grant replaced it — +// see Grant's GrantReplaced annotation), means there is nothing left to +// revoke. Writing Unlicensed in that case would wipe out access this revoke +// was never asked to touch and was already reported as GrantAlreadyExists +// (or GrantReplaced) by the operation that actually changed it. +func (l *licenseBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.Annotations, error) { + log := ctxzap.Extract(ctx) + principalID := g.Principal.Id.Resource + + tier, ok := licensesMap[g.Entitlement.Resource.Id.Resource] + if !ok { + return nil, fmt.Errorf("unknown license %s", g.Entitlement.Resource.Id.Resource) + } + + user, _, err := l.client.GetUser(ctx, principalID) + if err != nil { + if client.IsNotFoundError(err) { + log.Debug("license revoke: user not found, nothing to revoke", zap.String("user_id", principalID)) + return annotations.New(&v2.GrantAlreadyRevoked{}), nil + } + return nil, fmt.Errorf("failed to read user %s for license revoke: %w", principalID, err) + } + + if !slices.Contains(RolesPerLicense[tier], user.SiteRole) { + log.Debug("license revoke: user no longer in tier, nothing to revoke", + zap.String("user_id", principalID), zap.String("tier", tier), zap.String("site_role", user.SiteRole)) + return annotations.New(&v2.GrantAlreadyRevoked{}), nil + } annos, err := l.client.UpdateUserSiteRole(ctx, principalID, unlicensed) if err != nil { diff --git a/pkg/connector/license_test.go b/pkg/connector/license_test.go index 3b65087d..d4f358bc 100644 --- a/pkg/connector/license_test.go +++ b/pkg/connector/license_test.go @@ -1,10 +1,19 @@ package connector import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" "testing" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/conductorone/baton-tableau/pkg/client" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // TestLicenseResource_Trait asserts the LicenseProfileTrait is attached with the @@ -107,3 +116,256 @@ func TestCapacitySeats(t *testing.T) { }) } } + +// ============================================================================= +// Grant / Revoke (CXH-2368) +// ============================================================================= + +const licenseTestSiteID = "site-1" + +type recordedPut struct { + userID string + siteRole string +} + +// newLicenseTestClient starts a mock Tableau API server that authenticates +// any login, serves GetUser from users (keyed by user ID, valued by current +// site role; a missing key means a 404), and records every PUT +// (UpdateUserSiteRole) it receives so tests can assert exact write counts +// and bodies. +func newLicenseTestClient(t *testing.T, users map[string]string, puts *[]recordedPut) *client.Client { + t.Helper() + + usersPathPrefix := "/sites/" + licenseTestSiteID + "/users/" + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == "/auth/signin": + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "credentials": map[string]any{ + "site": map[string]any{"id": licenseTestSiteID}, + "user": map[string]any{"id": "me"}, + "token": "test-token", + }, + }) + case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, usersPathPrefix): + userID := strings.TrimPrefix(r.URL.Path, usersPathPrefix) + role, ok := users[userID] + if !ok { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":{"summary":"Not Found","detail":"user not found","code":"0x1"}}`)) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "user": map[string]any{"id": userID, "siteRole": role}, + }) + case r.Method == http.MethodPut && strings.HasPrefix(r.URL.Path, usersPathPrefix): + userID := strings.TrimPrefix(r.URL.Path, usersPathPrefix) + var body struct { + User struct { + SiteRole string `json:"siteRole"` + } `json:"user"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + *puts = append(*puts, recordedPut{userID: userID, siteRole: body.User.SiteRole}) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "user": map[string]any{"id": userID, "siteRole": body.User.SiteRole}, + }) + default: + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":{"summary":"Not Found","detail":"unexpected request","code":"0x2"}}`)) + } + })) + t.Cleanup(srv.Close) + + c, err := client.New(context.Background(), "unused", licenseTestSiteID, "token-name", "token-secret", "3.19", srv.URL) + require.NoError(t, err) + return c +} + +func testUserPrincipal(id string) *v2.Resource { + return &v2.Resource{Id: &v2.ResourceId{ResourceType: resourceTypeUser.Id, Resource: id}} +} + +func testLicenseEntitlement(tier string) *v2.Entitlement { + return &v2.Entitlement{ + Resource: &v2.Resource{ + Id: &v2.ResourceId{ResourceType: resourceTypeLicense.Id, Resource: strings.ToLower(tier)}, + }, + } +} + +// TestLicenseGrant_AlreadyInTier covers acceptance criteria 1 and 3: granting +// a tier to a user whose current site role already satisfies that tier must +// perform no write and report GrantAlreadyExists -- this was the exact shape +// of the CXH-2368 bug (a SiteAdministratorCreator re-granted Creator was +// demoted to plain Creator). +func TestLicenseGrant_AlreadyInTier(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + tier string + curRole string + }{ + {"creator tier: Creator", creator, creator}, + {"creator tier: SiteAdministratorCreator", creator, siteAdministratorCreator}, + {"creator tier: ServerAdministrator", creator, serverAdministrator}, + {"explorer tier: Explorer", explorer, explorer}, + {"explorer tier: SiteAdministratorExplorer", explorer, siteAdministratorExplorer}, + {"explorer tier: ExplorerCanPublish", explorer, explorerCanPublish}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{"user-1": tt.curRole}, &puts) + l := newLicenseBuilder(c) + + annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(tt.tier)) + require.NoError(t, err) + require.Empty(t, puts, "already-in-tier grant must not write") + require.True(t, annos.Contains(&v2.GrantAlreadyExists{})) + }) + } +} + +// TestLicenseGrant_CrossTierNonAdmin covers the decision doc's 3b: a +// non-admin role outside the target tier is replaced, with exactly one PUT +// and a GrantReplaced annotation naming the user's previous license grant. +func TestLicenseGrant_CrossTierNonAdmin(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{"user-1": viewer}, &puts) + l := newLicenseBuilder(c) + + annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + require.NoError(t, err) + + require.Len(t, puts, 1, "cross-tier grant must write exactly once") + require.Equal(t, "user-1", puts[0].userID) + require.Equal(t, creator, puts[0].siteRole) + + require.False(t, annos.Contains(&v2.GrantAlreadyExists{})) + + var replaced v2.GrantReplaced + found, err := annos.Pick(&replaced) + require.NoError(t, err) + require.True(t, found, "expected a GrantReplaced annotation") + require.Equal(t, "license:viewer:member:user:user-1", replaced.GetReplacedGrantId()) +} + +// TestLicenseGrant_CrossTierAdmin covers decision 3a: granting a different +// tier to a user currently holding an admin role must refuse with +// FailedPrecondition rather than silently stripping admin rights, and must +// not write. +func TestLicenseGrant_CrossTierAdmin(t *testing.T) { + t.Parallel() + + adminRolesUnderTest := []string{siteAdministrator, siteAdministratorCreator, siteAdministratorExplorer, serverAdministrator} + + for _, role := range adminRolesUnderTest { + t.Run(role, func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{"user-1": role}, &puts) + l := newLicenseBuilder(c) + + // Pick a target tier the role does not already satisfy. + target := explorer + if role == siteAdministratorExplorer || role == siteAdministrator { + target = creator + } + + _, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(target)) + require.Error(t, err) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.Empty(t, puts, "refused admin cross-tier grant must not write") + }) + } +} + +// TestLicenseGrant_UnknownUser covers decision step 4: a 404 on the +// single-user GET means there is nothing to grant to. +func TestLicenseGrant_UnknownUser(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{}, &puts) + l := newLicenseBuilder(c) + + _, err := l.Grant(context.Background(), testUserPrincipal("ghost"), testLicenseEntitlement(creator)) + require.Error(t, err) + require.Equal(t, codes.NotFound, status.Code(err)) + require.Empty(t, puts) +} + +// TestLicenseRevoke_Stale covers acceptance criterion 2: revoking a tier the +// user no longer holds (e.g. a later Grant already moved them elsewhere) +// must not write and must report GrantAlreadyRevoked. +func TestLicenseRevoke_Stale(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{"user-1": creator}, &puts) + l := newLicenseBuilder(c) + + gr := &v2.Grant{ + Entitlement: testLicenseEntitlement(viewer), + Principal: testUserPrincipal("user-1"), + } + + annos, err := l.Revoke(context.Background(), gr) + require.NoError(t, err) + require.Empty(t, puts, "stale revoke must not write") + require.True(t, annos.Contains(&v2.GrantAlreadyRevoked{})) +} + +// TestLicenseRevoke_HoldsTier is the non-regression companion: a user who +// still holds the tier is unlicensed via exactly one PUT. +func TestLicenseRevoke_HoldsTier(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{"user-1": creator}, &puts) + l := newLicenseBuilder(c) + + gr := &v2.Grant{ + Entitlement: testLicenseEntitlement(creator), + Principal: testUserPrincipal("user-1"), + } + + _, err := l.Revoke(context.Background(), gr) + require.NoError(t, err) + require.Len(t, puts, 1) + require.Equal(t, "user-1", puts[0].userID) + require.Equal(t, unlicensed, puts[0].siteRole) +} + +// TestLicenseRevoke_UserNotFound covers the 404 case in decision step 1: a +// deleted user has nothing left to revoke. +func TestLicenseRevoke_UserNotFound(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{}, &puts) + l := newLicenseBuilder(c) + + gr := &v2.Grant{ + Entitlement: testLicenseEntitlement(creator), + Principal: testUserPrincipal("ghost"), + } + + annos, err := l.Revoke(context.Background(), gr) + require.NoError(t, err) + require.Empty(t, puts) + require.True(t, annos.Contains(&v2.GrantAlreadyRevoked{})) +} From d721c262285bcd4fd34f499d86fe463466d70fa7 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Tue, 20 Oct 2026 19:55:48 -0300 Subject: [PATCH 02/13] [CXH-2368] - Address review feedback on license grant/revoke --- README.md | 1 + docs/connector.mdx | 2 + pkg/connector/group.go | 2 +- pkg/connector/helpers.go | 24 ++- pkg/connector/license.go | 62 +++++--- pkg/connector/license_test.go | 286 +++++++++++++++++++++++++++++++--- 6 files changed, 326 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 8b05cb93..9a1cd142 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,7 @@ baton resources - **"All Users" group**: Tableau automatically adds every user to the built-in "All Users" group. This group cannot be modified via the API — Grant and Revoke operations on it are not supported. The connector syncs its membership to show each user's base site role. - **License revoke constraints**: Revoking a license (setting a user to "Unlicensed") will fail if the user belongs to a group with a "Grant role on sign in" minimum site role. Remove the user from the constraining group first, then revoke the license. - **License grant is tier-aware**: A license tier is a view of the user's single site role (e.g. `SiteAdministratorCreator` already satisfies the Creator tier), so granting a tier the user already holds is a no-op — it never overwrites their current role with the tier's base role. Granting a *different* tier to a user currently holding a `SiteAdministrator*`/`ServerAdministrator` role is refused instead of silently demoting them; use the Site Role entitlement to change an admin role explicitly. +- **License revoke sets the user to Unlicensed**: Revoking a tier sets the user's site role to `Unlicensed`, including when their current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, `ServerAdministrator`). `ServerAdministrator` cannot be re-assigned through the REST API, so use the Site Role entitlement to change admin roles deliberately. Revoking the Unlicensed tier itself is rejected, since it is the floor tier and has nothing to remove. - **Idempotency**: Granting a permission that already exists returns success (not an error). Revoking a permission that is already removed also returns success. # Credentials Setup diff --git a/docs/connector.mdx b/docs/connector.mdx index 94d85720..af2114af 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -35,6 +35,8 @@ When your Tableau site has per-tier seat capacities configured, C1 tracks the pu Because a tier is a view of the user's single site role, several roles can satisfy the same tier (for example, `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op and never overwrites a more privileged role with the tier's base role. Granting a *different* tier to a user who currently holds a Site Administrator or Server Administrator role is refused rather than silently removing their admin access — use the Site Role entitlement to change an admin role explicitly. +Revoking a tier sets the user's site role to Unlicensed, including when the user's current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`). `ServerAdministrator` cannot be re-assigned through the REST API, so use the Site Role entitlement to change admin roles deliberately. Revoking the Unlicensed tier itself is rejected, because Unlicensed is the floor tier and has nothing to remove. + ## Permission inheritance Tableau uses a permission inheritance model that affects how access is synced and provisioned: diff --git a/pkg/connector/group.go b/pkg/connector/group.go index 80b512b2..6d1406c5 100644 --- a/pkg/connector/group.go +++ b/pkg/connector/group.go @@ -186,7 +186,7 @@ func (g *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations annos, err := g.client.RemoveUserFromGroup(ctx, entitlement.Resource.Id.Resource, principal.Id.Resource) if err != nil { - if isNotFoundError(err) { + if client.IsNotFoundError(err) { return annotations.New(&v2.GrantAlreadyRevoked{}), nil } return annos, fmt.Errorf("failed to remove user from group: %w", err) diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index bb6f013d..5ddf8b23 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -3,6 +3,7 @@ package connector import ( "context" "fmt" + "slices" "strings" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -13,6 +14,7 @@ import ( "github.com/conductorone/baton-tableau/pkg/client" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/anypb" ) const ( @@ -208,9 +210,23 @@ func isAlreadyExistsError(err error) bool { return status.Code(err) == codes.AlreadyExists } -// isNotFoundError returns true if the error indicates the resource was not found (HTTP 404). -func isNotFoundError(err error) bool { - return status.Code(err) == codes.NotFound +// mergeAnnotations concatenates annotation sets in order. When the same +// message type appears more than once (e.g. a RateLimitDescription from each +// of several API calls), the most recent one wins so a result carries at most +// one entry per type. +func mergeAnnotations(sets ...annotations.Annotations) annotations.Annotations { + var merged annotations.Annotations + for _, set := range sets { + for _, a := range set { + idx := slices.IndexFunc(merged, func(m *anypb.Any) bool { return m.GetTypeUrl() == a.GetTypeUrl() }) + if idx >= 0 { + merged[idx] = a + continue + } + merged = append(merged, a) + } + } + return merged } // permissionFunc is the signature shared by all Add/Delete permission client methods. @@ -297,7 +313,7 @@ func revokePermission( } if err != nil { - if isNotFoundError(err) { + if client.IsNotFoundError(err) { return annotations.New(&v2.GrantAlreadyRevoked{}), nil } return annos, fmt.Errorf("failed to revoke permission: %w", err) diff --git a/pkg/connector/license.go b/pkg/connector/license.go index f2606134..3ddce1d6 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -41,8 +41,8 @@ var RolesPerLicense = map[string][]string{ // silently demote. A license tier is a derived view of a user's single site // role, so granting a different tier is itself a role change; stripping an // admin role as a side effect of that change would be an unannounced -// privilege reduction. See the CXH-2368 decision doc: use the Site Role -// entitlement to change an admin role explicitly instead. +// privilege reduction. Use the Site Role entitlement to change an admin role +// explicitly instead. var adminLicenseRoles = map[string]bool{ siteAdministrator: true, siteAdministratorCreator: true, @@ -267,17 +267,20 @@ func (l *licenseBuilder) Grants(ctx context.Context, resource *v2.Resource, opts // // A Tableau user has exactly one site role, and RolesPerLicense groups which // roles satisfy which tier, so a license tier is a derived view of that one -// role rather than an independent attribute. Per CXH-2368, Grant therefore -// reads the user's current role before writing: +// role rather than an independent attribute. Grant therefore reads the user's +// current role before writing: // - already in the target tier -> no write, GrantAlreadyExists (this was -// the ticket's bug: granting a held tier used to overwrite e.g. +// CXH-2368 regression: granting a held tier used to overwrite e.g. // SiteAdministratorCreator with plain Creator). // - current role is an admin role outside the tier -> refuse with // FailedPrecondition; a license grant must not double as a silent // demotion, and ServerAdministrator cannot be restored via this API. -// - otherwise -> write the tier's base role and report GrantReplaced -// naming the license grant the write displaced (tiers are exclusive, so -// a cross-tier grant is itself the requested role change). +// - otherwise -> write the tier's base role and, when the user's previous +// role belonged to another tier, report GrantReplaced naming the license +// grant the write displaced (tiers are exclusive, so a cross-tier grant +// is itself the requested role change). +// +// Rate-limit annotations from the initial read are merged into every result. func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { log := ctxzap.Extract(ctx) @@ -286,29 +289,30 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti return nil, fmt.Errorf("unknown license %s", entitlement.Resource.Id.Resource) } - user, _, err := l.client.GetUser(ctx, principal.Id.Resource) + user, getAnnos, err := l.client.GetUser(ctx, principal.Id.Resource) if err != nil { if client.IsNotFoundError(err) { - return nil, status.Errorf(codes.NotFound, "license: cannot grant %s: user %s not found", targetTier, principal.Id.Resource) + return getAnnos, status.Errorf(codes.NotFound, "license: cannot grant %s: user %s not found", targetTier, principal.Id.Resource) } - return nil, fmt.Errorf("failed to read user %s for license grant: %w", principal.Id.Resource, err) + return getAnnos, fmt.Errorf("failed to read user %s for license grant: %w", principal.Id.Resource, err) } if slices.Contains(RolesPerLicense[targetTier], user.SiteRole) { log.Debug("license grant: user already in target tier, no write needed", zap.String("user_id", principal.Id.Resource), zap.String("tier", targetTier), zap.String("site_role", user.SiteRole)) - return annotations.New(&v2.GrantAlreadyExists{}), nil + return mergeAnnotations(getAnnos, annotations.New(&v2.GrantAlreadyExists{})), nil } if adminLicenseRoles[user.SiteRole] { log.Warn("license grant: refusing to move an admin role across license tiers", zap.String("user_id", principal.Id.Resource), zap.String("tier", targetTier), zap.String("site_role", user.SiteRole)) - return nil, status.Errorf(codes.FailedPrecondition, + return getAnnos, status.Errorf(codes.FailedPrecondition, "license: cannot grant %s to user %s: current site role %q is an administrative role; use the Site Role entitlement to change it explicitly", targetTier, principal.Id.Resource, user.SiteRole) } - annos, err := l.client.UpdateUserSiteRole(ctx, principal.Id.Resource, targetTier) + putAnnos, err := l.client.UpdateUserSiteRole(ctx, principal.Id.Resource, targetTier) + annos := mergeAnnotations(getAnnos, putAnnos) if err != nil { return annos, fmt.Errorf("failed to grant %s license to user %s: %w", targetTier, principal.Id.Resource, err) } @@ -329,12 +333,14 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti // Revoke removes the license by setting the user's site role to Unlicensed. // -// Per CXH-2368, Revoke reads the user's current role first: a 404, or a role -// that has already moved out of this tier (e.g. a later Grant replaced it — -// see Grant's GrantReplaced annotation), means there is nothing left to -// revoke. Writing Unlicensed in that case would wipe out access this revoke -// was never asked to touch and was already reported as GrantAlreadyExists -// (or GrantReplaced) by the operation that actually changed it. +// Revoke reads the user's current role first: a 404, or a role that has +// already moved out of this tier (e.g. a later Grant replaced it, see Grant's +// GrantReplaced annotation), means there is nothing left to revoke. Writing +// Unlicensed in that case would wipe out access this revoke was never asked +// to touch. Revoking the Unlicensed tier from a user who is Unlicensed is +// rejected with FailedPrecondition: it is the floor tier and has nothing to +// remove. Rate-limit annotations from the initial read are merged into every +// result. func (l *licenseBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.Annotations, error) { log := ctxzap.Extract(ctx) principalID := g.Principal.Id.Resource @@ -344,22 +350,28 @@ func (l *licenseBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.A return nil, fmt.Errorf("unknown license %s", g.Entitlement.Resource.Id.Resource) } - user, _, err := l.client.GetUser(ctx, principalID) + user, getAnnos, err := l.client.GetUser(ctx, principalID) if err != nil { if client.IsNotFoundError(err) { log.Debug("license revoke: user not found, nothing to revoke", zap.String("user_id", principalID)) - return annotations.New(&v2.GrantAlreadyRevoked{}), nil + return mergeAnnotations(getAnnos, annotations.New(&v2.GrantAlreadyRevoked{})), nil } - return nil, fmt.Errorf("failed to read user %s for license revoke: %w", principalID, err) + return getAnnos, fmt.Errorf("failed to read user %s for license revoke: %w", principalID, err) } if !slices.Contains(RolesPerLicense[tier], user.SiteRole) { log.Debug("license revoke: user no longer in tier, nothing to revoke", zap.String("user_id", principalID), zap.String("tier", tier), zap.String("site_role", user.SiteRole)) - return annotations.New(&v2.GrantAlreadyRevoked{}), nil + return mergeAnnotations(getAnnos, annotations.New(&v2.GrantAlreadyRevoked{})), nil + } + + if tier == unlicensed { + return getAnnos, status.Errorf(codes.FailedPrecondition, + "license: cannot revoke the Unlicensed tier from user %s: Unlicensed is the floor tier and has nothing to remove", principalID) } - annos, err := l.client.UpdateUserSiteRole(ctx, principalID, unlicensed) + putAnnos, err := l.client.UpdateUserSiteRole(ctx, principalID, unlicensed) + annos := mergeAnnotations(getAnnos, putAnnos) if err != nil { return annos, fmt.Errorf("failed to revoke license from user %s: %w", principalID, err) } diff --git a/pkg/connector/license_test.go b/pkg/connector/license_test.go index d4f358bc..f6b5eae7 100644 --- a/pkg/connector/license_test.go +++ b/pkg/connector/license_test.go @@ -5,10 +5,12 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strconv" "strings" "testing" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/conductorone/baton-tableau/pkg/client" "github.com/stretchr/testify/require" @@ -121,7 +123,15 @@ func TestCapacitySeats(t *testing.T) { // Grant / Revoke (CXH-2368) // ============================================================================= -const licenseTestSiteID = "site-1" +const ( + licenseTestSiteID = "site-1" + + // licenseTestServerError is a pseudo role that makes the mock GetUser return HTTP 500. + licenseTestServerError = "__server_error__" + + licenseTestRateLimit = 100 + licenseTestRateRemaining = 42 +) type recordedPut struct { userID string @@ -130,7 +140,9 @@ type recordedPut struct { // newLicenseTestClient starts a mock Tableau API server that authenticates // any login, serves GetUser from users (keyed by user ID, valued by current -// site role; a missing key means a 404), and records every PUT +// site role; a missing key means a 404 and the licenseTestServerError role +// means a 500), stamps rate-limit headers on every GET and PUT so tests can check the +// annotations are propagated, and records every PUT // (UpdateUserSiteRole) it receives so tests can assert exact write counts // and bodies. func newLicenseTestClient(t *testing.T, users map[string]string, puts *[]recordedPut) *client.Client { @@ -154,11 +166,22 @@ func newLicenseTestClient(t *testing.T, users map[string]string, puts *[]recorde role, ok := users[userID] if !ok { w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Ratelimit-Limit", strconv.Itoa(licenseTestRateLimit)) + w.Header().Set("X-Ratelimit-Remaining", strconv.Itoa(licenseTestRateRemaining)) + w.Header().Set("X-Ratelimit-Reset", "30") w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"error":{"summary":"Not Found","detail":"user not found","code":"0x1"}}`)) return } w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Ratelimit-Limit", strconv.Itoa(licenseTestRateLimit)) + w.Header().Set("X-Ratelimit-Remaining", strconv.Itoa(licenseTestRateRemaining)) + w.Header().Set("X-Ratelimit-Reset", "30") + if role == licenseTestServerError { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":{"summary":"Internal Server Error","detail":"boom","code":"0x3"}}`)) + return + } _ = json.NewEncoder(w).Encode(map[string]any{ "user": map[string]any{"id": userID, "siteRole": role}, }) @@ -172,6 +195,9 @@ func newLicenseTestClient(t *testing.T, users map[string]string, puts *[]recorde _ = json.NewDecoder(r.Body).Decode(&body) *puts = append(*puts, recordedPut{userID: userID, siteRole: body.User.SiteRole}) w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Ratelimit-Limit", strconv.Itoa(licenseTestRateLimit)) + w.Header().Set("X-Ratelimit-Remaining", strconv.Itoa(licenseTestRateRemaining)) + w.Header().Set("X-Ratelimit-Reset", "30") _ = json.NewEncoder(w).Encode(map[string]any{ "user": map[string]any{"id": userID, "siteRole": body.User.SiteRole}, }) @@ -199,11 +225,10 @@ func testLicenseEntitlement(tier string) *v2.Entitlement { } } -// TestLicenseGrant_AlreadyInTier covers acceptance criteria 1 and 3: granting -// a tier to a user whose current site role already satisfies that tier must -// perform no write and report GrantAlreadyExists -- this was the exact shape -// of the CXH-2368 bug (a SiteAdministratorCreator re-granted Creator was -// demoted to plain Creator). +// TestLicenseGrant_AlreadyInTier asserts that granting a tier to a user whose +// current site role already satisfies that tier performs no write and reports +// GrantAlreadyExists. This is the CXH-2368 regression: a +// SiteAdministratorCreator re-granted Creator was demoted to plain Creator. func TestLicenseGrant_AlreadyInTier(t *testing.T) { t.Parallel() @@ -236,9 +261,9 @@ func TestLicenseGrant_AlreadyInTier(t *testing.T) { } } -// TestLicenseGrant_CrossTierNonAdmin covers the decision doc's 3b: a -// non-admin role outside the target tier is replaced, with exactly one PUT -// and a GrantReplaced annotation naming the user's previous license grant. +// TestLicenseGrant_CrossTierNonAdmin asserts that a non-admin role outside the +// target tier is replaced, with exactly one PUT and a GrantReplaced annotation +// naming the user's previous license grant. func TestLicenseGrant_CrossTierNonAdmin(t *testing.T) { t.Parallel() @@ -262,10 +287,9 @@ func TestLicenseGrant_CrossTierNonAdmin(t *testing.T) { require.Equal(t, "license:viewer:member:user:user-1", replaced.GetReplacedGrantId()) } -// TestLicenseGrant_CrossTierAdmin covers decision 3a: granting a different -// tier to a user currently holding an admin role must refuse with -// FailedPrecondition rather than silently stripping admin rights, and must -// not write. +// TestLicenseGrant_CrossTierAdmin asserts that granting a different tier to a +// user currently holding an admin role is refused with FailedPrecondition +// rather than silently stripping admin rights, and does not write. func TestLicenseGrant_CrossTierAdmin(t *testing.T) { t.Parallel() @@ -293,8 +317,8 @@ func TestLicenseGrant_CrossTierAdmin(t *testing.T) { } } -// TestLicenseGrant_UnknownUser covers decision step 4: a 404 on the -// single-user GET means there is nothing to grant to. +// TestLicenseGrant_UnknownUser asserts that a 404 on the single-user GET means +// there is nothing to grant to. func TestLicenseGrant_UnknownUser(t *testing.T) { t.Parallel() @@ -308,9 +332,9 @@ func TestLicenseGrant_UnknownUser(t *testing.T) { require.Empty(t, puts) } -// TestLicenseRevoke_Stale covers acceptance criterion 2: revoking a tier the -// user no longer holds (e.g. a later Grant already moved them elsewhere) -// must not write and must report GrantAlreadyRevoked. +// TestLicenseRevoke_Stale asserts that revoking a tier the user no longer +// holds (e.g. a later Grant already moved them elsewhere) does not write and +// reports GrantAlreadyRevoked. func TestLicenseRevoke_Stale(t *testing.T) { t.Parallel() @@ -350,8 +374,8 @@ func TestLicenseRevoke_HoldsTier(t *testing.T) { require.Equal(t, unlicensed, puts[0].siteRole) } -// TestLicenseRevoke_UserNotFound covers the 404 case in decision step 1: a -// deleted user has nothing left to revoke. +// TestLicenseRevoke_UserNotFound asserts that a deleted user (404) has nothing +// left to revoke. func TestLicenseRevoke_UserNotFound(t *testing.T) { t.Parallel() @@ -369,3 +393,223 @@ func TestLicenseRevoke_UserNotFound(t *testing.T) { require.Empty(t, puts) require.True(t, annos.Contains(&v2.GrantAlreadyRevoked{})) } + +// requireSingleRateLimit asserts annos carries exactly one RateLimitDescription +// and that it is the one the mock GET responses stamped. +func requireSingleRateLimit(t *testing.T, annos annotations.Annotations) { + t.Helper() + + count := 0 + for _, a := range annos { + if a.MessageIs(&v2.RateLimitDescription{}) { + count++ + } + } + require.Equal(t, 1, count, "expected exactly one RateLimitDescription") + + var rl v2.RateLimitDescription + found, err := annos.Pick(&rl) + require.NoError(t, err) + require.True(t, found) + require.EqualValues(t, licenseTestRateLimit, rl.GetLimit()) + require.EqualValues(t, licenseTestRateRemaining, rl.GetRemaining()) +} + +// TestLicense_GetUserServerError asserts that a non-404 failure reading the +// user surfaces as an error (not NotFound) and never falls through to a write. +func TestLicense_GetUserServerError(t *testing.T) { + t.Parallel() + + t.Run("grant", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{"user-1": licenseTestServerError}, &puts) + l := newLicenseBuilder(c) + + _, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + require.Error(t, err) + require.NotEqual(t, codes.NotFound, status.Code(err)) + require.Empty(t, puts, "failed read must not write") + }) + + t.Run("revoke", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{"user-1": licenseTestServerError}, &puts) + l := newLicenseBuilder(c) + + annos, err := l.Revoke(context.Background(), &v2.Grant{ + Entitlement: testLicenseEntitlement(creator), + Principal: testUserPrincipal("user-1"), + }) + require.Error(t, err) + require.NotEqual(t, codes.NotFound, status.Code(err)) + require.False(t, annos.Contains(&v2.GrantAlreadyRevoked{}), "a server error must not be reported as already revoked") + require.Empty(t, puts, "failed read must not write") + }) +} + +// TestLicenseGrant_RoleOutsideEveryTier asserts that a current role belonging +// to no tier (e.g. Guest) is overwritten with one PUT and, since there is no +// previous license grant to name, no GrantReplaced. +func TestLicenseGrant_RoleOutsideEveryTier(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{"user-1": "Guest"}, &puts) + l := newLicenseBuilder(c) + + annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + require.NoError(t, err) + require.Equal(t, []recordedPut{{userID: "user-1", siteRole: creator}}, puts) + require.False(t, annos.Contains(&v2.GrantReplaced{})) + require.False(t, annos.Contains(&v2.GrantAlreadyExists{})) +} + +// TestLicenseGrant_ExplorerCanPublishToCreator asserts that ExplorerCanPublish +// counts as the Explorer tier: granting Creator writes Creator once and +// reports the Explorer license grant as replaced. +func TestLicenseGrant_ExplorerCanPublishToCreator(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{"user-1": explorerCanPublish}, &puts) + l := newLicenseBuilder(c) + + annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + require.NoError(t, err) + require.Equal(t, []recordedPut{{userID: "user-1", siteRole: creator}}, puts) + + var replaced v2.GrantReplaced + found, err := annos.Pick(&replaced) + require.NoError(t, err) + require.True(t, found, "expected a GrantReplaced annotation") + require.Equal(t, "license:explorer:member:user:user-1", replaced.GetReplacedGrantId()) +} + +// TestLicense_PropagatesGetUserAnnotations asserts that the rate-limit +// annotations from the initial GetUser read are present on every result, +// merged without duplicating the PUT's own RateLimitDescription. +func TestLicense_PropagatesGetUserAnnotations(t *testing.T) { + t.Parallel() + + revokeOf := func(tier string) *v2.Grant { + return &v2.Grant{Entitlement: testLicenseEntitlement(tier), Principal: testUserPrincipal("user-1")} + } + + t.Run("grant already exists", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": creator}, &puts)) + annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + require.NoError(t, err) + require.True(t, annos.Contains(&v2.GrantAlreadyExists{})) + requireSingleRateLimit(t, annos) + }) + + t.Run("grant refused for admin role", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": siteAdministratorCreator}, &puts)) + annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(explorer)) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + requireSingleRateLimit(t, annos) + }) + + t.Run("grant user not found", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{}, &puts)) + annos, err := l.Grant(context.Background(), testUserPrincipal("ghost"), testLicenseEntitlement(creator)) + require.Equal(t, codes.NotFound, status.Code(err)) + requireSingleRateLimit(t, annos) + }) + + t.Run("grant write path", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": viewer}, &puts)) + annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + require.NoError(t, err) + require.True(t, annos.Contains(&v2.GrantReplaced{})) + requireSingleRateLimit(t, annos) + }) + + t.Run("revoke already revoked", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": creator}, &puts)) + annos, err := l.Revoke(context.Background(), revokeOf(viewer)) + require.NoError(t, err) + require.True(t, annos.Contains(&v2.GrantAlreadyRevoked{})) + requireSingleRateLimit(t, annos) + }) + + t.Run("revoke user not found", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{}, &puts)) + annos, err := l.Revoke(context.Background(), &v2.Grant{Entitlement: testLicenseEntitlement(creator), Principal: testUserPrincipal("ghost")}) + require.NoError(t, err) + require.True(t, annos.Contains(&v2.GrantAlreadyRevoked{})) + requireSingleRateLimit(t, annos) + }) + + t.Run("revoke write path", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": creator}, &puts)) + annos, err := l.Revoke(context.Background(), revokeOf(creator)) + require.NoError(t, err) + require.Len(t, puts, 1) + requireSingleRateLimit(t, annos) + }) +} + +// TestLicenseRevoke_UnlicensedTier asserts that Unlicensed is the floor tier: +// revoking it from an Unlicensed user is rejected without a write, while +// revoking it from a user who holds some other tier is a stale revoke. +func TestLicenseRevoke_UnlicensedTier(t *testing.T) { + t.Parallel() + + t.Run("user is unlicensed", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": unlicensed}, &puts)) + + annos, err := l.Revoke(context.Background(), &v2.Grant{ + Entitlement: testLicenseEntitlement(unlicensed), + Principal: testUserPrincipal("user-1"), + }) + require.Error(t, err) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.Contains(t, err.Error(), "Unlicensed is the floor tier") + require.Empty(t, puts, "rejected revoke must not write") + requireSingleRateLimit(t, annos) + }) + + t.Run("user holds another tier", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": creator}, &puts)) + + annos, err := l.Revoke(context.Background(), &v2.Grant{ + Entitlement: testLicenseEntitlement(unlicensed), + Principal: testUserPrincipal("user-1"), + }) + require.NoError(t, err) + require.Empty(t, puts) + require.True(t, annos.Contains(&v2.GrantAlreadyRevoked{})) + }) +} From a9a7016632d2c40c3623a6d3a1ae37ef12812ab1 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Wed, 21 Oct 2026 14:49:51 -0300 Subject: [PATCH 03/13] [CXH-2368] - Reword comments and docs to describe current behavior --- README.md | 2 +- docs/connector.mdx | 2 +- pkg/connector/license.go | 4 +--- pkg/connector/license_test.go | 9 ++++----- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 9a1cd142..b2dc86b7 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ baton resources - **Grant expansion**: Group-based permissions on projects, workbooks, and views are supported. When a group has a permission, all members inherit it. - **"All Users" group**: Tableau automatically adds every user to the built-in "All Users" group. This group cannot be modified via the API — Grant and Revoke operations on it are not supported. The connector syncs its membership to show each user's base site role. - **License revoke constraints**: Revoking a license (setting a user to "Unlicensed") will fail if the user belongs to a group with a "Grant role on sign in" minimum site role. Remove the user from the constraining group first, then revoke the license. -- **License grant is tier-aware**: A license tier is a view of the user's single site role (e.g. `SiteAdministratorCreator` already satisfies the Creator tier), so granting a tier the user already holds is a no-op — it never overwrites their current role with the tier's base role. Granting a *different* tier to a user currently holding a `SiteAdministrator*`/`ServerAdministrator` role is refused instead of silently demoting them; use the Site Role entitlement to change an admin role explicitly. +- **License grant is tier-aware**: A license tier is a view of the user's single site role (e.g. `SiteAdministratorCreator` already satisfies the Creator tier), so granting a tier the user already holds is a no-op and keeps their current role. Granting a *different* tier to a user currently holding a `SiteAdministrator*`/`ServerAdministrator` role is refused instead of silently demoting them; use the Site Role entitlement to change an admin role explicitly. - **License revoke sets the user to Unlicensed**: Revoking a tier sets the user's site role to `Unlicensed`, including when their current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, `ServerAdministrator`). `ServerAdministrator` cannot be re-assigned through the REST API, so use the Site Role entitlement to change admin roles deliberately. Revoking the Unlicensed tier itself is rejected, since it is the floor tier and has nothing to remove. - **Idempotency**: Granting a permission that already exists returns success (not an error). Revoking a permission that is already removed also returns success. diff --git a/docs/connector.mdx b/docs/connector.mdx index af2114af..4dc850b5 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -33,7 +33,7 @@ In Tableau, licensing is role-based: each user's **site role** determines their When your Tableau site has per-tier seat capacities configured, C1 tracks the purchased and consumed seat counts for each license tier, so you can review license utilization alongside who holds each tier. -Because a tier is a view of the user's single site role, several roles can satisfy the same tier (for example, `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op and never overwrites a more privileged role with the tier's base role. Granting a *different* tier to a user who currently holds a Site Administrator or Server Administrator role is refused rather than silently removing their admin access — use the Site Role entitlement to change an admin role explicitly. +Because a tier is a view of the user's single site role, several roles can satisfy the same tier (for example, `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op and keeps the user's current role. Granting a *different* tier to a user who currently holds a Site Administrator or Server Administrator role is refused rather than silently removing their admin access — use the Site Role entitlement to change an admin role explicitly. Revoking a tier sets the user's site role to Unlicensed, including when the user's current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`). `ServerAdministrator` cannot be re-assigned through the REST API, so use the Site Role entitlement to change admin roles deliberately. Revoking the Unlicensed tier itself is rejected, because Unlicensed is the floor tier and has nothing to remove. diff --git a/pkg/connector/license.go b/pkg/connector/license.go index 3ddce1d6..f78d534b 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -269,9 +269,7 @@ func (l *licenseBuilder) Grants(ctx context.Context, resource *v2.Resource, opts // roles satisfy which tier, so a license tier is a derived view of that one // role rather than an independent attribute. Grant therefore reads the user's // current role before writing: -// - already in the target tier -> no write, GrantAlreadyExists (this was -// CXH-2368 regression: granting a held tier used to overwrite e.g. -// SiteAdministratorCreator with plain Creator). +// - already in the target tier -> no write, GrantAlreadyExists. // - current role is an admin role outside the tier -> refuse with // FailedPrecondition; a license grant must not double as a silent // demotion, and ServerAdministrator cannot be restored via this API. diff --git a/pkg/connector/license_test.go b/pkg/connector/license_test.go index f6b5eae7..e85dfc29 100644 --- a/pkg/connector/license_test.go +++ b/pkg/connector/license_test.go @@ -120,7 +120,7 @@ func TestCapacitySeats(t *testing.T) { } // ============================================================================= -// Grant / Revoke (CXH-2368) +// Grant / Revoke // ============================================================================= const ( @@ -227,8 +227,7 @@ func testLicenseEntitlement(tier string) *v2.Entitlement { // TestLicenseGrant_AlreadyInTier asserts that granting a tier to a user whose // current site role already satisfies that tier performs no write and reports -// GrantAlreadyExists. This is the CXH-2368 regression: a -// SiteAdministratorCreator re-granted Creator was demoted to plain Creator. +// GrantAlreadyExists. func TestLicenseGrant_AlreadyInTier(t *testing.T) { t.Parallel() @@ -353,8 +352,8 @@ func TestLicenseRevoke_Stale(t *testing.T) { require.True(t, annos.Contains(&v2.GrantAlreadyRevoked{})) } -// TestLicenseRevoke_HoldsTier is the non-regression companion: a user who -// still holds the tier is unlicensed via exactly one PUT. +// TestLicenseRevoke_HoldsTier asserts that a user who still holds the tier is +// unlicensed via exactly one PUT. func TestLicenseRevoke_HoldsTier(t *testing.T) { t.Parallel() From d3273cc420533ad746439f21522b967158ba29bf Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Mon, 26 Oct 2026 21:04:55 -0300 Subject: [PATCH 04/13] [CXH-2368] - Bypass response cache in GetUser and cover stale-read cases 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. --- pkg/client/client.go | 10 ++++-- pkg/client/client_test.go | 5 +-- pkg/connector/license_test.go | 64 ++++++++++++++++++++++++++++++++++- 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index a5b551e7..41d74d7b 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -290,7 +290,9 @@ func (c *Client) GetUser(ctx context.Context, userId string) (*User, annotations } var res userResponse - annos, err := c.doRequest(ctx, http.MethodGet, urlGetUser, &res, nil) + // Bypass the SDK response cache: provisioning reads the role it is about to + // act on, and a cached copy goes stale after this client's own writes. + annos, err := c.doRequest(ctx, http.MethodGet, urlGetUser, &res, nil, uhttp.WithNoCache()) if err != nil { return nil, annos, err } @@ -796,8 +798,9 @@ func IsNotFoundError(err error) bool { return status.Code(err) == codes.NotFound } -// doRequest executes an HTTP request against the Tableau API. -func (c *Client) doRequest(ctx context.Context, method string, url *url.URL, res, body any) (annotations.Annotations, error) { +// doRequest executes an HTTP request against the Tableau API. Extra request +// options are applied after the defaults. +func (c *Client) doRequest(ctx context.Context, method string, url *url.URL, res, body any, extraOpts ...uhttp.RequestOption) (annotations.Annotations, error) { reqOpts := []uhttp.RequestOption{ uhttp.WithAcceptJSONHeader(), uhttp.WithContentTypeJSONHeader(), @@ -806,6 +809,7 @@ func (c *Client) doRequest(ctx context.Context, method string, url *url.URL, res if body != nil { reqOpts = append(reqOpts, uhttp.WithJSONBody(body)) } + reqOpts = append(reqOpts, extraOpts...) req, err := c.httpClient.NewRequest(ctx, method, url, reqOpts...) if err != nil { diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index a429ce39..dc44dacd 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -2,7 +2,6 @@ package client import ( "context" - "encoding/json" "net/http" "net/http/httptest" "testing" @@ -40,9 +39,7 @@ func TestGetUser_Found(t *testing.T) { return } w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(userResponse{ - User: User{ID: "user-1", Email: "user1@example.com", SiteRole: "SiteAdministratorCreator"}, - }) + _, _ = w.Write([]byte(`{"user":{"id":"user-1","email":"user1@example.com","siteRole":"SiteAdministratorCreator"}}`)) })) t.Cleanup(srv.Close) diff --git a/pkg/connector/license_test.go b/pkg/connector/license_test.go index e85dfc29..a86037bd 100644 --- a/pkg/connector/license_test.go +++ b/pkg/connector/license_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "strconv" "strings" + "sync" "testing" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -133,6 +134,11 @@ const ( licenseTestRateRemaining = 42 ) +// licenseTestUsersMu guards the users maps handed to newLicenseTestClient: the +// mock server reads and applies PUTs to them from its own goroutine while a +// test may change a role between calls. +var licenseTestUsersMu sync.Mutex + type recordedPut struct { userID string siteRole string @@ -140,7 +146,7 @@ type recordedPut struct { // newLicenseTestClient starts a mock Tableau API server that authenticates // any login, serves GetUser from users (keyed by user ID, valued by current -// site role; a missing key means a 404 and the licenseTestServerError role +// site role, updated by every PUT; a missing key means a 404 and the licenseTestServerError role // means a 500), stamps rate-limit headers on every GET and PUT so tests can check the // annotations are propagated, and records every PUT // (UpdateUserSiteRole) it receives so tests can assert exact write counts @@ -163,7 +169,9 @@ func newLicenseTestClient(t *testing.T, users map[string]string, puts *[]recorde }) case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, usersPathPrefix): userID := strings.TrimPrefix(r.URL.Path, usersPathPrefix) + licenseTestUsersMu.Lock() role, ok := users[userID] + licenseTestUsersMu.Unlock() if !ok { w.Header().Set("Content-Type", "application/json") w.Header().Set("X-Ratelimit-Limit", strconv.Itoa(licenseTestRateLimit)) @@ -193,7 +201,10 @@ func newLicenseTestClient(t *testing.T, users map[string]string, puts *[]recorde } `json:"user"` } _ = json.NewDecoder(r.Body).Decode(&body) + licenseTestUsersMu.Lock() *puts = append(*puts, recordedPut{userID: userID, siteRole: body.User.SiteRole}) + users[userID] = body.User.SiteRole + licenseTestUsersMu.Unlock() w.Header().Set("Content-Type", "application/json") w.Header().Set("X-Ratelimit-Limit", strconv.Itoa(licenseTestRateLimit)) w.Header().Set("X-Ratelimit-Remaining", strconv.Itoa(licenseTestRateRemaining)) @@ -612,3 +623,54 @@ func TestLicenseRevoke_UnlicensedTier(t *testing.T) { require.True(t, annos.Contains(&v2.GrantAlreadyRevoked{})) }) } + +// TestLicense_GrantThenRevokeSameClient asserts that a revoke following a +// grant on one client reads the role the grant wrote rather than a cached +// pre-grant response. +func TestLicense_GrantThenRevokeSameClient(t *testing.T) { + t.Parallel() + + var puts []recordedPut + c := newLicenseTestClient(t, map[string]string{"user-1": viewer}, &puts) + l := newLicenseBuilder(c) + + _, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + require.NoError(t, err) + require.Len(t, puts, 1) + require.Equal(t, creator, puts[0].siteRole) + + annos, err := l.Revoke(context.Background(), &v2.Grant{ + Entitlement: testLicenseEntitlement(creator), + Principal: testUserPrincipal("user-1"), + }) + require.NoError(t, err) + require.False(t, annos.Contains(&v2.GrantAlreadyRevoked{}), "revoke must see the granted role") + require.Len(t, puts, 2) + require.Equal(t, unlicensed, puts[1].siteRole) +} + +// TestLicenseGrant_RoleChangedBetweenGrants asserts that a second grant on the +// same client sees a role changed outside the connector after the first read. +func TestLicenseGrant_RoleChangedBetweenGrants(t *testing.T) { + t.Parallel() + + var puts []recordedPut + users := map[string]string{"user-1": siteAdministratorCreator} + c := newLicenseTestClient(t, users, &puts) + l := newLicenseBuilder(c) + + annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + require.NoError(t, err) + require.True(t, annos.Contains(&v2.GrantAlreadyExists{})) + require.Empty(t, puts) + + licenseTestUsersMu.Lock() + users["user-1"] = viewer + licenseTestUsersMu.Unlock() + + annos, err = l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + require.NoError(t, err) + require.False(t, annos.Contains(&v2.GrantAlreadyExists{}), "grant must see the externally changed role") + require.Len(t, puts, 1) + require.Equal(t, creator, puts[0].siteRole) +} From 06ef62d064d80219040babb473a6c6247ab6b32e Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Mon, 26 Oct 2026 21:29:01 -0300 Subject: [PATCH 05/13] [CXH-2368] - Reword ServerAdministrator notes to match Tableau docs --- README.md | 4 ++-- docs/connector.mdx | 2 +- docs/docs-info.md | 8 ++++---- pkg/connector/license.go | 3 ++- pkg/connector/site.go | 10 +++++----- 5 files changed, 14 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index b2dc86b7..2997e07f 100644 --- a/README.md +++ b/README.md @@ -72,8 +72,8 @@ baton resources - **Grant expansion**: Group-based permissions on projects, workbooks, and views are supported. When a group has a permission, all members inherit it. - **"All Users" group**: Tableau automatically adds every user to the built-in "All Users" group. This group cannot be modified via the API — Grant and Revoke operations on it are not supported. The connector syncs its membership to show each user's base site role. - **License revoke constraints**: Revoking a license (setting a user to "Unlicensed") will fail if the user belongs to a group with a "Grant role on sign in" minimum site role. Remove the user from the constraining group first, then revoke the license. -- **License grant is tier-aware**: A license tier is a view of the user's single site role (e.g. `SiteAdministratorCreator` already satisfies the Creator tier), so granting a tier the user already holds is a no-op and keeps their current role. Granting a *different* tier to a user currently holding a `SiteAdministrator*`/`ServerAdministrator` role is refused instead of silently demoting them; use the Site Role entitlement to change an admin role explicitly. -- **License revoke sets the user to Unlicensed**: Revoking a tier sets the user's site role to `Unlicensed`, including when their current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, `ServerAdministrator`). `ServerAdministrator` cannot be re-assigned through the REST API, so use the Site Role entitlement to change admin roles deliberately. Revoking the Unlicensed tier itself is rejected, since it is the floor tier and has nothing to remove. +- **License grant is tier-aware**: A license tier is a view of the user's single site role (e.g. `SiteAdministratorCreator` already satisfies the Creator tier), so granting a tier the user already holds is a no-op and keeps their current role. Granting a *different* tier to a user currently holding a `SiteAdministrator*`/`ServerAdministrator` role is refused instead of silently demoting them; use the Site Role entitlement to change a `SiteAdministrator*` role explicitly. +- **License revoke sets the user to Unlicensed**: Revoking a tier sets the user's site role to `Unlicensed`, including when their current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, `ServerAdministrator`). `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server, and this connector does not assign it. Revoking a tier from a Server Administrator leaves the user Unlicensed, and the role has to be restored by a server administrator in Tableau. Revoking the Unlicensed tier itself is rejected, since it is the floor tier and has nothing to remove. - **Idempotency**: Granting a permission that already exists returns success (not an error). Revoking a permission that is already removed also returns success. # Credentials Setup diff --git a/docs/connector.mdx b/docs/connector.mdx index 4dc850b5..9b8469a4 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -35,7 +35,7 @@ When your Tableau site has per-tier seat capacities configured, C1 tracks the pu Because a tier is a view of the user's single site role, several roles can satisfy the same tier (for example, `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op and keeps the user's current role. Granting a *different* tier to a user who currently holds a Site Administrator or Server Administrator role is refused rather than silently removing their admin access — use the Site Role entitlement to change an admin role explicitly. -Revoking a tier sets the user's site role to Unlicensed, including when the user's current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`). `ServerAdministrator` cannot be re-assigned through the REST API, so use the Site Role entitlement to change admin roles deliberately. Revoking the Unlicensed tier itself is rejected, because Unlicensed is the floor tier and has nothing to remove. +Revoking a tier sets the user's site role to Unlicensed, including when the user's current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`). `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server, and this connector does not assign it. Revoking a tier from a Server Administrator leaves the user Unlicensed, and the role has to be restored by a server administrator in Tableau. Revoking the Unlicensed tier itself is rejected, because Unlicensed is the floor tier and has nothing to remove. ## Permission inheritance diff --git a/docs/docs-info.md b/docs/docs-info.md index 8bb8e29b..bc199b74 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -135,7 +135,7 @@ > | Viewer | `Viewer` | Yes | | > | Unlicensed | `Unlicensed` | Yes | | > | Site Administrator | `SiteAdministrator` | Yes | Legacy role, maps to SiteAdministratorExplorer | -> | Server Administrator | `ServerAdministrator` | Sync only | Server-level role, appears in sync but cannot be assigned via the REST API | +> | Server Administrator | `ServerAdministrator` | Sync only | Exists only on Tableau Server; appears in sync, and this connector does not assign it | ### Users @@ -205,7 +205,7 @@ > | Viewer | `Viewer` | > | Unlicensed | `Unlicensed` | > -> `SiteAdministrator` is a legacy role (maps to `SiteAdministratorExplorer`) that cannot be used with the Tableau REST API filter endpoint, so the connector fetches all users and filters client-side when processing Explorer license grants. `ServerAdministrator` consumes a Creator license per Tableau's licensing model and is counted under the Creator tier (sync only — it cannot be assigned via the sites API). See [Tableau REST API Site Roles](https://help.tableau.com/current/api/rest_api/en-us/REST/rest_api_concepts_new_site_roles.htm) for the full role reference. +> `SiteAdministrator` is a legacy role (maps to `SiteAdministratorExplorer`) that cannot be used with the Tableau REST API filter endpoint, so the connector fetches all users and filters client-side when processing Explorer license grants. `ServerAdministrator` consumes a Creator license per Tableau's licensing model and is counted under the Creator tier (sync only — this connector does not assign it). See [Tableau REST API Site Roles](https://help.tableau.com/current/api/rest_api/en-us/REST/rest_api_concepts_new_site_roles.htm) for the full role reference. > **Important — License Revoke Constraints**: Revoking a license (setting a user to "Unlicensed") will fail if the user belongs to a group with "Grant role on sign in" configured. Tableau enforces that a user's site role cannot be lower than the minimum site role required by any group they belong to. The error message from Tableau will be: *"Site role of link user that belongs to license upon login cannot be lower than minimum site role of group"*. To resolve this, remove the user from the constraining group first, then revoke the license. @@ -319,7 +319,7 @@ - Users with unknown or unexpected site roles (not in the connector's role map) are **skipped** during site grant sync instead of creating malformed grants. A warning is logged. - The role map covers all valid [Tableau REST API site roles](https://help.tableau.com/current/api/rest_api/en-us/REST/rest_api_concepts_new_site_roles.htm): `SiteAdministratorCreator`, `SiteAdministratorExplorer`, `SiteAdministrator`, `Creator`, `Explorer`, `ExplorerCanPublish`, `Viewer`, `Unlicensed`. -- `ServerAdministrator` is synced (read-only) and consumes a Creator license, but it is never a grant target — the assignable license is Creator, and the `ServerAdministrator` role itself cannot be set via the sites API. +- `ServerAdministrator` is synced (read-only) and consumes a Creator license, but it is never a grant target — the assignable license is Creator, and this connector does not assign the `ServerAdministrator` role itself. Revoking a tier from a Server Administrator leaves the user Unlicensed, and the role has to be restored by a server administrator in Tableau. ### License Grants @@ -417,7 +417,7 @@ Tested against: `us-east-1.online.tableau.com` (Tableau Cloud) | viewer | OK | OK | | | unlicensed | OK | OK | | | site administrator | OK | OK | Legacy role (maps to SiteAdministratorExplorer) | -| server administrator | Blocked | N/A | Read-only: server-level role, cannot be assigned via REST API | +| server administrator | Blocked | N/A | Read-only: exists only on Tableau Server; this connector does not assign it | ### Group Membership diff --git a/pkg/connector/license.go b/pkg/connector/license.go index f78d534b..06a38cc7 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -272,7 +272,8 @@ func (l *licenseBuilder) Grants(ctx context.Context, resource *v2.Resource, opts // - already in the target tier -> no write, GrantAlreadyExists. // - current role is an admin role outside the tier -> refuse with // FailedPrecondition; a license grant must not double as a silent -// demotion, and ServerAdministrator cannot be restored via this API. +// demotion. ServerAdministrator exists only on Tableau Server and is +// not assigned by this connector. // - otherwise -> write the tier's base role and, when the user's previous // role belonged to another tier, report GrantReplaced naming the license // grant the write displaced (tiers are exclusive, so a cross-tier grant diff --git a/pkg/connector/site.go b/pkg/connector/site.go index 4e3efc35..d99137d8 100644 --- a/pkg/connector/site.go +++ b/pkg/connector/site.go @@ -30,8 +30,8 @@ const ( // roles maps Tableau API role names to display slugs. // ServerAdministrator is included for sync (Tableau returns it for server-level -// admins) but cannot be granted via the REST API — it is a server-level role, -// not a site-level role. +// admins) but this connector does not assign it — it exists only on Tableau +// Server. var roles = map[string]string{ siteAdministrator: "site administrator", siteAdministratorCreator: "site administrator creator", @@ -44,8 +44,8 @@ var roles = map[string]string{ unlicensed: "unlicensed", } -// nonGrantableRoles are roles that can appear during sync but cannot be -// assigned via the REST API's Update User Site Role endpoint. +// nonGrantableRoles are roles that can appear during sync but that this +// connector does not assign. var nonGrantableRoles = map[string]bool{ serverAdministrator: true, } @@ -155,7 +155,7 @@ func (s *siteBuilder) Grant(ctx context.Context, principal *v2.Resource, entitle } if nonGrantableRoles[apiRoleName] { - return nil, fmt.Errorf("role %q cannot be assigned via the Tableau REST API (server-level role, not site-level)", apiRoleName) + return nil, fmt.Errorf("role %q is not assigned by this connector (it exists only on Tableau Server)", apiRoleName) } annos, err := s.client.UpdateUserSiteRole(ctx, principalID, apiRoleName) From 094978fd91dcf0219f5cb9b82b429d2115a3b353 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Tue, 27 Oct 2026 10:33:57 -0300 Subject: [PATCH 06/13] [CXH-2368] - Drop duplicate warn log on admin license grant refusal --- pkg/connector/license.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/connector/license.go b/pkg/connector/license.go index 06a38cc7..053f1261 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -303,8 +303,6 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti } if adminLicenseRoles[user.SiteRole] { - log.Warn("license grant: refusing to move an admin role across license tiers", - zap.String("user_id", principal.Id.Resource), zap.String("tier", targetTier), zap.String("site_role", user.SiteRole)) return getAnnos, status.Errorf(codes.FailedPrecondition, "license: cannot grant %s to user %s: current site role %q is an administrative role; use the Site Role entitlement to change it explicitly", targetTier, principal.Id.Resource, user.SiteRole) From 491f8ad6121c6934297d4ca3b66ff017b9f3f078 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Tue, 27 Oct 2026 13:04:05 -0300 Subject: [PATCH 07/13] fix: treat Unlicensed license revoke as an annotated no-op --- README.md | 2 +- docs/connector.mdx | 2 +- pkg/connector/license.go | 12 +++++++----- pkg/connector/license_test.go | 12 ++++++------ 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 2997e07f..e0384b7e 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ baton resources - **"All Users" group**: Tableau automatically adds every user to the built-in "All Users" group. This group cannot be modified via the API — Grant and Revoke operations on it are not supported. The connector syncs its membership to show each user's base site role. - **License revoke constraints**: Revoking a license (setting a user to "Unlicensed") will fail if the user belongs to a group with a "Grant role on sign in" minimum site role. Remove the user from the constraining group first, then revoke the license. - **License grant is tier-aware**: A license tier is a view of the user's single site role (e.g. `SiteAdministratorCreator` already satisfies the Creator tier), so granting a tier the user already holds is a no-op and keeps their current role. Granting a *different* tier to a user currently holding a `SiteAdministrator*`/`ServerAdministrator` role is refused instead of silently demoting them; use the Site Role entitlement to change a `SiteAdministrator*` role explicitly. -- **License revoke sets the user to Unlicensed**: Revoking a tier sets the user's site role to `Unlicensed`, including when their current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, `ServerAdministrator`). `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server, and this connector does not assign it. Revoking a tier from a Server Administrator leaves the user Unlicensed, and the role has to be restored by a server administrator in Tableau. Revoking the Unlicensed tier itself is rejected, since it is the floor tier and has nothing to remove. +- **License revoke sets the user to Unlicensed**: Revoking a tier sets the user's site role to `Unlicensed`, including when their current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, `ServerAdministrator`). `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server, and this connector does not assign it. Revoking a tier from a Server Administrator leaves the user Unlicensed, and the role has to be restored by a server administrator in Tableau. Revoking the Unlicensed tier from a user who is already Unlicensed is a no-op: the connector writes nothing and reports the grant as already revoked, because Unlicensed is the floor tier and has nothing to remove. - **Idempotency**: Granting a permission that already exists returns success (not an error). Revoking a permission that is already removed also returns success. # Credentials Setup diff --git a/docs/connector.mdx b/docs/connector.mdx index 9b8469a4..a139d5f6 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -35,7 +35,7 @@ When your Tableau site has per-tier seat capacities configured, C1 tracks the pu Because a tier is a view of the user's single site role, several roles can satisfy the same tier (for example, `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op and keeps the user's current role. Granting a *different* tier to a user who currently holds a Site Administrator or Server Administrator role is refused rather than silently removing their admin access — use the Site Role entitlement to change an admin role explicitly. -Revoking a tier sets the user's site role to Unlicensed, including when the user's current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`). `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server, and this connector does not assign it. Revoking a tier from a Server Administrator leaves the user Unlicensed, and the role has to be restored by a server administrator in Tableau. Revoking the Unlicensed tier itself is rejected, because Unlicensed is the floor tier and has nothing to remove. +Revoking a tier sets the user's site role to Unlicensed, including when the user's current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`). `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server, and this connector does not assign it. Revoking a tier from a Server Administrator leaves the user Unlicensed, and the role has to be restored by a server administrator in Tableau. Revoking the Unlicensed tier from a user who is already Unlicensed is a no-op: the connector writes nothing and reports the grant as already revoked, because Unlicensed is the floor tier and has nothing to remove. ## Permission inheritance diff --git a/pkg/connector/license.go b/pkg/connector/license.go index 053f1261..ed8db0ab 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -335,9 +335,10 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti // GrantReplaced annotation), means there is nothing left to revoke. Writing // Unlicensed in that case would wipe out access this revoke was never asked // to touch. Revoking the Unlicensed tier from a user who is Unlicensed is -// rejected with FailedPrecondition: it is the floor tier and has nothing to -// remove. Rate-limit annotations from the initial read are merged into every -// result. +// likewise a no-op reported as GrantAlreadyRevoked: it is the floor tier and +// has nothing to remove, and an error would only fail an offboarding task +// with nothing actionable upstream. Rate-limit annotations from the initial +// read are merged into every result. func (l *licenseBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.Annotations, error) { log := ctxzap.Extract(ctx) principalID := g.Principal.Id.Resource @@ -363,8 +364,9 @@ func (l *licenseBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.A } if tier == unlicensed { - return getAnnos, status.Errorf(codes.FailedPrecondition, - "license: cannot revoke the Unlicensed tier from user %s: Unlicensed is the floor tier and has nothing to remove", principalID) + log.Debug("license revoke: user already Unlicensed (floor tier), nothing to remove", + zap.String("user_id", principalID)) + return mergeAnnotations(getAnnos, annotations.New(&v2.GrantAlreadyRevoked{})), nil } putAnnos, err := l.client.UpdateUserSiteRole(ctx, principalID, unlicensed) diff --git a/pkg/connector/license_test.go b/pkg/connector/license_test.go index a86037bd..9de9b2c3 100644 --- a/pkg/connector/license_test.go +++ b/pkg/connector/license_test.go @@ -586,8 +586,9 @@ func TestLicense_PropagatesGetUserAnnotations(t *testing.T) { } // TestLicenseRevoke_UnlicensedTier asserts that Unlicensed is the floor tier: -// revoking it from an Unlicensed user is rejected without a write, while -// revoking it from a user who holds some other tier is a stale revoke. +// revoking it from an Unlicensed user is an annotated no-op (GrantAlreadyRevoked, +// no write, nil error) because there is nothing left to remove, and revoking it +// from a user who holds some other tier is a stale revoke. func TestLicenseRevoke_UnlicensedTier(t *testing.T) { t.Parallel() @@ -601,10 +602,9 @@ func TestLicenseRevoke_UnlicensedTier(t *testing.T) { Entitlement: testLicenseEntitlement(unlicensed), Principal: testUserPrincipal("user-1"), }) - require.Error(t, err) - require.Equal(t, codes.FailedPrecondition, status.Code(err)) - require.Contains(t, err.Error(), "Unlicensed is the floor tier") - require.Empty(t, puts, "rejected revoke must not write") + require.NoError(t, err) + require.True(t, annos.Contains(&v2.GrantAlreadyRevoked{})) + require.Empty(t, puts, "floor-tier revoke must not write") requireSingleRateLimit(t, annos) }) From 7871521da00d67cd2c702f4ed941f4e1dff69eee Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Tue, 27 Oct 2026 14:40:17 -0300 Subject: [PATCH 08/13] docs: clarify tier-aware license provisioning and floor-role no-op --- README.md | 5 ++--- docs/connector.mdx | 4 ++-- docs/docs-info.md | 3 +++ pkg/connector/license.go | 43 +++++++++------------------------------- 4 files changed, 16 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index e0384b7e..0eefddf0 100644 --- a/README.md +++ b/README.md @@ -71,9 +71,8 @@ baton resources - **Server Administrators**: Users with `ServerAdministrator` site role are skipped during group membership sync (server-level admins are not site-scoped). - **Grant expansion**: Group-based permissions on projects, workbooks, and views are supported. When a group has a permission, all members inherit it. - **"All Users" group**: Tableau automatically adds every user to the built-in "All Users" group. This group cannot be modified via the API — Grant and Revoke operations on it are not supported. The connector syncs its membership to show each user's base site role. -- **License revoke constraints**: Revoking a license (setting a user to "Unlicensed") will fail if the user belongs to a group with a "Grant role on sign in" minimum site role. Remove the user from the constraining group first, then revoke the license. -- **License grant is tier-aware**: A license tier is a view of the user's single site role (e.g. `SiteAdministratorCreator` already satisfies the Creator tier), so granting a tier the user already holds is a no-op and keeps their current role. Granting a *different* tier to a user currently holding a `SiteAdministrator*`/`ServerAdministrator` role is refused instead of silently demoting them; use the Site Role entitlement to change a `SiteAdministrator*` role explicitly. -- **License revoke sets the user to Unlicensed**: Revoking a tier sets the user's site role to `Unlicensed`, including when their current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, `ServerAdministrator`). `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server, and this connector does not assign it. Revoking a tier from a Server Administrator leaves the user Unlicensed, and the role has to be restored by a server administrator in Tableau. Revoking the Unlicensed tier from a user who is already Unlicensed is a no-op: the connector writes nothing and reports the grant as already revoked, because Unlicensed is the floor tier and has nothing to remove. +- **License grant is tier-aware**: A license tier is a view of the user's single site role (e.g. `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op that keeps their current role and returns `GrantAlreadyExists`. Granting a *different* tier to a user holding `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator` fails with `FailedPrecondition`; the Site Role entitlement can assign the `SiteAdministrator*` roles, but this connector does not assign `ServerAdministrator`. Granting a different tier to any other user sets their site role to the tier's base role, and reports `GrantReplaced` only when their previous role belonged to a recognized tier. A user that does not exist returns `NotFound`. +- **License revoke sets the user to Unlicensed**: If the user still holds the tier, revoking sets their site role to `Unlicensed`, including when that role is `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`. If the user is not found or no longer holds the tier, revoke returns `GrantAlreadyRevoked` without writing. Revoking the Unlicensed tier from a user who is already Unlicensed is the same no-op; the next sync still shows the user as Unlicensed in C1, because that is their current role. `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server and this connector does not assign it, so revoking a tier from a Server Administrator leaves the user Unlicensed until a server administrator restores the role in Tableau. Revoking a license will also fail if the user belongs to a group with a "Grant role on sign in" minimum site role; remove the user from the constraining group first, then revoke the license. - **Idempotency**: Granting a permission that already exists returns success (not an error). Revoking a permission that is already removed also returns success. # Credentials Setup diff --git a/docs/connector.mdx b/docs/connector.mdx index a139d5f6..4aa0990d 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -33,9 +33,9 @@ In Tableau, licensing is role-based: each user's **site role** determines their When your Tableau site has per-tier seat capacities configured, C1 tracks the purchased and consumed seat counts for each license tier, so you can review license utilization alongside who holds each tier. -Because a tier is a view of the user's single site role, several roles can satisfy the same tier (for example, `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op and keeps the user's current role. Granting a *different* tier to a user who currently holds a Site Administrator or Server Administrator role is refused rather than silently removing their admin access — use the Site Role entitlement to change an admin role explicitly. +Because a tier is a view of the user's single site role, several roles can satisfy the same tier (for example, `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op that keeps the user's current role and returns `GrantAlreadyExists`. Granting a *different* tier to a user holding `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator` fails with `FailedPrecondition`; the Site Role entitlement can assign the `SiteAdministrator*` roles, but this connector does not assign `ServerAdministrator`. Granting a different tier to any other user sets their site role to the tier's base role, and reports `GrantReplaced` only when their previous role belonged to a recognized tier. A user that does not exist returns `NotFound`. -Revoking a tier sets the user's site role to Unlicensed, including when the user's current site role is an administrator role (`SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`). `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server, and this connector does not assign it. Revoking a tier from a Server Administrator leaves the user Unlicensed, and the role has to be restored by a server administrator in Tableau. Revoking the Unlicensed tier from a user who is already Unlicensed is a no-op: the connector writes nothing and reports the grant as already revoked, because Unlicensed is the floor tier and has nothing to remove. +If the user still holds the tier, revoking sets their site role to Unlicensed, including when that role is `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`. If the user is not found or no longer holds the tier, revoke returns `GrantAlreadyRevoked` without writing. Revoking the Unlicensed tier from a user who is already Unlicensed is the same no-op; the next sync still shows the user as Unlicensed in C1, because that is their current role. `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server and this connector does not assign it, so revoking a tier from a Server Administrator leaves the user Unlicensed until a server administrator restores the role in Tableau. Revoking a license also fails if the user belongs to a group with a "Grant role on sign in" minimum site role; remove the user from the constraining group first. ## Permission inheritance diff --git a/docs/docs-info.md b/docs/docs-info.md index bc199b74..351c44e0 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -325,6 +325,9 @@ - The Explorer license includes `SiteAdministrator` (legacy role). This role is not supported by the Tableau API's `siteRole` filter, so the connector falls back to fetching all users and filtering client-side for the Explorer license. - For Creator, Viewer, and Unlicensed licenses, server-side filtering is used for efficiency. +- **Grant** is tier-aware: a user whose site role already belongs to the target tier gets `GrantAlreadyExists` with no write and keeps their role. A different tier for a user holding `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator` fails with `FailedPrecondition` (the Site Role entitlement assigns the `SiteAdministrator*` roles; this connector does not assign `ServerAdministrator`). Any other user is set to the tier's base role, with `GrantReplaced` only when the previous role belonged to a recognized tier. A missing user returns `NotFound`. +- **Revoke** sets a user who still holds the tier to `Unlicensed`, including the four admin roles above. A missing user, or one who no longer holds the tier, returns `GrantAlreadyRevoked` without a write. Revoking the Unlicensed tier from an already Unlicensed user is the same successful no-op; the next sync still shows Unlicensed in C1 because it is the floor role and current state. +- Grant and Revoke read the user first without caching and merge the read's rate-limit annotations into every result. - Consumed-seat counts reuse the same server-side `siteRole` filter but read only `pagination.totalAvailable` per tier (no full user walk). The Explorer count excludes the unfilterable legacy `SiteAdministrator` role, which does not exist on Tableau Cloud. ### View Grants and showTabs diff --git a/pkg/connector/license.go b/pkg/connector/license.go index ed8db0ab..6e279539 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -37,12 +37,7 @@ var RolesPerLicense = map[string][]string{ unlicensed: {unlicensed}, } -// adminLicenseRoles are the site roles a cross-tier license Grant must never -// silently demote. A license tier is a derived view of a user's single site -// role, so granting a different tier is itself a role change; stripping an -// admin role as a side effect of that change would be an unannounced -// privilege reduction. Use the Site Role entitlement to change an admin role -// explicitly instead. +// adminLicenseRoles are refused on a cross-tier license Grant. var adminLicenseRoles = map[string]bool{ siteAdministrator: true, siteAdministratorCreator: true, @@ -263,23 +258,11 @@ func (l *licenseBuilder) Grants(ctx context.Context, resource *v2.Resource, opts return rv, &rs.SyncOpResults{NextPageToken: nextToken}, nil } -// Grant assigns the target license tier by setting the user's site role. -// -// A Tableau user has exactly one site role, and RolesPerLicense groups which -// roles satisfy which tier, so a license tier is a derived view of that one -// role rather than an independent attribute. Grant therefore reads the user's -// current role before writing: -// - already in the target tier -> no write, GrantAlreadyExists. -// - current role is an admin role outside the tier -> refuse with -// FailedPrecondition; a license grant must not double as a silent -// demotion. ServerAdministrator exists only on Tableau Server and is -// not assigned by this connector. -// - otherwise -> write the tier's base role and, when the user's previous -// role belonged to another tier, report GrantReplaced naming the license -// grant the write displaced (tiers are exclusive, so a cross-tier grant -// is itself the requested role change). -// -// Rate-limit annotations from the initial read are merged into every result. +// Grant sets the user's site role to the tier's base role. It reads the user +// first: a missing user returns NotFound, a role already in the tier returns +// GrantAlreadyExists without writing, an admin role fails with +// FailedPrecondition, and otherwise it writes, reporting 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) { log := ctxzap.Extract(ctx) @@ -328,17 +311,9 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti return annos, nil } -// Revoke removes the license by setting the user's site role to Unlicensed. -// -// Revoke reads the user's current role first: a 404, or a role that has -// already moved out of this tier (e.g. a later Grant replaced it, see Grant's -// GrantReplaced annotation), means there is nothing left to revoke. Writing -// Unlicensed in that case would wipe out access this revoke was never asked -// to touch. Revoking the Unlicensed tier from a user who is Unlicensed is -// likewise a no-op reported as GrantAlreadyRevoked: it is the floor tier and -// has nothing to remove, and an error would only fail an offboarding task -// with nothing actionable upstream. Rate-limit annotations from the initial -// read are merged into every result. +// Revoke sets the user's site role to Unlicensed. It reads the user first and +// returns GrantAlreadyRevoked without writing when the user is missing, no +// longer in the tier, or already Unlicensed. func (l *licenseBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.Annotations, error) { log := ctxzap.Extract(ctx) principalID := g.Principal.Id.Resource From 2b4009aebdb3f75f6a6ea1edc2c14e2350514cbc Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Thu, 29 Oct 2026 09:47:03 -0300 Subject: [PATCH 09/13] [CXH-2368] - Declare license tiers as one exclusion group and tighten license docs/comments --- README.md | 4 +- docs/connector.mdx | 4 +- docs/docs-info.md | 2 +- pkg/connector/license.go | 22 +++-- pkg/connector/license_exclusion_test.go | 122 ++++++++++++++++++++++++ pkg/connector/license_test.go | 52 +++++++--- 6 files changed, 181 insertions(+), 25 deletions(-) create mode 100644 pkg/connector/license_exclusion_test.go diff --git a/README.md b/README.md index 0eefddf0..c1104f44 100644 --- a/README.md +++ b/README.md @@ -71,8 +71,8 @@ baton resources - **Server Administrators**: Users with `ServerAdministrator` site role are skipped during group membership sync (server-level admins are not site-scoped). - **Grant expansion**: Group-based permissions on projects, workbooks, and views are supported. When a group has a permission, all members inherit it. - **"All Users" group**: Tableau automatically adds every user to the built-in "All Users" group. This group cannot be modified via the API — Grant and Revoke operations on it are not supported. The connector syncs its membership to show each user's base site role. -- **License grant is tier-aware**: A license tier is a view of the user's single site role (e.g. `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op that keeps their current role and returns `GrantAlreadyExists`. Granting a *different* tier to a user holding `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator` fails with `FailedPrecondition`; the Site Role entitlement can assign the `SiteAdministrator*` roles, but this connector does not assign `ServerAdministrator`. Granting a different tier to any other user sets their site role to the tier's base role, and reports `GrantReplaced` only when their previous role belonged to a recognized tier. A user that does not exist returns `NotFound`. -- **License revoke sets the user to Unlicensed**: If the user still holds the tier, revoking sets their site role to `Unlicensed`, including when that role is `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`. If the user is not found or no longer holds the tier, revoke returns `GrantAlreadyRevoked` without writing. Revoking the Unlicensed tier from a user who is already Unlicensed is the same no-op; the next sync still shows the user as Unlicensed in C1, because that is their current role. `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server and this connector does not assign it, so revoking a tier from a Server Administrator leaves the user Unlicensed until a server administrator restores the role in Tableau. Revoking a license will also fail if the user belongs to a group with a "Grant role on sign in" minimum site role; remove the user from the constraining group first, then revoke the license. +- **License grant is tier-aware**: A tier is a view of the user's single site role (for example, `SiteAdministratorCreator` already satisfies Creator), and the four tiers form one exclusion group (`tableau-license`) with no default tier or ranking. Granting a tier the user already holds is a no-op that keeps their role and returns `GrantAlreadyExists`. Granting a different tier to a user holding `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator` fails with `FailedPrecondition`: change those roles through the Site Role entitlement (it assigns `SiteAdministrator*`; this connector does not assign `ServerAdministrator`). Any other user is set to the tier's base role, with `GrantReplaced` only when the previous role belonged to a recognized tier. A missing user returns `NotFound`. +- **License revoke sets the user to Unlicensed**: A user who still holds the tier is set to `Unlicensed`, including the four admin roles above. A missing user, or one who no longer holds the tier, returns `GrantAlreadyRevoked` with no write; so does revoking Unlicensed from an Unlicensed user, and the next sync still shows that user as Unlicensed. Re-assign `SiteAdministrator*` roles through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server and this connector does not assign it, so a revoked Server Administrator stays Unlicensed until the role is restored in Tableau. Revoke also fails if the user belongs to a group with a "Grant role on sign in" minimum site role; remove them from that group first. - **Idempotency**: Granting a permission that already exists returns success (not an error). Revoking a permission that is already removed also returns success. # Credentials Setup diff --git a/docs/connector.mdx b/docs/connector.mdx index 4aa0990d..1fc74525 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -33,9 +33,9 @@ In Tableau, licensing is role-based: each user's **site role** determines their When your Tableau site has per-tier seat capacities configured, C1 tracks the purchased and consumed seat counts for each license tier, so you can review license utilization alongside who holds each tier. -Because a tier is a view of the user's single site role, several roles can satisfy the same tier (for example, `SiteAdministratorCreator` already satisfies the Creator tier). Granting a tier the user already holds is a no-op that keeps the user's current role and returns `GrantAlreadyExists`. Granting a *different* tier to a user holding `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator` fails with `FailedPrecondition`; the Site Role entitlement can assign the `SiteAdministrator*` roles, but this connector does not assign `ServerAdministrator`. Granting a different tier to any other user sets their site role to the tier's base role, and reports `GrantReplaced` only when their previous role belonged to a recognized tier. A user that does not exist returns `NotFound`. +Because a tier is a view of the user's single site role, several roles can satisfy one tier (for example, `SiteAdministratorCreator` satisfies Creator), and the four tiers form one exclusion group (`tableau-license`) with no default tier or ranking. Granting a tier the user already holds is a no-op that keeps their role and returns `GrantAlreadyExists`. Granting a *different* tier to a user holding `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator` fails with `FailedPrecondition`: change those roles through the Site Role entitlement (it assigns `SiteAdministrator*`; this connector does not assign `ServerAdministrator`). Any other user is set to the tier's base role, with `GrantReplaced` only when the previous role belonged to a recognized tier. A missing user returns `NotFound`. -If the user still holds the tier, revoking sets their site role to Unlicensed, including when that role is `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator`. If the user is not found or no longer holds the tier, revoke returns `GrantAlreadyRevoked` without writing. Revoking the Unlicensed tier from a user who is already Unlicensed is the same no-op; the next sync still shows the user as Unlicensed in C1, because that is their current role. `SiteAdministrator*` roles can be re-assigned through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server and this connector does not assign it, so revoking a tier from a Server Administrator leaves the user Unlicensed until a server administrator restores the role in Tableau. Revoking a license also fails if the user belongs to a group with a "Grant role on sign in" minimum site role; remove the user from the constraining group first. +A user who still holds the tier is set to Unlicensed on revoke, including the four admin roles above. A missing user, or one who no longer holds the tier, returns `GrantAlreadyRevoked` with no write; so does revoking Unlicensed from an Unlicensed user, and the next sync still shows that user as Unlicensed. Re-assign `SiteAdministrator*` roles through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server and this connector does not assign it, so a revoked Server Administrator stays Unlicensed until the role is restored in Tableau. Revoke also fails if the user belongs to a group with a "Grant role on sign in" minimum site role; remove them from that group first. ## Permission inheritance diff --git a/docs/docs-info.md b/docs/docs-info.md index 351c44e0..447cd699 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -187,7 +187,7 @@ - `Viewer` — View and interact with published dashboards - `Unlicensed` — No access to Tableau site features - **Entitlements**: - - `member` — License is assigned to user (static entitlement) + - `member` — License is assigned to user (static entitlement); all four tiers share exclusion group `tableau-license` (unscoped, no default or order) - **Seat counts** (License Profile trait): - **Purchased** — read from the site's per-tier capacity fields (`tierCreatorCapacity`, `tierExplorerCapacity`, `tierViewerCapacity`), which the Tableau API returns as JSON strings. Attached only when the value is a positive number; a `0` or absent capacity attaches no seats (avoids rendering "0 of 0"). The `Unlicensed` floor tier is never capped. - **Consumed** — counted server-side per tier via the users `siteRole` filter, reading `pagination.totalAvailable` (one request per tier instead of walking every user page). diff --git a/pkg/connector/license.go b/pkg/connector/license.go index 6e279539..80bc1745 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -21,6 +21,12 @@ import ( var licenses = []string{creator, explorer, viewer, unlicensed} +// licenseExclusionGroup marks the four tier entitlements as mutually exclusive: +// a user's single site role puts them in exactly one tier. It is deliberately +// unscoped, so the SDK shares one group across the tier resources, and sets no +// default or order, so C1 gets no fallback tier or privilege ranking. +const licenseExclusionGroup = "tableau-license" + var licensesMap = map[string]string{ "creator": creator, "explorer": explorer, @@ -211,6 +217,7 @@ func (l *licenseBuilder) StaticEntitlements(_ context.Context, _ rs.SyncOpAttrs) ent.WithGrantableTo(resourceTypeUser), ent.WithDisplayName("License member"), ent.WithDescription("Member of License in Tableau"), + ent.WithExclusionGroup(licenseExclusionGroup), ) return []*v2.Entitlement{en}, nil, nil @@ -258,11 +265,10 @@ func (l *licenseBuilder) Grants(ctx context.Context, resource *v2.Resource, opts return rv, &rs.SyncOpResults{NextPageToken: nextToken}, nil } -// Grant sets the user's site role to the tier's base role. It reads the user -// first: a missing user returns NotFound, a role already in the tier returns -// GrantAlreadyExists without writing, an admin role fails with -// FailedPrecondition, and otherwise it writes, reporting GrantReplaced when the -// previous role belonged to a known tier. +// Grant sets the user's site role to the tier's base role after a fresh read: +// 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) { log := ctxzap.Extract(ctx) @@ -311,9 +317,9 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti return annos, nil } -// Revoke sets the user's site role to Unlicensed. It reads the user first and -// returns GrantAlreadyRevoked without writing when the user is missing, no -// longer in the tier, or already Unlicensed. +// Revoke sets the user's site role to Unlicensed after a fresh read, returning +// GrantAlreadyRevoked without writing when the user is missing, no longer in the +// tier, or already Unlicensed. func (l *licenseBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.Annotations, error) { log := ctxzap.Extract(ctx) principalID := g.Principal.Id.Resource diff --git a/pkg/connector/license_exclusion_test.go b/pkg/connector/license_exclusion_test.go new file mode 100644 index 00000000..5f3cd005 --- /dev/null +++ b/pkg/connector/license_exclusion_test.go @@ -0,0 +1,122 @@ +package connector + +import ( + "context" + "path/filepath" + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/connectorbuilder" + "github.com/conductorone/baton-sdk/pkg/dotc1z" + "github.com/conductorone/baton-sdk/pkg/sync" + "github.com/conductorone/baton-sdk/pkg/types" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" +) + +// licenseOnlyConnector syncs just the license resource type, so the test runs a +// real SDK sync without mocking the rest of the Tableau API. +type licenseOnlyConnector struct { + *Connector +} + +// Validate skips the site read the mock does not serve. +func (c *licenseOnlyConnector) Validate(_ context.Context) (annotations.Annotations, error) { + return nil, nil +} + +func (c *licenseOnlyConnector) ResourceSyncers(_ context.Context) []connectorbuilder.ResourceSyncerV2 { + return []connectorbuilder.ResourceSyncerV2{newLicenseBuilder(c.client)} +} + +// inProcessClient lets the SDK syncer call a connector server directly. Only +// the RPCs a license-only sync issues are forwarded; any other call panics on +// the nil embedded client, which flags a new syncer dependency. +type inProcessClient struct { + types.ConnectorClient + server types.ConnectorServer +} + +func (c *inProcessClient) GetMetadata(ctx context.Context, in *v2.ConnectorServiceGetMetadataRequest, _ ...grpc.CallOption) (*v2.ConnectorServiceGetMetadataResponse, error) { + return c.server.GetMetadata(ctx, in) +} + +func (c *inProcessClient) Validate(ctx context.Context, in *v2.ConnectorServiceValidateRequest, _ ...grpc.CallOption) (*v2.ConnectorServiceValidateResponse, error) { + return c.server.Validate(ctx, in) +} + +func (c *inProcessClient) Cleanup(ctx context.Context, in *v2.ConnectorServiceCleanupRequest, _ ...grpc.CallOption) (*v2.ConnectorServiceCleanupResponse, error) { + return c.server.Cleanup(ctx, in) +} + +func (c *inProcessClient) ListResourceTypes(ctx context.Context, in *v2.ResourceTypesServiceListResourceTypesRequest, _ ...grpc.CallOption) (*v2.ResourceTypesServiceListResourceTypesResponse, error) { + return c.server.ListResourceTypes(ctx, in) +} + +func (c *inProcessClient) ListResources(ctx context.Context, in *v2.ResourcesServiceListResourcesRequest, _ ...grpc.CallOption) (*v2.ResourcesServiceListResourcesResponse, error) { + return c.server.ListResources(ctx, in) +} + +func (c *inProcessClient) ListStaticEntitlements( + ctx context.Context, in *v2.EntitlementsServiceListStaticEntitlementsRequest, _ ...grpc.CallOption, +) (*v2.EntitlementsServiceListStaticEntitlementsResponse, error) { + return c.server.ListStaticEntitlements(ctx, in) +} + +func (c *inProcessClient) ListEntitlements(ctx context.Context, in *v2.EntitlementsServiceListEntitlementsRequest, _ ...grpc.CallOption) (*v2.EntitlementsServiceListEntitlementsResponse, error) { + return c.server.ListEntitlements(ctx, in) +} + +func (c *inProcessClient) ListGrants(ctx context.Context, in *v2.GrantsServiceListGrantsRequest, _ ...grpc.CallOption) (*v2.GrantsServiceListGrantsResponse, error) { + return c.server.ListGrants(ctx, in) +} + +// TestLicenseSync_TierEntitlementsShareExclusionGroup runs a real SDK sync and +// reads the stored entitlements back, so it covers how the SDK stamps the +// static member entitlement onto each tier: one shared, unscoped group with no +// default or order, and entitlement IDs unchanged. +func TestLicenseSync_TierEntitlementsShareExclusionGroup(t *testing.T) { + ctx := context.Background() + + var puts []recordedPut + // The mock only serves sign-in and single-user reads: the site read fails, so + // tiers sync without seats, and the user list 404s, so grants fail soft. + tc := newLicenseTestClient(t, map[string]string{}, &puts) + + server, err := connectorbuilder.NewConnector(ctx, &licenseOnlyConnector{Connector: &Connector{client: tc}}) + require.NoError(t, err) + + c1zPath := filepath.Join(t.TempDir(), "sync.c1z") + syncer, err := sync.NewSyncer(ctx, &inProcessClient{server: server}, sync.WithC1ZPath(c1zPath), sync.WithTmpDir(t.TempDir())) + require.NoError(t, err) + require.NoError(t, syncer.Sync(ctx)) + require.NoError(t, syncer.Close(ctx)) + + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) + require.NoError(t, err) + t.Cleanup(func() { _ = store.Close(ctx) }) + + resp, err := store.ListEntitlements(ctx, &v2.EntitlementsServiceListEntitlementsRequest{}) + require.NoError(t, err) + + got := map[string]*v2.EntitlementExclusionGroup{} + for _, e := range resp.GetList() { + group := &v2.EntitlementExclusionGroup{} + annos := annotations.Annotations(e.GetAnnotations()) + ok, err := annos.Pick(group) + require.NoError(t, err) + require.True(t, ok, "entitlement %s has no exclusion group", e.GetId()) + got[e.GetId()] = group + } + + require.Len(t, got, len(licenses)) + for _, id := range []string{"license:creator:member", "license:explorer:member", "license:viewer:member", "license:unlicensed:member"} { + group, ok := got[id] + require.True(t, ok, "missing entitlement ID %s", id) + require.Equal(t, "tableau-license", group.GetExclusionGroupId()) + require.False(t, group.GetScopeToResource(), "group must be shared across tiers, not scoped per resource") + require.False(t, group.GetIsDefault(), "no fallback tier is authorized") + require.Zero(t, group.GetOrder(), "no privilege order is authorized") + } +} diff --git a/pkg/connector/license_test.go b/pkg/connector/license_test.go index 9de9b2c3..b7c9df5c 100644 --- a/pkg/connector/license_test.go +++ b/pkg/connector/license_test.go @@ -134,9 +134,8 @@ const ( licenseTestRateRemaining = 42 ) -// licenseTestUsersMu guards the users maps handed to newLicenseTestClient: the -// mock server reads and applies PUTs to them from its own goroutine while a -// test may change a role between calls. +// licenseTestUsersMu guards the users maps handed to newLicenseTestClient, which +// the mock server's goroutine reads and writes while a test may change a role. var licenseTestUsersMu sync.Mutex type recordedPut struct { @@ -144,13 +143,10 @@ type recordedPut struct { siteRole string } -// newLicenseTestClient starts a mock Tableau API server that authenticates -// any login, serves GetUser from users (keyed by user ID, valued by current -// site role, updated by every PUT; a missing key means a 404 and the licenseTestServerError role -// means a 500), stamps rate-limit headers on every GET and PUT so tests can check the -// annotations are propagated, and records every PUT -// (UpdateUserSiteRole) it receives so tests can assert exact write counts -// and bodies. +// newLicenseTestClient starts a mock Tableau API that accepts any login and +// serves GetUser from users (user ID -> site role, updated by each PUT; a +// missing key is a 404, licenseTestServerError a 500). Every GET and PUT carries +// rate-limit headers, and every PUT is recorded in puts. func newLicenseTestClient(t *testing.T, users map[string]string, puts *[]recordedPut) *client.Client { t.Helper() @@ -267,6 +263,7 @@ func TestLicenseGrant_AlreadyInTier(t *testing.T) { require.NoError(t, err) require.Empty(t, puts, "already-in-tier grant must not write") require.True(t, annos.Contains(&v2.GrantAlreadyExists{})) + require.False(t, annos.Contains(&v2.GrantReplaced{}), "same-tier no-op replaces nothing") }) } } @@ -319,9 +316,10 @@ func TestLicenseGrant_CrossTierAdmin(t *testing.T) { target = creator } - _, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(target)) + annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(target)) require.Error(t, err) require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.False(t, annos.Contains(&v2.GrantReplaced{})) require.Empty(t, puts, "refused admin cross-tier grant must not write") }) } @@ -437,9 +435,10 @@ func TestLicense_GetUserServerError(t *testing.T) { c := newLicenseTestClient(t, map[string]string{"user-1": licenseTestServerError}, &puts) l := newLicenseBuilder(c) - _, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.Error(t, err) require.NotEqual(t, codes.NotFound, status.Code(err)) + require.False(t, annos.Contains(&v2.GrantReplaced{})) require.Empty(t, puts, "failed read must not write") }) @@ -674,3 +673,32 @@ func TestLicenseGrant_RoleChangedBetweenGrants(t *testing.T) { require.Len(t, puts, 1) require.Equal(t, creator, puts[0].siteRole) } + +// TestLicenseGrant_FailedWriteNoReplacement asserts that a cross-tier grant +// whose write fails reports no GrantReplaced, since nothing was replaced. +func TestLicenseGrant_FailedWriteNoReplacement(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case r.Method == http.MethodPost && r.URL.Path == "/auth/signin": + _ = json.NewEncoder(w).Encode(map[string]any{ + "credentials": map[string]any{"site": map[string]any{"id": licenseTestSiteID}, "user": map[string]any{"id": "me"}, "token": "t"}, + }) + case r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode(map[string]any{"user": map[string]any{"id": "user-1", "siteRole": viewer}}) + default: + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":{"summary":"Bad Request","detail":"rejected","code":"0x4"}}`)) + } + })) + t.Cleanup(srv.Close) + + c, err := client.New(context.Background(), "unused", licenseTestSiteID, "token-name", "token-secret", "3.19", srv.URL) + require.NoError(t, err) + + annos, err := newLicenseBuilder(c).Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + require.ErrorContains(t, err, "failed to grant Creator license") + require.False(t, annos.Contains(&v2.GrantReplaced{})) +} From 42f1601b5e5c148cacd9858afa6a0e46e78e147a Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Thu, 29 Oct 2026 10:17:32 -0300 Subject: [PATCH 10/13] [CXH-2368] - Consolidate license tests in license_test.go --- pkg/connector/license_exclusion_test.go | 122 ------------------------ pkg/connector/license_test.go | 112 ++++++++++++++++++++++ 2 files changed, 112 insertions(+), 122 deletions(-) delete mode 100644 pkg/connector/license_exclusion_test.go diff --git a/pkg/connector/license_exclusion_test.go b/pkg/connector/license_exclusion_test.go deleted file mode 100644 index 5f3cd005..00000000 --- a/pkg/connector/license_exclusion_test.go +++ /dev/null @@ -1,122 +0,0 @@ -package connector - -import ( - "context" - "path/filepath" - "testing" - - v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" - "github.com/conductorone/baton-sdk/pkg/annotations" - "github.com/conductorone/baton-sdk/pkg/connectorbuilder" - "github.com/conductorone/baton-sdk/pkg/dotc1z" - "github.com/conductorone/baton-sdk/pkg/sync" - "github.com/conductorone/baton-sdk/pkg/types" - "github.com/stretchr/testify/require" - "google.golang.org/grpc" -) - -// licenseOnlyConnector syncs just the license resource type, so the test runs a -// real SDK sync without mocking the rest of the Tableau API. -type licenseOnlyConnector struct { - *Connector -} - -// Validate skips the site read the mock does not serve. -func (c *licenseOnlyConnector) Validate(_ context.Context) (annotations.Annotations, error) { - return nil, nil -} - -func (c *licenseOnlyConnector) ResourceSyncers(_ context.Context) []connectorbuilder.ResourceSyncerV2 { - return []connectorbuilder.ResourceSyncerV2{newLicenseBuilder(c.client)} -} - -// inProcessClient lets the SDK syncer call a connector server directly. Only -// the RPCs a license-only sync issues are forwarded; any other call panics on -// the nil embedded client, which flags a new syncer dependency. -type inProcessClient struct { - types.ConnectorClient - server types.ConnectorServer -} - -func (c *inProcessClient) GetMetadata(ctx context.Context, in *v2.ConnectorServiceGetMetadataRequest, _ ...grpc.CallOption) (*v2.ConnectorServiceGetMetadataResponse, error) { - return c.server.GetMetadata(ctx, in) -} - -func (c *inProcessClient) Validate(ctx context.Context, in *v2.ConnectorServiceValidateRequest, _ ...grpc.CallOption) (*v2.ConnectorServiceValidateResponse, error) { - return c.server.Validate(ctx, in) -} - -func (c *inProcessClient) Cleanup(ctx context.Context, in *v2.ConnectorServiceCleanupRequest, _ ...grpc.CallOption) (*v2.ConnectorServiceCleanupResponse, error) { - return c.server.Cleanup(ctx, in) -} - -func (c *inProcessClient) ListResourceTypes(ctx context.Context, in *v2.ResourceTypesServiceListResourceTypesRequest, _ ...grpc.CallOption) (*v2.ResourceTypesServiceListResourceTypesResponse, error) { - return c.server.ListResourceTypes(ctx, in) -} - -func (c *inProcessClient) ListResources(ctx context.Context, in *v2.ResourcesServiceListResourcesRequest, _ ...grpc.CallOption) (*v2.ResourcesServiceListResourcesResponse, error) { - return c.server.ListResources(ctx, in) -} - -func (c *inProcessClient) ListStaticEntitlements( - ctx context.Context, in *v2.EntitlementsServiceListStaticEntitlementsRequest, _ ...grpc.CallOption, -) (*v2.EntitlementsServiceListStaticEntitlementsResponse, error) { - return c.server.ListStaticEntitlements(ctx, in) -} - -func (c *inProcessClient) ListEntitlements(ctx context.Context, in *v2.EntitlementsServiceListEntitlementsRequest, _ ...grpc.CallOption) (*v2.EntitlementsServiceListEntitlementsResponse, error) { - return c.server.ListEntitlements(ctx, in) -} - -func (c *inProcessClient) ListGrants(ctx context.Context, in *v2.GrantsServiceListGrantsRequest, _ ...grpc.CallOption) (*v2.GrantsServiceListGrantsResponse, error) { - return c.server.ListGrants(ctx, in) -} - -// TestLicenseSync_TierEntitlementsShareExclusionGroup runs a real SDK sync and -// reads the stored entitlements back, so it covers how the SDK stamps the -// static member entitlement onto each tier: one shared, unscoped group with no -// default or order, and entitlement IDs unchanged. -func TestLicenseSync_TierEntitlementsShareExclusionGroup(t *testing.T) { - ctx := context.Background() - - var puts []recordedPut - // The mock only serves sign-in and single-user reads: the site read fails, so - // tiers sync without seats, and the user list 404s, so grants fail soft. - tc := newLicenseTestClient(t, map[string]string{}, &puts) - - server, err := connectorbuilder.NewConnector(ctx, &licenseOnlyConnector{Connector: &Connector{client: tc}}) - require.NoError(t, err) - - c1zPath := filepath.Join(t.TempDir(), "sync.c1z") - syncer, err := sync.NewSyncer(ctx, &inProcessClient{server: server}, sync.WithC1ZPath(c1zPath), sync.WithTmpDir(t.TempDir())) - require.NoError(t, err) - require.NoError(t, syncer.Sync(ctx)) - require.NoError(t, syncer.Close(ctx)) - - store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) - require.NoError(t, err) - t.Cleanup(func() { _ = store.Close(ctx) }) - - resp, err := store.ListEntitlements(ctx, &v2.EntitlementsServiceListEntitlementsRequest{}) - require.NoError(t, err) - - got := map[string]*v2.EntitlementExclusionGroup{} - for _, e := range resp.GetList() { - group := &v2.EntitlementExclusionGroup{} - annos := annotations.Annotations(e.GetAnnotations()) - ok, err := annos.Pick(group) - require.NoError(t, err) - require.True(t, ok, "entitlement %s has no exclusion group", e.GetId()) - got[e.GetId()] = group - } - - require.Len(t, got, len(licenses)) - for _, id := range []string{"license:creator:member", "license:explorer:member", "license:viewer:member", "license:unlicensed:member"} { - group, ok := got[id] - require.True(t, ok, "missing entitlement ID %s", id) - require.Equal(t, "tableau-license", group.GetExclusionGroupId()) - require.False(t, group.GetScopeToResource(), "group must be shared across tiers, not scoped per resource") - require.False(t, group.GetIsDefault(), "no fallback tier is authorized") - require.Zero(t, group.GetOrder(), "no privilege order is authorized") - } -} diff --git a/pkg/connector/license_test.go b/pkg/connector/license_test.go index b7c9df5c..22ccc7a7 100644 --- a/pkg/connector/license_test.go +++ b/pkg/connector/license_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "path/filepath" "strconv" "strings" "sync" @@ -12,9 +13,14 @@ import ( v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/connectorbuilder" + "github.com/conductorone/baton-sdk/pkg/dotc1z" + sdksync "github.com/conductorone/baton-sdk/pkg/sync" + "github.com/conductorone/baton-sdk/pkg/types" "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/conductorone/baton-tableau/pkg/client" "github.com/stretchr/testify/require" + "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -702,3 +708,109 @@ func TestLicenseGrant_FailedWriteNoReplacement(t *testing.T) { require.ErrorContains(t, err, "failed to grant Creator license") require.False(t, annos.Contains(&v2.GrantReplaced{})) } + +// licenseOnlyConnector syncs just the license resource type, so the test runs a +// real SDK sync without mocking the rest of the Tableau API. +type licenseOnlyConnector struct { + *Connector +} + +// Validate skips the site read the mock does not serve. +func (c *licenseOnlyConnector) Validate(_ context.Context) (annotations.Annotations, error) { + return nil, nil +} + +func (c *licenseOnlyConnector) ResourceSyncers(_ context.Context) []connectorbuilder.ResourceSyncerV2 { + return []connectorbuilder.ResourceSyncerV2{newLicenseBuilder(c.client)} +} + +// inProcessClient lets the SDK syncer call a connector server directly. Only +// the RPCs a license-only sync issues are forwarded; any other call panics on +// the nil embedded client, which flags a new syncer dependency. +type inProcessClient struct { + types.ConnectorClient + server types.ConnectorServer +} + +func (c *inProcessClient) GetMetadata(ctx context.Context, in *v2.ConnectorServiceGetMetadataRequest, _ ...grpc.CallOption) (*v2.ConnectorServiceGetMetadataResponse, error) { + return c.server.GetMetadata(ctx, in) +} + +func (c *inProcessClient) Validate(ctx context.Context, in *v2.ConnectorServiceValidateRequest, _ ...grpc.CallOption) (*v2.ConnectorServiceValidateResponse, error) { + return c.server.Validate(ctx, in) +} + +func (c *inProcessClient) Cleanup(ctx context.Context, in *v2.ConnectorServiceCleanupRequest, _ ...grpc.CallOption) (*v2.ConnectorServiceCleanupResponse, error) { + return c.server.Cleanup(ctx, in) +} + +func (c *inProcessClient) ListResourceTypes(ctx context.Context, in *v2.ResourceTypesServiceListResourceTypesRequest, _ ...grpc.CallOption) (*v2.ResourceTypesServiceListResourceTypesResponse, error) { + return c.server.ListResourceTypes(ctx, in) +} + +func (c *inProcessClient) ListResources(ctx context.Context, in *v2.ResourcesServiceListResourcesRequest, _ ...grpc.CallOption) (*v2.ResourcesServiceListResourcesResponse, error) { + return c.server.ListResources(ctx, in) +} + +func (c *inProcessClient) ListStaticEntitlements( + ctx context.Context, in *v2.EntitlementsServiceListStaticEntitlementsRequest, _ ...grpc.CallOption, +) (*v2.EntitlementsServiceListStaticEntitlementsResponse, error) { + return c.server.ListStaticEntitlements(ctx, in) +} + +func (c *inProcessClient) ListEntitlements(ctx context.Context, in *v2.EntitlementsServiceListEntitlementsRequest, _ ...grpc.CallOption) (*v2.EntitlementsServiceListEntitlementsResponse, error) { + return c.server.ListEntitlements(ctx, in) +} + +func (c *inProcessClient) ListGrants(ctx context.Context, in *v2.GrantsServiceListGrantsRequest, _ ...grpc.CallOption) (*v2.GrantsServiceListGrantsResponse, error) { + return c.server.ListGrants(ctx, in) +} + +// TestLicenseSync_TierEntitlementsShareExclusionGroup runs a real SDK sync and +// reads the stored entitlements back, so it covers how the SDK stamps the +// static member entitlement onto each tier: one shared, unscoped group with no +// default or order, and entitlement IDs unchanged. +func TestLicenseSync_TierEntitlementsShareExclusionGroup(t *testing.T) { + ctx := context.Background() + + var puts []recordedPut + // The mock only serves sign-in and single-user reads: the site read fails, so + // tiers sync without seats, and the user list 404s, so grants fail soft. + tc := newLicenseTestClient(t, map[string]string{}, &puts) + + server, err := connectorbuilder.NewConnector(ctx, &licenseOnlyConnector{Connector: &Connector{client: tc}}) + require.NoError(t, err) + + c1zPath := filepath.Join(t.TempDir(), "sync.c1z") + syncer, err := sdksync.NewSyncer(ctx, &inProcessClient{server: server}, sdksync.WithC1ZPath(c1zPath), sdksync.WithTmpDir(t.TempDir())) + require.NoError(t, err) + require.NoError(t, syncer.Sync(ctx)) + require.NoError(t, syncer.Close(ctx)) + + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) + require.NoError(t, err) + t.Cleanup(func() { _ = store.Close(ctx) }) + + resp, err := store.ListEntitlements(ctx, &v2.EntitlementsServiceListEntitlementsRequest{}) + require.NoError(t, err) + + got := map[string]*v2.EntitlementExclusionGroup{} + for _, e := range resp.GetList() { + group := &v2.EntitlementExclusionGroup{} + annos := annotations.Annotations(e.GetAnnotations()) + ok, err := annos.Pick(group) + require.NoError(t, err) + require.True(t, ok, "entitlement %s has no exclusion group", e.GetId()) + got[e.GetId()] = group + } + + require.Len(t, got, len(licenses)) + for _, id := range []string{"license:creator:member", "license:explorer:member", "license:viewer:member", "license:unlicensed:member"} { + group, ok := got[id] + require.True(t, ok, "missing entitlement ID %s", id) + require.Equal(t, "tableau-license", group.GetExclusionGroupId()) + require.False(t, group.GetScopeToResource(), "group must be shared across tiers, not scoped per resource") + require.False(t, group.GetIsDefault(), "no fallback tier is authorized") + require.Zero(t, group.GetOrder(), "no privilege order is authorized") + } +} From 9977f65a532f0b5ae2c4115dbf2b60fdf6348460 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Wed, 4 Nov 2026 13:39:36 -0300 Subject: [PATCH 11/13] [CXH-2368] - Address review: wrap NotFound, simplify replaced-grant check, plain-language docs --- docs/connector.mdx | 4 ++-- pkg/connector/license.go | 4 ++-- pkg/connector/license_test.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/connector.mdx b/docs/connector.mdx index 1fc74525..7271eca9 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -33,9 +33,9 @@ In Tableau, licensing is role-based: each user's **site role** determines their When your Tableau site has per-tier seat capacities configured, C1 tracks the purchased and consumed seat counts for each license tier, so you can review license utilization alongside who holds each tier. -Because a tier is a view of the user's single site role, several roles can satisfy one tier (for example, `SiteAdministratorCreator` satisfies Creator), and the four tiers form one exclusion group (`tableau-license`) with no default tier or ranking. Granting a tier the user already holds is a no-op that keeps their role and returns `GrantAlreadyExists`. Granting a *different* tier to a user holding `SiteAdministrator`, `SiteAdministratorCreator`, `SiteAdministratorExplorer`, or `ServerAdministrator` fails with `FailedPrecondition`: change those roles through the Site Role entitlement (it assigns `SiteAdministrator*`; this connector does not assign `ServerAdministrator`). Any other user is set to the tier's base role, with `GrantReplaced` only when the previous role belonged to a recognized tier. A missing user returns `NotFound`. +Because a tier is a view of the user's single site role, several roles can satisfy one tier (for example, a Site Administrator Creator counts as Creator), and the four tiers are mutually exclusive in C1 with no default tier or ranking. Granting a tier the user already holds changes nothing and keeps their current role. Administrators are refused when you grant them a different tier: change their role through the Site Role entitlement instead. For any other user, granting a tier sets their site role to that tier's base role, and C1 replaces their previous tier grant when the previous role belonged to a recognized tier. Granting to a user that no longer exists returns a not-found error. -A user who still holds the tier is set to Unlicensed on revoke, including the four admin roles above. A missing user, or one who no longer holds the tier, returns `GrantAlreadyRevoked` with no write; so does revoking Unlicensed from an Unlicensed user, and the next sync still shows that user as Unlicensed. Re-assign `SiteAdministrator*` roles through the Site Role entitlement. `ServerAdministrator` exists only on Tableau Server and this connector does not assign it, so a revoked Server Administrator stays Unlicensed until the role is restored in Tableau. Revoke also fails if the user belongs to a group with a "Grant role on sign in" minimum site role; remove them from that group first. +Revoking a tier a user still holds sets them to Unlicensed, including users with an administrator role. Revoking from a user who no longer exists, or who no longer holds the tier, changes nothing; the same applies to revoking Unlicensed from an Unlicensed user, and the next sync still shows that user as Unlicensed. To give a revoked administrator their role back, assign it through the Site Role entitlement. Server Administrator exists only on Tableau Server and this connector cannot assign it, so a revoked Server Administrator stays Unlicensed until a server administrator restores the role in Tableau. Revoke also fails if the user belongs to a group with a "Grant role on sign in" minimum site role; remove them from that group first. ## Permission inheritance diff --git a/pkg/connector/license.go b/pkg/connector/license.go index 80bc1745..35a6119c 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -280,7 +280,7 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti user, getAnnos, err := l.client.GetUser(ctx, principal.Id.Resource) if err != nil { if client.IsNotFoundError(err) { - return getAnnos, status.Errorf(codes.NotFound, "license: cannot grant %s: user %s not found", targetTier, principal.Id.Resource) + return getAnnos, fmt.Errorf("license: cannot grant %s: user %s not found: %w", targetTier, principal.Id.Resource, err) } return getAnnos, fmt.Errorf("failed to read user %s for license grant: %w", principal.Id.Resource, err) } @@ -303,7 +303,7 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti return annos, fmt.Errorf("failed to grant %s license to user %s: %w", targetTier, principal.Id.Resource, err) } - if oldTier, ok := tierForRole(user.SiteRole); ok && oldTier != targetTier { + if oldTier, ok := tierForRole(user.SiteRole); ok { oldResource := &v2.Resource{ Id: &v2.ResourceId{ ResourceType: resourceTypeLicense.Id, diff --git a/pkg/connector/license_test.go b/pkg/connector/license_test.go index 22ccc7a7..54b3aa71 100644 --- a/pkg/connector/license_test.go +++ b/pkg/connector/license_test.go @@ -302,7 +302,7 @@ func TestLicenseGrant_CrossTierNonAdmin(t *testing.T) { // TestLicenseGrant_CrossTierAdmin asserts that granting a different tier to a // user currently holding an admin role is refused with FailedPrecondition -// rather than silently stripping admin rights, and does not write. +// and does not write. func TestLicenseGrant_CrossTierAdmin(t *testing.T) { t.Parallel() From e04a03fcef81c4a2442cd5a9301315a38a642db6 Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Wed, 4 Nov 2026 15:29:26 -0300 Subject: [PATCH 12/13] [CXH-2368] - Return InvalidArgument for an unknown license tier --- pkg/connector/license.go | 4 ++-- pkg/connector/license_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/pkg/connector/license.go b/pkg/connector/license.go index 35a6119c..045758df 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -274,7 +274,7 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti targetTier, ok := licensesMap[entitlement.Resource.Id.Resource] if !ok { - return nil, fmt.Errorf("unknown license %s", entitlement.Resource.Id.Resource) + return nil, status.Errorf(codes.InvalidArgument, "unknown license %s", entitlement.Resource.Id.Resource) } user, getAnnos, err := l.client.GetUser(ctx, principal.Id.Resource) @@ -326,7 +326,7 @@ func (l *licenseBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.A tier, ok := licensesMap[g.Entitlement.Resource.Id.Resource] if !ok { - return nil, fmt.Errorf("unknown license %s", g.Entitlement.Resource.Id.Resource) + return nil, status.Errorf(codes.InvalidArgument, "unknown license %s", g.Entitlement.Resource.Id.Resource) } user, getAnnos, err := l.client.GetUser(ctx, principalID) diff --git a/pkg/connector/license_test.go b/pkg/connector/license_test.go index 54b3aa71..20626f59 100644 --- a/pkg/connector/license_test.go +++ b/pkg/connector/license_test.go @@ -408,6 +408,41 @@ func TestLicenseRevoke_UserNotFound(t *testing.T) { require.True(t, annos.Contains(&v2.GrantAlreadyRevoked{})) } +// TestLicense_UnknownLicense asserts that an unknown license resource ID is +// rejected with InvalidArgument before any write. +func TestLicense_UnknownLicense(t *testing.T) { + t.Parallel() + + ent := testLicenseEntitlement("platinum") + + t.Run("grant", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": creator}, &puts)) + + _, err := l.Grant(context.Background(), testUserPrincipal("user-1"), ent) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.Empty(t, puts) + }) + + t.Run("revoke", func(t *testing.T) { + t.Parallel() + + var puts []recordedPut + l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": creator}, &puts)) + + _, err := l.Revoke(context.Background(), &v2.Grant{ + Entitlement: ent, + Principal: testUserPrincipal("user-1"), + }) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.Empty(t, puts) + }) +} + // requireSingleRateLimit asserts annos carries exactly one RateLimitDescription // and that it is the one the mock GET responses stamped. func requireSingleRateLimit(t *testing.T, annos annotations.Annotations) { From fad2cab84bd2bd84241f7adfa5016419bcf674cb Mon Sep 17 00:00:00 2001 From: "sergio.corral" Date: Fri, 6 Nov 2026 13:54:32 -0300 Subject: [PATCH 13/13] [CXH-2368] - Return the new grant from license Grant (V2 signature) --- pkg/connector/license.go | 23 ++++++++----- pkg/connector/license_test.go | 64 ++++++++++++++++++++++++++--------- 2 files changed, 62 insertions(+), 25 deletions(-) diff --git a/pkg/connector/license.go b/pkg/connector/license.go index 045758df..40fec2a1 100644 --- a/pkg/connector/license.go +++ b/pkg/connector/license.go @@ -9,6 +9,7 @@ import ( v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/connectorbuilder" ent "github.com/conductorone/baton-sdk/pkg/types/entitlement" grant "github.com/conductorone/baton-sdk/pkg/types/grant" rs "github.com/conductorone/baton-sdk/pkg/types/resource" @@ -265,34 +266,35 @@ func (l *licenseBuilder) Grants(ctx context.Context, resource *v2.Resource, opts return rv, &rs.SyncOpResults{NextPageToken: nextToken}, nil } -// Grant sets the user's site role to the tier's base role after a fresh read: +// Grant sets the user's site role to the tier's base role after a fresh read +// and returns the new grant on a write (nil on every no-op or error path): // 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) { +func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) ([]*v2.Grant, annotations.Annotations, error) { log := ctxzap.Extract(ctx) targetTier, ok := licensesMap[entitlement.Resource.Id.Resource] if !ok { - return nil, status.Errorf(codes.InvalidArgument, "unknown license %s", entitlement.Resource.Id.Resource) + return nil, nil, status.Errorf(codes.InvalidArgument, "unknown license %s", entitlement.Resource.Id.Resource) } user, getAnnos, err := l.client.GetUser(ctx, principal.Id.Resource) if err != nil { if client.IsNotFoundError(err) { - return getAnnos, fmt.Errorf("license: cannot grant %s: user %s not found: %w", targetTier, principal.Id.Resource, err) + return nil, getAnnos, fmt.Errorf("license: cannot grant %s: user %s not found: %w", targetTier, principal.Id.Resource, err) } - return getAnnos, fmt.Errorf("failed to read user %s for license grant: %w", principal.Id.Resource, err) + return nil, getAnnos, fmt.Errorf("failed to read user %s for license grant: %w", principal.Id.Resource, err) } if slices.Contains(RolesPerLicense[targetTier], user.SiteRole) { log.Debug("license grant: user already in target tier, no write needed", zap.String("user_id", principal.Id.Resource), zap.String("tier", targetTier), zap.String("site_role", user.SiteRole)) - return mergeAnnotations(getAnnos, annotations.New(&v2.GrantAlreadyExists{})), nil + return nil, mergeAnnotations(getAnnos, annotations.New(&v2.GrantAlreadyExists{})), nil } if adminLicenseRoles[user.SiteRole] { - return getAnnos, status.Errorf(codes.FailedPrecondition, + return nil, getAnnos, status.Errorf(codes.FailedPrecondition, "license: cannot grant %s to user %s: current site role %q is an administrative role; use the Site Role entitlement to change it explicitly", targetTier, principal.Id.Resource, user.SiteRole) } @@ -300,7 +302,7 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti putAnnos, err := l.client.UpdateUserSiteRole(ctx, principal.Id.Resource, targetTier) annos := mergeAnnotations(getAnnos, putAnnos) if err != nil { - return annos, fmt.Errorf("failed to grant %s license to user %s: %w", targetTier, principal.Id.Resource, err) + return nil, annos, fmt.Errorf("failed to grant %s license to user %s: %w", targetTier, principal.Id.Resource, err) } if oldTier, ok := tierForRole(user.SiteRole); ok { @@ -314,7 +316,8 @@ func (l *licenseBuilder) Grant(ctx context.Context, principal *v2.Resource, enti annos.Append(&v2.GrantReplaced{ReplacedGrantId: grant.NewGrantID(principal.Id, oldEntitlement)}) } - return annos, nil + // Same shape as Grants() emits, so the grant ID matches the one sync reports. + return []*v2.Grant{grant.NewGrant(entitlement.Resource, memberEntitlement, principal.Id)}, annos, nil } // Revoke sets the user's site role to Unlicensed after a fresh read, returning @@ -359,6 +362,8 @@ func (l *licenseBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.A return annos, nil } +var _ connectorbuilder.ResourceProvisionerV2 = (*licenseBuilder)(nil) + func newLicenseBuilder(client *client.Client) *licenseBuilder { return &licenseBuilder{ client: client, diff --git a/pkg/connector/license_test.go b/pkg/connector/license_test.go index 20626f59..00de592c 100644 --- a/pkg/connector/license_test.go +++ b/pkg/connector/license_test.go @@ -17,6 +17,7 @@ import ( "github.com/conductorone/baton-sdk/pkg/dotc1z" sdksync "github.com/conductorone/baton-sdk/pkg/sync" "github.com/conductorone/baton-sdk/pkg/types" + "github.com/conductorone/baton-sdk/pkg/types/grant" "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/conductorone/baton-tableau/pkg/client" "github.com/stretchr/testify/require" @@ -238,6 +239,29 @@ func testLicenseEntitlement(tier string) *v2.Entitlement { } } +// syncGrantID returns the ID Grants() emits for the user holding the tier, built +// from the same license resource and user resource the sync uses. +func syncGrantID(t *testing.T, tier, userID string) string { + t.Helper() + + lic, err := licenseResource(tier, nil, 0) + require.NoError(t, err) + u, err := userResource(&client.User{ID: userID}, lic.Id) + require.NoError(t, err) + return grant.NewGrant(lic, memberEntitlement, u.Id).GetId() +} + +// requireWriteGrant asserts that Grant returned exactly the grant sync would emit. +func requireWriteGrant(t *testing.T, grants []*v2.Grant, tier, userID string) { + t.Helper() + + require.Len(t, grants, 1) + require.Equal(t, strings.ToLower(tier), grants[0].GetEntitlement().GetResource().GetId().GetResource()) + require.Equal(t, "license:"+strings.ToLower(tier)+":"+memberEntitlement, grants[0].GetEntitlement().GetId()) + require.Equal(t, userID, grants[0].GetPrincipal().GetId().GetResource()) + require.Equal(t, syncGrantID(t, tier, userID), grants[0].GetId()) +} + // TestLicenseGrant_AlreadyInTier asserts that granting a tier to a user whose // current site role already satisfies that tier performs no write and reports // GrantAlreadyExists. @@ -265,8 +289,9 @@ func TestLicenseGrant_AlreadyInTier(t *testing.T) { c := newLicenseTestClient(t, map[string]string{"user-1": tt.curRole}, &puts) l := newLicenseBuilder(c) - annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(tt.tier)) + grants, annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(tt.tier)) require.NoError(t, err) + require.Empty(t, grants, "no-op grant returns no grants") require.Empty(t, puts, "already-in-tier grant must not write") require.True(t, annos.Contains(&v2.GrantAlreadyExists{})) require.False(t, annos.Contains(&v2.GrantReplaced{}), "same-tier no-op replaces nothing") @@ -284,8 +309,9 @@ func TestLicenseGrant_CrossTierNonAdmin(t *testing.T) { c := newLicenseTestClient(t, map[string]string{"user-1": viewer}, &puts) l := newLicenseBuilder(c) - annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + grants, annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.NoError(t, err) + requireWriteGrant(t, grants, creator, "user-1") require.Len(t, puts, 1, "cross-tier grant must write exactly once") require.Equal(t, "user-1", puts[0].userID) @@ -322,8 +348,9 @@ func TestLicenseGrant_CrossTierAdmin(t *testing.T) { target = creator } - annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(target)) + grants, annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(target)) require.Error(t, err) + require.Empty(t, grants) require.Equal(t, codes.FailedPrecondition, status.Code(err)) require.False(t, annos.Contains(&v2.GrantReplaced{})) require.Empty(t, puts, "refused admin cross-tier grant must not write") @@ -340,8 +367,9 @@ func TestLicenseGrant_UnknownUser(t *testing.T) { c := newLicenseTestClient(t, map[string]string{}, &puts) l := newLicenseBuilder(c) - _, err := l.Grant(context.Background(), testUserPrincipal("ghost"), testLicenseEntitlement(creator)) + grants, _, err := l.Grant(context.Background(), testUserPrincipal("ghost"), testLicenseEntitlement(creator)) require.Error(t, err) + require.Empty(t, grants) require.Equal(t, codes.NotFound, status.Code(err)) require.Empty(t, puts) } @@ -421,8 +449,9 @@ func TestLicense_UnknownLicense(t *testing.T) { var puts []recordedPut l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": creator}, &puts)) - _, err := l.Grant(context.Background(), testUserPrincipal("user-1"), ent) + grants, _, err := l.Grant(context.Background(), testUserPrincipal("user-1"), ent) require.Error(t, err) + require.Empty(t, grants) require.Equal(t, codes.InvalidArgument, status.Code(err)) require.Empty(t, puts) }) @@ -476,7 +505,7 @@ func TestLicense_GetUserServerError(t *testing.T) { c := newLicenseTestClient(t, map[string]string{"user-1": licenseTestServerError}, &puts) l := newLicenseBuilder(c) - annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + _, annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.Error(t, err) require.NotEqual(t, codes.NotFound, status.Code(err)) require.False(t, annos.Contains(&v2.GrantReplaced{})) @@ -511,7 +540,7 @@ func TestLicenseGrant_RoleOutsideEveryTier(t *testing.T) { c := newLicenseTestClient(t, map[string]string{"user-1": "Guest"}, &puts) l := newLicenseBuilder(c) - annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + _, annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.NoError(t, err) require.Equal(t, []recordedPut{{userID: "user-1", siteRole: creator}}, puts) require.False(t, annos.Contains(&v2.GrantReplaced{})) @@ -528,7 +557,7 @@ func TestLicenseGrant_ExplorerCanPublishToCreator(t *testing.T) { c := newLicenseTestClient(t, map[string]string{"user-1": explorerCanPublish}, &puts) l := newLicenseBuilder(c) - annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + _, annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.NoError(t, err) require.Equal(t, []recordedPut{{userID: "user-1", siteRole: creator}}, puts) @@ -554,7 +583,7 @@ func TestLicense_PropagatesGetUserAnnotations(t *testing.T) { var puts []recordedPut l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": creator}, &puts)) - annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + _, annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.NoError(t, err) require.True(t, annos.Contains(&v2.GrantAlreadyExists{})) requireSingleRateLimit(t, annos) @@ -565,7 +594,7 @@ func TestLicense_PropagatesGetUserAnnotations(t *testing.T) { var puts []recordedPut l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": siteAdministratorCreator}, &puts)) - annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(explorer)) + _, annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(explorer)) require.Equal(t, codes.FailedPrecondition, status.Code(err)) requireSingleRateLimit(t, annos) }) @@ -575,7 +604,7 @@ func TestLicense_PropagatesGetUserAnnotations(t *testing.T) { var puts []recordedPut l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{}, &puts)) - annos, err := l.Grant(context.Background(), testUserPrincipal("ghost"), testLicenseEntitlement(creator)) + _, annos, err := l.Grant(context.Background(), testUserPrincipal("ghost"), testLicenseEntitlement(creator)) require.Equal(t, codes.NotFound, status.Code(err)) requireSingleRateLimit(t, annos) }) @@ -585,7 +614,7 @@ func TestLicense_PropagatesGetUserAnnotations(t *testing.T) { var puts []recordedPut l := newLicenseBuilder(newLicenseTestClient(t, map[string]string{"user-1": viewer}, &puts)) - annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + _, annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.NoError(t, err) require.True(t, annos.Contains(&v2.GrantReplaced{})) requireSingleRateLimit(t, annos) @@ -674,7 +703,7 @@ func TestLicense_GrantThenRevokeSameClient(t *testing.T) { c := newLicenseTestClient(t, map[string]string{"user-1": viewer}, &puts) l := newLicenseBuilder(c) - _, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + _, _, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.NoError(t, err) require.Len(t, puts, 1) require.Equal(t, creator, puts[0].siteRole) @@ -699,8 +728,9 @@ func TestLicenseGrant_RoleChangedBetweenGrants(t *testing.T) { c := newLicenseTestClient(t, users, &puts) l := newLicenseBuilder(c) - annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + grants, annos, err := l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.NoError(t, err) + require.Empty(t, grants) require.True(t, annos.Contains(&v2.GrantAlreadyExists{})) require.Empty(t, puts) @@ -708,8 +738,9 @@ func TestLicenseGrant_RoleChangedBetweenGrants(t *testing.T) { users["user-1"] = viewer licenseTestUsersMu.Unlock() - annos, err = l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + grants, annos, err = l.Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.NoError(t, err) + requireWriteGrant(t, grants, creator, "user-1") require.False(t, annos.Contains(&v2.GrantAlreadyExists{}), "grant must see the externally changed role") require.Len(t, puts, 1) require.Equal(t, creator, puts[0].siteRole) @@ -739,8 +770,9 @@ func TestLicenseGrant_FailedWriteNoReplacement(t *testing.T) { c, err := client.New(context.Background(), "unused", licenseTestSiteID, "token-name", "token-secret", "3.19", srv.URL) require.NoError(t, err) - annos, err := newLicenseBuilder(c).Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) + grants, annos, err := newLicenseBuilder(c).Grant(context.Background(), testUserPrincipal("user-1"), testLicenseEntitlement(creator)) require.ErrorContains(t, err, "failed to grant Creator license") + require.Empty(t, grants) require.False(t, annos.Contains(&v2.GrantReplaced{})) }