feat: migrate agents-access to org-scoped system role for proper chat RBAC (#24438)

The agents-access role previously granted chat permissions at user scope, but chats are org-scoped objects. Rego skips user-level perms when org_owner is set, making the grants invisible. Handler-level band-aids used synthetic non-org-scoped objects as a workaround. - Migrates agents-access from users.rbac_roles (site-level) to organization_members.roles (org-scoped) via DB migration - Redefines agents-access as a predefined org-scoped builtin role alongside organization-admin, organization-auditor, etc., with Member permissions granting chat create/read/update - Excludes ResourceChat from OrgMemberPermissions so org membership alone no longer grants chat access - Fixes handler Authorize checks to use org-scoped objects with semantically correct actions (ActionUpdate for message/tool operations) - Grants org admins the ability to assign agents-access Closes #24250 Fixes CODAGT-174 Note: this does not update the "Usage" endpoints. Tracked by CODAGT-161. > 🤖
2026-09-22 05:05:20 +08:00 · 2026-04-23 17:59:42 +01:00
parent 7efccfa996
commit b5a625549e
19 changed files with 441 additions and 174 deletions
@@ -2566,14 +2566,20 @@ func (q *querier) GetChatByIDForUpdate(ctx context.Context, id uuid.UUID) (datab
 }

 func (q *querier) GetChatCostPerChat(ctx context.Context, arg database.GetChatCostPerChatParams) ([]database.GetChatCostPerChatRow, error) {
-	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceChat.WithOwner(arg.OwnerID.String())); err != nil {
+	// The owner's chats, may cross orgs. AnyOrganization() authorizes
+	// the caller if they hold read permission on chats owned by
+	// arg.OwnerID in any org they belong to.
+	// TODO(CODAGT-161): the underlying SQL queries filter only by owner_id, not
+	// organization_id.
+	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceChat.WithOwner(arg.OwnerID.String()).AnyOrganization()); err != nil {
 		return nil, err
 	}
 	return q.db.GetChatCostPerChat(ctx, arg)
 }

 func (q *querier) GetChatCostPerModel(ctx context.Context, arg database.GetChatCostPerModelParams) ([]database.GetChatCostPerModelRow, error) {
-	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceChat.WithOwner(arg.OwnerID.String())); err != nil {
+	// See GetChatCostPerChat for the authorization rationale.
+	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceChat.WithOwner(arg.OwnerID.String()).AnyOrganization()); err != nil {
 		return nil, err
 	}
 	return q.db.GetChatCostPerModel(ctx, arg)
@@ -2587,7 +2593,8 @@ func (q *querier) GetChatCostPerUser(ctx context.Context, arg database.GetChatCo
 }

 func (q *querier) GetChatCostSummary(ctx context.Context, arg database.GetChatCostSummaryParams) (database.GetChatCostSummaryRow, error) {
-	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceChat.WithOwner(arg.OwnerID.String())); err != nil {
+	// See GetChatCostPerChat for the authorization rationale.
+	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceChat.WithOwner(arg.OwnerID.String()).AnyOrganization()); err != nil {
 		return database.GetChatCostSummaryRow{}, err
 	}
 	return q.db.GetChatCostSummary(ctx, arg)
@@ -3025,17 +3032,15 @@ func (q *querier) GetDERPMeshKey(ctx context.Context) (string, error) {
 }

 func (q *querier) GetDefaultChatModelConfig(ctx context.Context) (database.ChatModelConfig, error) {
-	// Any user who can read chat resources can read the default
-	// model config, since model resolution is required to create
-	// a chat. This avoids gating on ResourceDeploymentConfig
-	// which regular members lack.
-	act, ok := ActorFromContext(ctx)
-	if !ok {
+	// Reading the default model config is needed for chat creation.
+	// TODO(CODAGT-161): scope this check when org context is available.
+	// This function has no org context to scope the check, and
+	// ResourceDeploymentConfig is too restrictive (admin-only).
+	// The handler layer gates chat creation via ActionCreate on
+	// the org-scoped ResourceChat.
+	if _, ok := ActorFromContext(ctx); !ok {
 		return database.ChatModelConfig{}, ErrNoActor
 	}
-	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceChat.WithOwner(act.ID)); err != nil {
-		return database.ChatModelConfig{}, err
-	}
 	return q.db.GetDefaultChatModelConfig(ctx)
 }

@@ -618,7 +618,7 @@ func (s *MethodTestSuite) TestChats() {
 			TotalOutputTokens: 89,
 		}}
 		dbm.EXPECT().GetChatCostPerChat(gomock.Any(), arg).Return(rows, nil).AnyTimes()
-		check.Args(arg).Asserts(rbac.ResourceChat.WithOwner(arg.OwnerID.String()), policy.ActionRead).Returns(rows)
+		check.Args(arg).Asserts(rbac.ResourceChat.WithOwner(arg.OwnerID.String()).AnyOrganization(), policy.ActionRead).Returns(rows)
 	}))
 	s.Run("GetChatCostPerModel", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) {
 		arg := database.GetChatCostPerModelParams{
@@ -637,7 +637,7 @@ func (s *MethodTestSuite) TestChats() {
 			TotalOutputTokens: 233,
 		}}
 		dbm.EXPECT().GetChatCostPerModel(gomock.Any(), arg).Return(rows, nil).AnyTimes()
-		check.Args(arg).Asserts(rbac.ResourceChat.WithOwner(arg.OwnerID.String()), policy.ActionRead).Returns(rows)
+		check.Args(arg).Asserts(rbac.ResourceChat.WithOwner(arg.OwnerID.String()).AnyOrganization(), policy.ActionRead).Returns(rows)
 	}))
 	s.Run("GetChatCostPerUser", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) {
 		arg := database.GetChatCostPerUserParams{
@@ -676,7 +676,7 @@ func (s *MethodTestSuite) TestChats() {
 			TotalOutputTokens:    800,
 		}
 		dbm.EXPECT().GetChatCostSummary(gomock.Any(), arg).Return(row, nil).AnyTimes()
-		check.Args(arg).Asserts(rbac.ResourceChat.WithOwner(arg.OwnerID.String()), policy.ActionRead).Returns(row)
+		check.Args(arg).Asserts(rbac.ResourceChat.WithOwner(arg.OwnerID.String()).AnyOrganization(), policy.ActionRead).Returns(row)
 	}))
 	s.Run("CountEnabledModelsWithoutPricing", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) {
 		dbm.EXPECT().CountEnabledModelsWithoutPricing(gomock.Any()).Return(int64(3), nil).AnyTimes()
@@ -795,7 +795,7 @@ func (s *MethodTestSuite) TestChats() {
 	s.Run("GetDefaultChatModelConfig", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
 		config := testutil.Fake(s.T(), faker, database.ChatModelConfig{})
 		dbm.EXPECT().GetDefaultChatModelConfig(gomock.Any()).Return(config, nil).AnyTimes()
-		check.Asserts(rbac.ResourceChat.WithOwner(testActorID.String()), policy.ActionRead).Returns(config)
+		check.Asserts().Returns(config)
 	}))
 	s.Run("GetChatModelConfigs", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
 		configA := testutil.Fake(s.T(), faker, database.ChatModelConfig{})
@@ -242,6 +242,7 @@ func (s *MethodTestSuite) SubtestWithDB(db database.Store, testCaseF func(db dat
 			slice.Contains([]string{
 				"GetAuthorizedWorkspaces",
 				"GetAuthorizedTemplates",
+				"GetDefaultChatModelConfig",
 			}, methodName) {
 			// Some methods do not make RBAC assertions because they use
 			// SQL. We still want to test that they return an error if the
@@ -0,0 +1,18 @@
+-- WARNING: this rollback is lossy. If an admin later revoked
+-- agents-access from a specific org, rolling back will re-grant the
+-- site-wide role (which covers ALL orgs) to any user who still holds
+-- agents-access in at least one org.
+
+-- Step 1: Move agents-access back to site-level for any user who has it in any org.
+UPDATE users
+SET rbac_roles = array_append(rbac_roles, 'agents-access')
+WHERE id IN (
+    SELECT DISTINCT user_id FROM organization_members
+    WHERE 'agents-access' = ANY(roles)
+)
+AND NOT ('agents-access' = ANY(rbac_roles));
+
+-- Step 2: Remove from org memberships.
+UPDATE organization_members
+SET roles = array_remove(roles, 'agents-access')
+WHERE 'agents-access' = ANY(roles);
@@ -0,0 +1,16 @@
+-- Transition 'agents-access' from a site-wide role to a per-org role.
+
+-- For every user who has 'agents-access' in users.rbac_roles,
+-- grant the org-scoped role in each org they belong to.
+UPDATE organization_members
+SET roles = array_append(roles, 'agents-access')
+WHERE user_id IN (
+    SELECT id FROM users
+    WHERE 'agents-access' = ANY(rbac_roles)
+)
+AND NOT ('agents-access' = ANY(roles));
+
+-- Remove 'agents-access' from site-level roles.
+UPDATE users
+SET rbac_roles = array_remove(rbac_roles, 'agents-access')
+WHERE 'agents-access' = ANY(rbac_roles);
@@ -1023,3 +1023,165 @@ func TestMigration000457ChatAccessRole(t *testing.T) {
 	require.Contains(t, roles, "template-admin",
 		"existing roles should be preserved")
 }
+
+func TestMigration000475AgentsAccessOrgRole(t *testing.T) {
+	t.Parallel()
+
+	const migrationVersion = 475
+
+	sqlDB := testSQLDB(t)
+
+	// Migrate up to the migration before 000475.
+	next, err := migrations.Stepper(sqlDB)
+	require.NoError(t, err)
+	for {
+		version, more, err := next()
+		require.NoError(t, err)
+		if !more {
+			t.Fatalf("migration %d not found", migrationVersion)
+		}
+		if version == migrationVersion-1 {
+			break
+		}
+	}
+
+	ctx := testutil.Context(t, testutil.WaitSuperLong)
+
+	// Seed: a user with site-level agents-access who is a member of
+	// two orgs, plus a second user who is a member of one org and
+	// does not have the role.
+	userWithRole := uuid.New()
+	userWithoutRole := uuid.New()
+	org1ID := uuid.New()
+	org2ID := uuid.New()
+
+	now := time.Now().UTC().Truncate(time.Microsecond)
+
+	tx, err := sqlDB.BeginTx(ctx, nil)
+	require.NoError(t, err)
+	defer tx.Rollback()
+
+	fixtures := []struct {
+		query string
+		args  []any
+	}{
+		{
+			`INSERT INTO users (id, username, email, hashed_password, created_at, updated_at, status, rbac_roles, login_type)
+			VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)`,
+			[]any{userWithRole, "user-with-role", "withrole@test.com", []byte{}, now, now, "active", pq.StringArray{"agents-access"}, "password"},
+		},
+		{
+			`INSERT INTO users (id, username, email, hashed_password, created_at, updated_at, status, rbac_roles, login_type)
+			VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)`,
+			[]any{userWithoutRole, "user-without-role", "withoutrole@test.com", []byte{}, now, now, "active", pq.StringArray{}, "password"},
+		},
+		{
+			`INSERT INTO organizations (id, name, display_name, description, icon, created_at, updated_at, is_default)
+			VALUES ($1, $2, $3, $4, $5, $6, $7, $8)`,
+			[]any{org1ID, "org-1", "Org 1", "", "", now, now, false},
+		},
+		{
+			`INSERT INTO organizations (id, name, display_name, description, icon, created_at, updated_at, is_default)
+			VALUES ($1, $2, $3, $4, $5, $6, $7, $8)`,
+			[]any{org2ID, "org-2", "Org 2", "", "", now, now, false},
+		},
+		{
+			`INSERT INTO organization_members (organization_id, user_id, created_at, updated_at, roles)
+			VALUES ($1, $2, $3, $4, $5)`,
+			[]any{org1ID, userWithRole, now, now, pq.StringArray{}},
+		},
+		{
+			`INSERT INTO organization_members (organization_id, user_id, created_at, updated_at, roles)
+			VALUES ($1, $2, $3, $4, $5)`,
+			[]any{org2ID, userWithRole, now, now, pq.StringArray{}},
+		},
+		{
+			`INSERT INTO organization_members (organization_id, user_id, created_at, updated_at, roles)
+			VALUES ($1, $2, $3, $4, $5)`,
+			[]any{org1ID, userWithoutRole, now, now, pq.StringArray{}},
+		},
+	}
+
+	for i, f := range fixtures {
+		_, err := tx.ExecContext(ctx, f.query, f.args...)
+		require.NoError(t, err, "fixture %d", i)
+	}
+	require.NoError(t, tx.Commit())
+
+	// Run migration 000475.
+	version, _, err := next()
+	require.NoError(t, err)
+	require.EqualValues(t, migrationVersion, version)
+
+	// Verify: userWithRole no longer has agents-access at site level.
+	var siteRoles pq.StringArray
+	err = sqlDB.QueryRowContext(ctx,
+		"SELECT rbac_roles FROM users WHERE id = $1", userWithRole,
+	).Scan(&siteRoles)
+	require.NoError(t, err)
+	require.NotContains(t, siteRoles, "agents-access",
+		"agents-access should be removed from users.rbac_roles")
+
+	// Verify: userWithRole has agents-access in both orgs.
+	for _, orgID := range []uuid.UUID{org1ID, org2ID} {
+		var orgRoles pq.StringArray
+		err = sqlDB.QueryRowContext(ctx,
+			"SELECT roles FROM organization_members WHERE user_id = $1 AND organization_id = $2",
+			userWithRole, orgID,
+		).Scan(&orgRoles)
+		require.NoError(t, err)
+		require.Contains(t, orgRoles, "agents-access",
+			"agents-access should be granted in org %s", orgID)
+	}
+
+	// Verify: userWithoutRole did not gain agents-access.
+	var orgRoles pq.StringArray
+	err = sqlDB.QueryRowContext(ctx,
+		"SELECT roles FROM organization_members WHERE user_id = $1 AND organization_id = $2",
+		userWithoutRole, org1ID,
+	).Scan(&orgRoles)
+	require.NoError(t, err)
+	require.NotContains(t, orgRoles, "agents-access",
+		"agents-access should not be granted to a user who didn't have it")
+
+	// Verify: no DB row exists for agents-access as a custom_role.
+	// The role is now a builtin, resolved in Go via RoleByName.
+	var customRoleCount int
+	err = sqlDB.QueryRowContext(ctx,
+		"SELECT COUNT(*) FROM custom_roles WHERE name = 'agents-access'",
+	).Scan(&customRoleCount)
+	require.NoError(t, err)
+	require.Equal(t, 0, customRoleCount,
+		"no custom_roles row should exist for agents-access")
+
+	// Verify: creating a new organization does NOT insert an
+	// agents-access custom_role via the trigger. It should only
+	// insert organization-member and organization-service-account.
+	newOrgID := uuid.New()
+	_, err = sqlDB.ExecContext(ctx,
+		`INSERT INTO organizations (id, name, display_name, description, icon, created_at, updated_at, is_default)
+		VALUES ($1, $2, $3, $4, $5, $6, $7, $8)`,
+		newOrgID, "new-org", "New Org", "", "", now, now, false,
+	)
+	require.NoError(t, err)
+
+	rows, err := sqlDB.QueryContext(ctx,
+		"SELECT name FROM custom_roles WHERE organization_id = $1 AND is_system = true ORDER BY name",
+		newOrgID,
+	)
+	require.NoError(t, err)
+	defer rows.Close()
+
+	var gotRoleNames []string
+	for rows.Next() {
+		var name string
+		require.NoError(t, rows.Scan(&name))
+		gotRoleNames = append(gotRoleNames, name)
+	}
+	require.NoError(t, rows.Err())
+	require.ElementsMatch(t,
+		[]string{"organization-member", "organization-service-account"},
+		gotRoleNames,
+		"trigger should only create org-member and org-service-account system roles",
+	)
+}
@@ -1251,17 +1251,13 @@ func TestGetAuthorizedChats(t *testing.T) {
 	owner := dbgen.User(t, db, database.User{
 		RBACRoles: []string{rbac.RoleOwner().String()},
 	})
-	member := dbgen.User(t, db, database.User{
-		RBACRoles: pq.StringArray{rbac.RoleAgentsAccess().String()},
-	})
-	secondMember := dbgen.User(t, db, database.User{
-		RBACRoles: pq.StringArray{rbac.RoleAgentsAccess().String()},
-	})
+	member := dbgen.User(t, db, database.User{})
+	secondMember := dbgen.User(t, db, database.User{})

 	org := dbgen.Organization(t, db, database.Organization{})
 	dbgen.OrganizationMember(t, db, database.OrganizationMember{UserID: owner.ID, OrganizationID: org.ID})
-	dbgen.OrganizationMember(t, db, database.OrganizationMember{UserID: member.ID, OrganizationID: org.ID})
-	dbgen.OrganizationMember(t, db, database.OrganizationMember{UserID: secondMember.ID, OrganizationID: org.ID})
+	dbgen.OrganizationMember(t, db, database.OrganizationMember{UserID: member.ID, OrganizationID: org.ID, Roles: []string{rbac.RoleAgentsAccess()}})
+	dbgen.OrganizationMember(t, db, database.OrganizationMember{UserID: secondMember.ID, OrganizationID: org.ID, Roles: []string{rbac.RoleAgentsAccess()}})

 	// Create FK dependencies: a chat provider and model config.
 	ctx := testutil.Context(t, testutil.WaitMedium)
@@ -1438,10 +1434,8 @@ func TestGetAuthorizedChats(t *testing.T) {

 		// Use a dedicated user for pagination to avoid interference
 		// with the other parallel subtests.
-		paginationUser := dbgen.User(t, db, database.User{
-			RBACRoles: pq.StringArray{rbac.RoleAgentsAccess().String()},
-		})
-		dbgen.OrganizationMember(t, db, database.OrganizationMember{UserID: paginationUser.ID, OrganizationID: org.ID})
+		paginationUser := dbgen.User(t, db, database.User{})
+		dbgen.OrganizationMember(t, db, database.OrganizationMember{UserID: paginationUser.ID, OrganizationID: org.ID, Roles: []string{rbac.RoleAgentsAccess()}})
 		for i := range 7 {
 			_, err := db.InsertChat(ctx, database.InsertChatParams{
 				OrganizationID:    org.ID,