Add rbacstore.TransferOwner and enterprise-auth -transfer-owner-*
RevokeMembership refuses to revoke a tenant's current Owner (would leave tenants.owner_user_id dangling), but there was no way to actually hand ownership to someone else -- only the raw SetOwner primitive, which -grant-membership-role=owner used without downgrading whoever held Owner before, leaving tenant_memberships claiming two owners while owner_user_id can only name one. Named as a real, disclosed gap in docs/phase-4-runbook.md's "Known gaps". rbacstore.TransferOwner closes it: downgrades the current owner's membership to admin, promotes the new owner, and updates tenants.owner_user_id, all in one pgx transaction -- this package's first use of one. Every other mutation here is a single independent statement because nothing else needs more than one row to agree; this does, for the same reason RevokeMembership's doc comment already gives for refusing to revoke Owner in the first place. -grant-membership-role=owner now refuses when a *different* owner already exists, pointing at the new -transfer-owner-tenant/ -transfer-owner-user-email flags instead of silently producing the inconsistent state -- it remains correct, unchanged, for a tenant's first owner assignment. Verified with the same skip-gated live-Postgres discipline as the rest of this package (TestTransferOwnerMovesOwnershipAndDowngradesPrevious Owner proves the downgrade is real by then successfully revoking the former owner's now-non-Owner membership; two refusal-path tests cover no-current-owner and transfer-to-self) -- not run against a live database in this environment, same disclosed gap as everything else here.
This commit is contained in:
@@ -233,6 +233,63 @@ func (s *Store) SetOwner(ctx context.Context, tenantID, userID string) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// TransferOwner atomically moves a tenant's Owner from whoever currently
|
||||
// holds tenants.owner_user_id to newOwnerUserID: downgrades the
|
||||
// current owner's tenant_memberships row to downgradeRole (the RBAC
|
||||
// matrix's "Transfer tenant Owner -- Owner only" -- the previous owner
|
||||
// keeps a real role, typically RoleAdmin, rather than being silently
|
||||
// ejected from the tenant), upserts the new owner's membership to
|
||||
// RoleOwner, and updates tenants.owner_user_id -- all in one
|
||||
// transaction, the first in this package. Every other mutation here is
|
||||
// a single independent statement because nothing else needs more than
|
||||
// one row to agree; this genuinely does, for the exact reason
|
||||
// RevokeMembership's doc comment already gives: owner_user_id pointing
|
||||
// at a user whose tenant_memberships row doesn't say 'owner' (or vice
|
||||
// versa) is an inconsistent state, and calling SetOwner/SetMembership
|
||||
// separately outside a transaction risks landing in it on any failure
|
||||
// between the two calls, not just caller error.
|
||||
func (s *Store) TransferOwner(ctx context.Context, tenantID, newOwnerUserID string, downgradeRole Role) error {
|
||||
tenant, err := s.GetTenant(ctx, tenantID)
|
||||
if err != nil {
|
||||
return fmt.Errorf("rbacstore: getting tenant to transfer ownership: %w", err)
|
||||
}
|
||||
if tenant.OwnerUserID == "" {
|
||||
return fmt.Errorf("rbacstore: tenant %q has no current owner -- use SetMembership+SetOwner directly for a first assignment, TransferOwner is for an existing owner handing off", tenantID)
|
||||
}
|
||||
if tenant.OwnerUserID == newOwnerUserID {
|
||||
return fmt.Errorf("rbacstore: %q is already tenant %q's owner", newOwnerUserID, tenantID)
|
||||
}
|
||||
|
||||
tx, err := s.pool.Begin(ctx)
|
||||
if err != nil {
|
||||
return fmt.Errorf("rbacstore: beginning ownership transfer: %w", err)
|
||||
}
|
||||
defer tx.Rollback(ctx) // no-op once Commit succeeds
|
||||
|
||||
upsertMembership := `
|
||||
INSERT INTO tenant_memberships (id, tenant_id, user_id, role)
|
||||
VALUES ($1, $2, $3, $4)
|
||||
ON CONFLICT (tenant_id, user_id) DO UPDATE
|
||||
SET role = EXCLUDED.role, updated_at = now()`
|
||||
if _, err := tx.Exec(ctx, upsertMembership, uuid.NewString(), tenantID, tenant.OwnerUserID, string(downgradeRole)); err != nil {
|
||||
return fmt.Errorf("rbacstore: downgrading previous owner's membership: %w", err)
|
||||
}
|
||||
if _, err := tx.Exec(ctx, upsertMembership, uuid.NewString(), tenantID, newOwnerUserID, string(RoleOwner)); err != nil {
|
||||
return fmt.Errorf("rbacstore: setting new owner's membership: %w", err)
|
||||
}
|
||||
tag, err := tx.Exec(ctx, `UPDATE tenants SET owner_user_id = $2, updated_at = now() WHERE id = $1`, tenantID, newOwnerUserID)
|
||||
if err != nil {
|
||||
return fmt.Errorf("rbacstore: setting tenant owner: %w", err)
|
||||
}
|
||||
if tag.RowsAffected() == 0 {
|
||||
return ErrNotFound
|
||||
}
|
||||
if err := tx.Commit(ctx); err != nil {
|
||||
return fmt.Errorf("rbacstore: committing ownership transfer: %w", err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// SetMembership upserts a user's role for a tenant -- the sole mutation
|
||||
// path for tenant_memberships, so every role change naturally funnels
|
||||
// through one method a future audit-log hook (EventRoleChange, see
|
||||
|
||||
@@ -180,6 +180,107 @@ func TestSetOwnerAndMembershipRoundTrip(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestTransferOwnerMovesOwnershipAndDowngradesPreviousOwner(t *testing.T) {
|
||||
s := testStore(t)
|
||||
ctx := context.Background()
|
||||
tenantID := "test-tenant-" + uniqueSuffix()
|
||||
if _, err := s.CreateTenant(ctx, tenantID, "Test Tenant"); err != nil {
|
||||
t.Fatalf("CreateTenant: %v", err)
|
||||
}
|
||||
oldOwner, err := s.UpsertUserBySSO(ctx, "sub-old-owner-"+uniqueSuffix(), "old-owner-"+uniqueSuffix()+"@example.com", "Old Owner")
|
||||
if err != nil {
|
||||
t.Fatalf("UpsertUserBySSO (old owner): %v", err)
|
||||
}
|
||||
newOwner, err := s.UpsertUserBySSO(ctx, "sub-new-owner-"+uniqueSuffix(), "new-owner-"+uniqueSuffix()+"@example.com", "New Owner")
|
||||
if err != nil {
|
||||
t.Fatalf("UpsertUserBySSO (new owner): %v", err)
|
||||
}
|
||||
if err := s.SetMembership(ctx, tenantID, oldOwner.ID, RoleOwner); err != nil {
|
||||
t.Fatalf("SetMembership: %v", err)
|
||||
}
|
||||
if err := s.SetOwner(ctx, tenantID, oldOwner.ID); err != nil {
|
||||
t.Fatalf("SetOwner: %v", err)
|
||||
}
|
||||
|
||||
if err := s.TransferOwner(ctx, tenantID, newOwner.ID, RoleAdmin); err != nil {
|
||||
t.Fatalf("TransferOwner: %v", err)
|
||||
}
|
||||
|
||||
tenant, err := s.GetTenant(ctx, tenantID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetTenant: %v", err)
|
||||
}
|
||||
if tenant.OwnerUserID != newOwner.ID {
|
||||
t.Fatalf("tenant OwnerUserID = %q, want %q", tenant.OwnerUserID, newOwner.ID)
|
||||
}
|
||||
|
||||
newOwnerMembership, err := s.GetMembership(ctx, tenantID, newOwner.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetMembership (new owner): %v", err)
|
||||
}
|
||||
if newOwnerMembership.Role != RoleOwner {
|
||||
t.Fatalf("new owner's role = %q, want owner", newOwnerMembership.Role)
|
||||
}
|
||||
|
||||
// The point of TransferOwner over calling SetOwner alone: the
|
||||
// previous owner keeps a real membership (downgraded, not deleted or
|
||||
// left stale at "owner") -- otherwise tenant_memberships would claim
|
||||
// two owners while tenants.owner_user_id can only name one.
|
||||
oldOwnerMembership, err := s.GetMembership(ctx, tenantID, oldOwner.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetMembership (old owner): %v", err)
|
||||
}
|
||||
if oldOwnerMembership.Role != RoleAdmin {
|
||||
t.Fatalf("old owner's role after transfer = %q, want admin (downgraded, not left as owner)", oldOwnerMembership.Role)
|
||||
}
|
||||
|
||||
// The previous owner is no longer Owner, so RevokeMembership must now
|
||||
// accept revoking them -- proves the downgrade is real, not cosmetic.
|
||||
if err := s.RevokeMembership(ctx, tenantID, oldOwner.ID); err != nil {
|
||||
t.Fatalf("RevokeMembership on the downgraded former owner: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestTransferOwnerRefusesTenantWithNoCurrentOwner(t *testing.T) {
|
||||
s := testStore(t)
|
||||
ctx := context.Background()
|
||||
tenantID := "test-tenant-" + uniqueSuffix()
|
||||
if _, err := s.CreateTenant(ctx, tenantID, "Test Tenant"); err != nil {
|
||||
t.Fatalf("CreateTenant: %v", err)
|
||||
}
|
||||
newOwner, err := s.UpsertUserBySSO(ctx, "sub-"+uniqueSuffix(), "user-"+uniqueSuffix()+"@example.com", "Someone")
|
||||
if err != nil {
|
||||
t.Fatalf("UpsertUserBySSO: %v", err)
|
||||
}
|
||||
|
||||
if err := s.TransferOwner(ctx, tenantID, newOwner.ID, RoleAdmin); err == nil {
|
||||
t.Fatal("expected TransferOwner to refuse a tenant with no current owner")
|
||||
}
|
||||
}
|
||||
|
||||
func TestTransferOwnerRefusesTransferToCurrentOwner(t *testing.T) {
|
||||
s := testStore(t)
|
||||
ctx := context.Background()
|
||||
tenantID := "test-tenant-" + uniqueSuffix()
|
||||
if _, err := s.CreateTenant(ctx, tenantID, "Test Tenant"); err != nil {
|
||||
t.Fatalf("CreateTenant: %v", err)
|
||||
}
|
||||
owner, err := s.UpsertUserBySSO(ctx, "sub-"+uniqueSuffix(), "owner-"+uniqueSuffix()+"@example.com", "Owner")
|
||||
if err != nil {
|
||||
t.Fatalf("UpsertUserBySSO: %v", err)
|
||||
}
|
||||
if err := s.SetMembership(ctx, tenantID, owner.ID, RoleOwner); err != nil {
|
||||
t.Fatalf("SetMembership: %v", err)
|
||||
}
|
||||
if err := s.SetOwner(ctx, tenantID, owner.ID); err != nil {
|
||||
t.Fatalf("SetOwner: %v", err)
|
||||
}
|
||||
|
||||
if err := s.TransferOwner(ctx, tenantID, owner.ID, RoleAdmin); err == nil {
|
||||
t.Fatal("expected TransferOwner to refuse transferring ownership to its current holder")
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetMembershipNotFound(t *testing.T) {
|
||||
s := testStore(t)
|
||||
ctx := context.Background()
|
||||
|
||||
Reference in New Issue
Block a user