Add enterprise-auth -revoke-membership-*/-list-memberships-tenant flags

Rounds out the tenant_memberships operator flags added earlier this phase (-create-tenant/-grant-membership-*): grant had no way to undo itself, and there was no way to see who was actually in a tenant without querying Postgres directly. rbacstore.RevokeMembership deletes a tenant_memberships row, but refuses to revoke a tenant's current Owner -- Owner is also a dedicated tenants.owner_user_id column (SetOwner), so deleting that membership without transferring ownership first would leave owner_user_id pointing at a user with no membership in the tenant at all. Ownership transfer is a deliberate, separate action per the RBAC matrix ("Transfer tenant Owner -- Owner only"), not a side effect of revoking access. rbacstore.ListMembershipsForTenant is ListMembershipsForUser's inverse -- joined with users so the result is actually useful (email, display name), not just a bare user ID list. enterprise-auth gains -revoke-membership-tenant/ -revoke-membership-user-email (both required together, same shape as -grant-membership-*) and -list-memberships-tenant (prints tab-separated id/email/display-name/role to stdout and exits -- an operator convenience, not a machine-readable API; this binary deliberately has no admin HTTP surface, per its own doc comment on why). Changing a role is unchanged: re-run -grant-membership-* with a different -grant-membership-role, SetMembership's upsert already handles it. Covered by five new skip-gated integration tests in rbacstore_test.go (RBACSTORE_TEST_POSTGRES_ADDR), including the Owner-revocation refusal and cross-tenant leak check for ListMembershipsForTenant -- not run against a live database in this environment, same disclosed gap as the rest of this phase's Postgres-backed work. Docs updated: phase-4-runbook.md's known-gaps bullet, enterprise/README.md's bootstrap walkthrough and package layout table.
2026-08-14 09:11:01 -07:00
parent 823f5d48d1
commit cfcbc77507
5 changed files with 281 additions and 14 deletions
@@ -266,6 +266,75 @@ func (s *Store) GetMembership(ctx context.Context, tenantID, userID string) (*Me
 	return &m, nil
 }

+// RevokeMembership deletes a user's membership in a tenant -- the
+// counterpart to SetMembership's upsert. Refuses to revoke a tenant's
+// current Owner: unlike every other role, Owner is also a dedicated
+// tenants.owner_user_id column (see SetOwner's doc comment), so
+// revoking that membership without first transferring ownership would
+// leave owner_user_id pointing at a user with no membership in the
+// tenant at all -- an inconsistent state, not something this method
+// silently allows. Ownership transfer is a deliberate, separate action
+// (the RBAC matrix's "Transfer tenant Owner -- Owner only"), not a
+// side effect of revoking a membership.
+func (s *Store) RevokeMembership(ctx context.Context, tenantID, userID string) error {
+	tenant, err := s.GetTenant(ctx, tenantID)
+	if err != nil {
+		return fmt.Errorf("rbacstore: getting tenant to check ownership: %w", err)
+	}
+	if tenant.OwnerUserID == userID {
+		return fmt.Errorf("rbacstore: refusing to revoke tenant %q's current Owner (%s) -- transfer ownership first", tenantID, userID)
+	}
+
+	tag, err := s.pool.Exec(ctx, `DELETE FROM tenant_memberships WHERE tenant_id = $1 AND user_id = $2`, tenantID, userID)
+	if err != nil {
+		return fmt.Errorf("rbacstore: revoking membership: %w", err)
+	}
+	if tag.RowsAffected() == 0 {
+		return ErrNotFound
+	}
+	return nil
+}
+
+// TenantMember is one row of ListMembershipsForTenant's result -- joined
+// with users so a caller (e.g. enterprise-auth's -list-memberships-tenant
+// operator flag) can show something more useful than a bare user ID.
+type TenantMember struct {
+	UserID      string
+	Email       string
+	DisplayName string
+	Role        Role
+}
+
+// ListMembershipsForTenant is ListMembershipsForUser's inverse -- "who
+// is in this tenant, and at what role," the shape an admin reviewing or
+// revoking access needs. Joined with users (INNER, not LEFT: a
+// tenant_memberships row's user_id is NOT NULL and FK-constrained, so
+// every membership has a real user).
+func (s *Store) ListMembershipsForTenant(ctx context.Context, tenantID string) ([]TenantMember, error) {
+	rows, err := s.pool.Query(ctx, `
+		SELECT u.id, u.email, u.display_name, m.role
+		FROM tenant_memberships m
+		JOIN users u ON u.id = m.user_id
+		WHERE m.tenant_id = $1
+		ORDER BY u.email`, tenantID)
+	if err != nil {
+		return nil, fmt.Errorf("rbacstore: listing tenant members: %w", err)
+	}
+	defer rows.Close()
+
+	var out []TenantMember
+	for rows.Next() {
+		var m TenantMember
+		var role string
+		if err := rows.Scan(&m.UserID, &m.Email, &m.DisplayName, &role); err != nil {
+			return nil, fmt.Errorf("rbacstore: scanning tenant member: %w", err)
+		}
+		m.Role = Role(role)
+		out = append(out, m)
+	}
+	return out, rows.Err()
+}
+
 // ListMembershipsForUser supports "which tenants can this user act in,
 // and at what role" -- the shape a login/session-issuance handler needs
 // when a user belongs to more than one tenant and must pick (or be
@@ -652,3 +652,122 @@ func TestTenantIsActiveNonexistentTenant(t *testing.T) {
 		t.Fatal("a nonexistent tenant must not be reported active")
 	}
 }
+
+func TestRevokeMembership(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)
+	}
+	user, err := s.UpsertUserBySSO(ctx, "sub-"+uniqueSuffix(), "user-"+uniqueSuffix()+"@example.com", "User")
+	if err != nil {
+		t.Fatalf("UpsertUserBySSO: %v", err)
+	}
+	if err := s.SetMembership(ctx, tenantID, user.ID, RoleEditor); err != nil {
+		t.Fatalf("SetMembership: %v", err)
+	}
+
+	if err := s.RevokeMembership(ctx, tenantID, user.ID); err != nil {
+		t.Fatalf("RevokeMembership: %v", err)
+	}
+	if _, err := s.GetMembership(ctx, tenantID, user.ID); err != ErrNotFound {
+		t.Fatalf("GetMembership after revoke = %v, want ErrNotFound", err)
+	}
+}
+
+func TestRevokeMembershipNotFound(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)
+	}
+	if err := s.RevokeMembership(ctx, tenantID, uuid.NewString()); err != ErrNotFound {
+		t.Fatalf("RevokeMembership error = %v, want ErrNotFound", err)
+	}
+}
+
+// TestRevokeMembershipRefusesCurrentOwner is the regression test for
+// RevokeMembership's doc comment: deleting the Owner's membership
+// without transferring ownership first would leave tenants.owner_user_id
+// pointing at a user with no membership in the tenant at all.
+func TestRevokeMembershipRefusesCurrentOwner(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-owner-"+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.RevokeMembership(ctx, tenantID, owner.ID); err == nil {
+		t.Fatal("expected RevokeMembership to refuse revoking the tenant's current Owner")
+	}
+	if _, err := s.GetMembership(ctx, tenantID, owner.ID); err != nil {
+		t.Fatalf("owner's membership must still exist after the refused revoke, GetMembership: %v", err)
+	}
+}
+
+func TestListMembershipsForTenant(t *testing.T) {
+	s := testStore(t)
+	ctx := context.Background()
+	tenantID := "test-tenant-" + uniqueSuffix()
+	otherTenantID := "test-tenant-" + uniqueSuffix()
+	if _, err := s.CreateTenant(ctx, tenantID, "Test Tenant"); err != nil {
+		t.Fatalf("CreateTenant: %v", err)
+	}
+	if _, err := s.CreateTenant(ctx, otherTenantID, "Other Tenant"); err != nil {
+		t.Fatalf("CreateTenant other: %v", err)
+	}
+
+	viewer, err := s.UpsertUserBySSO(ctx, "sub-viewer-"+uniqueSuffix(), "viewer-"+uniqueSuffix()+"@example.com", "Viewer")
+	if err != nil {
+		t.Fatalf("UpsertUserBySSO viewer: %v", err)
+	}
+	if err := s.SetMembership(ctx, tenantID, viewer.ID, RoleViewer); err != nil {
+		t.Fatalf("SetMembership viewer: %v", err)
+	}
+	editor, err := s.UpsertUserBySSO(ctx, "sub-editor-"+uniqueSuffix(), "editor-"+uniqueSuffix()+"@example.com", "Editor")
+	if err != nil {
+		t.Fatalf("UpsertUserBySSO editor: %v", err)
+	}
+	if err := s.SetMembership(ctx, tenantID, editor.ID, RoleEditor); err != nil {
+		t.Fatalf("SetMembership editor: %v", err)
+	}
+	// A membership in a different tenant must not leak into this list.
+	elsewhere, err := s.UpsertUserBySSO(ctx, "sub-elsewhere-"+uniqueSuffix(), "elsewhere-"+uniqueSuffix()+"@example.com", "Elsewhere")
+	if err != nil {
+		t.Fatalf("UpsertUserBySSO elsewhere: %v", err)
+	}
+	if err := s.SetMembership(ctx, otherTenantID, elsewhere.ID, RoleAdmin); err != nil {
+		t.Fatalf("SetMembership elsewhere: %v", err)
+	}
+
+	members, err := s.ListMembershipsForTenant(ctx, tenantID)
+	if err != nil {
+		t.Fatalf("ListMembershipsForTenant: %v", err)
+	}
+	if len(members) != 2 {
+		t.Fatalf("len(members) = %d, want 2", len(members))
+	}
+	byEmail := map[string]TenantMember{}
+	for _, m := range members {
+		byEmail[m.Email] = m
+	}
+	if got := byEmail[viewer.Email]; got.Role != RoleViewer || got.UserID != viewer.ID {
+		t.Fatalf("unexpected viewer entry: %+v", got)
+	}
+	if got := byEmail[editor.Email]; got.Role != RoleEditor || got.UserID != editor.ID {
+		t.Fatalf("unexpected editor entry: %+v", got)
+	}
+}