rbac: don't include OWNERSHIP permissions to USER role by default. (#53121)

Test plan:
Unit test added.
Local sg run and check.

cmd/frontend/internal/bg/BUILD.bazel

@@ -1,3 +1,4 @@
+load("//dev:go_defs.bzl", "go_test")
 load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
 go_library(
@@ -15,10 +16,12 @@ go_library(
     deps = [
         "//cmd/frontend/globals",
         "//internal/auth/userpasswd",
+        "//internal/collections",
         "//internal/conf",
         "//internal/conf/deploy",
         "//internal/database",
         "//internal/rbac",
+        "//internal/rbac/types",
         "//internal/rcache",
         "//internal/redispool",
         "//internal/types",
@@ -29,3 +32,22 @@ go_library(
         "@com_github_sourcegraph_log//:log",
     ],
 )
+
+go_test(
+    name = "bg_test",
+    srcs = ["update_permissions_test.go"],
+    embed = [":bg"],
+    tags = [
+        # Test requires localhost database
+        "requires-network",
+    ],
+    deps = [
+        "//internal/database",
+        "//internal/database/dbtest",
+        "//internal/rbac/types",
+        "//internal/types",
+        "@com_github_sourcegraph_log//logtest",
+        "@com_github_stretchr_testify//assert",
+        "@com_github_stretchr_testify//require",
+    ],
+)

cmd/frontend/internal/bg/update_permissions.go

@@ -4,9 +4,11 @@ import (
 	"context"
 
 	"github.com/sourcegraph/log"
+	"github.com/sourcegraph/sourcegraph/internal/collections"
 
 	"github.com/sourcegraph/sourcegraph/internal/database"
 	"github.com/sourcegraph/sourcegraph/internal/rbac"
+	rtypes "github.com/sourcegraph/sourcegraph/internal/rbac/types"
 	"github.com/sourcegraph/sourcegraph/internal/types"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
@@ -29,7 +31,8 @@ func UpdatePermissions(ctx context.Context, logger log.Logger, db database.DB) {
 			return errors.Wrap(err, "fetching permissions from database")
 		}
 
-		toBeAdded, toBeDeleted := rbac.ComparePermissions(dbPerms, rbac.RBACSchema)
+		rbacSchema := rbac.RBACSchema
+		toBeAdded, toBeDeleted := rbac.ComparePermissions(dbPerms, rbacSchema)
 		scopedLog.Info("RBAC Permissions update", log.Int("added", len(toBeAdded)), log.Int("deleted", len(toBeDeleted)))
 
 		if len(toBeDeleted) > 0 {
@@ -49,13 +52,24 @@ func UpdatePermissions(ctx context.Context, logger log.Logger, db database.DB) {
 				return errors.Wrap(err, "creating new permissions")
 			}
 
+			roles := []types.SystemRole{types.SiteAdministratorSystemRole, types.UserSystemRole}
+			excludedRoles := collections.NewSet[rtypes.PermissionNamespace](rbacSchema.ExcludeFromUserRole...)
 			for _, permission := range permissions {
-				// Assign the permission to both SITE_ADMINISTRATOR and USER roles. We do this so that we don't break the
-				// current experience and always assume that everyone has access until a site administrator revokes that
-				// access.
-				// Context: https://sourcegraph.slack.com/archives/C044BUJET7C/p1675292124253779?thread_ts=1675280399.192819&cid=C044BUJET7C
+				// Assign the permission to both SITE_ADMINISTRATOR and USER roles. We do this so
+				// that we don't break the current experience and always assume that everyone has
+				// access until a site administrator revokes that access. Context:
+				// https://sourcegraph.slack.com/archives/C044BUJET7C/p1675292124253779?thread_ts=1675280399.192819&cid=C044BUJET7C
+
+				rolesToAssign := roles
+				if excludedRoles.Has(permission.Namespace) {
+					// The only exception to the above rule (at the moment) is Ownership, because it
+					// is clearly a permission which should be explicitly granted and only
+					// SITE_ADMINISTRATOR has it by default. All exceptions can be added to the
+					// `excludeFromUserRole` attribute of RBAC schema.
+					rolesToAssign = []types.SystemRole{types.SiteAdministratorSystemRole}
+				}
 				if err := rolePermissionStore.BulkAssignPermissionsToSystemRoles(ctx, database.BulkAssignPermissionsToSystemRolesOpts{
-					Roles:        []types.SystemRole{types.SiteAdministratorSystemRole, types.UserSystemRole},
+					Roles:        rolesToAssign,
 					PermissionID: permission.ID,
 				}); err != nil {
 					return errors.Wrap(err, "assigning permission to system roles")

cmd/frontend/internal/bg/update_permissions_test.go

@@ -0,0 +1,55 @@
+package bg
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/sourcegraph/log/logtest"
+	"github.com/sourcegraph/sourcegraph/internal/database"
+	"github.com/sourcegraph/sourcegraph/internal/database/dbtest"
+	rtypes "github.com/sourcegraph/sourcegraph/internal/rbac/types"
+	"github.com/sourcegraph/sourcegraph/internal/types"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestUpdatePermissions(t *testing.T) {
+	logger := logtest.Scoped(t)
+	db := database.NewDB(logger, dbtest.NewDB(logger, t))
+	ctx := context.Background()
+
+	allPerms := []*types.Permission{
+		{Namespace: rtypes.BatchChangesNamespace, Action: rtypes.BatchChangesReadAction},
+		{Namespace: rtypes.BatchChangesNamespace, Action: rtypes.BatchChangesWriteAction},
+		{Namespace: rtypes.RepoMetadataNamespace, Action: rtypes.RepoMetadataWriteAction},
+		{Namespace: rtypes.OwnershipNamespace, Action: rtypes.OwnershipAssignAction},
+	}
+
+	// Updating permissions.
+	UpdatePermissions(ctx, logger, db)
+	// SITE_ADMINISTRATOR should have all the permissions.
+	roleStore := db.Roles()
+	adminRole, err := roleStore.Get(ctx, database.GetRoleOpts{Name: string(types.SiteAdministratorSystemRole)})
+	require.NoError(t, err)
+	permissionStore := db.Permissions()
+	adminPermissions, err := permissionStore.List(ctx, database.PermissionListOpts{RoleID: adminRole.ID, PaginationArgs: &database.PaginationArgs{}})
+	require.NoError(t, err)
+	adminPermissions = clearTimeAndID(adminPermissions)
+	assert.ElementsMatch(t, allPerms, adminPermissions)
+	// USER should have all the permissions except OWNERSHIP.
+	userRole, err := roleStore.Get(ctx, database.GetRoleOpts{Name: string(types.UserSystemRole)})
+	require.NoError(t, err)
+	userPermissions, err := permissionStore.List(ctx, database.PermissionListOpts{RoleID: userRole.ID, PaginationArgs: &database.PaginationArgs{}})
+	require.NoError(t, err)
+	userPermissions = clearTimeAndID(userPermissions)
+	assert.ElementsMatch(t, allPerms[:3], userPermissions, "unexpected number of permissions")
+}
+
+func clearTimeAndID(perms []*types.Permission) []*types.Permission {
+	for _, perm := range perms {
+		perm.CreatedAt = time.Time{}
+		perm.ID = 0
+	}
+	return perms
+}

internal/database/role_permissions.go

@@ -65,7 +65,7 @@ type RolePermissionStore interface {
 	AssignToSystemRole(ctx context.Context, opts AssignToSystemRoleOpts) error
 	// BulkAssignPermissionsToRole is used to assign multiple permissions to a role.
 	BulkAssignPermissionsToRole(ctx context.Context, opts BulkAssignPermissionsToRoleOpts) error
-	// BulkAssignToSystemRole is used to assign a permission to multiple system roles.
+	// BulkAssignPermissionsToSystemRoles is used to assign a permission to multiple system roles.
 	BulkAssignPermissionsToSystemRoles(ctx context.Context, opts BulkAssignPermissionsToSystemRolesOpts) error
 	// BulkRevokePermissionsForRole revokes bulk permissions assigned to a role.
 	BulkRevokePermissionsForRole(ctx context.Context, opts BulkRevokePermissionsForRoleOpts) error

internal/rbac/schema.yaml

@@ -9,3 +9,5 @@ namespaces:
   - name: REPO_METADATA
     actions:
       - WRITE
+excludeFromUserRole:
+  - OWNERSHIP

internal/rbac/types.go

@@ -5,7 +5,8 @@ import rtypes "github.com/sourcegraph/sourcegraph/internal/rbac/types"
 // Schema refers to the RBAC structure which acts as a source of truth for permissions within
 // the RBAC system.
 type Schema struct {
-	Namespaces []Namespace `yaml:"namespaces"`
+	Namespaces          []Namespace                  `yaml:"namespaces"`
+	ExcludeFromUserRole []rtypes.PermissionNamespace `yaml:"excludeFromUserRole"`
 }
 
 // Namespace represents a feature to be guarded by RBAC. (example: Batch Changes, Code Insights e.t.c)