diff --git a/api/types/accesslist/accesslist.go b/api/types/accesslist/accesslist.go index 19ce49e21cb54..08e47cb038e4d 100644 --- a/api/types/accesslist/accesslist.go +++ b/api/types/accesslist/accesslist.go @@ -180,6 +180,11 @@ type Spec struct { type Type string const ( + // TODO(kopiczko) v21: Remove DeprecatedDynamic. The only version setting this type is 17.5.4. + + // DeprecatedDynamic is deprecated and should not be used. Use [Default] instead. It has + // the same semantic meaning. + DeprecatedDynamic Type = "dynamic" // Default Access Lists are the default type supposed to be managed with the web UI. They // require periodic audit reviews. Default Type = "" @@ -191,23 +196,19 @@ const ( SCIM Type = "scim" ) -func NewTypeFromString(s string) (Type, error) { - switch s { - case string(Default): - return Default, nil - case string(Static): - return Static, nil - case string(SCIM): - return SCIM, nil +func validateType(t Type) error { + switch t { + case DeprecatedDynamic, Default, Static, SCIM: + return nil default: - return "", trace.BadParameter("unknown access_list type %q", s) + return trace.BadParameter("unknown access_list type %q", t) } } // IsReviewable returns true if the AccessList type supports the audit reviews in the web UI. func (t Type) IsReviewable() bool { switch t { - case Default: + case DeprecatedDynamic, Default: return true default: return false @@ -339,7 +340,12 @@ func (a *AccessList) CheckAndSetDefaults() error { return trace.Wrap(err) } - if _, err := NewTypeFromString(string(a.Spec.Type)); err != nil { + // Restore the type if the cluster was ever running in version 17.5.4. + if a.Spec.Type == DeprecatedDynamic { + a.Spec.Type = Default + } + + if err := validateType(a.Spec.Type); err != nil { return trace.Wrap(err) } diff --git a/api/types/accesslist/accesslist_test.go b/api/types/accesslist/accesslist_test.go index b151aff4c081e..f980e3392e612 100644 --- a/api/types/accesslist/accesslist_test.go +++ b/api/types/accesslist/accesslist_test.go @@ -18,6 +18,7 @@ package accesslist import ( "encoding/json" + "fmt" "testing" "time" @@ -280,12 +281,12 @@ func TestAccessListDefaults(t *testing.T) { require.ErrorContains(t, err, `unknown access_list type "test_unknown_type"`) }) } - func TestSelectNextReviewDate(t *testing.T) { t.Parallel() tests := []struct { name string + accessListTypes []Type frequency ReviewFrequency dayOfMonth ReviewDayOfMonth currentReviewDate time.Time @@ -293,6 +294,7 @@ func TestSelectNextReviewDate(t *testing.T) { }{ { name: "one month, first day", + accessListTypes: []Type{Default, DeprecatedDynamic}, frequency: OneMonth, dayOfMonth: FirstDayOfMonth, currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC), @@ -300,6 +302,7 @@ func TestSelectNextReviewDate(t *testing.T) { }, { name: "one month, fifteenth day", + accessListTypes: []Type{Default, DeprecatedDynamic}, frequency: OneMonth, dayOfMonth: FifteenthDayOfMonth, currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC), @@ -307,6 +310,7 @@ func TestSelectNextReviewDate(t *testing.T) { }, { name: "one month, last day", + accessListTypes: []Type{Default, DeprecatedDynamic}, frequency: OneMonth, dayOfMonth: LastDayOfMonth, currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC), @@ -314,6 +318,7 @@ func TestSelectNextReviewDate(t *testing.T) { }, { name: "six months, last day", + accessListTypes: []Type{Default, DeprecatedDynamic}, frequency: SixMonths, dayOfMonth: LastDayOfMonth, currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC), @@ -324,14 +329,18 @@ func TestSelectNextReviewDate(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - t.Parallel() - accessList := AccessList{} - accessList.Spec.Audit.NextAuditDate = test.currentReviewDate - accessList.Spec.Audit.Recurrence = Recurrence{ - Frequency: test.frequency, - DayOfMonth: test.dayOfMonth, + for _, typ := range test.accessListTypes { + t.Run(fmt.Sprintf("type=%q", typ), func(t *testing.T) { + accessList := AccessList{} + accessList.Spec.Type = typ + accessList.Spec.Audit.NextAuditDate = test.currentReviewDate + accessList.Spec.Audit.Recurrence = Recurrence{ + Frequency: test.frequency, + DayOfMonth: test.dayOfMonth, + } + require.Equal(t, test.expected, accessList.SelectNextReviewDate()) + }) } - require.Equal(t, test.expected, accessList.SelectNextReviewDate()) }) } } diff --git a/api/types/accesslist/convert/v1/accesslist_test.go b/api/types/accesslist/convert/v1/accesslist_test.go index 7d68d6c10a26d..ca9c1af619b78 100644 --- a/api/types/accesslist/convert/v1/accesslist_test.go +++ b/api/types/accesslist/convert/v1/accesslist_test.go @@ -89,26 +89,57 @@ func TestWithOwnersIneligibleStatusField(t *testing.T) { } func TestRoundtrip(t *testing.T) { - t.Run("with custom subkind", func(t *testing.T) { - accessList := newAccessList(t, "access-list") - accessList.ResourceHeader.SetSubKind("access-list-subkind") + t.Parallel() - converted, err := FromProto(ToProto(accessList)) - require.NoError(t, err) + type testCase struct { + name string + modificationFn func(*accesslist.AccessList) + } - require.Empty(t, cmp.Diff(accessList, converted)) - }) + for _, tc := range []testCase{ + { + name: "no-modifications", + modificationFn: func(accessList *accesslist.AccessList) {}, + }, + { + name: "with-subkind", + modificationFn: func(accessList *accesslist.AccessList) { + accessList.ResourceHeader.SetSubKind("access-list-subkind") + }, + }, + { + name: "deprecated-dynamic-type", + modificationFn: func(accessList *accesslist.AccessList) { + accessList.Spec.Type = accesslist.DeprecatedDynamic + }, + }, + { + name: "default-type", + modificationFn: func(accessList *accesslist.AccessList) { + accessList.Spec.Type = accesslist.Default + }, + }, + { + name: "static-type", + modificationFn: func(accessList *accesslist.AccessList) { + accessList.Spec.Type = accesslist.Static + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + accessList := newAccessList(t, "access-list") + tc.modificationFn(accessList) - t.Run("with non-default type", func(t *testing.T) { - accessList := newAccessList(t, "access-list") - accessList.Spec.Type = accesslist.Static + converted, err := FromProto(ToProto(accessList)) + require.NoError(t, err) - converted, err := FromProto(ToProto(accessList)) - require.NoError(t, err) + if accessList.Spec.Type == accesslist.DeprecatedDynamic { + accessList.Spec.Type = accesslist.Default + } - require.Empty(t, cmp.Diff(accessList, converted)) - require.Equal(t, accesslist.Static, converted.Spec.Type) - }) + require.Empty(t, cmp.Diff(accessList, converted)) + }) + } } func Test_FromProto_withBadType(t *testing.T) { diff --git a/integrations/access/accesslist/app_test.go b/integrations/access/accesslist/app_test.go index 6cafaae089417..f196913abe687 100644 --- a/integrations/access/accesslist/app_test.go +++ b/integrations/access/accesslist/app_test.go @@ -148,62 +148,65 @@ func TestAccessListReminders_Single(t *testing.T) { require.NoError(t, app.Err()) }) - accessList, err := accesslist.NewAccessList(header.Metadata{ - Name: "test-access-list", - }, accesslist.Spec{ - Title: "test access list", - Owners: []accesslist.Owner{{Name: "owner1"}, {Name: "not-found"}}, - Grants: accesslist.Grants{ - Roles: []string{"role"}, - }, - Audit: accesslist.Audit{ - NextAuditDate: clock.Now().Add(28 * 24 * time.Hour), // Four weeks out from today - Notifications: accesslist.Notifications{ - Start: oneDay * 14, // Start alerting at two weeks before audit date + for _, typ := range []accesslist.Type{accesslist.Default, accesslist.DeprecatedDynamic} { + accessList, err := accesslist.NewAccessList(header.Metadata{ + Name: "test-access-list", + }, accesslist.Spec{ + Type: typ, + Title: "test access list", + Owners: []accesslist.Owner{{Name: "owner1"}, {Name: "not-found"}}, + Grants: accesslist.Grants{ + Roles: []string{"role"}, }, - }, - }) - require.NoError(t, err) + Audit: accesslist.Audit{ + NextAuditDate: clock.Now().Add(28 * 24 * time.Hour), // Four weeks out from today + Notifications: accesslist.Notifications{ + Start: oneDay * 14, // Start alerting at two weeks before audit date + }, + }, + }) + require.NoError(t, err) - accessLists := []*accesslist.AccessList{accessList} + accessLists := []*accesslist.AccessList{accessList} - // No notifications for today - advanceAndLookForRecipients(t, bot, as, clock, 0, accessLists) + // No notifications for today + advanceAndLookForRecipients(t, bot, as, clock, 0, accessLists) - // Advance by one week, expect no notifications. - advanceAndLookForRecipients(t, bot, as, clock, oneDay*7, accessLists) + // Advance by one week, expect no notifications. + advanceAndLookForRecipients(t, bot, as, clock, oneDay*7, accessLists) - // Advance by one week, expect a notification. "not-found" will be missing as a recipient. - advanceAndLookForRecipients(t, bot, as, clock, oneDay*7, accessLists, "owner1") + // Advance by one week, expect a notification. "not-found" will be missing as a recipient. + advanceAndLookForRecipients(t, bot, as, clock, oneDay*7, accessLists, "owner1") - // Add a new owner. - accessList.Spec.Owners = append(accessList.Spec.Owners, accesslist.Owner{Name: "owner2"}) + // Add a new owner. + accessList.Spec.Owners = append(accessList.Spec.Owners, accesslist.Owner{Name: "owner2"}) - // Advance by one day, expect a notification only to the new owner. - advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessLists, "owner2") + // Advance by one day, expect a notification only to the new owner. + advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessLists, "owner2") - // Advance by one day, expect no notifications. - advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessLists) + // Advance by one day, expect no notifications. + advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessLists) - // Advance by five more days, to the next week, expect two notifications - advanceAndLookForRecipients(t, bot, as, clock, oneDay*5, accessLists, "owner1", "owner2") + // Advance by five more days, to the next week, expect two notifications + advanceAndLookForRecipients(t, bot, as, clock, oneDay*5, accessLists, "owner1", "owner2") - // Advance by one day, expect no notifications - advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessLists) + // Advance by one day, expect no notifications + advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessLists) - // Advance by one day, expect no notifications - advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessLists) + // Advance by one day, expect no notifications + advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessLists) - // Advance by five more days, to the next week, expect two notifications - advanceAndLookForRecipients(t, bot, as, clock, oneDay*5, accessLists, "owner1", "owner2") + // Advance by five more days, to the next week, expect two notifications + advanceAndLookForRecipients(t, bot, as, clock, oneDay*5, accessLists, "owner1", "owner2") - // Advance 60 days a day at a time, expect two notifications each time. - for i := 0; i < 60; i++ { - // Make sure we only get a notification once per day by iterating through each 6 hours at a time. - for j := 0; j < 3; j++ { - advanceAndLookForRecipients(t, bot, as, clock, 6*time.Hour, accessLists) + // Advance 60 days a day at a time, expect two notifications each time. + for range 60 { + // Make sure we only get a notification once per day by iterating through each 6 hours at a time. + for range 3 { + advanceAndLookForRecipients(t, bot, as, clock, 6*time.Hour, accessLists) + } + advanceAndLookForRecipients(t, bot, as, clock, 6*time.Hour, accessLists, "owner1", "owner2") } - advanceAndLookForRecipients(t, bot, as, clock, 6*time.Hour, accessLists, "owner1", "owner2") } } diff --git a/lib/services/local/access_list_test.go b/lib/services/local/access_list_test.go index a594c5fa0cd1b..264d75c294acf 100644 --- a/lib/services/local/access_list_test.go +++ b/lib/services/local/access_list_test.go @@ -722,7 +722,7 @@ func TestAccessListReviewCRUD(t *testing.T) { // Create a couple access lists. accessList1 := newAccessList(t, "accessList1", clock) - accessList2 := newAccessList(t, "accessList2", clock) + accessList2 := newAccessList(t, "accessList2", clock, withType(accesslist.DeprecatedDynamic)) accessList1OrigDate := accessList1.Spec.Audit.NextAuditDate accessList2OrigDate := accessList2.Spec.Audit.NextAuditDate @@ -1017,7 +1017,19 @@ func TestAccessListRequiresEqual(t *testing.T) { } } -func newAccessList(t *testing.T, name string, clock clockwork.Clock) *accesslist.AccessList { +type newAccessListOptions struct { + typ accesslist.Type +} + +type newAccessListOpt func(*newAccessListOptions) + +func withType(typ accesslist.Type) newAccessListOpt { + return func(o *newAccessListOptions) { + o.typ = typ + } +} + +func newAccessList(t *testing.T, name string, clock clockwork.Clock, opts ...newAccessListOpt) *accesslist.AccessList { t.Helper() accessList, err := accesslist.NewAccessList(