Skip to content

[v17] Align Access List Owners/Members Eligibility status. #58796

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

Open
smallinsky wants to merge 1 commit into
base: branch/v17
Choose a base branch
from
Open

[v17] Align Access List Owners/Members Eligibility status. #58796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 37 additions & 0 deletions lib/services/local/access_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
return trace.Wrap(err)
}
preserveAccessListFields(existingAccessList, accessList)
setOwnersEligibility(accessList)

listMembers, err := a.memberService.WithPrefix(accessList.GetName()).GetResources(ctx)
if err != nil {
Expand Down Expand Up @@ -545,6 +546,7 @@ func (a *AccessListService) UpsertAccessListMember(ctx context.Context, member *
return trace.Wrap(err)
}
keepAWSIdentityCenterLabels(existingMember, member)
setMemberEligibility(memberList, member)

if err := accesslists.ValidateAccessListMember(ctx, memberList, member, &accessListAndMembersGetter{a.service, a.memberService}); err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -584,6 +586,7 @@ func (a *AccessListService) UpdateAccessListMember(ctx context.Context, member *
return trace.Wrap(err)
}
keepAWSIdentityCenterLabels(existingMember, member)
setMemberEligibility(memberList, member)

if err := accesslists.ValidateAccessListMember(ctx, memberList, member, &accessListAndMembersGetter{a.service, a.memberService}); err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -673,6 +676,7 @@ func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, acc
if err := accessList.CheckAndSetDefaults(); err != nil {
return nil, nil, trace.Wrap(err)
}
setOwnersEligibility(accessList)

for _, m := range membersIn {
if err := m.CheckAndSetDefaults(); err != nil {
Expand Down Expand Up @@ -760,6 +764,11 @@ func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, acc

// Add any remaining members to the access list.
for _, member := range membersMap {
// Set Eligibility status for new members if this is possible
// to avoid updating the user object by access list eligibility reconciler
// and save backend operations.
setMemberEligibility(accessList, member)

upserted, err := a.memberService.WithPrefix(accessList.GetName()).UpsertResource(ctx, member)
if err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -1058,3 +1067,31 @@ func keepAWSIdentityCenterLabels(old, new *accesslist.AccessListMember) {
new.Metadata.Labels = old.GetAllLabels()
}
}

// setMemberEligibility sets the member eligibility status to eligible if possible to avoid
// unnecessary updates via access list eligibility reconciler.
func setMemberEligibility(acl *accesslist.AccessList, member *accesslist.AccessListMember) {
if !member.Spec.Expires.IsZero() || !acl.Spec.MembershipRequires.IsEmpty() {
// If the member has an expiration date or the Access List Requirements are not empty
// we cant assume the eligibility status. That needs to be calculated
// by the eligibility reconsider based on the user object.
return
}
member.Spec.IneligibleStatus = accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_ELIGIBLE.String()
}

// setOwnersEligibility sets the owners eligibility status to eligible if possible to avoid
// unnecessary updated via access list eligibility reconsider.
func setOwnersEligibility(accessList *accesslist.AccessList) {
if !accessList.Spec.OwnershipRequires.IsEmpty() {
// Owners eligibility needs to be calculated based on the user object.
// This is done by the eligibility reconsider.
return
}

for i := range accessList.Spec.Owners {
// If the ownership requirements are empty, all owners are eligible.
// There is no owner ineligibility expiration date
accessList.Spec.Owners[i].IneligibleStatus = accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_ELIGIBLE.String()
}
}
137 changes: 114 additions & 23 deletions lib/services/local/access_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"github.com/jonboulle/clockwork"
"github.com/stretchr/testify/require"

accesslistv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/accesslist/v1"
"github.com/gravitational/teleport/api/types/accesslist"
"github.com/gravitational/teleport/api/types/common"
"github.com/gravitational/teleport/api/types/header"
Expand Down Expand Up @@ -1019,9 +1020,76 @@ func TestAccessListRequiresEqual(t *testing.T) {
}
}

func TestAccessListMemberOwnerEligibility(t *testing.T) {
clock := clockwork.NewFakeClock()
ctx := context.Background()

mem, err := memory.New(memory.Config{
Context: ctx,
Clock: clock,
})
require.NoError(t, err)

service := newAccessListService(t, mem, clock, true /* igsEnabled */)

acl := newAccessList(t, "test-access-list-1", clock,
withOwners([]accesslist.Owner{{Name: "test-owner-1"}}),
withOwnerRequires(accesslist.Requires{}),
withMemberRequires(accesslist.Requires{}),
)

aclR := newAccessList(t, "test-access-list-2", clock,
withOwners([]accesslist.Owner{{Name: "test-owner-1"}}),
withOwnerRequires(accesslist.Requires{Roles: []string{"role1"}}),
withMemberRequires(accesslist.Requires{Roles: []string{"role2"}}),
)

_, err = service.UpsertAccessList(ctx, acl)
require.NoError(t, err)
item, err := service.GetAccessList(ctx, acl.GetName())
require.NoError(t, err)
require.Equal(t, accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_ELIGIBLE.String(), item.GetOwners()[0].IneligibleStatus)

_, err = service.UpsertAccessList(ctx, aclR)
require.NoError(t, err)
item2, err := service.GetAccessList(ctx, aclR.GetName())
require.NoError(t, err)
require.Empty(t, item2.GetOwners()[0].IneligibleStatus)

_, err = service.UpsertAccessListMember(ctx, newAccessListMember(t, acl.GetName(), "member1", withExpire(time.Time{})))
require.NoError(t, err)
m, err := service.GetAccessListMember(ctx, acl.GetName(), "member1")
require.NoError(t, err)
require.Equal(t, accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_ELIGIBLE.String(), m.Spec.IneligibleStatus)

_, err = service.UpsertAccessListMember(ctx, newAccessListMember(t, aclR.GetName(), "member1"))
require.NoError(t, err)
m, err = service.GetAccessListMember(ctx, aclR.GetName(), "member1")
require.NoError(t, err)
require.Empty(t, m.Spec.IneligibleStatus)

require.NoError(t, service.DeleteAllAccessLists(ctx))

_, _, err = service.UpsertAccessListWithMembers(ctx, acl, []*accesslist.AccessListMember{
newAccessListMember(t, acl.GetName(), "member1", withExpire(time.Time{})),
newAccessListMember(t, acl.GetName(), "member2", withExpire(time.Now().Add(time.Hour))),
})
require.NoError(t, err)
m1, err := service.GetAccessListMember(ctx, acl.GetName(), "member1")
require.NoError(t, err)
require.Equal(t, accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_ELIGIBLE.String(), m1.Spec.IneligibleStatus)

m2, err := service.GetAccessListMember(ctx, acl.GetName(), "member2")
require.NoError(t, err)
require.Empty(t, m2.Spec.IneligibleStatus)

}

type newAccessListOptions struct {
typ accesslist.Type
owners []accesslist.Owner
typ accesslist.Type
owners []accesslist.Owner
ownerRequires accesslist.Requires
memberRequires accesslist.Requires
}

type newAccessListOpt func(*newAccessListOptions)
Expand All @@ -1038,6 +1106,18 @@ func withOwners(owners []accesslist.Owner) newAccessListOpt {
}
}

func withOwnerRequires(req accesslist.Requires) newAccessListOpt {
return func(o *newAccessListOptions) {
o.ownerRequires = req
}
}

func withMemberRequires(req accesslist.Requires) newAccessListOpt {
return func(o *newAccessListOptions) {
o.memberRequires = req
}
}

func newAccessList(t *testing.T, name string, clock clockwork.Clock, opts ...newAccessListOpt) *accesslist.AccessList {
t.Helper()

Expand All @@ -1052,6 +1132,20 @@ func newAccessList(t *testing.T, name string, clock clockwork.Clock, opts ...new
Description: "test user 2",
},
},
memberRequires: accesslist.Requires{
Roles: []string{"mrole1", "mrole2"},
Traits: map[string][]string{
"mtrait1": {"mvalue1", "mvalue2"},
"mtrait2": {"mvalue3", "mvalue4"},
},
},
ownerRequires: accesslist.Requires{
Roles: []string{"orole1", "orole2"},
Traits: map[string][]string{
"otrait1": {"ovalue1", "ovalue2"},
"otrait2": {"ovalue3", "ovalue4"},
},
},
}
for _, o := range opts {
o(&options)
Expand All @@ -1067,25 +1161,13 @@ func newAccessList(t *testing.T, name string, clock clockwork.Clock, opts ...new
Name: name,
},
accesslist.Spec{
Type: options.typ,
Title: name + " title",
Description: "test access list",
Owners: options.owners,
Audit: audit,
MembershipRequires: accesslist.Requires{
Roles: []string{"mrole1", "mrole2"},
Traits: map[string][]string{
"mtrait1": {"mvalue1", "mvalue2"},
"mtrait2": {"mvalue3", "mvalue4"},
},
},
OwnershipRequires: accesslist.Requires{
Roles: []string{"orole1", "orole2"},
Traits: map[string][]string{
"otrait1": {"ovalue1", "ovalue2"},
"otrait2": {"ovalue3", "ovalue4"},
},
},
Type: options.typ,
Title: name + " title",
Description: "test access list",
Owners: options.owners,
Audit: audit,
MembershipRequires: options.memberRequires,
OwnershipRequires: options.ownerRequires,
Grants: accesslist.Grants{
Roles: []string{"grole1", "grole2"},
Traits: map[string][]string{
Expand All @@ -1111,6 +1193,7 @@ func createAccessList(t *testing.T, service *AccessListService, name string, clo

type accessListMemberOptions struct {
membershipKind string
expires time.Time
}

type accessListMemberOpt func(*accessListMemberOptions)
Expand All @@ -1121,10 +1204,18 @@ func withMembershipKind(membershipKind string) accessListMemberOpt {
}
}

func withExpire(t time.Time) accessListMemberOpt {
return func(o *accessListMemberOptions) {
o.expires = t
}
}

func newAccessListMember(t *testing.T, accessList, name string, opts ...accessListMemberOpt) *accesslist.AccessListMember {
t.Helper()

options := accessListMemberOptions{}
options := accessListMemberOptions{
expires: time.Now().Add(time.Hour * 24),
}
for _, o := range opts {
o(&options)
}
Expand All @@ -1137,7 +1228,7 @@ func newAccessListMember(t *testing.T, accessList, name string, opts ...accessLi
AccessList: accessList,
Name: name,
Joined: time.Now(),
Expires: time.Now().Add(time.Hour * 24),
Expires: options.expires,
Reason: "a reason",
AddedBy: "dummy",
MembershipKind: options.membershipKind,
Expand Down
Loading