Skip to content

Commit

Permalink
fix: cancel notifications on missing channels and configurable (twili…
Browse files Browse the repository at this point in the history
…o) error codes (zitadel#9185)

# Which Problems Are Solved

If a notification channel was not present, notification workers would
retry to the max attempts. This leads to unnecessary load.
Additionally, a client noticed  bad actors trying to abuse SMS MFA. 

# How the Problems Are Solved

- Directly cancel the notification on:
  - a missing channel and stop retries.
  - any `4xx` errors from Twilio Verify

# Additional Changes

None

# Additional Context

reported by customer
  • Loading branch information
livio-a authored Jan 17, 2025
1 parent 69372e5 commit 60857c8
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 10 deletions.
44 changes: 38 additions & 6 deletions internal/notification/channels/twilio/channel.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,16 @@ import (
verify "github.com/twilio/twilio-go/rest/verify/v2"
"github.com/zitadel/logging"

"github.com/zitadel/zitadel/internal/eventstore"
"github.com/zitadel/zitadel/internal/notification/channels"
"github.com/zitadel/zitadel/internal/notification/messages"
"github.com/zitadel/zitadel/internal/zerrors"
)

const (
aggregateTypeNotification = "notification"
)

func InitChannel(config Config) channels.NotificationChannel {
client := twilio.NewRestClientWithParams(twilio.ClientParams{Username: config.SID, Password: config.Token})
logging.Debug("successfully initialized twilio sms channel")
Expand All @@ -30,13 +35,19 @@ func InitChannel(config Config) channels.NotificationChannel {

resp, err := client.VerifyV2.CreateVerification(config.VerifyServiceSID, params)

// In case of any client error (4xx), we should not retry sending the verification code
// as it would be a waste of resources and could potentially result in a rate limit.
var twilioErr *twilioClient.TwilioRestError
if errors.As(err, &twilioErr) && twilioErr.Code == 60203 {
// If there were too many attempts to send a verification code (more than 5 times)
// without a verification check, even retries with backoff might not solve the problem.
// Instead, let the user initiate the verification again (e.g. using "resend code")
// https://www.twilio.com/docs/api/errors/60203
logging.WithFields("error", twilioErr.Message, "code", twilioErr.Code).Warn("twilio create verification error")
if errors.As(err, &twilioErr) && twilioErr.Status >= 400 && twilioErr.Status < 500 {
userID, notificationID := userAndNotificationIDsFromEvent(twilioMsg.TriggeringEvent)
logging.WithFields(
"error", twilioErr.Message,
"status", twilioErr.Status,
"code", twilioErr.Code,
"instanceID", twilioMsg.TriggeringEvent.Aggregate().InstanceID,
"userID", userID,
"notificationID", notificationID).
Warn("twilio create verification error")
return channels.NewCancelError(twilioErr)
}

Expand Down Expand Up @@ -65,3 +76,24 @@ func InitChannel(config Config) channels.NotificationChannel {
return nil
})
}

func userAndNotificationIDsFromEvent(event eventstore.Event) (userID, notificationID string) {
aggID := event.Aggregate().ID

// we cannot cast to the actual event type because of circular dependencies
// so we just check the type...
if event.Aggregate().Type != aggregateTypeNotification {
// in case it's not a notification event, we can directly return the aggregate ID (as it's a user event)
return aggID, ""
}
// ...and unmarshal the event data from the notification event into a struct that contains the fields we need
var data struct {
Request struct {
UserID string `json:"userID"`
} `json:"request"`
}
if err := event.Unmarshal(&data); err != nil {
return "", aggID
}
return data.Request.UserID, aggID
}
9 changes: 7 additions & 2 deletions internal/notification/types/user_email.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/zitadel/logging"

"github.com/zitadel/zitadel/internal/eventstore"
zchannels "github.com/zitadel/zitadel/internal/notification/channels"
"github.com/zitadel/zitadel/internal/notification/messages"
"github.com/zitadel/zitadel/internal/notification/templates"
"github.com/zitadel/zitadel/internal/query"
Expand All @@ -27,7 +28,9 @@ func generateEmail(
emailChannels, config, err := channels.Email(ctx)
logging.OnError(err).Error("could not create email channel")
if emailChannels == nil || emailChannels.Len() == 0 {
return zerrors.ThrowPreconditionFailed(nil, "PHONE-w8nfow", "Errors.Notification.Channels.NotPresent")
return zchannels.NewCancelError(
zerrors.ThrowPreconditionFailed(nil, "MAIL-w8nfow", "Errors.Notification.Channels.NotPresent"),
)
}
recipient := user.VerifiedEmail
if lastEmail {
Expand Down Expand Up @@ -67,7 +70,9 @@ func generateEmail(
}
return webhookChannels.HandleMessage(message)
}
return zerrors.ThrowPreconditionFailed(nil, "MAIL-83nof", "Errors.Notification.Channels.NotPresent")
return zchannels.NewCancelError(
zerrors.ThrowPreconditionFailed(nil, "MAIL-83nof", "Errors.Notification.Channels.NotPresent"),
)
}

func mapNotifyUserToArgs(user *query.NotifyUser, args map[string]interface{}) map[string]interface{} {
Expand Down
9 changes: 7 additions & 2 deletions internal/notification/types/user_phone.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/zitadel/logging"

"github.com/zitadel/zitadel/internal/eventstore"
zchannels "github.com/zitadel/zitadel/internal/notification/channels"
"github.com/zitadel/zitadel/internal/notification/messages"
"github.com/zitadel/zitadel/internal/notification/senders"
"github.com/zitadel/zitadel/internal/notification/templates"
Expand All @@ -33,7 +34,9 @@ func generateSms(
smsChannels, config, err := channels.SMS(ctx)
logging.OnError(err).Error("could not create sms channel")
if smsChannels == nil || smsChannels.Len() == 0 {
return zerrors.ThrowPreconditionFailed(nil, "PHONE-w8nfow", "Errors.Notification.Channels.NotPresent")
return zchannels.NewCancelError(
zerrors.ThrowPreconditionFailed(nil, "PHONE-w8nfow", "Errors.Notification.Channels.NotPresent"),
)
}
recipient := user.VerifiedPhone
if lastPhone {
Expand Down Expand Up @@ -85,5 +88,7 @@ func generateSms(
}
return webhookChannels.HandleMessage(message)
}
return zerrors.ThrowPreconditionFailed(nil, "PHONE-w8nfow", "Errors.Notification.Channels.NotPresent")
return zchannels.NewCancelError(
zerrors.ThrowPreconditionFailed(nil, "PHONE-83nof", "Errors.Notification.Channels.NotPresent"),
)
}

0 comments on commit 60857c8

Please sign in to comment.