Skip to content

Add nested AL tests + change IsAccessListMember behaviour #60271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hugoShaka merged 5 commits into from
Oct 22, 2025
Merged

Add nested AL tests + change IsAccessListMember behaviour #60271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1d42fa0
Add nested AL tests + change IsAccessListMember signature
hugoShaka Oct 15, 2025
41f526f
address marek's feedback
hugoShaka Oct 16, 2025
2916d1e
fixup! address marek's feedback
hugoShaka Oct 16, 2025
b4a5cfb
Apply suggestions from code review
hugoShaka Oct 16, 2025
c38c2d1
address pawel's feedback
hugoShaka Oct 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 34 additions & 20 deletions lib/accesslists/hierarchy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,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,
Expand Down Expand Up @@ -301,30 +302,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,
Expand All @@ -347,7 +352,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
Expand All @@ -371,14 +376,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) {
Expand All @@ -394,6 +404,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)
}

Expand Down
219 changes: 213 additions & 6 deletions lib/accesslists/hierarchy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package accesslists

import (
"context"
"slices"
"sort"
"testing"
"time"
Expand Down Expand Up @@ -147,7 +148,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))
Copy link
Copy Markdown
Contributor

@kopiczko kopiczko Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not verify those error messages?

Copy link
Copy Markdown
Contributor Author

@hugoShaka hugoShaka Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they will change with the visitor PR (it doesn't traverse the graph the same way and cannot produce the same errors). I want the tests to pass for both implementations, without changes

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)

Expand All @@ -162,7 +163,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.
Expand All @@ -183,8 +184,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)
}

Expand Down Expand Up @@ -229,7 +231,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)
}

Expand All @@ -252,8 +255,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"})
Expand All @@ -266,7 +270,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)
Copy link
Copy Markdown
Contributor

@kopiczko kopiczko Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it wouldn't hurt to check IsAccessListMember for all lists in the cycle. Same for the test above.

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) {
Expand Down
Loading