Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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)
})
}
Loading