Batch access list review reminders and provide link (slack) (#43782)

* Batch access list review reminder (slack)

* Address CRs

* Address CR 2

* Fix lint
This commit is contained in:
Lisa Kim
2024-07-10 11:54:24 -07:00
committed by GitHub
parent fb8ef40e01
commit cdc35489b1
9 changed files with 283 additions and 51 deletions
+40 -19
View File
@@ -150,6 +150,7 @@ func (a *App) remindIfNecessary(ctx context.Context) error {
var nextToken string
var err error
remindersLookup := make(map[common.Recipient][]*accesslist.AccessList)
for {
var accessLists []*accesslist.AccessList
accessLists, nextToken, err = a.apiClient.ListAccessLists(ctx, 0 /* default page size */, nextToken)
@@ -167,8 +168,16 @@ func (a *App) remindIfNecessary(ctx context.Context) error {
}
for _, accessList := range accessLists {
if err := a.notifyForAccessListReviews(ctx, accessList); err != nil {
log.WithError(err).Warn("Error notifying for access list reviews")
recipients, err := a.getRecipientsRequiringReminders(ctx, accessList)
if err != nil {
log.WithError(err).Warnf("Error getting recipients to notify for review due for access list %q", accessList.Spec.Title)
continue
}
// Store all recipients and the accesslist needing review
// for later processing.
for _, recipient := range recipients {
remindersLookup[recipient] = append(remindersLookup[recipient], accessList)
}
}
@@ -177,12 +186,25 @@ func (a *App) remindIfNecessary(ctx context.Context) error {
}
}
// Send reminders for each collected recipients.
var errs []error
for recipient, accessLists := range remindersLookup {
if err := a.bot.SendReviewReminders(ctx, recipient, accessLists); err != nil {
errs = append(errs, err)
}
}
if len(errs) > 0 {
log.WithError(trace.NewAggregate(errs...)).Warn("Error notifying for access list reviews")
}
return nil
}
// notifyForAccessListReviews will notify if access list review dates are getting close. At the moment, this
// getRecipientsRequiringReminders will return recipients that require reminders only
// if the access list review dates are getting close. At the moment, this
// only supports notifying owners.
func (a *App) notifyForAccessListReviews(ctx context.Context, accessList *accesslist.AccessList) error {
func (a *App) getRecipientsRequiringReminders(ctx context.Context, accessList *accesslist.AccessList) ([]common.Recipient, error) {
log := logger.Get(ctx)
// Find the current notification window.
@@ -192,12 +214,12 @@ func (a *App) notifyForAccessListReviews(ctx context.Context, accessList *access
// If the current time before the notification start time, skip notifications.
if now.Before(notificationStart) {
log.Debugf("Access list %s is not ready for notifications, notifications start at %s", accessList.GetName(), notificationStart.Format(time.RFC3339))
return nil
return nil, nil
}
allRecipients := a.fetchRecipients(ctx, accessList, now, notificationStart)
if len(allRecipients) == 0 {
return trace.NotFound("no recipients could be fetched for access list %s", accessList.GetName())
return nil, trace.NotFound("no recipients could be fetched for access list %s", accessList.GetName())
}
// Try to create base notification data with a zero notification date. If these objects already
@@ -212,10 +234,15 @@ func (a *App) notifyForAccessListReviews(ctx context.Context, accessList *access
// Error is okay so long as it's already exists.
if err != nil && !trace.IsAlreadyExists(err) {
return trace.Wrap(err, "during create")
return nil, trace.Wrap(err, "during create")
}
return trace.Wrap(a.sendMessages(ctx, accessList, allRecipients, now, notificationStart))
recipients, err := a.updatePluginDataAndGetRecipientsRequiringReminders(ctx, accessList, allRecipients, now, notificationStart)
if err != nil {
return nil, trace.Wrap(err)
}
return recipients, nil
}
// fetchRecipients will return all recipients.
@@ -237,8 +264,9 @@ func (a *App) fetchRecipients(ctx context.Context, accessList *accesslist.Access
return allRecipients
}
// sendMessages will send review notifications to owners and update the plugin data.
func (a *App) sendMessages(ctx context.Context, accessList *accesslist.AccessList, allRecipients map[string]common.Recipient, now, notificationStart time.Time) error {
// updatePluginDataAndGetRecipientsRequiringReminders will return recipients requiring reminders
// and update the plugin data about when the recipient got notified.
func (a *App) updatePluginDataAndGetRecipientsRequiringReminders(ctx context.Context, accessList *accesslist.AccessList, allRecipients map[string]common.Recipient, now, notificationStart time.Time) ([]common.Recipient, error) {
log := logger.Get(ctx)
var windowStart time.Time
@@ -276,15 +304,8 @@ func (a *App) sendMessages(ctx context.Context, accessList *accesslist.AccessLis
return pd.AccessListNotificationData{UserNotifications: userNotifications}, nil
})
if err != nil {
return trace.Wrap(err)
return nil, trace.Wrap(err)
}
var errs []error
for _, recipient := range recipients {
if err := a.bot.SendReviewReminders(ctx, recipient, accessList); err != nil {
errs = append(errs, err)
}
}
return trace.NewAggregate(errs...)
return recipients, nil
}
+115 -19
View File
@@ -33,9 +33,11 @@ import (
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/api/types/accesslist"
"github.com/gravitational/teleport/api/types/header"
"github.com/gravitational/teleport/entitlements"
"github.com/gravitational/teleport/integrations/access/common"
"github.com/gravitational/teleport/integrations/access/common/teleport"
"github.com/gravitational/teleport/lib/auth"
"github.com/gravitational/teleport/lib/modules"
"github.com/gravitational/teleport/lib/services"
)
@@ -49,7 +51,7 @@ func (m *mockMessagingBot) CheckHealth(ctx context.Context) error {
return nil
}
func (m *mockMessagingBot) SendReviewReminders(ctx context.Context, recipient common.Recipient, accessList *accesslist.AccessList) error {
func (m *mockMessagingBot) SendReviewReminders(ctx context.Context, recipient common.Recipient, accessLists []*accesslist.AccessList) error {
m.mutex.Lock()
defer m.mutex.Unlock()
m.lastReminderRecipients = append(m.lastReminderRecipients, recipient)
@@ -106,7 +108,7 @@ func (m *mockPluginConfig) GetPluginType() types.PluginType {
return types.PluginTypeSlack
}
func TestAccessListReminders(t *testing.T) {
func TestAccessListReminders_Single(t *testing.T) {
t.Parallel()
clock := clockwork.NewFakeClockAt(time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC))
@@ -120,8 +122,8 @@ func TestAccessListReminders(t *testing.T) {
bot := &mockMessagingBot{
recipients: map[string]*common.Recipient{
"owner1": {Name: "owner1"},
"owner2": {Name: "owner2"},
"owner1": {Name: "owner1", ID: "owner1"},
"owner2": {Name: "owner2", ID: "owner2"},
},
}
app := common.NewApp(&mockPluginConfig{client: as, bot: bot}, "test-plugin")
@@ -158,46 +160,138 @@ func TestAccessListReminders(t *testing.T) {
})
require.NoError(t, err)
accessLists := []*accesslist.AccessList{accessList}
// No notifications for today
advanceAndLookForRecipients(t, bot, as, clock, 0, accessList)
advanceAndLookForRecipients(t, bot, as, clock, 0, accessLists)
// Advance by one week, expect no notifications.
advanceAndLookForRecipients(t, bot, as, clock, oneDay*7, accessList)
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, accessList, "owner1")
advanceAndLookForRecipients(t, bot, as, clock, oneDay*7, accessLists, "owner1")
// 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, accessList, "owner2")
advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessLists, "owner2")
// Advance by one day, expect no notifications.
advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessList)
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, accessList, "owner1", "owner2")
advanceAndLookForRecipients(t, bot, as, clock, oneDay*5, accessLists, "owner1", "owner2")
// Advance by one day, expect no notifications
advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessList)
advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessLists)
// Advance by one day, expect no notifications
advanceAndLookForRecipients(t, bot, as, clock, oneDay, accessList)
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, accessList, "owner1", "owner2")
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, accessList)
advanceAndLookForRecipients(t, bot, as, clock, 6*time.Hour, accessLists)
}
advanceAndLookForRecipients(t, bot, as, clock, 6*time.Hour, accessList, "owner1", "owner2")
advanceAndLookForRecipients(t, bot, as, clock, 6*time.Hour, accessLists, "owner1", "owner2")
}
}
func TestAccessListReminders_Batched(t *testing.T) {
modules.SetTestModules(t, &modules.TestModules{
TestFeatures: modules.Features{
Entitlements: map[entitlements.EntitlementKind]modules.EntitlementInfo{
entitlements.Identity: {Enabled: true},
},
},
})
clock := clockwork.NewFakeClockAt(time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC))
server := newTestAuth(t)
as := server.Auth()
t.Cleanup(func() {
require.NoError(t, as.Close())
})
bot := &mockMessagingBot{
recipients: map[string]*common.Recipient{
"owner1": {Name: "owner1", ID: "owner1"},
"owner2": {Name: "owner2", ID: "owner2"},
},
}
app := common.NewApp(&mockPluginConfig{client: as, bot: bot}, "test-plugin")
app.Clock = clock
ctx := context.Background()
go func() {
app.Run(ctx)
}()
ready, err := app.WaitReady(ctx)
require.NoError(t, err)
require.True(t, ready)
t.Cleanup(func() {
app.Terminate()
<-app.Done()
require.NoError(t, app.Err())
})
accessList1, err := accesslist.NewAccessList(header.Metadata{
Name: "test-access-list",
}, accesslist.Spec{
Title: "test access list",
Owners: []accesslist.Owner{{Name: "owner1"}, {Name: "owner2"}, {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
},
},
})
require.NoError(t, err)
accessList2, err := accesslist.NewAccessList(header.Metadata{
Name: "test-access-list-2",
}, accesslist.Spec{
Title: "test access list 2",
Owners: []accesslist.Owner{{Name: "owner1"}, {Name: "owner2"}, {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
},
},
})
require.NoError(t, err)
accessLists := []*accesslist.AccessList{accessList1, accessList2}
// 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 a notification. "not-found" will be missing as a recipient.
advanceAndLookForRecipients(t, bot, as, clock, oneDay*7, accessLists, "owner1", "owner2")
// Advance another week, expect notifications.
advanceAndLookForRecipients(t, bot, as, clock, oneDay*7, accessLists, "owner1", "owner2")
}
type mockClient struct {
mock.Mock
teleport.Client
@@ -261,13 +355,15 @@ func advanceAndLookForRecipients(t *testing.T,
alSvc services.AccessLists,
clock clockwork.FakeClock,
advance time.Duration,
accessList *accesslist.AccessList,
accessLists []*accesslist.AccessList,
recipients ...string) {
ctx := context.Background()
_, err := alSvc.UpsertAccessList(ctx, accessList)
require.NoError(t, err)
for _, accessList := range accessLists {
_, err := alSvc.UpsertAccessList(ctx, accessList)
require.NoError(t, err)
}
bot.resetLastRecipients()
@@ -275,7 +371,7 @@ func advanceAndLookForRecipients(t *testing.T,
if len(recipients) > 0 {
expectedRecipients = make([]common.Recipient, len(recipients))
for i, r := range recipients {
expectedRecipients[i] = common.Recipient{Name: r}
expectedRecipients[i] = common.Recipient{Name: r, ID: r}
}
}
clock.Advance(advance)
+1 -1
View File
@@ -29,5 +29,5 @@ type MessagingBot interface {
common.MessagingBot
// SendReviewReminders will send a review reminder that an access list needs to be reviewed.
SendReviewReminders(ctx context.Context, recipient common.Recipient, accessList *accesslist.AccessList) error
SendReviewReminders(ctx context.Context, recipient common.Recipient, accessLists []*accesslist.AccessList) error
}
+1 -1
View File
@@ -118,7 +118,7 @@ func (b DiscordBot) SupportedApps() []common.App {
}
// SendReviewReminders will send a review reminder that an access list needs to be reviewed.
func (b DiscordBot) SendReviewReminders(ctx context.Context, recipients []common.Recipient, accessList *accesslist.AccessList) error {
func (b DiscordBot) SendReviewReminders(ctx context.Context, recipients []common.Recipient, accessLists []*accesslist.AccessList) error {
return trace.NotImplemented("access list review reminder is not yet implemented")
}
+1 -1
View File
@@ -241,7 +241,7 @@ func (b Bot) GetMe(ctx context.Context) (User, error) {
}
// SendReviewReminders will send a review reminder that an access list needs to be reviewed.
func (b Bot) SendReviewReminders(ctx context.Context, recipients []common.Recipient, accessList *accesslist.AccessList) error {
func (b Bot) SendReviewReminders(ctx context.Context, recipients []common.Recipient, accessLists []*accesslist.AccessList) error {
return trace.NotImplemented("access list review reminder is not yet implemented")
}
+1 -1
View File
@@ -55,7 +55,7 @@ func (b *Bot) CheckHealth(ctx context.Context) error {
}
// SendReviewReminders will send a review reminder that an access list needs to be reviewed.
func (b Bot) SendReviewReminders(ctx context.Context, recipients []common.Recipient, accessList *accesslist.AccessList) error {
func (b Bot) SendReviewReminders(ctx context.Context, recipients []common.Recipient, accessLists []*accesslist.AccessList) error {
return trace.NotImplemented("access list review reminder is not yet implemented")
}
+1 -1
View File
@@ -54,7 +54,7 @@ func (b *Bot) CheckHealth(ctx context.Context) error {
}
// SendReviewReminders will send a review reminder that an access list needs to be reviewed.
func (b Bot) SendReviewReminders(ctx context.Context, recipients []common.Recipient, accessList *accesslist.AccessList) error {
func (b Bot) SendReviewReminders(ctx context.Context, recipients []common.Recipient, accessLists []*accesslist.AccessList) error {
return trace.NotImplemented("access list review reminder is not yet implemented")
}
+61 -6
View File
@@ -23,6 +23,7 @@ import (
"encoding/json"
"fmt"
"net/url"
"slices"
"time"
"github.com/go-resty/resty/v2"
@@ -111,11 +112,19 @@ func (b Bot) CheckHealth(ctx context.Context) error {
}
// SendReviewReminders will send a review reminder that an access list needs to be reviewed.
func (b Bot) SendReviewReminders(ctx context.Context, recipient common.Recipient, accessList *accesslist.AccessList) error {
func (b Bot) SendReviewReminders(ctx context.Context, recipient common.Recipient, accessLists []*accesslist.AccessList) error {
var blockItem []BlockItem
if len(accessLists) > 1 {
blockItem = b.slackAccessListBatchedReminderMsgSection(accessLists)
} else if len(accessLists) == 1 {
blockItem = b.slackAccessListReminderMsgSection(accessLists[0])
}
var result ChatMsgResponse
_, err := b.client.NewRequest().
SetContext(ctx).
SetBody(Message{BaseMessage: BaseMessage{Channel: recipient.ID}, BlockItems: b.slackAccessListReminderMsgSection(accessList)}).
SetBody(Message{BaseMessage: BaseMessage{Channel: recipient.ID}, BlockItems: blockItem}).
SetResult(&result).
Post("chat.postMessage")
return trace.Wrap(err)
@@ -273,16 +282,23 @@ func (b Bot) FetchRecipient(ctx context.Context, name string) (*common.Recipient
func (b Bot) slackAccessListReminderMsgSection(accessList *accesslist.AccessList) []BlockItem {
nextAuditDate := accessList.Spec.Audit.NextAuditDate
link := ""
if b.webProxyURL != nil {
reqURL := *b.webProxyURL
reqURL.Path = lib.BuildURLPath("web", "accesslists", accessList.Metadata.Name)
link = fmt.Sprintf("*Link*: %s", reqURL.String())
}
name := fmt.Sprintf("*%s*", accessList.Spec.Title)
var msg string
if b.clock.Now().After(nextAuditDate) {
daysSinceDue := int(b.clock.Since(nextAuditDate).Hours() / 24)
msg = fmt.Sprintf("Access List %s is %d day(s) past due for a review! Please review it.",
name, daysSinceDue)
msg = fmt.Sprintf("Access List %s is %d day(s) past due for a review! Please review it.\n%s",
name, daysSinceDue, link)
} else {
msg = fmt.Sprintf(
"Access List %s is due for a review by %s. Please review it soon!",
name, accessList.Spec.Audit.NextAuditDate.Format(time.DateOnly))
"Access List %s is due for a review by %s. Please review it soon!\n%s",
name, accessList.Spec.Audit.NextAuditDate.Format(time.DateOnly), link)
}
sections := []BlockItem{
@@ -294,6 +310,45 @@ func (b Bot) slackAccessListReminderMsgSection(accessList *accesslist.AccessList
return sections
}
// slackAccessListReminderMsgSection builds an access list reminder Slack message section (obeys markdown).
func (b Bot) slackAccessListBatchedReminderMsgSection(accessLists []*accesslist.AccessList) []BlockItem {
// Sort by earliest date due.
slices.SortFunc(accessLists, func(a, b *accesslist.AccessList) int {
return a.Spec.Audit.NextAuditDate.Compare(b.Spec.Audit.NextAuditDate)
})
accessList := accessLists[0]
earliestNextAuditDate := accessList.Spec.Audit.NextAuditDate
numOfReviewsRequired := len(accessLists)
link := ""
dueDate := ""
if b.webProxyURL != nil {
reqURL := *b.webProxyURL
reqURL.Path = lib.BuildURLPath("web", "accesslists")
link = fmt.Sprintf("*Link*: %s", reqURL.String())
}
if b.clock.Now().After(earliestNextAuditDate) {
daysSinceDue := int(b.clock.Since(earliestNextAuditDate).Hours() / 24)
dueDate = fmt.Sprintf("earliest of which is %d day(s) past due. Please review!",
daysSinceDue)
} else {
dueDate = fmt.Sprintf(
"earliest of which is due by %s. Please review them soon!",
accessList.Spec.Audit.NextAuditDate.Format(time.DateOnly))
}
sections := []BlockItem{
NewBlockItem(SectionBlock{
Text: NewTextObjectItem(MarkdownObject{Text: fmt.Sprintf("%d Access Lists are due for reviews, %s\n%s", numOfReviewsRequired, dueDate, link)}),
}),
}
return sections
}
// slackAccessRequestMsgSection builds an access request Slack message section (obeys markdown).
func (b Bot) slackAccessRequestMsgSections(reqID string, reqData pd.AccessRequestData) []BlockItem {
fields := accessrequest.MsgFields(reqID, reqData, b.clusterName, b.webProxyURL)
+62 -2
View File
@@ -831,7 +831,7 @@ func (s *SlackSuiteEnterprise) TestRace() {
// TestAccessListReminder validates that Access List reminders are sent before
// the Access List expires.
func (s *SlackSuiteEnterprise) TestAccessListReminder() {
func (s *SlackSuiteEnterprise) TestAccessListReminder_Singular() {
t := s.T()
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
t.Cleanup(cancel)
@@ -885,6 +885,66 @@ func (s *SlackSuiteEnterprise) TestAccessListReminder() {
s.requireReminderMsgEqual(ctx, s.reviewer1SlackUser.ID, "Access List *simple title* is 7 day(s) past due for a review! Please review it.")
}
// TestAccessListReminder_Batched validates that Access List reminders are sent in batches
// if multiple access lists are given.
func (s *SlackSuiteEnterprise) TestAccessListReminder_Batched() {
t := s.T()
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
t.Cleanup(cancel)
clock := clockwork.NewFakeClockAt(time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC))
s.appConfig.Clock = clock
s.startApp()
// Test setup: create a couple accesslists
accessList1, err := accesslist.NewAccessList(header.Metadata{
Name: "access-list1",
}, accesslist.Spec{
Title: "simple title one",
Grants: accesslist.Grants{
Roles: []string{"grant"},
},
Owners: []accesslist.Owner{
{Name: integration.Reviewer1UserName},
},
Audit: accesslist.Audit{
NextAuditDate: time.Date(2023, 3, 2, 0, 0, 0, 0, time.UTC),
},
})
require.NoError(t, err)
_, err = s.Ruler().AccessListClient().UpsertAccessList(ctx, accessList1)
require.NoError(t, err)
accessList2, err := accesslist.NewAccessList(header.Metadata{
Name: "access-list2",
}, accesslist.Spec{
Title: "simple title two",
Grants: accesslist.Grants{
Roles: []string{"grant"},
},
Owners: []accesslist.Owner{
{Name: integration.Reviewer1UserName},
},
Audit: accesslist.Audit{
NextAuditDate: time.Date(2023, 3, 1, 0, 0, 0, 0, time.UTC),
},
})
require.NoError(t, err)
_, err = s.Ruler().AccessListClient().UpsertAccessList(ctx, accessList2)
require.NoError(t, err)
// Trigger a reminder.
clock.BlockUntil(1)
clock.Advance(46 * 25 * time.Hour)
s.requireReminderMsgEqual(ctx, s.reviewer1SlackUser.ID, "2 Access Lists are due for reviews, earliest of which is due by 2023-03-01")
// Make it overdue.
clock.BlockUntil(1)
clock.Advance(20 * 24 * time.Hour)
s.requireReminderMsgEqual(ctx, s.reviewer1SlackUser.ID, "2 Access Lists are due for reviews, earliest of which is 8 day(s) past due")
}
func (s *SlackBaseSuite) requireReminderMsgEqual(ctx context.Context, id, text string) {
s.T().Helper()
t := s.T()
@@ -893,5 +953,5 @@ func (s *SlackBaseSuite) requireReminderMsgEqual(ctx context.Context, id, text s
require.NoError(t, err)
require.Equal(t, id, msg.Channel)
require.IsType(t, slack.SectionBlock{}, msg.BlockItems[0].Block)
require.Equal(t, text, (msg.BlockItems[0].Block).(slack.SectionBlock).Text.GetText())
require.Contains(t, (msg.BlockItems[0].Block).(slack.SectionBlock).Text.GetText(), text)
}