Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jul 10, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36cde31
Batch access list review reminder (slack)
kimlisa Jul 3, 2024
00ae5a0
Address CRs
kimlisa Jul 6, 2024
334dc1e
Address CR 2
kimlisa Jul 8, 2024
182470a
Fix lint
kimlisa Jul 9, 2024
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
59 changes: 40 additions & 19 deletions integrations/access/accesslist/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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).Warn("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)
}
}

Expand All @@ -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.
Expand All @@ -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
Copy link
Contributor

@smallinsky smallinsky Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, nil
return []common.Recipient,, nil

I would assume that:

val, err := getRecipientsRequiringReminders
if err != nil {
  return err
}
val = append(val, common.Recipient{...})

will not panic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i understood you correctly, iterating over a nil slice and spreading a nil slice, doesn't result in panic: https://go.dev/play/p/VbdQF2X6s5f

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. I always confusing when I'm seeing 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
Expand All @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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)
}

var errs []error
for _, recipient := range recipients {
if err := a.bot.SendReviewReminders(ctx, recipient, accessList); err != nil {
errs = append(errs, err)
}
return nil, trace.Wrap(err)
}

return trace.NewAggregate(errs...)
return recipients, nil
}
134 changes: 115 additions & 19 deletions integrations/access/accesslist/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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)
Expand Down Expand Up @@ -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))
Expand All @@ -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")
Expand Down Expand Up @@ -158,44 +160,136 @@ 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 {
Expand Down Expand Up @@ -261,21 +355,23 @@ 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()

var expectedRecipients []common.Recipient
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)
Expand Down
2 changes: 1 addition & 1 deletion integrations/access/accesslist/bot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
2 changes: 1 addition & 1 deletion integrations/access/discord/bot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down
2 changes: 1 addition & 1 deletion integrations/access/mattermost/bot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down
2 changes: 1 addition & 1 deletion integrations/access/opsgenie/bot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down
2 changes: 1 addition & 1 deletion integrations/access/servicenow/bot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down
Loading
Loading