diff --git a/lib/services/local/access_list.go b/lib/services/local/access_list.go index dc311456cec05..11ca36474ad40 100644 --- a/lib/services/local/access_list.go +++ b/lib/services/local/access_list.go @@ -236,8 +236,6 @@ 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 { return trace.Wrap(err) @@ -537,7 +535,6 @@ 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) @@ -585,7 +582,6 @@ 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) @@ -685,7 +681,6 @@ 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 { @@ -773,11 +768,6 @@ 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) @@ -1089,49 +1079,3 @@ 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 isEligibilityStatusKnown(member.Spec.IneligibleStatus) { - // If the status was explicit set don't change it. - return - } - 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 isEligibilityStatusKnown(accessList.Spec.Owners[i].IneligibleStatus) { - // If the status was explicit set don't change it. - continue - } - // 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() - } -} - -// isEligibilityStatusKnown returns true if the status is known (not empty and not undefined) -func isEligibilityStatusKnown(status string) bool { - switch status { - case accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_UNSPECIFIED.String(), "": - return false - default: - return true - } -} diff --git a/lib/services/local/access_list_test.go b/lib/services/local/access_list_test.go index 6024592c12405..dc27882c26234 100644 --- a/lib/services/local/access_list_test.go +++ b/lib/services/local/access_list_test.go @@ -33,7 +33,6 @@ 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" @@ -1441,80 +1440,6 @@ 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) - - require.NoError(t, service.DeleteAllAccessLists(ctx)) - member := newAccessListMember(t, acl.GetName(), "member1", withExpire(time.Time{})) - member.Spec.IneligibleStatus = accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_USER_NOT_EXIST.String() - _, _, err = service.UpsertAccessListWithMembers(ctx, acl, []*accesslist.AccessListMember{member}) - require.NoError(t, err) - - m3, err := service.GetAccessListMember(ctx, acl.GetName(), "member1") - require.NoError(t, err) - require.Equal(t, accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_USER_NOT_EXIST.String(), m3.Spec.IneligibleStatus) -} - type newAccessListOptions struct { typ accesslist.Type owners []accesslist.Owner @@ -1536,18 +1461,6 @@ 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() @@ -1634,12 +1547,6 @@ 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()