diff --git a/api/types/accesslist/accesslist.go b/api/types/accesslist/accesslist.go index acbcef98a58fa..b9f42f553e1e4 100644 --- a/api/types/accesslist/accesslist.go +++ b/api/types/accesslist/accesslist.go @@ -112,28 +112,6 @@ func parseReviewDayOfMonth(input string) ReviewDayOfMonth { return 0 } -// Inclusion values indicate how membership and ownership of an AccessList -// should be applied. -type Inclusion string - -const ( - // InclusionUnspecified is the default, un-set inclusion value used to - // detect when inclusion is not specified in an access list. The only times - // you should encounter this value in practice is when un-marshaling an - // AccessList that pre-dates the implementation of dynamic access lists. - InclusionUnspecified Inclusion = "" - - // InclusionImplicit indicates that a user need only meet a requirement set - // to be considered included in a list. Both list membership and ownership - // may be Implicit. - InclusionImplicit Inclusion = "implicit" - - // InclusionExplicit indicates that a user must meet a requirement set AND - // be explicitly added to an access list to be included in it. Both list - // membership and ownership may be Explicit. - InclusionExplicit Inclusion = "explicit" -) - // AccessList describes the basic building block of access grants, which are // similar to access requests but for longer lived permissions that need to be // regularly audited. @@ -162,29 +140,11 @@ type Spec struct { // Audit describes the frequency that this access list must be audited. Audit Audit `json:"audit" yaml:"audit"` - // Membership defines how list ownership of this list is determined. There - // are two possible values: - // Explicit: To be considered an member of the access list, a user must - // both meet the `membership_conditions` AND be explicitly added - // to the list. - // Implicit: Any user meeting the `membership_conditions` will automatically - // be considered an owner of this list. - Membership Inclusion `json:"membership" yaml:"membership"` - // MembershipRequires describes the requirements for a user to be a member of the access list. // For a membership to an access list to be effective, the user must meet the requirements of // MembershipRequires and must be in the members list. MembershipRequires Requires `json:"membership_requires" yaml:"membership_requires"` - // Ownership defines how list ownership of this list is determined. There - // are two possible values: - // Explicit: To be considered an owner of the access list, a user must - // both meet the `ownership_conditions` AND be explicitly added - // to the list. - // Implicit: Any user meeting the `ownership_conditions` will automatically - // be considered an owner of this list. - Ownership Inclusion `json:"ownership" yaml:"ownership"` - // OwnershipRequires describes the requirements for a user to be an owner of the access list. // For ownership of an access list to be effective, the user must meet the requirements of // OwnershipRequires and must be in the owners list. @@ -281,23 +241,6 @@ func NewAccessList(metadata header.Metadata, spec Spec) (*AccessList, error) { return accessList, nil } -// checkInclusion validates an Inclusion value, defaulting to "Explicit" if not -// set. Any other invalid value is an error. -func checkInclusion(i Inclusion) (Inclusion, error) { - switch i { - case InclusionUnspecified: - return InclusionExplicit, nil - - case InclusionExplicit, InclusionImplicit: - return i, nil - - default: - return InclusionUnspecified, - trace.BadParameter("invalid inclusion mode %s (must be %s or %s)", - i, InclusionExplicit, InclusionImplicit) - } -} - // CheckAndSetDefaults validates fields and populates empty fields with default values. func (a *AccessList) CheckAndSetDefaults() error { a.SetKind(types.KindAccessList) @@ -311,16 +254,7 @@ func (a *AccessList) CheckAndSetDefaults() error { return trace.BadParameter("access list title required") } - var err error - if a.Spec.Ownership, err = checkInclusion(a.Spec.Ownership); err != nil { - return trace.Wrap(err, "ownership") - } - - if a.Spec.Membership, err = checkInclusion(a.Spec.Membership); err != nil { - return trace.Wrap(err, "membership") - } - - if a.Spec.Ownership != InclusionImplicit && len(a.Spec.Owners) == 0 { + if len(a.Spec.Owners) == 0 { return trace.BadParameter("owners are missing") } @@ -423,30 +357,6 @@ func (a *AccessList) CloneResource() types.ResourceWithLabels { return copy } -// HasImplicitOwnership returns true if the supplied AccessList uses -// implicit ownership -func (a *AccessList) HasImplicitOwnership() bool { - return a.Spec.Ownership == InclusionImplicit -} - -// HasExplicitOwnership returns true if the supplied AccessList uses -// explicit ownership -func (a *AccessList) HasExplicitOwnership() bool { - return a.Spec.Ownership == InclusionExplicit -} - -// HasImplicitMembership returns true if the supplied AccessList uses -// implicit membership -func (a *AccessList) HasImplicitMembership() bool { - return a.Spec.Membership == InclusionImplicit -} - -// HasExplicitMembership returns true if the supplied AccessList uses -// explicit membership -func (a *AccessList) HasExplicitMembership() bool { - return a.Spec.Membership == InclusionExplicit -} - func (a *Audit) UnmarshalJSON(data []byte) error { type Alias Audit audit := struct { diff --git a/api/types/accesslist/accesslist_test.go b/api/types/accesslist/accesslist_test.go index 6a902754d031a..765f398c77c89 100644 --- a/api/types/accesslist/accesslist_test.go +++ b/api/types/accesslist/accesslist_test.go @@ -262,60 +262,14 @@ func TestAccessListDefaults(t *testing.T) { } } - t.Run("ownership defaults to explicit", func(t *testing.T) { + t.Run("owners are required", func(t *testing.T) { uut := newValidAccessList() - uut.Spec.Ownership = InclusionUnspecified - - err := uut.CheckAndSetDefaults() - require.NoError(t, err) - require.Equal(t, InclusionExplicit, uut.Spec.Ownership) - }) - - t.Run("invalid ownership is an error", func(t *testing.T) { - uut := newValidAccessList() - uut.Spec.Ownership = Inclusion("potato") - - err := uut.CheckAndSetDefaults() - require.Error(t, err) - require.Contains(t, err.Error(), "ownership") - }) - - t.Run("membership defaults to explicit", func(t *testing.T) { - uut := newValidAccessList() - uut.Spec.Membership = InclusionUnspecified - - err := uut.CheckAndSetDefaults() - require.NoError(t, err) - require.Equal(t, InclusionExplicit, uut.Spec.Membership) - }) - - t.Run("invalid membership is an error", func(t *testing.T) { - uut := newValidAccessList() - uut.Spec.Membership = Inclusion("banana") - - err := uut.CheckAndSetDefaults() - require.Error(t, err) - require.Contains(t, err.Error(), "membership") - }) - - t.Run("owners are required for explicit owner lists", func(t *testing.T) { - uut := newValidAccessList() - uut.Spec.Ownership = InclusionExplicit uut.Spec.Owners = []Owner{} err := uut.CheckAndSetDefaults() require.Error(t, err) require.Contains(t, err.Error(), "owners") }) - - t.Run("owners are not required for implicit owner lists", func(t *testing.T) { - uut := newValidAccessList() - uut.Spec.Ownership = InclusionImplicit - uut.Spec.Owners = []Owner{} - - err := uut.CheckAndSetDefaults() - require.NoError(t, err) - }) } func TestSelectNextReviewDate(t *testing.T) { diff --git a/api/types/accesslist/convert/v1/accesslist.go b/api/types/accesslist/convert/v1/accesslist.go index 1220ba415fbf6..fb0d00c8a090f 100644 --- a/api/types/accesslist/convert/v1/accesslist.go +++ b/api/types/accesslist/convert/v1/accesslist.go @@ -108,12 +108,10 @@ func FromProto(msg *accesslistv1.AccessList, opts ...AccessListOption) (*accessl Recurrence: recurrence, Notifications: notifications, }, - Membership: accesslist.Inclusion(msg.Spec.Membership), MembershipRequires: accesslist.Requires{ Roles: msg.Spec.MembershipRequires.Roles, Traits: traitv1.FromProto(msg.Spec.MembershipRequires.Traits), }, - Ownership: accesslist.Inclusion(msg.Spec.Ownership), OwnershipRequires: accesslist.Requires{ Roles: msg.Spec.OwnershipRequires.Roles, Traits: traitv1.FromProto(msg.Spec.OwnershipRequires.Traits), @@ -185,8 +183,6 @@ func ToProto(accessList *accesslist.AccessList) *accesslistv1.AccessList { Spec: &accesslistv1.AccessListSpec{ Title: accessList.Spec.Title, Description: accessList.Spec.Description, - Ownership: string(accessList.Spec.Ownership), - Membership: string(accessList.Spec.Membership), Owners: owners, Audit: &accesslistv1.AccessListAudit{ NextAuditDate: nextAuditDate, diff --git a/api/types/accesslist/convert/v1/accesslist_test.go b/api/types/accesslist/convert/v1/accesslist_test.go index 4639ba74126b6..ae278ab251130 100644 --- a/api/types/accesslist/convert/v1/accesslist_test.go +++ b/api/types/accesslist/convert/v1/accesslist_test.go @@ -159,24 +159,6 @@ func TestFromProtoNils(t *testing.T) { require.Error(t, err) }) - t.Run("membership", func(t *testing.T) { - msg := ToProto(newAccessList(t, "access-list")) - msg.Spec.Membership = "" - - uut, err := FromProto(msg) - require.NoError(t, err) - require.Equal(t, accesslist.InclusionExplicit, uut.Spec.Membership) - }) - - t.Run("ownership", func(t *testing.T) { - msg := ToProto(newAccessList(t, "access-list")) - msg.Spec.Ownership = "" - - uut, err := FromProto(msg) - require.NoError(t, err) - require.Equal(t, accesslist.InclusionExplicit, uut.Spec.Ownership) - }) - t.Run("owner_grants", func(t *testing.T) { msg := ToProto(newAccessList(t, "access-list")) msg.Spec.OwnerGrants = nil diff --git a/api/types/accesslist/convert/v1/member.go b/api/types/accesslist/convert/v1/member.go index 94d4bd063658b..f624287dbb17e 100644 --- a/api/types/accesslist/convert/v1/member.go +++ b/api/types/accesslist/convert/v1/member.go @@ -44,7 +44,6 @@ func FromMemberProto(msg *accesslistv1.Member, opts ...MemberOption) (*accesslis Expires: msg.Spec.Expires.AsTime(), Reason: msg.Spec.Reason, AddedBy: msg.Spec.AddedBy, - Membership: accesslist.Inclusion(msg.Spec.Membership), // Set it to empty as default. // Must provide as options to set it with the provided value. IneligibleStatus: "", @@ -89,7 +88,6 @@ func ToMemberProto(member *accesslist.AccessListMember) *accesslistv1.Member { Expires: timestamppb.New(member.Spec.Expires), Reason: member.Spec.Reason, AddedBy: member.Spec.AddedBy, - Membership: string(member.Spec.Membership), IneligibleStatus: ineligibleStatus, }, } diff --git a/api/types/accesslist/convert/v1/member_test.go b/api/types/accesslist/convert/v1/member_test.go index bd6930e6f9776..de44fb67be7c0 100644 --- a/api/types/accesslist/convert/v1/member_test.go +++ b/api/types/accesslist/convert/v1/member_test.go @@ -93,14 +93,6 @@ func TestMemberFromProtoNils(t *testing.T) { mutate: func(m *accesslistv1.Member) { m.Spec.AddedBy = "" }, checkErr: require.Error, }, - { - name: "membership", - mutate: func(m *accesslistv1.Member) { m.Spec.Membership = "" }, - checkErr: require.NoError, - checkVal: func(t *testing.T, m *accesslist.AccessListMember) { - require.Equal(t, accesslist.InclusionExplicit, m.Spec.Membership) - }, - }, } for _, tt := range testCases { diff --git a/api/types/accesslist/member.go b/api/types/accesslist/member.go index 7bf129db5d7f0..c2ce3f7c8793a 100644 --- a/api/types/accesslist/member.go +++ b/api/types/accesslist/member.go @@ -57,11 +57,6 @@ type AccessListMemberSpec struct { // IneligibleStatus describes the reason why this member is not eligible. IneligibleStatus string `json:"ineligible_status" yaml:"ineligible_status"` - - // Membership indicates how the user the user acquired membership; either - // `Explicit` (i.e. being explicitly added to a list) or `Implicit` (by - // matching the requirements of an AccessList with Implicit membership. - Membership Inclusion `json:"membership" yaml:"membership"` } // NewAccessListMember will create a new access listm member. @@ -95,19 +90,12 @@ func (a *AccessListMember) CheckAndSetDefaults() error { return trace.BadParameter("member name is missing") } - var err error - if a.Spec.Membership, err = checkInclusion(a.Spec.Membership); err != nil { - return trace.Wrap(err, "membership") + if a.Spec.Joined.IsZero() || a.Spec.Joined.Unix() == 0 { + return trace.BadParameter("member %s: joined field empty or missing", a.Spec.Name) } - if a.Spec.Membership == InclusionExplicit { - if a.Spec.Joined.IsZero() || a.Spec.Joined.Unix() == 0 { - return trace.BadParameter("member %s: joined field empty or missing", a.Spec.Name) - } - - if a.Spec.AddedBy == "" { - return trace.BadParameter("member %s: added_by field is empty", a.Spec.Name) - } + if a.Spec.AddedBy == "" { + return trace.BadParameter("member %s: added_by field is empty", a.Spec.Name) } return nil diff --git a/api/types/accesslist/member_test.go b/api/types/accesslist/member_test.go index 150e440c45e52..e9bc6d8c6dc1b 100644 --- a/api/types/accesslist/member_test.go +++ b/api/types/accesslist/member_test.go @@ -44,65 +44,25 @@ func TestAccessListMemberDefaults(t *testing.T) { Spec: AccessListMemberSpec{ AccessList: accesslist, Name: user, - Membership: InclusionExplicit, Joined: time.Date(1969, time.July, 20, 20, 17, 40, 0, time.UTC), AddedBy: "some-other-user", }, } } - t.Run("membership defaults to explicit", func(t *testing.T) { + t.Run("join date required for member", func(t *testing.T) { uut := newValidAccessListMember() - uut.Spec.Membership = InclusionUnspecified - - err := uut.CheckAndSetDefaults() - require.NoError(t, err) - require.Equal(t, InclusionExplicit, uut.Spec.Membership) - }) - - t.Run("bad membership value is an error", func(t *testing.T) { - uut := newValidAccessListMember() - uut.Spec.Membership = Inclusion("nonsense") - - err := uut.CheckAndSetDefaults() - require.Error(t, err) - }) - - t.Run("join date required for explicit member", func(t *testing.T) { - uut := newValidAccessListMember() - uut.Spec.Membership = InclusionExplicit uut.Spec.Joined = time.Time{} err := uut.CheckAndSetDefaults() require.Error(t, err) }) - t.Run("join date not required for implicit member", func(t *testing.T) { + t.Run("added-by required", func(t *testing.T) { uut := newValidAccessListMember() - uut.Spec.Membership = InclusionImplicit - uut.Spec.Joined = time.Time{} - - err := uut.CheckAndSetDefaults() - require.NoError(t, err) - require.Equal(t, time.Time{}, uut.Spec.Joined) - }) - - t.Run("added-by required for explicit member", func(t *testing.T) { - uut := newValidAccessListMember() - uut.Spec.Membership = InclusionExplicit uut.Spec.AddedBy = "" err := uut.CheckAndSetDefaults() require.Error(t, err) }) - - t.Run("added-by not required for implicit member", func(t *testing.T) { - uut := newValidAccessListMember() - uut.Spec.Membership = InclusionImplicit - uut.Spec.AddedBy = "" - - err := uut.CheckAndSetDefaults() - require.NoError(t, err) - require.Empty(t, uut.Spec.AddedBy) - }) } diff --git a/lib/services/access_list.go b/lib/services/access_list.go index b872e158eb477..8691568c6041c 100644 --- a/lib/services/access_list.go +++ b/lib/services/access_list.go @@ -18,6 +18,7 @@ package services import ( "context" + "slices" "time" "github.com/gravitational/trace" @@ -224,22 +225,15 @@ func IsAccessListOwner(identity tlsca.Identity, accessList *accesslist.AccessLis // An opaque access denied error. accessDenied := trace.AccessDenied("access denied") - if accessList.HasExplicitOwnership() { - isOwner := false - - for _, owner := range accessList.GetOwners() { - if owner.Name == identity.Username { - isOwner = true - break - } - } - - // User is not an owner, so we'll access denied. - if !isOwner { - return accessDenied - } + // Is the supplied identity in the owners list? + ownerIdx := slices.IndexFunc(accessList.GetOwners(), func(owner accesslist.Owner) bool { + return owner.Name == identity.Username + }) + if ownerIdx == -1 { + return accessDenied } + // Does the supplied Identity meet the ownership requirements? if !UserMeetsRequirements(identity, accessList.Spec.OwnershipRequires) { return accessDenied } @@ -283,20 +277,18 @@ func (a AccessListMembershipChecker) IsAccessListMember(ctx context.Context, ide } } - if accessList.HasExplicitMembership() { - member, err := a.members.GetAccessListMember(ctx, accessList.GetName(), username) - if trace.IsNotFound(err) { - // The member has not been found, so we know they're not a member of this list. - return trace.NotFound("user %s is not a member of the access list", username) - } else if err != nil { - // Some other error has occurred - return trace.Wrap(err) - } + member, err := a.members.GetAccessListMember(ctx, accessList.GetName(), username) + if trace.IsNotFound(err) { + // The member has not been found, so we know they're not a member of this list. + return trace.NotFound("user %s is not a member of the access list", username) + } else if err != nil { + // Some other error has occurred + return trace.Wrap(err) + } - expires := member.Spec.Expires - if !expires.IsZero() && !a.clock.Now().Before(expires) { - return trace.AccessDenied("user %s's membership has expired in the access list", username) - } + expires := member.Spec.Expires + if !expires.IsZero() && !a.clock.Now().Before(expires) { + return trace.AccessDenied("user %s's membership has expired in the access list", username) } if !UserMeetsRequirements(identity, accessList.Spec.MembershipRequires) { diff --git a/lib/services/access_list_test.go b/lib/services/access_list_test.go index 23f14e1a2d6c1..93f6f122a6cce 100644 --- a/lib/services/access_list_test.go +++ b/lib/services/access_list_test.go @@ -103,7 +103,6 @@ func TestAccessListMarshal(t *testing.T) { accesslist.Spec{ Title: "title", Description: "test access list", - Ownership: accesslist.InclusionExplicit, Owners: []accesslist.Owner{ { Name: "test-user1", @@ -117,7 +116,6 @@ func TestAccessListMarshal(t *testing.T) { Audit: accesslist.Audit{ NextAuditDate: time.Date(2023, 02, 02, 0, 0, 0, 0, time.UTC), }, - Membership: accesslist.InclusionImplicit, MembershipRequires: accesslist.Requires{ Roles: []string{"mrole1", "mrole2"}, Traits: map[string][]string{ @@ -164,7 +162,6 @@ func TestAccessListMemberUnmarshal(t *testing.T) { Expires: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), Reason: "because", AddedBy: "test-user1", - Membership: accesslist.InclusionExplicit, }, ) require.NoError(t, err) @@ -202,7 +199,6 @@ func TestIsAccessListOwner(t *testing.T) { tests := []struct { name string identity tlsca.Identity - ownership accesslist.Inclusion errAssertionFunc require.ErrorAssertionFunc }{ { @@ -215,7 +211,6 @@ func TestIsAccessListOwner(t *testing.T) { "otrait2": {"ovalue3", "ovalue4"}, }, }, - ownership: accesslist.InclusionExplicit, errAssertionFunc: require.NoError, }, { @@ -228,7 +223,6 @@ func TestIsAccessListOwner(t *testing.T) { "otrait2": {"ovalue3", "ovalue4"}, }, }, - ownership: accesslist.InclusionExplicit, errAssertionFunc: requireAccessDenied, }, { @@ -241,7 +235,6 @@ func TestIsAccessListOwner(t *testing.T) { "otrait2": {"ovalue3", "ovalue4"}, }, }, - ownership: accesslist.InclusionExplicit, errAssertionFunc: requireAccessDenied, }, { @@ -254,45 +247,6 @@ func TestIsAccessListOwner(t *testing.T) { "otrait2": {"ovalue3"}, }, }, - ownership: accesslist.InclusionExplicit, - errAssertionFunc: requireAccessDenied, - }, - { - name: "is implicit owner", - identity: tlsca.Identity{ - Username: "not-specified-owner", - Groups: []string{"orole1", "orole2"}, - Traits: map[string][]string{ - "otrait1": {"ovalue1", "ovalue2"}, - "otrait2": {"ovalue3", "ovalue4"}, - }, - }, - ownership: accesslist.InclusionImplicit, - errAssertionFunc: require.NoError, - }, - { - name: "implicit owner with missing roles", - identity: tlsca.Identity{ - Username: "not-specified-owner", - Groups: []string{"orole1"}, - Traits: map[string][]string{ - "otrait1": {"ovalue1", "ovalue2"}, - "otrait2": {"ovalue3", "ovalue4"}, - }, - }, - ownership: accesslist.InclusionImplicit, - errAssertionFunc: requireAccessDenied, - }, - { - name: "implicit owner with missing traits", - identity: tlsca.Identity{ - Username: "not-specified-owner", - Groups: []string{"orole1", "orole2"}, - Traits: map[string][]string{ - "otrait1": {"ovalue1", "ovalue2"}, - }, - }, - ownership: accesslist.InclusionImplicit, errAssertionFunc: requireAccessDenied, }, } @@ -303,7 +257,6 @@ func TestIsAccessListOwner(t *testing.T) { t.Parallel() accessList := newAccessList(t) - accessList.Spec.Ownership = test.ownership test.errAssertionFunc(t, IsAccessListOwner(test.identity, accessList)) }) @@ -384,7 +337,6 @@ func TestIsAccessListMemberChecker(t *testing.T) { name string identity tlsca.Identity memberCtx context.Context - membership accesslist.Inclusion currentTime time.Time locks map[string]types.Lock errAssertionFunc require.ErrorAssertionFunc @@ -399,21 +351,6 @@ func TestIsAccessListMemberChecker(t *testing.T) { "mtrait2": {"mvalue3", "mvalue4"}, }, }, - membership: accesslist.InclusionExplicit, - currentTime: time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC), - errAssertionFunc: require.NoError, - }, - { - name: "is member (dynamic)", - identity: tlsca.Identity{ - Username: member1, - Groups: []string{"mrole1", "mrole2"}, - Traits: map[string][]string{ - "mtrait1": {"mvalue1", "mvalue2"}, - "mtrait2": {"mvalue3", "mvalue4"}, - }, - }, - membership: accesslist.InclusionImplicit, currentTime: time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC), errAssertionFunc: require.NoError, }, @@ -427,7 +364,6 @@ func TestIsAccessListMemberChecker(t *testing.T) { "mtrait2": {"mvalue3", "mvalue4"}, }, }, - membership: accesslist.InclusionExplicit, locks: map[string]types.Lock{ "test-lock": newUserLock(t, "test-lock", member1), }, @@ -446,25 +382,11 @@ func TestIsAccessListMemberChecker(t *testing.T) { "mtrait2": {"mvalue3", "mvalue4"}, }, }, - membership: accesslist.InclusionExplicit, currentTime: time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC), errAssertionFunc: func(t require.TestingT, err error, i ...interface{}) { require.True(t, trace.IsNotFound(err)) }, }, - { - name: "is not a member (dynamic)", - identity: tlsca.Identity{ - Username: member4, - Groups: []string{"nonmatching-role"}, - Traits: map[string][]string{ - "nonmatching-trait": {"mvalue1", "mvalue2"}, - }, - }, - membership: accesslist.InclusionImplicit, - currentTime: time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC), - errAssertionFunc: requireAccessDenied, - }, { name: "is expired member", identity: tlsca.Identity{ @@ -475,7 +397,6 @@ func TestIsAccessListMemberChecker(t *testing.T) { "mtrait2": {"mvalue3", "mvalue4"}, }, }, - membership: accesslist.InclusionExplicit, currentTime: time.Date(2026, 7, 1, 0, 0, 0, 0, time.UTC), errAssertionFunc: requireAccessDenied, }, @@ -489,7 +410,6 @@ func TestIsAccessListMemberChecker(t *testing.T) { "mtrait2": {"mvalue3", "mvalue4"}, }, }, - membership: accesslist.InclusionExplicit, currentTime: time.Date(2030, 7, 1, 0, 0, 0, 0, time.UTC), errAssertionFunc: require.NoError, }, @@ -503,7 +423,6 @@ func TestIsAccessListMemberChecker(t *testing.T) { "mtrait2": {"mvalue3", "mvalue4"}, }, }, - membership: accesslist.InclusionExplicit, currentTime: time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC), errAssertionFunc: requireAccessDenied, }, @@ -517,7 +436,6 @@ func TestIsAccessListMemberChecker(t *testing.T) { "mtrait2": {"mvalue3", "mvalue4"}, }, }, - membership: accesslist.InclusionExplicit, currentTime: time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC), errAssertionFunc: requireAccessDenied, }, @@ -531,7 +449,6 @@ func TestIsAccessListMemberChecker(t *testing.T) { "mtrait2": {"mvalue3"}, }, }, - membership: accesslist.InclusionExplicit, currentTime: time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC), errAssertionFunc: requireAccessDenied, }, @@ -545,7 +462,6 @@ func TestIsAccessListMemberChecker(t *testing.T) { "mtrait2": {"mvalue3"}, }, }, - membership: accesslist.InclusionExplicit, currentTime: time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC), errAssertionFunc: requireAccessDenied, }, @@ -558,12 +474,8 @@ func TestIsAccessListMemberChecker(t *testing.T) { ctx := context.Background() - accessList := newAccessListWithMembership(t, test.membership) - - members := []*accesslist.AccessListMember{} - if test.membership == accesslist.InclusionExplicit { - members = newAccessListMembers(t) - } + accessList := newAccessList(t) + members := newAccessListMembers(t) memberMap := map[string]map[string]*accesslist.AccessListMember{} for _, member := range members { @@ -682,20 +594,7 @@ func TestAccessListReviewMarshal(t *testing.T) { require.Equal(t, expected, actual) } -// newAccessListWithMembership creates an access list for testing with the -// following requirements -// -// Membership: -// -// Roles: mrole1, mrole2, -// Traits: -// mtrait1: "mvalue1", "mvalue2" -// mtrait2: "mvalue3", "mvalue4" -// -// Ownership: -// -// nil -func newAccessListWithMembership(t *testing.T, membership accesslist.Inclusion) *accesslist.AccessList { +func newAccessList(t *testing.T) *accesslist.AccessList { t.Helper() accessList, err := accesslist.NewAccessList( @@ -722,7 +621,6 @@ func newAccessListWithMembership(t *testing.T, membership accesslist.Inclusion) DayOfMonth: accesslist.FifteenthDayOfMonth, }, }, - Membership: membership, MembershipRequires: accesslist.Requires{ Roles: []string{"mrole1", "mrole2"}, Traits: map[string][]string{ @@ -751,11 +649,6 @@ func newAccessListWithMembership(t *testing.T, membership accesslist.Inclusion) return accessList } -func newAccessList(t *testing.T) *accesslist.AccessList { - t.Helper() - return newAccessListWithMembership(t, accesslist.InclusionExplicit) -} - func newAccessListMembers(t *testing.T) []*accesslist.AccessListMember { t.Helper() diff --git a/lib/services/local/access_list.go b/lib/services/local/access_list.go index 30451a54c238e..34546e28fca76 100644 --- a/lib/services/local/access_list.go +++ b/lib/services/local/access_list.go @@ -145,40 +145,9 @@ func (a *AccessListService) GetAccessListsToReview(ctx context.Context) ([]*acce // UpsertAccessList creates or updates an access list resource. func (a *AccessListService) UpsertAccessList(ctx context.Context, accessList *accesslist.AccessList) (*accesslist.AccessList, error) { - if accessList.HasImplicitOwnership() { - if len(accessList.Spec.Owners) > 0 { - return nil, trace.BadParameter("implicit ownership requires empty ownership list") - } - - if accessList.Spec.OwnershipRequires.IsEmpty() { - return nil, trace.BadParameter("implicit ownership requires ownership requirements") - } - } - - if accessList.HasImplicitMembership() { - if accessList.Spec.MembershipRequires.IsEmpty() { - return nil, trace.BadParameter("implicit membership requires membership requirements") - } - } - var upserted *accesslist.AccessList upsertWithLockFn := func() error { return a.service.RunWhileLocked(ctx, lockName(accessList.GetName()), accessListLockTTL, func(ctx context.Context, _ backend.Backend) error { - oldAccessList, err := a.service.GetResource(ctx, accessList.GetName()) - if err != nil && !trace.IsNotFound(err) { - return trace.Wrap(err) - } - - if oldAccessList != nil { - if oldAccessList.Spec.Ownership != accessList.Spec.Ownership { - return trace.BadParameter("AccessList ownership cannot be changed") - } - - if oldAccessList.Spec.Membership != accessList.Spec.Membership { - return trace.BadParameter("AccessList membership cannot be changed") - } - } - ownerMap := make(map[string]struct{}, len(accessList.Spec.Owners)) for _, owner := range accessList.Spec.Owners { if _, ok := ownerMap[owner.Name]; ok { @@ -187,6 +156,7 @@ func (a *AccessListService) UpsertAccessList(ctx context.Context, accessList *ac ownerMap[owner.Name] = struct{}{} } + var err error upserted, err = a.service.UpsertResource(ctx, accessList) return trace.Wrap(err) }) @@ -258,19 +228,11 @@ func (a *AccessListService) CountAccessListMembers(ctx context.Context, accessLi // ListAccessListMembers returns a paginated list of all access list members. func (a *AccessListService) ListAccessListMembers(ctx context.Context, accessListName string, pageSize int, nextToken string) ([]*accesslist.AccessListMember, string, error) { var members []*accesslist.AccessListMember - var al *accesslist.AccessList err := a.service.RunWhileLocked(ctx, lockName(accessListName), accessListLockTTL, func(ctx context.Context, _ backend.Backend) error { - var err error - - al, err = a.service.GetResource(ctx, accessListName) + _, err := a.service.GetResource(ctx, accessListName) if err != nil { return trace.Wrap(err) } - - if al.HasImplicitMembership() { - return services.ImplicitAccessListError{} - } - members, nextToken, err = a.memberService.WithPrefix(accessListName).ListResources(ctx, pageSize, nextToken) return trace.Wrap(err) }) @@ -297,15 +259,10 @@ func (a *AccessListService) ListAllAccessListMembers(ctx context.Context, pageSi func (a *AccessListService) GetAccessListMember(ctx context.Context, accessList string, memberName string) (*accesslist.AccessListMember, error) { var member *accesslist.AccessListMember err := a.service.RunWhileLocked(ctx, lockName(accessList), accessListLockTTL, func(ctx context.Context, _ backend.Backend) error { - al, err := a.service.GetResource(ctx, accessList) + _, err := a.service.GetResource(ctx, accessList) if err != nil { return trace.Wrap(err) } - - if al.HasImplicitMembership() { - return services.ImplicitAccessListError{} - } - member, err = a.memberService.WithPrefix(accessList).GetResource(ctx, memberName) return trace.Wrap(err) }) @@ -316,15 +273,10 @@ func (a *AccessListService) GetAccessListMember(ctx context.Context, accessList func (a *AccessListService) UpsertAccessListMember(ctx context.Context, member *accesslist.AccessListMember) (*accesslist.AccessListMember, error) { var upserted *accesslist.AccessListMember err := a.service.RunWhileLocked(ctx, lockName(member.Spec.AccessList), accessListLockTTL, func(ctx context.Context, _ backend.Backend) error { - al, err := a.service.GetResource(ctx, member.Spec.AccessList) + _, err := a.service.GetResource(ctx, member.Spec.AccessList) if err != nil { return trace.Wrap(err) } - - if al.HasImplicitMembership() { - return services.ImplicitAccessListError{} - } - upserted, err = a.memberService.WithPrefix(member.Spec.AccessList).UpsertResource(ctx, member) return trace.Wrap(err) }) @@ -337,15 +289,10 @@ func (a *AccessListService) UpsertAccessListMember(ctx context.Context, member * // DeleteAccessListMember hard deletes the specified access list member resource. func (a *AccessListService) DeleteAccessListMember(ctx context.Context, accessList string, memberName string) error { err := a.service.RunWhileLocked(ctx, lockName(accessList), accessListLockTTL, func(ctx context.Context, _ backend.Backend) error { - al, err := a.service.GetResource(ctx, accessList) + _, err := a.service.GetResource(ctx, accessList) if err != nil { return trace.Wrap(err) } - - if al.HasImplicitMembership() { - return services.ImplicitAccessListError{} - } - return trace.Wrap(a.memberService.WithPrefix(accessList).DeleteResource(ctx, memberName)) }) return trace.Wrap(err) @@ -376,28 +323,9 @@ func (a *AccessListService) DeleteAllAccessListMembers(ctx context.Context) erro // UpsertAccessListWithMembers creates or updates an access list resource and its members. func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, accessList *accesslist.AccessList, membersIn []*accesslist.AccessListMember) (*accesslist.AccessList, []*accesslist.AccessListMember, error) { - if accessList.HasImplicitMembership() && len(membersIn) > 0 { - return nil, nil, trace.BadParameter("cannot explicitly add members to a list with implicit inclusion") - } - // Double the lock TTL to account for the time it takes to upsert the members. upsertWithLockFn := func() error { return a.service.RunWhileLocked(ctx, lockName(accessList.GetName()), 2*accessListLockTTL, func(ctx context.Context, _ backend.Backend) error { - oldAccessList, err := a.service.GetResource(ctx, accessList.GetName()) - if err != nil && !trace.IsNotFound(err) { - return trace.Wrap(err) - } - - if oldAccessList != nil { - if oldAccessList.Spec.Ownership != accessList.Spec.Ownership { - return trace.BadParameter("AccessList ownership cannot be changed") - } - - if oldAccessList.Spec.Membership != accessList.Spec.Membership { - return trace.BadParameter("AccessList membership cannot be changed") - } - } - // Create a map of the members from the request for easier lookup. membersMap := make(map[string]*accesslist.AccessListMember) @@ -413,6 +341,7 @@ func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, acc for { // List all members for the access list. + var err error members, membersToken, err = a.memberService.WithPrefix(accessList.GetName()).ListResources(ctx, 0 /* default size */, membersToken) if err != nil { return trace.Wrap(err) @@ -456,6 +385,7 @@ func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, acc member.SetRevision(upserted.GetRevision()) } + var err error accessList, err = a.service.UpsertResource(ctx, accessList) return trace.Wrap(err) }) diff --git a/lib/services/local/access_list_test.go b/lib/services/local/access_list_test.go index bd8d8a8f75d63..ef4197c48d5b4 100644 --- a/lib/services/local/access_list_test.go +++ b/lib/services/local/access_list_test.go @@ -25,7 +25,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "github.com/google/uuid" "github.com/gravitational/trace" "github.com/jonboulle/clockwork" "github.com/stretchr/testify/require" @@ -36,7 +35,6 @@ import ( "github.com/gravitational/teleport/lib/backend" "github.com/gravitational/teleport/lib/backend/memory" "github.com/gravitational/teleport/lib/modules" - "github.com/gravitational/teleport/lib/services" ) // TestAccessListCRUD tests backend operations with access list resources. @@ -1032,367 +1030,6 @@ func newAccessListReview(t *testing.T, accessList, name string) *accesslist.Revi return review } -func TestDynamicAccessListOwnersCRUD(t *testing.T) { - ctx := context.Background() - clock := clockwork.NewFakeClock() - - mem, err := memory.New(memory.Config{ - Context: ctx, - Clock: clock, - }) - require.NoError(t, err) - - service, err := NewAccessListService(backend.NewSanitizer(mem), clock) - require.NoError(t, err) - - t.Run("inserting without set conditions is an error", func(t *testing.T) { - // Given an access list with implicit ownership AND no ownership - // conditions set - list := newAccessList(t, t.Name(), clock) - list.Spec.Ownership = accesslist.InclusionImplicit - list.Spec.OwnershipRequires = accesslist.Requires{} - - // When I try to insert that access list - _, err := service.UpsertAccessList(ctx, list) - - // Expect that the operation fails - require.Error(t, err) - }) - - t.Run("inserting with explicit owners is an error", func(t *testing.T) { - // Given an access list with implicit ownership AND a non-empty owner - // list - list := newAccessList(t, t.Name(), clock) - list.Spec.Ownership = accesslist.InclusionImplicit - list.Spec.Owners = []accesslist.Owner{ - { - Name: "some-user", - Description: "just coz", - }, - } - - // When I try to insert that access list - _, err := service.UpsertAccessList(ctx, list) - - // Expect that the operation fails - require.Error(t, err) - }) -} - -func TestDynamicAccessListMembersCRUD(t *testing.T) { - ctx := context.Background() - clock := clockwork.NewFakeClock() - - mem, err := memory.New(memory.Config{ - Context: ctx, - Clock: clock, - }) - require.NoError(t, err) - - service, err := NewAccessListService(backend.NewSanitizer(mem), clock) - require.NoError(t, err) - - t.Run("inserting without set member conditions is an error", func(t *testing.T) { - // Given an access list with implicit ownership AND no ownership - // conditions set - list := newAccessList(t, t.Name(), clock) - list.Spec.Membership = accesslist.InclusionImplicit - list.Spec.MembershipRequires = accesslist.Requires{} - - // When I try to insert that access list - _, err := service.UpsertAccessList(ctx, list) - - // Expect that the operation fails - require.Error(t, err) - }) - - t.Run("listing implicit members returns ImplicitAccessListError", func(t *testing.T) { - var err error - - // Given an access list with implicit membership - implicitlist := newAccessList(t, t.Name(), clock) - implicitlist.Spec.Membership = accesslist.InclusionImplicit - implicitlist, err = service.UpsertAccessList(ctx, implicitlist) - require.NoError(t, err) - - // When I try to list the members of that list - _, _, err = service.ListAccessListMembers(ctx, implicitlist.GetName(), 100, "") - - // The AccessList service lets me know that it has implicit membership - // and can't compute the result for me - require.ErrorIs(t, err, services.ImplicitAccessListError{}) - }) - - t.Run("getting list members returns ImplicitAccessListError", func(t *testing.T) { - var err error - - // Given an access list with implicit membership - implicitlist := newAccessList(t, t.Name(), clock) - implicitlist.Spec.Membership = accesslist.InclusionImplicit - implicitlist, err = service.UpsertAccessList(ctx, implicitlist) - require.NoError(t, err) - - // When I try to query a member of that list - _, err = service.GetAccessListMember(ctx, implicitlist.GetName(), "somebody") - - // The AccessList service lets me know that it has implicit membership - // and can't compute the result for me - require.ErrorIs(t, err, services.ImplicitAccessListError{}) - }) - - t.Run("deleting list members returns ImplicitAccessListError", func(t *testing.T) { - var err error - - // Given an access list with implicit membership - implicitlist := newAccessList(t, t.Name(), clock) - implicitlist.Spec.Membership = accesslist.InclusionImplicit - implicitlist, err = service.UpsertAccessList(ctx, implicitlist) - require.NoError(t, err) - - // When I try to delete a member of that list - err = service.DeleteAccessListMember(ctx, implicitlist.GetName(), "somebody") - - // The AccessList service lets me know that the list has implicit - // membership and it can't honor the request - require.ErrorIs(t, err, services.ImplicitAccessListError{}) - }) - - t.Run("inserting with no members is allowed", func(t *testing.T) { - // Given an access list with implicit membership - implicitlist := newAccessList(t, t.Name(), clock) - implicitlist.Spec.Membership = accesslist.InclusionImplicit - - // When I attempt to upsert the list via UpsertAccessListWithMembers, - // but do not supply any members - _, _, err = service.UpsertAccessListWithMembers( - ctx, - implicitlist, - []*accesslist.AccessListMember{}) - - // Expect that the overall operation succeeds - require.NoError(t, err) - - // ALSO Expect that the AccessList has been inserted - _, err = service.GetAccessList(ctx, implicitlist.GetName()) - require.NoError(t, err) - }) - - t.Run("inserting list members is an error", func(t *testing.T) { - // Given an access list with implicit membership - implicitlist := newAccessList(t, t.Name(), clock) - implicitlist.Spec.Membership = accesslist.InclusionImplicit - - // And a membership for that list - m, err := accesslist.NewAccessListMember( - header.Metadata{ - Name: implicitlist.GetName() + "/" + "some-user", - }, - accesslist.AccessListMemberSpec{ - AccessList: implicitlist.GetName(), - Name: "some-user", - Membership: accesslist.InclusionExplicit, - AddedBy: "system", - Joined: time.Date(2016, time.December, 17, 14, 30, 55, 60, time.UTC), - }) - require.NoError(t, err) - - // When I attempt to upsert the list with members - _, _, err = service.UpsertAccessListWithMembers( - ctx, - implicitlist, - []*accesslist.AccessListMember{m}) - - // Expect that the overall operation fails - require.Error(t, err) - - // Expect that the AccessList has not been inserted - _, err = service.GetAccessList(ctx, implicitlist.GetName()) - require.Error(t, err) - require.True(t, trace.IsNotFound(err)) - }) - - t.Run("deleting all list members is allowed", func(t *testing.T) { - var err error - - // Given an access list with implicit membership - implicitlist := newAccessList(t, t.Name(), clock) - implicitlist.Spec.Membership = accesslist.InclusionImplicit - implicitlist, err = service.UpsertAccessList(ctx, implicitlist) - require.NoError(t, err) - - // AND a membership record fora user in that list (this can happen if an - // existing Explicit list is made Implicit) - m := newAccessListMember(t, implicitlist.GetName(), "scooby") - m, err = service.memberService.WithPrefix(m.Spec.AccessList).UpsertResource(ctx, m) - require.NoError(t, err) - - // When I try to remove all the members of that list - err = service.DeleteAllAccessListMembersForAccessList( - ctx, - implicitlist.GetName()) - - // Expect that the operation succeeds - require.NoError(t, err) - - // And that the membership record no longer exists - _, err = service.memberService.WithPrefix(m.Spec.AccessList).GetResource(ctx, m.GetName()) - require.Error(t, err) - require.True(t, trace.IsNotFound(err)) - }) - -} - -func TestChangingMembershipModeIsAnError(t *testing.T) { - ctx := context.Background() - clock := clockwork.NewFakeClock() - - mem, err := memory.New(memory.Config{ - Context: ctx, - Clock: clock, - }) - require.NoError(t, err) - - service, err := NewAccessListService(backend.NewSanitizer(mem), clock) - require.NoError(t, err) - - upsert := func(l *accesslist.AccessList) error { - _, err := service.UpsertAccessList(ctx, l) - return err - } - - upsertWithMembers := func(l *accesslist.AccessList) error { - _, _, err := service.UpsertAccessListWithMembers(ctx, l, []*accesslist.AccessListMember{}) - return err - } - - tests := []struct { - name string - oldMode accesslist.Inclusion - newMode accesslist.Inclusion - updateFn func(*accesslist.AccessList) error - }{ - { - name: "explicit to implicit", - oldMode: accesslist.InclusionExplicit, - newMode: accesslist.InclusionImplicit, - updateFn: upsert, - }, { - name: "implicit to explicit", - oldMode: accesslist.InclusionImplicit, - newMode: accesslist.InclusionExplicit, - updateFn: upsert, - }, { - name: "explicit to implicit (with members)", - oldMode: accesslist.InclusionExplicit, - newMode: accesslist.InclusionImplicit, - updateFn: upsertWithMembers, - }, { - name: "implicit to explicit (with members)", - oldMode: accesslist.InclusionImplicit, - newMode: accesslist.InclusionExplicit, - updateFn: upsertWithMembers, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - // Given an AccessList with a specific Membership Inclusion type - list := newAccessList(t, uuid.New().String(), clock) - list.Spec.Membership = test.oldMode - _, err := service.UpsertAccessList(ctx, list) - require.NoError(t, err) - - // When I try to change the membership type... - list.Spec.Membership = test.newMode - err = test.updateFn(list) - - // Expect that the operation fails with BadParameter - require.Error(t, err) - require.Truef(t, trace.IsBadParameter(err), - "Expected BadParameter, got %s", err.Error()) - }) - } -} - -func TestChangingOwnershipModeIsAnError(t *testing.T) { - ctx := context.Background() - clock := clockwork.NewFakeClock() - - mem, err := memory.New(memory.Config{ - Context: ctx, - Clock: clock, - }) - require.NoError(t, err) - - service, err := NewAccessListService(backend.NewSanitizer(mem), clock) - require.NoError(t, err) - - upsert := func(l *accesslist.AccessList) error { - _, err := service.UpsertAccessList(ctx, l) - return err - } - - upsertWithMembers := func(l *accesslist.AccessList) error { - _, _, err := service.UpsertAccessListWithMembers(ctx, l, []*accesslist.AccessListMember{}) - return err - } - - tests := []struct { - name string - oldMode accesslist.Inclusion - newMode accesslist.Inclusion - updateFn func(*accesslist.AccessList) error - }{ - { - name: "explicit to implicit", - oldMode: accesslist.InclusionExplicit, - newMode: accesslist.InclusionImplicit, - updateFn: upsert, - }, { - name: "implicit to explicit", - oldMode: accesslist.InclusionImplicit, - newMode: accesslist.InclusionExplicit, - updateFn: upsert, - }, { - name: "explicit to implicit (with members)", - oldMode: accesslist.InclusionExplicit, - newMode: accesslist.InclusionImplicit, - updateFn: upsertWithMembers, - }, { - name: "implicit to explicit (with members)", - oldMode: accesslist.InclusionImplicit, - newMode: accesslist.InclusionExplicit, - updateFn: upsertWithMembers, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - // Given an AccessList with a specific Ownership Inclusion type - list := newAccessList(t, uuid.New().String(), clock) - list.Spec.Ownership = test.oldMode - if test.oldMode == accesslist.InclusionImplicit { - list.Spec.Owners = []accesslist.Owner{} - } - - _, err := service.UpsertAccessList(ctx, list) - require.NoError(t, err) - - // When I try to change the ownership type... - list.Spec.Ownership = test.newMode - if test.newMode == accesslist.InclusionImplicit { - list.Spec.Owners = []accesslist.Owner{} - } - err = test.updateFn(list) - - // Expect that the operation fails with BadParameter - require.Error(t, err) - require.Truef(t, trace.IsBadParameter(err), - "Expected BadParameter, got %s", err.Error()) - }) - } -} - func TestAccessListService_ListAllAccessListMembers(t *testing.T) { ctx := context.Background() clock := clockwork.NewFakeClock() diff --git a/lib/services/simple/access_list.go b/lib/services/simple/access_list.go index 2f0560bc0fdf3..b49abc35bbc48 100644 --- a/lib/services/simple/access_list.go +++ b/lib/services/simple/access_list.go @@ -138,25 +138,11 @@ func (a *AccessListService) CountAccessListMembers(ctx context.Context, accessLi // ListAccessListMembers returns a paginated list of all access list members. func (a *AccessListService) ListAccessListMembers(ctx context.Context, accessListName string, pageSize int, nextToken string) ([]*accesslist.AccessListMember, string, error) { - // We'll make a best effort to determine if the access list is implicit, but will proceed if we can't figure it out. - al, err := a.GetAccessList(ctx, accessListName) - if err == nil { - if al.HasImplicitMembership() { - return nil, "", trace.Wrap(services.ImplicitAccessListError{}) - } - } return a.memberService.WithPrefix(accessListName).ListResources(ctx, pageSize, nextToken) } // GetAccessListMember returns the specified access list member resource. func (a *AccessListService) GetAccessListMember(ctx context.Context, accessListName string, memberName string) (*accesslist.AccessListMember, error) { - // We'll make a best effort to determine if the access list is implicit, but will proceed if we can't figure it out. - al, err := a.GetAccessList(ctx, accessListName) - if err == nil { - if al.HasImplicitMembership() { - return nil, trace.Wrap(services.ImplicitAccessListError{}) - } - } return a.memberService.WithPrefix(accessListName).GetResource(ctx, memberName) }