Skip to content
Merged
92 changes: 1 addition & 91 deletions api/types/accesslist/accesslist.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand All @@ -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")
}

Expand Down Expand Up @@ -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 {
Expand Down
48 changes: 1 addition & 47 deletions api/types/accesslist/accesslist_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 0 additions & 4 deletions api/types/accesslist/convert/v1/accesslist.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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,
Expand Down
18 changes: 0 additions & 18 deletions api/types/accesslist/convert/v1/accesslist_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions api/types/accesslist/convert/v1/member.go
Original file line number Diff line number Diff line change
Expand Up @@ -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: "",
Expand Down Expand Up @@ -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,
},
}
Expand Down
8 changes: 0 additions & 8 deletions api/types/accesslist/convert/v1/member_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
20 changes: 4 additions & 16 deletions api/types/accesslist/member.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
44 changes: 2 additions & 42 deletions api/types/accesslist/member_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,10 @@ func newAccessListSpec(nextAudit time.Time) accesslist.Spec {
},
NextAuditDate: nextAudit,
},
Membership: "explicit",
MembershipRequires: accesslist.Requires{
Roles: []string{"minion"},
Traits: trait.Traits{},
},
Ownership: "explicit",
OwnershipRequires: accesslist.Requires{
Roles: []string{"supervillain"},
Traits: trait.Traits{},
Expand Down
Loading