AccessLists: pick a next audit date if not specified

api/types/accesslist/accesslist.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/gravitational/trace"
+	"github.com/jonboulle/clockwork"
 
 	"github.com/gravitational/teleport/api/types"
 	"github.com/gravitational/teleport/api/types/header"
@@ -311,10 +312,6 @@ func (a *AccessList) CheckAndSetDefaults() error {
 		return trace.BadParameter("owners are missing")
 	}
 
-	if a.Spec.Audit.NextAuditDate.IsZero() {
-		return trace.BadParameter("next audit date is missing")
-	}
-
 	if a.Spec.Audit.Recurrence.Frequency == 0 {
 		a.Spec.Audit.Recurrence.Frequency = SixMonths
 	}
@@ -335,6 +332,10 @@ func (a *AccessList) CheckAndSetDefaults() error {
 		return trace.BadParameter("recurrence day of month is an invalid value")
 	}
 
+	if a.Spec.Audit.NextAuditDate.IsZero() {
+		a.setInitialAuditDate(clockwork.NewRealClock())
+	}
+
 	if a.Spec.Audit.Notifications.Start == 0 {
 		a.Spec.Audit.Notifications.Start = twoWeeks
 	}
@@ -527,3 +528,32 @@ func (n Notifications) MarshalJSON() ([]byte, error) {
 		Start: n.Start.String(),
 	})
 }
+
+// SelectNextReviewDate will select the next review date for the access list.
+func (a *AccessList) SelectNextReviewDate() time.Time {
+	numMonths := int(a.Spec.Audit.Recurrence.Frequency)
+	dayOfMonth := int(a.Spec.Audit.Recurrence.DayOfMonth)
+
+	// If the last day of the month has been specified, use the 0 day of the
+	// next month, which will result in the last day of the target month.
+	if dayOfMonth == int(LastDayOfMonth) {
+		numMonths += 1
+		dayOfMonth = 0
+	}
+
+	currentReviewDate := a.Spec.Audit.NextAuditDate
+	nextDate := time.Date(currentReviewDate.Year(), currentReviewDate.Month()+time.Month(numMonths), dayOfMonth,
+		0, 0, 0, 0, time.UTC)
+
+	return nextDate
+}
+
+// setInitialAuditDate sets the NextAuditDate for a newly created AccessList.
+// The function is extracted from CheckAndSetDefaults for the sake of testing
+// (we need to pass a fake clock).
+func (a *AccessList) setInitialAuditDate(clock clockwork.Clock) {
+	// We act as if the AccessList just got reviewed (we just created it, so
+	// we're pretty sure of what it does) and pick the next review date.
+	a.Spec.Audit.NextAuditDate = clock.Now()
+	a.Spec.Audit.NextAuditDate = a.SelectNextReviewDate()
+}

api/types/accesslist/accesslist_test.go

@@ -21,6 +21,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/jonboulle/clockwork"
 	"github.com/stretchr/testify/require"
 
 	"github.com/gravitational/teleport/api/types/header"
@@ -278,3 +279,113 @@ func TestAccessListDefaults(t *testing.T) {
 		require.NoError(t, err)
 	})
 }
+
+func TestSelectNextReviewDate(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name              string
+		frequency         ReviewFrequency
+		dayOfMonth        ReviewDayOfMonth
+		currentReviewDate time.Time
+		expected          time.Time
+	}{
+		{
+			name:              "one month, first day",
+			frequency:         OneMonth,
+			dayOfMonth:        FirstDayOfMonth,
+			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
+			expected:          time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC),
+		},
+		{
+			name:              "one month, fifteenth day",
+			frequency:         OneMonth,
+			dayOfMonth:        FifteenthDayOfMonth,
+			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
+			expected:          time.Date(2023, 2, 15, 0, 0, 0, 0, time.UTC),
+		},
+		{
+			name:              "one month, last day",
+			frequency:         OneMonth,
+			dayOfMonth:        LastDayOfMonth,
+			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
+			expected:          time.Date(2023, 2, 28, 0, 0, 0, 0, time.UTC),
+		},
+		{
+			name:              "six months, last day",
+			frequency:         SixMonths,
+			dayOfMonth:        LastDayOfMonth,
+			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
+			expected:          time.Date(2023, 7, 31, 0, 0, 0, 0, time.UTC),
+		},
+	}
+
+	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,
+			}
+			require.Equal(t, test.expected, accessList.SelectNextReviewDate())
+		})
+	}
+}
+
+func TestAccessList_setInitialReviewDate(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name              string
+		frequency         ReviewFrequency
+		dayOfMonth        ReviewDayOfMonth
+		currentReviewDate time.Time
+		expected          time.Time
+	}{
+		{
+			name:              "one month, first day",
+			frequency:         OneMonth,
+			dayOfMonth:        FirstDayOfMonth,
+			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
+			expected:          time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC),
+		},
+		{
+			name:              "one month, fifteenth day",
+			frequency:         OneMonth,
+			dayOfMonth:        FifteenthDayOfMonth,
+			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
+			expected:          time.Date(2023, 2, 15, 0, 0, 0, 0, time.UTC),
+		},
+		{
+			name:              "one month, last day",
+			frequency:         OneMonth,
+			dayOfMonth:        LastDayOfMonth,
+			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
+			expected:          time.Date(2023, 2, 28, 0, 0, 0, 0, time.UTC),
+		},
+		{
+			name:              "six months, last day",
+			frequency:         SixMonths,
+			dayOfMonth:        LastDayOfMonth,
+			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
+			expected:          time.Date(2023, 7, 31, 0, 0, 0, 0, time.UTC),
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+			accessList := AccessList{}
+			accessList.Spec.Audit.Recurrence = Recurrence{
+				Frequency:  test.frequency,
+				DayOfMonth: test.dayOfMonth,
+			}
+			accessList.setInitialAuditDate(clockwork.NewFakeClockAt(test.currentReviewDate))
+			require.Equal(t, test.expected, accessList.Spec.Audit.NextAuditDate)
+		})
+	}
+}

lib/services/access_list.go

@@ -345,25 +345,6 @@ func UserMeetsRequirements(identity tlsca.Identity, requires accesslist.Requires
 	return true
 }
 
-// SelectNextReviewDate will select the next review date for the access list.
-func SelectNextReviewDate(accessList *accesslist.AccessList) time.Time {
-	numMonths := int(accessList.Spec.Audit.Recurrence.Frequency)
-	dayOfMonth := int(accessList.Spec.Audit.Recurrence.DayOfMonth)
-
-	// If the last day of the month has been specified, use the 0 day of the
-	// next month, which will result in the last day of the target month.
-	if dayOfMonth == int(accesslist.LastDayOfMonth) {
-		numMonths += 1
-		dayOfMonth = 0
-	}
-
-	currentReviewDate := accessList.Spec.Audit.NextAuditDate
-	nextDate := time.Date(currentReviewDate.Year(), currentReviewDate.Month()+time.Month(numMonths), dayOfMonth,
-		0, 0, 0, 0, time.UTC)
-
-	return nextDate
-}
-
 // AccessListReviews defines an interface for managing Access List reviews.
 type AccessListReviews interface {
 	// ListAccessListReviews will list access list reviews for a particular access list.

lib/services/access_list_test.go

@@ -571,61 +571,6 @@ func TestIsAccessListMemberChecker(t *testing.T) {
 	}
 }
 
-func TestSelectNextReviewDate(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name              string
-		frequency         accesslist.ReviewFrequency
-		dayOfMonth        accesslist.ReviewDayOfMonth
-		currentReviewDate time.Time
-		expected          time.Time
-	}{
-		{
-			name:              "one month, first day",
-			frequency:         accesslist.OneMonth,
-			dayOfMonth:        accesslist.FirstDayOfMonth,
-			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
-			expected:          time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC),
-		},
-		{
-			name:              "one month, fifteenth day",
-			frequency:         accesslist.OneMonth,
-			dayOfMonth:        accesslist.FifteenthDayOfMonth,
-			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
-			expected:          time.Date(2023, 2, 15, 0, 0, 0, 0, time.UTC),
-		},
-		{
-			name:              "one month, last day",
-			frequency:         accesslist.OneMonth,
-			dayOfMonth:        accesslist.LastDayOfMonth,
-			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
-			expected:          time.Date(2023, 2, 28, 0, 0, 0, 0, time.UTC),
-		},
-		{
-			name:              "six months, last day",
-			frequency:         accesslist.SixMonths,
-			dayOfMonth:        accesslist.LastDayOfMonth,
-			currentReviewDate: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
-			expected:          time.Date(2023, 7, 31, 0, 0, 0, 0, time.UTC),
-		},
-	}
-
-	for _, test := range tests {
-		test := test
-		t.Run(test.name, func(t *testing.T) {
-			t.Parallel()
-			accessList := newAccessList(t)
-			accessList.Spec.Audit.NextAuditDate = test.currentReviewDate
-			accessList.Spec.Audit.Recurrence = accesslist.Recurrence{
-				Frequency:  test.frequency,
-				DayOfMonth: test.dayOfMonth,
-			}
-			require.Equal(t, test.expected, SelectNextReviewDate(accessList))
-		})
-	}
-}
-
 // TestAccessListReviewUnmarshal verifies an access list review resource can be unmarshaled.
 func TestAccessListReviewUnmarshal(t *testing.T) {
 	expected, err := accesslist.NewReview(

lib/services/local/access_list.go

@@ -514,7 +514,7 @@ func (a *AccessListService) CreateAccessListReview(ctx context.Context, review *
 			return trace.Wrap(err)
 		}
 
-		nextAuditDate = services.SelectNextReviewDate(accessList)
+		nextAuditDate = accessList.SelectNextReviewDate()
 		accessList.Spec.Audit.NextAuditDate = nextAuditDate
 
 		for _, removedMember := range review.Spec.Changes.RemovedMembers {