diff --git a/lib/accesslists/hierarchy.go b/lib/accesslists/hierarchy.go index 1bd122cbc9fb2..8949ee9e3e6ab 100644 --- a/lib/accesslists/hierarchy.go +++ b/lib/accesslists/hierarchy.go @@ -263,8 +263,9 @@ func IsUserLocked(err error) bool { return errors.As(err, &userLockedError{}) } -// IsAccessListOwner checks if the given user is the Access List owner. It returns an error matched -// by [IsUserLocked] if the user is locked. +// IsAccessListOwner checks if the given user is the Access List owner. +// It returns an error matched by [IsUserLocked] if the user is locked. +// If the user is not an owner, it returns a trace.AccessDenied error. func IsAccessListOwner( ctx context.Context, user types.User, @@ -316,30 +317,34 @@ func IsAccessListOwner( if owner.MembershipKind == accesslist.MembershipKindList { ownerAccessList, err := g.GetAccessList(ctx, owner.Name) if err != nil { - ownershipErr = trace.Wrap(err) - continue + return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, trace.Wrap(err) } // Since we already verified that the user is not locked, don't provide lockGetter here - membershipType, err := IsAccessListMember(ctx, user, ownerAccessList, g, nil, clock) - if err != nil { - ownershipErr = trace.Wrap(err) - continue - } - if membershipType != accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED { - if !UserMeetsRequirements(user, accessList.Spec.OwnershipRequires) { - ownershipErr = trace.AccessDenied("User '%s' does not meet the ownership requirements for Access List '%s'", user.GetName(), accessList.Spec.Title) + if _, err := IsAccessListMember(ctx, user, ownerAccessList, g, nil, clock); err != nil { + if trace.IsAccessDenied(err) { + ownershipErr = trace.Wrap(err) continue } - return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_INHERITED, nil + return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, trace.Wrap(err) + } + if !UserMeetsRequirements(user, accessList.Spec.OwnershipRequires) { + ownershipErr = trace.AccessDenied("User '%s' does not meet the ownership requirements for Access List '%s'", user.GetName(), accessList.Spec.Title) + continue } + return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_INHERITED, nil } } + if ownershipErr == nil { + ownershipErr = trace.AccessDenied("no ownership path found") + } + return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, trace.Wrap(ownershipErr) } -// IsAccessListMember checks if the given user is the Access List member. It returns an error -// matched by [IsUserLocked] if the user is locked. +// IsAccessListMember checks if the given user is the Access List member. +// It returns an error matched by [IsUserLocked] if the user is locked. +// If the user is not a member, it returns a trace.AccessDenied error. func IsAccessListMember( ctx context.Context, user types.User, @@ -375,7 +380,7 @@ func IsAccessListMember( members, err := fetchMembers(ctx, accessList.GetName(), g) if err != nil { - return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, trace.Wrap(err) + return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, trace.Wrap(err, "fetching access list %q members", accessList.GetName()) } var membershipErr error @@ -399,14 +404,19 @@ func IsAccessListMember( if member.Spec.MembershipKind == accesslist.MembershipKindList { memberAccessList, err := g.GetAccessList(ctx, member.GetName()) if err != nil { - membershipErr = trace.Wrap(err) - continue + if trace.IsNotFound(err) { + continue + } + return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, trace.Wrap(err, "getting access list %q", member.GetName()) } // Since we already verified that the user is not locked, don't provide lockGetter here membershipType, err := IsAccessListMember(ctx, user, memberAccessList, g, nil, clock) if err != nil { - membershipErr = trace.Wrap(err) - continue + if trace.IsAccessDenied(err) { + membershipErr = err + continue + } + return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, trace.Wrap(err) } if membershipType != accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED { if !UserMeetsRequirements(user, accessList.Spec.MembershipRequires) { @@ -422,6 +432,10 @@ func IsAccessListMember( } } + if membershipErr == nil { + membershipErr = trace.AccessDenied("no access path found") + } + return accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, trace.Wrap(membershipErr) } diff --git a/lib/accesslists/hierarchy_test.go b/lib/accesslists/hierarchy_test.go index c5ef715f23d57..f46b3fcd98a52 100644 --- a/lib/accesslists/hierarchy_test.go +++ b/lib/accesslists/hierarchy_test.go @@ -21,6 +21,7 @@ package accesslists import ( "context" "iter" + "slices" "sort" "testing" "time" @@ -185,7 +186,7 @@ func TestAccessListHierarchyIsOwner(t *testing.T) { ownershipType, err := IsAccessListOwner(ctx, stubUserNoRequires, acl4, accessListAndMembersGetter, nil, clock) require.Error(t, err) - require.ErrorIs(t, err, trace.AccessDenied("User '%s' does not meet the membership requirements for Access List '%s'", member1, acl1.Spec.Title)) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) // Should not have inherited ownership due to missing OwnershipRequires. require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, ownershipType) @@ -200,7 +201,7 @@ func TestAccessListHierarchyIsOwner(t *testing.T) { ownershipType, err = IsAccessListOwner(ctx, stubUserMeetsMemberRequires, acl4, accessListAndMembersGetter, nil, clock) require.Error(t, err) - require.ErrorIs(t, err, trace.AccessDenied("User '%s' does not meet the ownership requirements for Access List '%s'", member1, acl4.Spec.Title)) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, ownershipType) // User which meets acl1's Membership and acl1's Ownership requirements. @@ -221,8 +222,9 @@ func TestAccessListHierarchyIsOwner(t *testing.T) { stubUserMeetsAllRequires.SetName(member2) ownershipType, err = IsAccessListOwner(ctx, stubUserMeetsAllRequires, acl4, accessListAndMembersGetter, nil, clock) - require.NoError(t, err) // Should not have ownership. + require.Error(t, err) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, ownershipType) } @@ -267,7 +269,8 @@ func TestAccessListIsMember(t *testing.T) { locksGetter.targets[member1] = []types.Lock{lock} membershipType, err = IsAccessListMember(ctx, stubMember1, acl1, accessListAndMembersGetter, locksGetter, clock) - require.ErrorIs(t, err, trace.AccessDenied("User %q is currently locked", member1)) + require.Error(t, err) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, membershipType) } @@ -290,8 +293,9 @@ func TestAccessListIsMember_RequirementsAndExpiry(t *testing.T) { // Missing membershipRequires should be AccessDenied typ, err := IsAccessListMember(ctx, u, acl, aclGetter, locks, clock) + require.Error(t, err) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, typ) - require.ErrorIs(t, err, trace.AccessDenied("User '%s' does not meet the membership requirements for Access List '%s'", u.GetName(), acl.GetName())) // Give correct traits/roles, but expire the membership u.SetRoles([]string{"mrole1", "mrole2"}) @@ -304,7 +308,210 @@ func TestAccessListIsMember_RequirementsAndExpiry(t *testing.T) { typ, err = IsAccessListMember(ctx, u, acl, aclGetter, locks, clock) require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, typ) - require.ErrorIs(t, err, trace.AccessDenied("User '%s's membership in Access List '%s' has expired", u.GetName(), acl.GetName())) + require.Error(t, err) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) +} + +func TestAccessListIsMember_NestedRequirements(t *testing.T) { + clock := clockwork.NewFakeClock() + ctx := t.Context() + locks := &mockLocksGetter{} + + t.Run("nested lists with requirements at multiple levels", func(t *testing.T) { + rootList := newAccessList(t, "root", clock) + rootList.Spec.MembershipRequires = accesslist.Requires{ + Roles: []string{"root-role"}, + } + + middleList := newAccessList(t, "middle", clock) + middleList.Spec.MembershipRequires = accesslist.Requires{ + Roles: []string{"middle-role"}, + } + + leafList := newAccessList(t, "leaf", clock) + leafList.Spec.MembershipRequires = accesslist.Requires{ + Roles: []string{"leaf-role"}, + } + + const userName = "alice" + + userMember := newAccessListMember(t, leafList.GetName(), userName, accesslist.MembershipKindUser, clock) + leafInMiddle := newAccessListMember(t, middleList.GetName(), leafList.GetName(), accesslist.MembershipKindList, clock) + middleInRoot := newAccessListMember(t, rootList.GetName(), middleList.GetName(), accesslist.MembershipKindList, clock) + + aclGetter := &mockAccessListAndMembersGetter{ + accessLists: map[string]*accesslist.AccessList{ + "root": rootList, + "middle": middleList, + "leaf": leafList, + }, + members: map[string][]*accesslist.AccessListMember{ + "root": {middleInRoot}, + "middle": {leafInMiddle}, + "leaf": {userMember}, + }, + } + + user, err := types.NewUser(userName) + require.NoError(t, err) + allRoles := slices.Concat( + rootList.Spec.MembershipRequires.Roles, + middleList.Spec.MembershipRequires.Roles, + leafList.Spec.MembershipRequires.Roles, + ) + user.SetRoles(allRoles) + + typ, err := IsAccessListMember(ctx, user, rootList, aclGetter, locks, clock) + require.NoError(t, err) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_INHERITED, typ) + + typ, err = IsAccessListMember(ctx, user, middleList, aclGetter, locks, clock) + require.NoError(t, err) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_INHERITED, typ) + + typ, err = IsAccessListMember(ctx, user, leafList, aclGetter, locks, clock) + require.NoError(t, err) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_EXPLICIT, typ) + + // User missing middle role + missingMiddleRoles := slices.Concat( + rootList.Spec.MembershipRequires.Roles, + leafList.Spec.MembershipRequires.Roles, + ) + user.SetRoles(missingMiddleRoles) + + typ, err = IsAccessListMember(ctx, user, rootList, aclGetter, locks, clock) + require.Error(t, err) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, typ) + }) + + t.Run("nested lists with expired list membership in the middle", func(t *testing.T) { + rootList := newAccessList(t, "root", clock) + middleList := newAccessList(t, "middle", clock) + leafList := newAccessList(t, "leaf", clock) + + const userName = "alice" + + userMember := newAccessListMember(t, leafList.GetName(), userName, accesslist.MembershipKindUser, clock) + // middle -> leaf membership expires in 12 hours while the other memberships expire in 24h + leafInMiddle := newAccessListMember(t, middleList.GetName(), leafList.GetName(), accesslist.MembershipKindList, clockwork.NewFakeClockAt(clock.Now().Add(-12*time.Hour))) + middleInRoot := newAccessListMember(t, rootList.GetName(), middleList.GetName(), accesslist.MembershipKindList, clock) + + aclGetter := &mockAccessListAndMembersGetter{ + accessLists: map[string]*accesslist.AccessList{ + "root": rootList, + "middle": middleList, + "leaf": leafList, + }, + members: map[string][]*accesslist.AccessListMember{ + "root": {middleInRoot}, + "middle": {leafInMiddle}, + "leaf": {userMember}, + }, + } + + user, err := types.NewUser(userName) + require.NoError(t, err) + user.SetRoles(rootList.Spec.MembershipRequires.Roles) + user.SetTraits(rootList.Spec.MembershipRequires.Traits) + + typ, err := IsAccessListMember(ctx, user, rootList, aclGetter, locks, clock) + require.NoError(t, err) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_INHERITED, typ) + + // advancing the clock makes the middle -> leaf membership expire + clock.Advance(14 * time.Hour) + + typ, err = IsAccessListMember(ctx, user, rootList, aclGetter, locks, clock) + require.Error(t, err) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, typ) + }) + + t.Run("cyclic graph, no membership", func(t *testing.T) { + t.Skip("cyclic graph not supported yet") + firstList := newAccessList(t, "first", clock) + secondList := newAccessList(t, "second", clock) + thirdList := newAccessList(t, "third", clock) + + firstArc := newAccessListMember(t, firstList.GetName(), secondList.GetName(), accesslist.MembershipKindList, clock) + secondArc := newAccessListMember(t, secondList.GetName(), thirdList.GetName(), accesslist.MembershipKindList, clock) + thirdArc := newAccessListMember(t, thirdList.GetName(), firstList.GetName(), accesslist.MembershipKindList, clock) + + aclGetter := &mockAccessListAndMembersGetter{ + accessLists: map[string]*accesslist.AccessList{ + firstList.GetName(): firstList, + secondList.GetName(): secondList, + thirdList.GetName(): thirdList, + }, + members: map[string][]*accesslist.AccessListMember{ + firstList.GetName(): {firstArc}, + secondList.GetName(): {secondArc}, + thirdList.GetName(): {thirdArc}, + }, + } + + user, err := types.NewUser("alice") + require.NoError(t, err) + // Make sure the user meets the membership requirements. + user.SetRoles(firstList.Spec.MembershipRequires.Roles) + user.SetTraits(firstList.Spec.MembershipRequires.Traits) + + typ, err := IsAccessListMember(ctx, user, firstList, aclGetter, locks, clock) + require.Error(t, err) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, typ) + typ, err = IsAccessListMember(ctx, user, secondList, aclGetter, locks, clock) + require.Error(t, err) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, typ) + typ, err = IsAccessListMember(ctx, user, thirdList, aclGetter, locks, clock) + require.Error(t, err) + require.ErrorAs(t, err, new(*trace.AccessDeniedError)) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_UNSPECIFIED, typ) + }) + + t.Run("cyclic graph, user membership", func(t *testing.T) { + t.Skip("cyclic graph not supported yet") + firstList := newAccessList(t, "first", clock) + secondList := newAccessList(t, "second", clock) + thirdList := newAccessList(t, "third", clock) + + user, err := types.NewUser("alice") + require.NoError(t, err) + // Make sure the user meets the membership requirements. + user.SetRoles(firstList.Spec.MembershipRequires.Roles) + user.SetTraits(firstList.Spec.MembershipRequires.Traits) + + firstArc := newAccessListMember(t, firstList.GetName(), secondList.GetName(), accesslist.MembershipKindList, clock) + secondArc := newAccessListMember(t, secondList.GetName(), thirdList.GetName(), accesslist.MembershipKindList, clock) + thirdArc := newAccessListMember(t, thirdList.GetName(), firstList.GetName(), accesslist.MembershipKindList, clock) + userMembership := newAccessListMember(t, thirdList.GetName(), user.GetName(), accesslist.MembershipKindUser, clock) + + aclGetter := &mockAccessListAndMembersGetter{ + accessLists: map[string]*accesslist.AccessList{ + firstList.GetName(): firstList, + secondList.GetName(): secondList, + thirdList.GetName(): thirdList, + }, + members: map[string][]*accesslist.AccessListMember{ + firstList.GetName(): {firstArc}, + secondList.GetName(): {secondArc}, + thirdList.GetName(): {thirdArc, userMembership}, + }, + } + + typ, err := IsAccessListMember(ctx, user, firstList, aclGetter, locks, clock) + require.NoError(t, err) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_INHERITED, typ) + typ, err = IsAccessListMember(ctx, user, secondList, aclGetter, locks, clock) + require.NoError(t, err) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_INHERITED, typ) + typ, err = IsAccessListMember(ctx, user, thirdList, aclGetter, locks, clock) + require.NoError(t, err) + require.Equal(t, accesslistv1.AccessListUserAssignmentType_ACCESS_LIST_USER_ASSIGNMENT_TYPE_EXPLICIT, typ) + }) } func TestGetOwners(t *testing.T) {