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.

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

Update how we represent notification actions #2

Draft
wants to merge 1 commit into
base: becca/actions-for-notifications
Choose a base branch
from
Draft
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
30 changes: 26 additions & 4 deletions ee/control/consumers/notificationconsumer/notify_consumer.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,36 @@ func (nc *NotificationConsumer) notificationIsValid(notificationToCheck notify.N
return false
}

// If action URI is set, it must be a valid URI
if notificationToCheck.ActionUri != "" {
_, err := url.Parse(notificationToCheck.ActionUri)
// If actions are set, they must be valid
numDefaultActions := 0
for _, a := range notificationToCheck.Actions {
// Make sure we only have one default action
if a.Default {
numDefaultActions += 1
}
if numDefaultActions > 1 {
level.Debug(nc.logger).Log(
"msg", "can only have one default action, but received multiple from K2",
"notification_id", notificationToCheck.ID)
return false
}

// Make sure the action has a label
if a.Label == "" {
level.Debug(nc.logger).Log(
"msg", "received invalid action label from K2",
"notification_id", notificationToCheck.ID,
"action_uri", a.Action)
return false
}

// Make sure the action is a valid URL
_, err := url.Parse(a.Action)
if err != nil {
level.Debug(nc.logger).Log(
"msg", "received invalid action_uri from K2",
"notification_id", notificationToCheck.ID,
"action_uri", notificationToCheck.ActionUri,
"action_uri", a.Action,
"err", err)
return false
}
Expand Down
54 changes: 51 additions & 3 deletions ee/control/consumers/notificationconsumer/notify_consumer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,19 @@ func TestUpdate_HappyPath(t *testing.T) {
testId := ulid.New()
testTitle := fmt.Sprintf("Test title @ %d - %s", time.Now().UnixMicro(), testId)
testBody := fmt.Sprintf("Test body @ %d - %s", time.Now().UnixMicro(), testId)
testActionUri := "https://www.kolide.com"
testNotifications := []notify.Notification{
{
Title: testTitle,
Body: testBody,
ID: testId,
ValidUntil: getValidUntil(),
ActionUri: testActionUri,
Actions: []notify.Action{
{
Label: "Learn More",
Action: "https://www.kolide.com",
Default: true,
},
},
},
}
testNotificationsRaw, err := json.Marshal(testNotifications)
Expand Down Expand Up @@ -144,7 +149,50 @@ func TestUpdate_ValidatesNotifications(t *testing.T) {
Body: "This notification has an action URI that is not valid",
ID: ulid.New(),
ValidUntil: getValidUntil(),
ActionUri: "some_thing:foo/bar",
Actions: []notify.Action{
{
Label: "Learn More",
Action: "some_thing:foo/bar",
Default: true,
},
},
},
},
{
name: "Invalid because the action label is missing",
testNotification: notify.Notification{
Title: "Test notification",
Body: "This notification has an action URI that is not valid",
ID: ulid.New(),
ValidUntil: getValidUntil(),
Actions: []notify.Action{
{
Label: "",
Action: "https://www.kolide.com",
Default: true,
},
},
},
},
{
name: "Invalid because there are multiple default actions",
testNotification: notify.Notification{
Title: "Test notification",
Body: "This notification has an action URI that is not valid",
ID: ulid.New(),
ValidUntil: getValidUntil(),
Actions: []notify.Action{
{
Label: "Learn More",
Action: "https://www.kolide.com",
Default: true,
},
{
Label: "Another Label",
Action: "https://example.com",
Default: true,
},
},
},
},
}
Expand Down
7 changes: 1 addition & 6 deletions ee/desktop/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,7 @@ func (c *client) Refresh() error {
}

func (c *client) Notify(n notify.Notification) error {
notificationToSend := notify.Notification{
Title: n.Title,
Body: n.Body,
ActionUri: n.ActionUri,
}
bodyBytes, err := json.Marshal(notificationToSend)
bodyBytes, err := json.Marshal(n)
if err != nil {
return fmt.Errorf("could not marshal notification: %w", err)
}
Expand Down
24 changes: 16 additions & 8 deletions ee/desktop/notify/notify.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,19 @@ import "time"

// Represents notification received from control server; SentAt is set by this consumer after sending.
// For the time being, notifications are per-end user device and not per-user.
type Notification struct {
Title string `json:"title"`
Body string `json:"body"`
ActionUri string `json:"action_uri,omitempty"`
ID string `json:"id"`
ValidUntil int64 `json:"valid_until"` // timestamp
SentAt time.Time `json:"sent_at,omitempty"`
}
type (
Notification struct {
Title string `json:"title"`
Body string `json:"body"`
Actions []Action `json:"actions"`
ID string `json:"id"`
ValidUntil int64 `json:"valid_until"` // timestamp
SentAt time.Time `json:"sent_at,omitempty"`
}

Action struct {
Label string `json:"label"`
Action string `json:"action"`
Default bool `json:"default"`
}
)
12 changes: 11 additions & 1 deletion ee/desktop/notify/notify_darwin.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,17 @@ func (m *macNotifier) SendNotification(n Notification) error {
defer C.free(unsafe.Pointer(titleCStr))
bodyCStr := C.CString(n.Body)
defer C.free(unsafe.Pointer(bodyCStr))
actionUriCStr := C.CString(n.ActionUri)

actionUriCStr := C.CString("")
if len(n.Actions) > 0 {
// For now, we only expect one default action
for _, a := range n.Actions {
if !a.Default {
continue
}
actionUriCStr = C.CString(a.Action)
}
}
defer C.free(unsafe.Pointer(actionUriCStr))

success := C.sendNotification(titleCStr, bodyCStr, actionUriCStr)
Expand Down
12 changes: 8 additions & 4 deletions ee/desktop/notify/notify_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,8 @@ func (d *dbusNotifier) sendNotificationViaDbus(n Notification) error {
}

actions := []string{}
if n.ActionUri != "" {
actions = append(actions, n.ActionUri, "Learn More")
for _, a := range n.Actions {
actions = append(actions, a.Action, a.Label)
}

notificationsService := conn.Object(notificationServiceInterface, notificationServiceObj)
Expand Down Expand Up @@ -170,8 +170,12 @@ func (d *dbusNotifier) sendNotificationViaNotifySend(n Notification) error {

// notify-send doesn't support actions, but URLs in notifications are clickable in at least
// some desktop environments.
if n.ActionUri != "" {
n.Body += " Learn More: " + n.ActionUri
for _, a := range n.Actions {
if !a.Default {
continue
}
n.Body += fmt.Sprintf(". %s: %s", a.Label, a.Action)
break
}

args := []string{n.Title, n.Body}
Expand Down
13 changes: 7 additions & 6 deletions ee/desktop/notify/notify_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,17 @@ func (w *windowsNotifier) SendNotification(n Notification) error {
notification.Icon = w.iconFilepath
}

if n.ActionUri != "" {
// Set the default action when the user clicks on the notification
notification.ActivationArguments = n.ActionUri
for _, a := range n.Actions {
if a.Default {
// Set the default action when the user clicks on the notification
notification.ActivationArguments = a.Action
}

// Additionally, create a "Learn more" button that will open the same URL
notification.Actions = []toast.Action{
{
Type: "protocol",
Label: "Learn More",
Arguments: n.ActionUri,
Label: a.Label,
Arguments: a.Action,
},
}
}
Expand Down