From 60857c8d3e0108437a8a8eb5cc4efae5e440af85 Mon Sep 17 00:00:00 2001 From: Livio Spring Date: Fri, 17 Jan 2025 08:42:14 +0100 Subject: [PATCH] fix: cancel notifications on missing channels and configurable (twilio) error codes (#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 --- .../notification/channels/twilio/channel.go | 44 ++++++++++++++++--- internal/notification/types/user_email.go | 9 +++- internal/notification/types/user_phone.go | 9 +++- 3 files changed, 52 insertions(+), 10 deletions(-) diff --git a/internal/notification/channels/twilio/channel.go b/internal/notification/channels/twilio/channel.go index 8b7f0e24f27..e13f45e00b3 100644 --- a/internal/notification/channels/twilio/channel.go +++ b/internal/notification/channels/twilio/channel.go @@ -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") @@ -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) } @@ -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 +} diff --git a/internal/notification/types/user_email.go b/internal/notification/types/user_email.go index 985fe813910..d32ee868f0d 100644 --- a/internal/notification/types/user_email.go +++ b/internal/notification/types/user_email.go @@ -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" @@ -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 { @@ -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{} { diff --git a/internal/notification/types/user_phone.go b/internal/notification/types/user_phone.go index 0016f0f7a42..3ee202dfabf 100644 --- a/internal/notification/types/user_phone.go +++ b/internal/notification/types/user_phone.go @@ -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" @@ -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 { @@ -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"), + ) }