diff --git a/lib/services/local/access_list.go b/lib/services/local/access_list.go index 9fdd72fd2df74..446ecea6b86b0 100644 --- a/lib/services/local/access_list.go +++ b/lib/services/local/access_list.go @@ -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 { @@ -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) @@ -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) @@ -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 { @@ -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) @@ -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() + } +} diff --git a/lib/services/local/access_list_test.go b/lib/services/local/access_list_test.go index ffb29ff39b22d..feb247bba6895 100644 --- a/lib/services/local/access_list_test.go +++ b/lib/services/local/access_list_test.go @@ -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" @@ -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) @@ -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() @@ -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) @@ -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{ @@ -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) @@ -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) } @@ -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,