[scopes] feat: validate access list writes for scoped role assignments (#64407)

lib/accesslists/validate.go

@@ -24,6 +24,8 @@ import (
 	"github.com/gravitational/trace"
 
 	"github.com/gravitational/teleport/api/types/accesslist"
+	"github.com/gravitational/teleport/lib/scopes"
+	scopedaccess "github.com/gravitational/teleport/lib/scopes/access"
 )
 
 // ValidateAccessListWithMembers makes sure the given AccessList and it's members is valid before
@@ -79,6 +81,55 @@ func validateAccessList(a *accesslist.AccessList) error {
 		}
 	}
 
+	if err := validateScopedRoleGrants(a); err != nil {
+		return trace.Wrap(err)
+	}
+
+	return nil
+}
+
+func validateScopedRoleGrants(a *accesslist.AccessList) error {
+	// Access lists that assign scoped roles cannot contain membership_requires or ownership_requires.
+	hasScopedRoleGrants := len(a.Spec.Grants.ScopedRoles) > 0 || len(a.Spec.OwnerGrants.ScopedRoles) > 0
+	hasRequires := !a.Spec.MembershipRequires.IsEmpty() || !a.Spec.OwnershipRequires.IsEmpty()
+	if hasScopedRoleGrants && hasRequires {
+		return trace.BadParameter("access lists cannot contain both scoped_role grants and non-empty membership_requires or ownership_requires blocks")
+	}
+
+	uniqueScopedRoleGrants := make(map[accesslist.ScopedRoleGrant]struct{})
+	validateScopedRoleGrant := func(grant accesslist.ScopedRoleGrant) error {
+		if _, alreadyValidated := uniqueScopedRoleGrants[grant]; alreadyValidated {
+			return nil
+		}
+		switch {
+		case grant.Role == "":
+			return trace.BadParameter("role is empty")
+		case grant.Scope == "":
+			return trace.BadParameter("scope is empty")
+		}
+		if err := scopes.StrongValidate(grant.Scope); err != nil {
+			return trace.Wrap(err, "validating scope")
+		}
+		uniqueScopedRoleGrants[grant] = struct{}{}
+		return nil
+	}
+	for i, grant := range a.Spec.Grants.ScopedRoles {
+		if err := validateScopedRoleGrant(grant); err != nil {
+			return trace.Wrap(err, "validating grants.scoped_roles[%d]", i)
+		}
+	}
+	for i, grant := range a.Spec.OwnerGrants.ScopedRoles {
+		if err := validateScopedRoleGrant(grant); err != nil {
+			return trace.Wrap(err, "validating owner_grants.scoped_roles[%d]", i)
+		}
+	}
+	// Unique scoped role grants per access list have the same limit as role
+	// grants per scoped_role_assignment for the same reason: each role may
+	// require two backend.ConditionalActions to include in an AtomicWrite.
+	if len(uniqueScopedRoleGrants) > scopedaccess.MaxRolesPerAssignment {
+		return trace.BadParameter("access list contains too many unique scoped role grants (max %d)", scopedaccess.MaxRolesPerAssignment)
+	}
+
 	return nil
 }
 

lib/accesslists/validate_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/gravitational/teleport/api/types/accesslist"
+	scopedaccess "github.com/gravitational/teleport/lib/scopes/access"
 )
 
 func TestAccessListHierarchyCircularRefsCheck(t *testing.T) {
@@ -267,6 +268,83 @@ func TestAccessListValidateWithMembers_basic(t *testing.T) {
 		})
 	}
 
+	t.Run("scoped role grants are validated", func(t *testing.T) {
+		newScopedAccessList := func(name string) *accesslist.AccessList {
+			accessList := newAccessList(t, name, clock)
+			accessList.Spec.MembershipRequires = accesslist.Requires{}
+			accessList.Spec.OwnershipRequires = accesslist.Requires{}
+			accessList.Spec.Grants.ScopedRoles = nil
+			accessList.Spec.OwnerGrants.ScopedRoles = nil
+			return accessList
+		}
+
+		t.Run("scoped roles conflict with membership requires", func(t *testing.T) {
+			accessList := newScopedAccessList("test_access_list_membership_requires")
+			accessList.Spec.MembershipRequires = accesslist.Requires{Roles: []string{"member-role"}}
+			accessList.Spec.Grants.ScopedRoles = []accesslist.ScopedRoleGrant{{Role: "role-a", Scope: "/eng"}}
+
+			err := ValidateAccessListWithMembers(ctx, nil, accessList, nil, &mockAccessListAndMembersGetter{})
+			require.ErrorContains(t, err, "cannot contain both scoped_role grants")
+		})
+
+		t.Run("scoped roles conflict with ownership requires", func(t *testing.T) {
+			accessList := newScopedAccessList("test_access_list_ownership_requires")
+			accessList.Spec.OwnershipRequires = accesslist.Requires{Roles: []string{"owner-role"}}
+			accessList.Spec.Grants.ScopedRoles = []accesslist.ScopedRoleGrant{{Role: "role-a", Scope: "/eng"}}
+
+			err := ValidateAccessListWithMembers(ctx, nil, accessList, nil, &mockAccessListAndMembersGetter{})
+			require.ErrorContains(t, err, "cannot contain both scoped_role grants")
+		})
+
+		t.Run("empty scoped role name is rejected", func(t *testing.T) {
+			accessList := newScopedAccessList("test_access_list_empty_scoped_role")
+			accessList.Spec.Grants.ScopedRoles = []accesslist.ScopedRoleGrant{{Scope: "/eng"}}
+
+			err := ValidateAccessListWithMembers(ctx, nil, accessList, nil, &mockAccessListAndMembersGetter{})
+			require.ErrorContains(t, err, "validating grants.scoped_roles[0]")
+			require.ErrorContains(t, err, "role is empty")
+		})
+
+		t.Run("empty scoped role scope is rejected", func(t *testing.T) {
+			accessList := newScopedAccessList("test_access_list_empty_scope")
+			accessList.Spec.Grants.ScopedRoles = []accesslist.ScopedRoleGrant{{Role: "role-a"}}
+
+			err := ValidateAccessListWithMembers(ctx, nil, accessList, nil, &mockAccessListAndMembersGetter{})
+			require.ErrorContains(t, err, "validating grants.scoped_roles[0]")
+			require.ErrorContains(t, err, "scope is empty")
+		})
+
+		t.Run("invalid scoped role scope syntax is rejected", func(t *testing.T) {
+			accessList := newScopedAccessList("test_access_list_invalid_scope")
+			accessList.Spec.Grants.ScopedRoles = []accesslist.ScopedRoleGrant{{Role: "role-a", Scope: "not-a-scope"}}
+
+			err := ValidateAccessListWithMembers(ctx, nil, accessList, nil, &mockAccessListAndMembersGetter{})
+			require.ErrorContains(t, err, "validating grants.scoped_roles[0]")
+			require.ErrorContains(t, err, "validating scope")
+		})
+
+		t.Run("too many unique scoped role grants are rejected", func(t *testing.T) {
+			accessList := newScopedAccessList("test_access_list_too_many_scoped_roles")
+			for i := range scopedaccess.MaxRolesPerAssignment + 1 {
+				accessList.Spec.Grants.ScopedRoles = append(accessList.Spec.Grants.ScopedRoles, accesslist.ScopedRoleGrant{
+					Role:  fmt.Sprintf("role-%02d", i),
+					Scope: "/eng",
+				})
+			}
+
+			err := ValidateAccessListWithMembers(ctx, nil, accessList, nil, &mockAccessListAndMembersGetter{})
+			require.ErrorContains(t, err, "too many unique scoped role grants")
+		})
+
+		t.Run("valid owner grants scoped roles pass validation", func(t *testing.T) {
+			accessList := newScopedAccessList("test_access_list_valid_owner_scoped_roles")
+			accessList.Spec.OwnerGrants.ScopedRoles = []accesslist.ScopedRoleGrant{{Role: "owner-role", Scope: "/eng"}}
+
+			err := ValidateAccessListWithMembers(ctx, nil, accessList, nil, &mockAccessListAndMembersGetter{})
+			require.NoError(t, err)
+		})
+	})
+
 }
 
 func TestAccessListValidateWithMembers_members(t *testing.T) {

lib/services/local/access_list.go

@@ -20,6 +20,7 @@ package local
 
 import (
 	"context"
+	"errors"
 	"iter"
 	"maps"
 	"slices"
@@ -30,6 +31,7 @@ import (
 	"github.com/gravitational/trace"
 
 	accesslistv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/accesslist/v1"
+	scopedaccessv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/scopes/access/v1"
 	"github.com/gravitational/teleport/api/types"
 	"github.com/gravitational/teleport/api/types/accesslist"
 	"github.com/gravitational/teleport/api/types/common"
@@ -39,9 +41,12 @@ import (
 	"github.com/gravitational/teleport/lib/backend"
 	"github.com/gravitational/teleport/lib/itertools/stream"
 	"github.com/gravitational/teleport/lib/modules"
+	"github.com/gravitational/teleport/lib/scopes"
+	scopedaccess "github.com/gravitational/teleport/lib/scopes/access"
 	"github.com/gravitational/teleport/lib/services"
 	"github.com/gravitational/teleport/lib/services/local/generic"
 	"github.com/gravitational/teleport/lib/utils"
+	"github.com/gravitational/teleport/lib/utils/set"
 )
 
 const (
@@ -75,11 +80,12 @@ const (
 // consistent view to the rest of the Teleport application. It makes no decisions
 // about granting or withholding list membership.
 type AccessListService struct {
-	backend       backend.Backend
-	modules       modules.Modules
-	service       *generic.Service[*accesslist.AccessList]
-	memberService *generic.Service[*accesslist.AccessListMember]
-	reviewService *generic.Service[*accesslist.Review]
+	backend             backend.Backend
+	modules             modules.Modules
+	service             *generic.Service[*accesslist.AccessList]
+	memberService       *generic.Service[*accesslist.AccessListMember]
+	reviewService       *generic.Service[*accesslist.Review]
+	scopedAccessService *ScopedAccessService
 }
 
 type accessListAndMembersGetter struct {
@@ -162,12 +168,15 @@ func NewAccessListServiceV2(cfg AccessListServiceConfig) (*AccessListService, er
 		return nil, trace.Wrap(err)
 	}
 
+	scopedAccessService := NewScopedAccessService(cfg.Backend)
+
 	return &AccessListService{
-		backend:       cfg.Backend,
-		modules:       cfg.Modules,
-		service:       service,
-		memberService: memberService,
-		reviewService: reviewService,
+		backend:             cfg.Backend,
+		modules:             cfg.Modules,
+		service:             service,
+		memberService:       memberService,
+		reviewService:       reviewService,
+		scopedAccessService: scopedAccessService,
 	}, nil
 }
 
@@ -235,11 +244,7 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
 
 	var upserted *accesslist.AccessList
 	var existingAccessList *accesslist.AccessList
-
-	opFn := a.service.UpsertResource
-	if op == opTypeUpdate {
-		opFn = a.service.ConditionalUpdateResource
-	}
+	var scopedRoleConditions []backend.ConditionalAction
 
 	validateAccessList := func() error {
 		var err error
@@ -257,7 +262,18 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
 			return trace.Wrap(err)
 		}
 
-		return accesslists.ValidateAccessListWithMembers(ctx, existingAccessList, accessList, listMembers, &accessListAndMembersGetter{a.service, a.memberService})
+		if err := accesslists.ValidateAccessListWithMembers(ctx, existingAccessList, accessList, listMembers, &accessListAndMembersGetter{a.service, a.memberService}); err != nil {
+			return trace.Wrap(err)
+		}
+
+		// TODO(nklaassen): make full cross-resource validation opt-in.
+		validatedRoles, err := a.validateScopedRoleGrants(ctx, existingAccessList, accessList)
+		if err != nil {
+			return trace.Wrap(err, "validating scoped role grants")
+		}
+		scopedRoleConditions = a.conditionalActionsForValidatedScopedRoles(validatedRoles)
+
+		return nil
 	}
 
 	reconcileOldOwners := func() error {
@@ -287,9 +303,8 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
 		return nil
 	}
 
-	updateAccessList := func() error {
-		var err error
-		upserted, err = opFn(ctx, accessList)
+	updateAccessList := func() (err error) {
+		upserted, err = a.writeAccessListWithConditions(ctx, accessList, op, scopedRoleConditions)
 		return trace.Wrap(err)
 	}
 
@@ -690,18 +705,63 @@ func (a *AccessListService) DeleteAllAccessListMembers(ctx context.Context) erro
 	return trace.Wrap(a.memberService.DeleteAllResources(ctx))
 }
 
-type writeFn func(context.Context, *accesslist.AccessList) (*accesslist.AccessList, error)
-
-func (a *AccessListService) selectWriteFn(op opType) (writeFn, error) {
+func (a *AccessListService) writeAccessList(
+	ctx context.Context,
+	accessList *accesslist.AccessList,
+	op opType,
+) (*accesslist.AccessList, error) {
 	switch op {
 	case opTypeUpdate:
-		return a.service.ConditionalUpdateResource, nil
-
+		return a.service.ConditionalUpdateResource(ctx, accessList)
 	case opTypeUpsert:
-		return a.service.UpsertResource, nil
+		return a.service.UpsertResource(ctx, accessList)
+	default:
+		return nil, trace.BadParameter("Unknown Access List write operation: %d", op)
 	}
+}
 
-	return nil, trace.BadParameter("Unknown Access List write operation: %d", op)
+func (a *AccessListService) writeAccessListWithConditions(
+	ctx context.Context,
+	accessList *accesslist.AccessList,
+	op opType,
+	conditions []backend.ConditionalAction,
+) (*accesslist.AccessList, error) {
+	if len(conditions) == 0 {
+		return a.writeAccessList(ctx, accessList, op)
+	}
+	item, err := a.service.MakeBackendItem(accessList)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+	writeCondition, err := a.selectWriteCondition(op, item.Revision)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+	writeAction := backend.ConditionalAction{
+		Key:       item.Key,
+		Condition: writeCondition,
+		Action:    backend.Put(item),
+	}
+	conditionalActions := append(conditions, writeAction)
+	revision, err := a.backend.AtomicWrite(ctx, conditionalActions)
+	if err != nil {
+		if errors.Is(err, backend.ErrConditionFailed) {
+			return nil, trace.CompareFailed("access list %q or a related resource was concurrently modified", accessList.GetName())
+		}
+		return nil, trace.Wrap(err)
+	}
+	accessList.SetRevision(revision)
+	return accessList, nil
+}
+
+func (a *AccessListService) selectWriteCondition(op opType, revision string) (backend.Condition, error) {
+	switch op {
+	case opTypeUpdate:
+		return backend.Revision(revision), nil
+	case opTypeUpsert:
+		return backend.Whatever(), nil
+	}
+	return backend.Condition{}, trace.BadParameter("Unknown Access List write operation: %d", op)
 }
 
 // writeAccessListWithMembers holds all of the common logic for updating and
@@ -711,11 +771,6 @@ func (a *AccessListService) writeAccessListWithMembers(ctx context.Context, acce
 		return nil, nil, trace.Wrap(err)
 	}
 
-	writeFn, err := a.selectWriteFn(op)
-	if err != nil {
-		return nil, nil, trace.Wrap(err)
-	}
-
 	for _, m := range membersIn {
 		if err := m.CheckAndSetDefaults(); err != nil {
 			return nil, nil, trace.Wrap(err)
@@ -723,6 +778,7 @@ func (a *AccessListService) writeAccessListWithMembers(ctx context.Context, acce
 	}
 
 	var existingAccessList *accesslist.AccessList
+	var scopedRoleConditions []backend.ConditionalAction
 
 	validateAccessList := func() error {
 		var err error
@@ -747,6 +803,13 @@ func (a *AccessListService) writeAccessListWithMembers(ctx context.Context, acce
 			return trace.Wrap(err)
 		}
 
+		// TODO(nklaassen): make full cross-resource validation opt-in.
+		validatedRoles, err := a.validateScopedRoleGrants(ctx, existingAccessList, accessList)
+		if err != nil {
+			return trace.Wrap(err)
+		}
+		scopedRoleConditions = a.conditionalActionsForValidatedScopedRoles(validatedRoles)
+
 		return nil
 	}
 
@@ -838,9 +901,8 @@ func (a *AccessListService) writeAccessListWithMembers(ctx context.Context, acce
 		return nil
 	}
 
-	writeAccessList := func() error {
-		var err error
-		accessList, err = writeFn(ctx, accessList)
+	writeAccessList := func() (err error) {
+		accessList, err = a.writeAccessListWithConditions(ctx, accessList, op, scopedRoleConditions)
 		return trace.Wrap(err)
 	}
 
@@ -888,6 +950,83 @@ func (a *AccessListService) writeAccessListWithMembers(ctx context.Context, acce
 	return accessList, membersIn, nil
 }
 
+// validateScopedRoleGrants asserts that:
+// - newList only assigns scoped roles that exist
+// - newList only assigns scoped roles defined in the root scope
+// - newList only assigns scoped roles to an assignable_scope of the role
+//
+// In case the access list is already in an invalid state (existingList grants
+// a scoped role violating any of the above invariants) we would prefer not to
+// break unrelated list updates, so the checks are only performed if a role is
+// newly granted at any scope.
+//
+// It returns a map of all validated scoped roles keyed by role name.
+func (a *AccessListService) validateScopedRoleGrants(ctx context.Context, existingList, newList *accesslist.AccessList) (map[string]*scopedaccessv1.ScopedRole, error) {
+	if len(newList.Spec.Grants.ScopedRoles) == 0 && len(newList.Spec.OwnerGrants.ScopedRoles) == 0 {
+		// The new list does not grant any scoped roles, so no checks are needed.
+		return nil, nil
+	}
+
+	var existingListGrants set.Set[accesslist.ScopedRoleGrant]
+	if existingList != nil {
+		existingListGrants = set.New(existingList.Spec.Grants.ScopedRoles...).Add(existingList.Spec.OwnerGrants.ScopedRoles...)
+	}
+	newListGrants := set.New(newList.Spec.Grants.ScopedRoles...).Add(newList.Spec.OwnerGrants.ScopedRoles...)
+	addedGrants := newListGrants.Subtract(existingListGrants)
+
+	if addedGrants.Len() == 0 {
+		// The new list does not grant any scoped roles at scopes not already
+		// granted by the existing list, no validation is necessary.
+		return nil, nil
+	}
+
+	// This list has new scoped role grants, the scopes feature must be enabled.
+	if err := scopes.AssertFeatureEnabled(); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	// Deduplicate roles, which may be granted at multiple scopes.
+	validatedRoles := make(map[string]*scopedaccessv1.ScopedRole)
+	for grant := range addedGrants {
+		role, ok := validatedRoles[grant.Role]
+		if !ok {
+			rsp, err := a.scopedAccessService.GetScopedRole(ctx, &scopedaccessv1.GetScopedRoleRequest{Name: grant.Role})
+			if err != nil {
+				if trace.IsNotFound(err) {
+					return nil, trace.BadParameter("access_list %q has scoped role grant for non-existent role %q", newList.GetName(), grant.Role)
+				}
+				return nil, trace.Wrap(err)
+			}
+			role = rsp.GetRole()
+		}
+
+		if scopes.Compare(role.GetScope(), "/") != scopes.Equivalent {
+			return nil, trace.BadParameter("access_list %q has scoped role grant for role %q which is not defined in root scope", newList.GetName(), grant.Role)
+		}
+		if !scopedaccess.RoleIsAssignableToScopeOfEffect(role, grant.Scope) {
+			return nil, trace.BadParameter("access_list %q has scoped role grant for role %q at non-assignable scope %q", newList.GetName(), grant.Role, grant.Scope)
+		}
+
+		validatedRoles[grant.Role] = role
+	}
+
+	return validatedRoles, nil
+}
+
+// conditionalActionsForValidatedScopedRoles returns a set of
+// backend.ConditionalActions that must be used in an AtomicWrite to
+// prevent concurrent modifications granted scoped roles.
+func (a *AccessListService) conditionalActionsForValidatedScopedRoles(validatedRoles map[string]*scopedaccessv1.ScopedRole) []backend.ConditionalAction {
+	condacts := make([]backend.ConditionalAction, 0, 2*len(validatedRoles))
+	for roleName, role := range validatedRoles {
+		// Assert that the role has not changed since it was validated.
+		condacts = append(condacts, a.scopedAccessService.assertAssignedRoleStable(roleName, role.GetMetadata().GetRevision()))
+		// Prevent changes to the role concurrent with changes to the assignment.
+		condacts = append(condacts, a.scopedAccessService.touchRoleAssignmentLock(roleName))
+	}
+	return condacts
+}
+
 // UpsertAccessListWithMembers creates or updates an access list resource and its members.
 func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, accessList *accesslist.AccessList, membersIn []*accesslist.AccessListMember) (*accesslist.AccessList, []*accesslist.AccessListMember, error) {
 	upsertedACL, upsertedMembers, err := a.writeAccessListWithMembers(ctx, accessList, membersIn, opTypeUpsert)

lib/services/local/access_list_test.go

@@ -34,6 +34,9 @@ import (
 	"github.com/jonboulle/clockwork"
 	"github.com/stretchr/testify/require"
 
+	headerv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/header/v1"
+	scopedaccessv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/scopes/access/v1"
+	"github.com/gravitational/teleport/api/types"
 	"github.com/gravitational/teleport/api/types/accesslist"
 	"github.com/gravitational/teleport/api/types/common"
 	"github.com/gravitational/teleport/api/types/header"
@@ -47,6 +50,7 @@ import (
 	"github.com/gravitational/teleport/lib/itertools/stream"
 	"github.com/gravitational/teleport/lib/modules"
 	"github.com/gravitational/teleport/lib/modules/modulestest"
+	scopedaccess "github.com/gravitational/teleport/lib/scopes/access"
 	"github.com/gravitational/teleport/lib/utils"
 	sliceutils "github.com/gravitational/teleport/lib/utils/slices"
 )
@@ -153,6 +157,7 @@ func TestAccessListCRUD(t *testing.T) {
 }
 
 func TestAccessListCRUDScopedRoleGrants(t *testing.T) {
+	t.Setenv("TELEPORT_UNSTABLE_SCOPES", "yes")
 	ctx := t.Context()
 	clock := clockwork.NewFakeClock()
 
@@ -163,8 +168,26 @@ func TestAccessListCRUDScopedRoleGrants(t *testing.T) {
 	require.NoError(t, err)
 
 	service := newAccessListService(t, mem, modulestest.EnterpriseModules())
+	scopedAccessService := NewScopedAccessService(mem)
 
-	accessList := newAccessList(t, "accessList-scoped", clock)
+	for _, role := range []string{"scoped-role-1", "scoped-role-2", "scoped-role-3"} {
+		_, err = scopedAccessService.CreateScopedRole(ctx, &scopedaccessv1.CreateScopedRoleRequest{
+			Role: &scopedaccessv1.ScopedRole{
+				Kind: scopedaccess.KindScopedRole,
+				Metadata: &headerv1.Metadata{
+					Name: role,
+				},
+				Scope: "/",
+				Spec: &scopedaccessv1.ScopedRoleSpec{
+					AssignableScopes: []string{"/eng", "/platform", "/ops"},
+				},
+				Version: types.V1,
+			},
+		})
+		require.NoError(t, err)
+	}
+
+	accessList := newAccessList(t, "accessList-scoped", clock, withOwnerRequires(accesslist.Requires{}), withMemberRequires(accesslist.Requires{}))
 	accessList.Spec.Grants.ScopedRoles = []accesslist.ScopedRoleGrant{
 		{
 			Role:  "scoped-role-1",
@@ -204,6 +227,117 @@ func TestAccessListCRUDScopedRoleGrants(t *testing.T) {
 	require.Empty(t, cmp.Diff(updated, fetched, cmpOpts...))
 }
 
+func TestAccessListScopedRoleGrantInvariants(t *testing.T) {
+	t.Setenv("TELEPORT_UNSTABLE_SCOPES", "yes")
+	ctx := t.Context()
+	clock := clockwork.NewFakeClock()
+
+	mem, err := memory.New(memory.Config{
+		Context: ctx,
+		Clock:   clock,
+	})
+	require.NoError(t, err)
+
+	service := newAccessListService(t, mem, modulestest.EnterpriseModules())
+	scopedAccessService := NewScopedAccessService(mem)
+
+	for _, role := range []*scopedaccessv1.ScopedRole{
+		{
+			Kind: scopedaccess.KindScopedRole,
+			Metadata: &headerv1.Metadata{
+				Name: "root-role",
+			},
+			Scope: "/",
+			Spec: &scopedaccessv1.ScopedRoleSpec{
+				AssignableScopes: []string{"/eng", "/ops"},
+			},
+			Version: types.V1,
+		},
+		{
+			Kind: scopedaccess.KindScopedRole,
+			Metadata: &headerv1.Metadata{
+				Name: "non-root-role",
+			},
+			Scope: "/eng",
+			Spec: &scopedaccessv1.ScopedRoleSpec{
+				AssignableScopes: []string{"/eng"},
+			},
+			Version: types.V1,
+		},
+	} {
+		_, err := scopedAccessService.CreateScopedRole(ctx, &scopedaccessv1.CreateScopedRoleRequest{Role: role})
+		require.NoError(t, err)
+	}
+
+	baseList := newAccessList(t, "testlist", clock,
+		withOwnerRequires(accesslist.Requires{}), withMemberRequires(accesslist.Requires{}))
+	baseList, err = service.UpsertAccessList(ctx, baseList)
+	require.NoError(t, err)
+
+	tests := []struct {
+		name  string
+		apply func(*accesslist.AccessList)
+		msg   string
+	}{
+		{
+			name: "missing role",
+			apply: func(al *accesslist.AccessList) {
+				al.Spec.Grants.ScopedRoles = []accesslist.ScopedRoleGrant{{Role: "does-not-exist", Scope: "/eng"}}
+			},
+			msg: "non-existent role",
+		},
+		{
+			name: "non root role",
+			apply: func(al *accesslist.AccessList) {
+				al.Spec.Grants.ScopedRoles = []accesslist.ScopedRoleGrant{{Role: "non-root-role", Scope: "/eng"}}
+			},
+			msg: "not defined in root scope",
+		},
+		{
+			name: "non assignable scope",
+			apply: func(al *accesslist.AccessList) {
+				al.Spec.Grants.ScopedRoles = []accesslist.ScopedRoleGrant{{Role: "root-role", Scope: "/db"}}
+			},
+			msg: "non-assignable scope",
+		},
+		{
+			name: "owner grants checked too",
+			apply: func(al *accesslist.AccessList) {
+				al.Spec.OwnerGrants.ScopedRoles = []accesslist.ScopedRoleGrant{{Role: "root-role", Scope: "/db"}}
+			},
+			msg: "non-assignable scope",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			al := baseList.Clone()
+			tc.apply(al)
+
+			t.Run("upsert", func(t *testing.T) {
+				_, err := service.UpsertAccessList(ctx, al)
+				require.ErrorAs(t, err, new(*trace.BadParameterError))
+				require.ErrorContains(t, err, tc.msg)
+			})
+			t.Run("upsertWithMembers", func(t *testing.T) {
+				_, _, err := service.UpsertAccessListWithMembers(ctx, al, nil)
+				require.ErrorAs(t, err, new(*trace.BadParameterError))
+				require.ErrorContains(t, err, tc.msg)
+			})
+			t.Run("update", func(t *testing.T) {
+				_, err := service.UpdateAccessList(ctx, al)
+				require.ErrorAs(t, err, new(*trace.BadParameterError))
+				require.ErrorContains(t, err, tc.msg)
+			})
+			t.Run("updateAndOverwriteMembers", func(t *testing.T) {
+				_, _, err := service.UpdateAccessListAndOverwriteMembers(ctx, al, nil)
+				require.ErrorAs(t, err, new(*trace.BadParameterError))
+				require.ErrorContains(t, err, tc.msg)
+			})
+		})
+	}
+}
+
 func requireAccessDenied(t require.TestingT, err error, i ...any) {
 	require.True(
 		t,
@@ -1758,6 +1892,18 @@ func withOwners(owners []accesslist.Owner) newAccessListOpt {
 	}
 }
 
+func withOwnerRequires(requires accesslist.Requires) newAccessListOpt {
+	return func(o *newAccessListOptions) {
+		o.ownerRequires = requires
+	}
+}
+
+func withMemberRequires(requires accesslist.Requires) newAccessListOpt {
+	return func(o *newAccessListOptions) {
+		o.memberRequires = requires
+	}
+}
+
 func newAccessList(t *testing.T, name string, clock clockwork.Clock, opts ...newAccessListOpt) *accesslist.AccessList {
 	t.Helper()
 

lib/services/local/scoped_access.go

@@ -581,22 +581,12 @@ func (s *ScopedAccessService) CreateScopedRoleAssignment(ctx context.Context, re
 			continue
 		}
 
-		// assert that role is unchanged and modify associated role lock so that role modifications can
-		// detect concurrent modifications to their assignments.
-		condacts = append(condacts, []backend.ConditionalAction{
-			{
-				Key:       scopedRoleKey(subAssignment.GetRole()),
-				Condition: backend.Revision(roleRevision),
-				Action:    backend.Nop(),
-			},
-			{
-				Key:       roleAssignmentLockKey(subAssignment.GetRole()),
-				Condition: backend.Whatever(),
-				Action: backend.Put(backend.Item{
-					Value: newRoleAssignmentLockVal(subAssignment.GetRole()),
-				}),
-			},
-		}...)
+		condacts = append(condacts,
+			// assert that role is unchanged since it was checked.
+			s.assertAssignedRoleStable(subAssignment.GetRole(), roleRevision),
+			// touch role lock so that role modifications can detect concurrent
+			// modifications to their assignments.
+			s.touchRoleAssignmentLock(subAssignment.GetRole()))
 
 		assertedRoles[subAssignment.GetRole()] = struct{}{}
 	}
@@ -727,6 +717,32 @@ func (s *ScopedAccessService) DeleteScopedRoleAssignment(ctx context.Context, re
 	return &scopedaccessv1.DeleteScopedRoleAssignmentResponse{}, nil
 }
 
+// assertAssignedRoleStable returns a backend.ConditionalAction that asserts
+// that a scoped role has not been modified since it was checked at a given
+// roleRevision.
+func (s *ScopedAccessService) assertAssignedRoleStable(roleName, roleRevision string) backend.ConditionalAction {
+	return backend.ConditionalAction{
+		Key:       scopedRoleKey(roleName),
+		Condition: backend.Revision(roleRevision),
+		Action:    backend.Nop(),
+	}
+}
+
+// touchRoleAssignmentLock returns a backend.ConditionalAction to be included
+// in the list of backend.ConditionalActions for an AtomicWrite that modifies a
+// resource that assigns a scoped role. This allows operations that create,
+// modify, or delete a scoped role to efficiently assert that no assignment of
+// that role has been concurrently created or modified.
+func (s *ScopedAccessService) touchRoleAssignmentLock(roleName string) backend.ConditionalAction {
+	return backend.ConditionalAction{
+		Key:       roleAssignmentLockKey(roleName),
+		Condition: backend.Whatever(),
+		Action: backend.Put(backend.Item{
+			Value: newRoleAssignmentLockVal(roleName),
+		}),
+	}
+}
+
 func scopedRoleKey(roleName string) backend.Key {
 	return backend.NewKey(scopedRolePrefix, scopedRoleRoleComponent, roleName)
 }