fix(apiserver): require rbac:control scope all to create, extend or delete roles (bug-1958)

Cause: RoleController.CreateRole, AddPermissionsForRole and
DeleteRoleAndPermissions relied on RequirePermission("rbac:control"), which
accepts any scope, so a user with rbac:control scoped to self could add
cluster:control to a role it holds.

Fix: checkRoleChangePermission() in controllers/roles.go refuses with 403
unless one of the caller's grants (checked before deduplication) is
rbac:control with scope all, reusing canChangeUserRoles from bug-1952.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

src/frontend/apiserver/controllers/roles.go

@@ -507,6 +507,10 @@ func (rc *RoleController) AddPermissionsForRole(ctx *gin.Context) {
 		return
 	}
 
+	if !checkRoleChangePermission(ctx, "add permissions to", req.RoleName) {
+		return
+	}
+
 	err := preCheckRoleRequest(&req, false)
 	if err != nil {
 		logger.Error("Failed to create role request", zap.Error(err))
@@ -549,6 +553,11 @@ func (rc *RoleController) DeleteRoleAndPermissions(ctx *gin.Context) {
 		ReturnError(ctx, common.ERROR_CODE, err.Error(), nil)
 		return
 	}
+
+	if !checkRoleChangePermission(ctx, "delete", req_rolename) {
+		return
+	}
+
 	// step 1: role name whether exists
 	allRoleNames := store.GetAllRoleNames(false)
 	found := false
@@ -602,6 +611,10 @@ func (rc *RoleController) CreateRole(ctx *gin.Context) {
 		return
 	}
 
+	if !checkRoleChangePermission(ctx, "create", req.RoleName) {
+		return
+	}
+
 	// precheck the request parameter
 	err := preCheckRoleRequest(&req, true)
 	if err != nil {
@@ -632,6 +645,30 @@ func (rc *RoleController) CreateRole(ctx *gin.Context) {
 	return
 }
 
+const errNoRoleDefinitionPermission = "no permission: creating, changing or deleting roles requires rbac:control with scope all"
+
+// checkRoleChangePermission enforces that only an rbac:control grant with
+// scope all may create, extend or delete roles (bug-1958): with a narrower
+// scope such as self a user could add any permission, cluster:control
+// included, to a role it already holds. The rule is the one canChangeUserRoles
+// applies to user-role assignment (bug-1952). On refusal it writes the error
+// response and returns false; the caller must return.
+func checkRoleChangePermission(ctx *gin.Context, action string, roleName string) bool {
+	token := ctx.GetHeader("Authorization")
+	submitUser, err := GetUsernameFromToken(token)
+	if err != nil {
+		ReturnError(ctx, common.ERROR_CODE, err.Error(), nil)
+		return false
+	}
+	if !canChangeUserRoles(submitUser) {
+		logger.Error("no permission to "+action+" role", zap.String("submit_user", submitUser),
+			zap.String("role_name", roleName))
+		ReturnError(ctx, common.ERROR_AUTH_CODE, errNoRoleDefinitionPermission, nil)
+		return false
+	}
+	return true
+}
+
 func preCheckRole(role_name string) error {
 
 	return nil

src/frontend/apiserver/controllers/roles_rbac_test.go

@@ -0,0 +1,122 @@
+//go:build !cgo
+
+package controllers
+
+import (
+	"apiserver/store"
+	"apiserver/utils"
+	"net/http"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+)
+
+// RBAC fixture for bug-1958: mallory holds rbac:control scoped to self,
+// root holds it scoped to all. spare_role is a custom role nobody holds.
+const roleChangeRBAC = `
+permissions:
+  - rbac:control
+  - rbac:view
+  - cluster:control
+roles:
+  rbac_self:
+    permissions:
+      - action: rbac:control
+        domain:
+          - type: self
+  rbac_admin:
+    permissions:
+      - action: rbac:control
+        domain:
+          - type: all
+  spare_role:
+    permissions:
+      - action: rbac:view
+        domain:
+          - type: self
+users:
+  mallory:
+    roles:
+      - name: rbac_self
+  root:
+    roles:
+      - name: rbac_admin
+`
+
+func newRoleChangeRouter() *gin.Engine {
+	r := newTestRouter()
+	rc := &RoleController{}
+	r.POST("/xce/v1/roles/create", RequirePermission("rbac:control"), rc.CreateRole)
+	r.POST("/xce/v1/roles/:roleid/permissions", RequirePermission("rbac:control"), rc.AddPermissionsForRole)
+	r.DELETE("/xce/v1/roles/:rolename", RequirePermission("rbac:control"), rc.DeleteRoleAndPermissions)
+	return r
+}
+
+const clusterControlPerm = `{"permissions":[{"permission_name":"cluster:control","domain_type":"all"}]}`
+
+// bug-1958: rbac:control scoped to self must not add cluster:control (or
+// anything) to a role the user holds.
+func TestAddPermissionsForRole_SelfScopeCannotExtendOwnRole(t *testing.T) {
+	setupRBACTest(t, roleChangeRBAC)
+	r := newRoleChangeRouter()
+
+	w := doRoleRequest(r, http.MethodPost, "/xce/v1/roles/rbac_self/permissions", "mallory", clusterControlPerm)
+	if utils.ContainsField(store.GetRoleAllPermissions("rbac_self"), "cluster:control") {
+		t.Errorf("privilege escalation: mallory (rbac:control scope self) added cluster:control to its own role (status %d, %s)",
+			w.Code, w.Body.String())
+	}
+	if w.Code != http.StatusForbidden {
+		t.Errorf("status = %d, want 403 (%s)", w.Code, w.Body.String())
+	}
+}
+
+// bug-1958: rbac:control scoped to self must not create roles.
+func TestCreateRole_SelfScopeRejected(t *testing.T) {
+	setupRBACTest(t, roleChangeRBAC)
+	r := newRoleChangeRouter()
+
+	w := doRoleRequest(r, http.MethodPost, "/xce/v1/roles/create", "mallory",
+		`{"role_name":"evil","permissions":[{"permission_name":"cluster:control","domain_type":"all"}]}`)
+	if w.Code != http.StatusForbidden {
+		t.Errorf("status = %d, want 403 (%s)", w.Code, w.Body.String())
+	}
+	if utils.ContainsField(store.GetAllRoleNames(false), "evil") {
+		t.Error("role was created although the request was rejected")
+	}
+}
+
+// bug-1958: rbac:control scoped to self must not delete roles.
+func TestDeleteRole_SelfScopeRejected(t *testing.T) {
+	setupRBACTest(t, roleChangeRBAC)
+	r := newRoleChangeRouter()
+
+	w := doRoleRequest(r, http.MethodDelete, "/xce/v1/roles/spare_role", "mallory", "")
+	if w.Code != http.StatusForbidden {
+		t.Errorf("status = %d, want 403 (%s)", w.Code, w.Body.String())
+	}
+	if !utils.ContainsField(store.GetAllRoleNames(false), "spare_role") {
+		t.Error("role was deleted although the request was rejected")
+	}
+}
+
+// bug-1958: rbac:control scoped to all may still create, extend and delete
+// roles.
+func TestRoleChanges_AllScopeAllowed(t *testing.T) {
+	setupRBACTest(t, roleChangeRBAC)
+	r := newRoleChangeRouter()
+
+	// ReturnSuccess always answers HTTP 200 (code 201 is in the envelope).
+	w := doRoleRequest(r, http.MethodPost, "/xce/v1/roles/create", "root",
+		`{"role_name":"ops","permissions":[{"permission_name":"rbac:view","domain_type":"all"}]}`)
+	if w.Code != http.StatusOK || !utils.ContainsField(store.GetAllRoleNames(false), "ops") {
+		t.Fatalf("root creating role: status %d (%s)", w.Code, w.Body.String())
+	}
+	w = doRoleRequest(r, http.MethodPost, "/xce/v1/roles/ops/permissions", "root", clusterControlPerm)
+	if w.Code != http.StatusOK || !utils.ContainsField(store.GetRoleAllPermissions("ops"), "cluster:control") {
+		t.Fatalf("root extending role: status %d (%s), perms %v", w.Code, w.Body.String(), store.GetRoleAllPermissions("ops"))
+	}
+	w = doRoleRequest(r, http.MethodDelete, "/xce/v1/roles/ops", "root", "")
+	if w.Code != http.StatusOK || utils.ContainsField(store.GetAllRoleNames(false), "ops") {
+		t.Errorf("root deleting role: status %d (%s)", w.Code, w.Body.String())
+	}
+}

src/frontend/apiserver/controllers/users.go

@@ -480,6 +480,8 @@ const errNoRoleChangePermission = "no permission: granting or revoking user role
 // outright: letting it change its own roles would let it grant itself any
 // role, cluster_admin included. All grants are checked before
 // deduplication, which would otherwise prefer a narrower scope over all.
+// Role definitions (create, add permissions, delete) follow the same rule
+// (bug-1958, see canChangeRoles).
 func canChangeUserRoles(user string) bool {
 	for _, p := range collectUserPermissionsWithType(user) {
 		if p.Permission == "rbac:control" && p.DomainType == models.DomainTypeAll {