Repository: supabase/gotrue
Pull request #1203 (2023-07-31 17:11:15)

fix: support message IDs for Twilio Whatsapp
Support the use of Message IDs for Twilio Programmable Messaging on the WhatsApp channel.  Arising from a customer request during their launch.

The`whatsapp:` channel currently only works when using a phone number in the Message Service ID field. It was assumed that developers would only wish to use `whatsapp` channel with a phone number.

However, customers may wish to use [Geomatch](https://support.twilio.com/hc/en-us/articles/223181268-What-is-Geomatch-and-how-does-it-work-) in order to use both SMS and WhatsApp channels. When using Geomatch, Twilio will select the best suited number within the Sender pool of the Message Service and use that as the sender. For instance, if sending to the UK and there is a UK number Twilio and a German number will likely the UK number over the German number.

As Geomatch only works with Message IDs we need to add support for using MessageIDs  on the `whatsapp` channel on Programmable Messaging. 


Not needed for Twilio Verify as Twilio Verify does not require the channel to be prefixed before a phone number.


Patch:
diff --git a/internal/api/errors.go b/internal/api/errors.go
index 35564d1a..8efdbe81 100644
--- a/internal/api/errors.go
+++ b/internal/api/errors.go
@@ -120,6 +120,10 @@ func tooManyRequestsError(fmtString string, args ...interface{}) *HTTPError {
 	return httpError(http.StatusTooManyRequests, fmtString, args...)
 }
 
+func conflictError(fmtString string, args ...interface{}) *HTTPError {
+	return httpError(http.StatusConflict, fmtString, args...)
+}
+
 // HTTPError is an error with a message and an HTTP status code.
 type HTTPError struct {
 	Code            int    `json:"code"`
diff --git a/internal/api/phone.go b/internal/api/phone.go
index a9ffe1ff..28fcb0e4 100644
--- a/internal/api/phone.go
+++ b/internal/api/phone.go
@@ -67,30 +67,41 @@ func (a *API) sendPhoneConfirmation(ctx context.Context, tx *storage.Connection,
 		return "", internalServerError("invalid otp type")
 	}
 
+	// intentionally keeping this before the test OTP, so that the behavior
+	// of regular and test OTPs is similar
 	if sentAt != nil && !sentAt.Add(config.Sms.MaxFrequency).Before(time.Now()) {
 		return "", MaxFrequencyLimitError
 	}
-	oldToken := *token
-	otp, err := crypto.GenerateOtp(config.Sms.OtpLength)
-	if err != nil {
-		return "", internalServerError("error generating otp").WithInternalError(err)
-	}
-	*token = crypto.GenerateTokenHash(phone, otp)
 
-	var message string
-	if config.Sms.Template == "" {
-		message = fmt.Sprintf(defaultSmsMessage, otp)
-	} else {
-		message = strings.Replace(config.Sms.Template, "{{ .Code }}", otp, -1)
+	now := time.Now()
+
+	var otp, messageID string
+
+	if testOTP, ok := config.Sms.GetTestOTP(phone, now); ok {
+		otp = testOTP
+		messageID = "test-otp"
 	}
 
-	messageID, serr := smsProvider.SendMessage(phone, message, channel)
-	if serr != nil {
-		*token = oldToken
-		return messageID, serr
+	if otp == "" { // not using test OTPs
+		otp, err := crypto.GenerateOtp(config.Sms.OtpLength)
+		if err != nil {
+			return "", internalServerError("error generating otp").WithInternalError(err)
+		}
+
+		var message string
+		if config.Sms.Template == "" {
+			message = fmt.Sprintf(defaultSmsMessage, otp)
+		} else {
+			message = strings.Replace(config.Sms.Template, "{{ .Code }}", otp, -1)
+		}
+
+		messageID, err = smsProvider.SendMessage(phone, message, channel)
+		if err != nil {
+			return messageID, err
+		}
 	}
 
-	now := time.Now()
+	*token = crypto.GenerateTokenHash(phone, otp)
 
 	switch otpType {
 	case phoneConfirmationOtp:
diff --git a/internal/api/sms_provider/sms_provider.go b/internal/api/sms_provider/sms_provider.go
index e0f77f5f..4337ae37 100644
--- a/internal/api/sms_provider/sms_provider.go
+++ b/internal/api/sms_provider/sms_provider.go
@@ -51,7 +51,7 @@ func IsValidMessageChannel(channel string, smsProvider string) bool {
 	case SMSProvider:
 		return true
 	case WhatsappProvider:
-		return smsProvider == "twilio"
+		return smsProvider == "twilio" || smsProvider == "twilio_verify"
 	default:
 		return false
 	}
diff --git a/internal/api/sms_provider/twilio.go b/internal/api/sms_provider/twilio.go
index ed912ed2..ee20b65f 100644
--- a/internal/api/sms_provider/twilio.go
+++ b/internal/api/sms_provider/twilio.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"regexp"
 	"strings"
 
 	"github.com/supabase/gotrue/internal/conf"
@@ -21,6 +22,13 @@ type TwilioProvider struct {
 	APIPath string
 }
 
+var isPhoneNumber = regexp.MustCompile("^[1-9][0-9]{1,14}$")
+
+// formatPhoneNumber removes "+" and whitespaces in a phone number
+func formatPhoneNumber(phone string) string {
+	return strings.ReplaceAll(strings.TrimPrefix(phone, "+"), " ", "")
+}
+
 type SmsStatus struct {
 	To           string `json:"to"`
 	From         string `json:"from"`
@@ -70,7 +78,9 @@ func (t *TwilioProvider) SendSms(phone, message, channel string) (string, error)
 	receiver := "+" + phone
 	if channel == WhatsappProvider {
 		receiver = channel + ":" + receiver
-		sender = channel + ":" + sender
+		if isPhoneNumber.MatchString(formatPhoneNumber(sender)) {
+			sender = channel + ":" + sender
+		}
 	}
 	body := url.Values{
 		"To":      {receiver}, // twilio api requires "+" extension to be included
diff --git a/internal/api/token.go b/internal/api/token.go
index fa7dae29..afbf6057 100644
--- a/internal/api/token.go
+++ b/internal/api/token.go
@@ -320,11 +320,7 @@ func (a *API) issueRefreshToken(ctx context.Context, conn *storage.Connection, u
 			return internalServerError("Database error granting user").WithInternalError(terr)
 		}
 
-		session, terr := models.FindSessionByID(tx, *refreshToken.SessionId, false)
-		if terr != nil {
-			return terr
-		}
-		terr = models.AddClaimToSession(tx, session, authenticationMethod)
+		terr = models.AddClaimToSession(tx, *refreshToken.SessionId, authenticationMethod)
 		if terr != nil {
 			return terr
 		}
@@ -361,15 +357,10 @@ func (a *API) updateMFASessionAndClaims(r *http.Request, tx *storage.Connection,
 		return nil, internalServerError("Cannot read SessionId claim as UUID").WithInternalError(err)
 	}
 	err = tx.Transaction(func(tx *storage.Connection) error {
-		session, terr := models.FindSessionByID(tx, sessionId, false)
-		if terr != nil {
+		if terr := models.AddClaimToSession(tx, sessionId, authenticationMethod); terr != nil {
 			return terr
 		}
-		terr = models.AddClaimToSession(tx, session, authenticationMethod)
-		if terr != nil {
-			return terr
-		}
-		session, terr = models.FindSessionByID(tx, sessionId, false)
+		session, terr := models.FindSessionByID(tx, sessionId, false)
 		if terr != nil {
 			return terr
 		}
diff --git a/internal/api/token_refresh.go b/internal/api/token_refresh.go
index e2bdfca9..09f3199e 100644
--- a/internal/api/token_refresh.go
+++ b/internal/api/token_refresh.go
@@ -3,6 +3,7 @@ package api
 import (
 	"context"
 	"encoding/json"
+	mathRand "math/rand"
 	"net/http"
 	"time"
 
@@ -11,6 +12,8 @@ import (
 	"github.com/supabase/gotrue/internal/storage"
 )
 
+const retryLoopDuration = 5.0
+
 // RefreshTokenGrantParams are the parameters the RefreshTokenGrant method accepts
 type RefreshTokenGrantParams struct {
 	RefreshToken string `json:"refresh_token"`
@@ -36,111 +39,145 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h
 		return oauthError("invalid_request", "refresh_token required")
 	}
 
-	user, _, session, err := models.FindUserWithRefreshToken(db, params.RefreshToken, false)
-	if err != nil {
-		if models.IsNotFoundError(err) {
-			return oauthError("invalid_grant", "Invalid Refresh Token: Refresh Token Not Found")
-		}
-		return internalServerError(err.Error())
-	}
-
-	if user.IsBanned() {
-		return oauthError("invalid_grant", "Invalid Refresh Token: User Banned")
-	}
+	// A 5 second retry loop is used to make sure that refresh token
+	// requests do not waste database connections waiting for each other.
+	// Instead of waiting at the database level, they're waiting at the API
+	// level instead and retry to refresh the locked row every 10-30
+	// milliseconds.
+	retryStart := time.Now()
+	retry := true
 
-	if session != nil {
-		var notAfter time.Time
+	for retry && time.Since(retryStart).Seconds() < retryLoopDuration {
+		retry = false
 
-		if session.NotAfter != nil {
-			notAfter = *session.NotAfter
+		user, _, session, err := models.FindUserWithRefreshToken(db, params.RefreshToken, false)
+		if err != nil {
+			if models.IsNotFoundError(err) {
+				return oauthError("invalid_grant", "Invalid Refresh Token: Refresh Token Not Found")
+			}
+			return internalServerError(err.Error())
 		}
 
-		if !notAfter.IsZero() && time.Now().UTC().After(notAfter) {
-			return oauthError("invalid_grant", "Invalid Refresh Token: Session Expired")
+		if user.IsBanned() {
+			return oauthError("invalid_grant", "Invalid Refresh Token: User Banned")
 		}
-	}
 
-	// basic checks above passed, now we need to serialize access to the
-	// session in a transaction so that there's no concurrent modification
+		if session != nil {
+			var notAfter time.Time
 
-	var tokenString string
-	var expiresAt int64
-	var newTokenResponse *AccessTokenResponse
+			if session.NotAfter != nil {
+				notAfter = *session.NotAfter
+			}
 
-	err = db.Transaction(func(tx *storage.Connection) error {
-		user, token, _, terr := models.FindUserWithRefreshToken(tx, params.RefreshToken, true /* forUpdate */)
-		if terr != nil {
-			if models.IsNotFoundError(terr) {
-				return oauthError("invalid_grant", "Invalid Refresh Token: Refresh Token Not Found")
+			if !notAfter.IsZero() && time.Now().UTC().After(notAfter) {
+				return oauthError("invalid_grant", "Invalid Refresh Token: Session Expired")
 			}
-			return internalServerError(terr.Error())
 		}
 
-		// refresh token row and session are locked, cannot be concurrently refreshed
+		// Basic checks above passed, now we need to serialize access
+		// to the session in a transaction so that there's no
+		// concurrent modification. In the event that the refresh
+		// token's row or session is locked, the transaction is closed
+		// and the whole process will be retried a bit later so that
+		// the connection pool does not get exhausted.
+
+		var tokenString string
+		var expiresAt int64
+		var newTokenResponse *AccessTokenResponse
+
+		err = db.Transaction(func(tx *storage.Connection) error {
+			user, token, _, terr := models.FindUserWithRefreshToken(tx, params.RefreshToken, true /* forUpdate */)
+			if terr != nil {
+				if models.IsNotFoundError(terr) {
+					// because forUpdate was set, and the
+					// previous check outside the
+					// transaction found a refresh token
+					// and session, but now we're getting a
+					// IsNotFoundError, this means that the
+					// refresh token row and session are
+					// probably locked so we need to retry
+					// in a few milliseconds.
+					retry = true
+					return terr
+				}
+				return internalServerError(terr.Error())
+			}
+
+			// refresh token row and session are locked at this
+			// point, cannot be concurrently refreshed
 
-		if token.Revoked {
-			a.clearCookieTokens(config, w)
-			// For a revoked refresh token to be reused, it has to fall within the reuse interval.
+			if token.Revoked {
+				a.clearCookieTokens(config, w)
+				// For a revoked refresh token to be reused, it
+				// has to fall within the reuse interval.
 
-			reuseUntil := token.UpdatedAt.Add(
-				time.Second * time.Duration(config.Security.RefreshTokenReuseInterval))
+				reuseUntil := token.UpdatedAt.Add(
+					time.Second * time.Duration(config.Security.RefreshTokenReuseInterval))
 
-			if time.Now().After(reuseUntil) {
-				// not OK to reuse this token
+				if time.Now().After(reuseUntil) {
+					// not OK to reuse this token
 
-				if config.Security.RefreshTokenRotationEnabled {
-					// Revoke all tokens in token family
-					err = db.Transaction(func(tx *storage.Connection) error {
-						var terr error
-						if terr = models.RevokeTokenFamily(tx, token); terr != nil {
-							return terr
+					if config.Security.RefreshTokenRotationEnabled {
+						// Revoke all tokens in token family
+						if err := models.RevokeTokenFamily(tx, token); err != nil {
+							return internalServerError(err.Error())
 						}
-						return nil
-					})
-					if err != nil {
-						return internalServerError(err.Error())
 					}
+
+					return oauthError("invalid_grant", "Invalid Refresh Token: Already Used").WithInternalMessage("Possible abuse attempt: %v", token.ID)
 				}
+			}
 
-				return oauthError("invalid_grant", "Invalid Refresh Token: Already Used").WithInternalMessage("Possible abuse attempt: %v", token.ID)
+			if terr = models.NewAuditLogEntry(r, tx, user, models.TokenRefreshedAction, "", nil); terr != nil {
+				return terr
 			}
-		}
 
-		if terr = models.NewAuditLogEntry(r, tx, user, models.TokenRefreshedAction, "", nil); terr != nil {
-			return terr
-		}
+			// a new refresh token is generated and explicitly not reusing
+			// a previous one as it could have already been revoked while
+			// this handler was running
+			newToken, terr := models.GrantRefreshTokenSwap(r, tx, user, token)
+			if terr != nil {
+				return terr
+			}
 
-		// a new refresh token is generated and explicitly not reusing
-		// a previous one as it could have already been revoked while
-		// this handler was running
-		newToken, terr := models.GrantRefreshTokenSwap(r, tx, user, token)
-		if terr != nil {
-			return terr
-		}
+			tokenString, expiresAt, terr = generateAccessToken(tx, user, newToken.SessionId, &config.JWT)
 
-		tokenString, expiresAt, terr = generateAccessToken(tx, user, newToken.SessionId, &config.JWT)
+			if terr != nil {
+				return internalServerError("error generating jwt token").WithInternalError(terr)
+			}
 
-		if terr != nil {
-			return internalServerError("error generating jwt token").WithInternalError(terr)
-		}
+			newTokenResponse = &AccessTokenResponse{
+				Token:        tokenString,
+				TokenType:    "bearer",
+				ExpiresIn:    config.JWT.Exp,
+				ExpiresAt:    expiresAt,
+				RefreshToken: newToken.Token,
+				User:         user,
+			}
+			if terr = a.setCookieTokens(config, newTokenResponse, false, w); terr != nil {
+				return internalServerError("Failed to set JWT cookie. %s", terr)
+			}
 
-		newTokenResponse = &AccessTokenResponse{
-			Token:        tokenString,
-			TokenType:    "bearer",
-			ExpiresIn:    config.JWT.Exp,
-			ExpiresAt:    expiresAt,
-			RefreshToken: newToken.Token,
-			User:         user,
-		}
-		if terr = a.setCookieTokens(config, newTokenResponse, false, w); terr != nil {
-			return internalServerError("Failed to set JWT cookie. %s", terr)
+			return nil
+		})
+		if err == nil {
+			// success
+			metering.RecordLogin("token", user.ID)
+			return sendJSON(w, http.StatusOK, newTokenResponse)
 		}
 
-		return nil
-	})
-	if err != nil {
-		return err
+		if err != nil {
+			if retry && models.IsNotFoundError(err) {
+				// refresh token and session row were likely locked, so
+				// we need to wait a moment before retrying the whole
+				// process anew
+				time.Sleep(time.Duration(10+mathRand.Intn(20)) * time.Millisecond) // #nosec
+				continue
+			} else {
+				return err
+			}
+		}
 	}
-	metering.RecordLogin("token", user.ID)
-	return sendJSON(w, http.StatusOK, newTokenResponse)
+
+	return conflictError("Too many concurrent token refresh requests on the same session or refresh token")
 }
diff --git a/internal/conf/configuration.go b/internal/conf/configuration.go
index 3832e65a..c7086dd6 100644
--- a/internal/conf/configuration.go
+++ b/internal/conf/configuration.go
@@ -221,12 +221,15 @@ type PhoneProviderConfiguration struct {
 }
 
 type SmsProviderConfiguration struct {
-	Autoconfirm  bool                              `json:"autoconfirm"`
-	MaxFrequency time.Duration                     `json:"max_frequency" split_words:"true"`
-	OtpExp       uint                              `json:"otp_exp" split_words:"true"`
-	OtpLength    int                               `json:"otp_length" split_words:"true"`
-	Provider     string                            `json:"provider"`
-	Template     string                            `json:"template"`
+	Autoconfirm       bool              `json:"autoconfirm"`
+	MaxFrequency      time.Duration     `json:"max_frequency" split_words:"true"`
+	OtpExp            uint              `json:"otp_exp" split_words:"true"`
+	OtpLength         int               `json:"otp_length" split_words:"true"`
+	Provider          string            `json:"provider"`
+	Template          string            `json:"template"`
+	TestOTP           map[string]string `json:"test_otp" split_words:"true"`
+	TestOTPValidUntil time.Time         `json:"test_otp_valid_until" split_words:"true"`
+
 	Twilio       TwilioProviderConfiguration       `json:"twilio"`
 	TwilioVerify TwilioVerifyProviderConfiguration `json:"twilio_verify" split_words:"true"`
 	Messagebird  MessagebirdProviderConfiguration  `json:"messagebird"`
@@ -234,6 +237,15 @@ type SmsProviderConfiguration struct {
 	Vonage       VonageProviderConfiguration       `json:"vonage"`
 }
 
+func (c *SmsProviderConfiguration) GetTestOTP(phone string, now time.Time) (string, bool) {
+	if c.TestOTP != nil && (c.TestOTPValidUntil.IsZero() || now.Before(c.TestOTPValidUntil)) {
+		testOTP, ok := c.TestOTP[phone]
+		return testOTP, ok
+	}
+
+	return "", false
+}
+
 type TwilioProviderConfiguration struct {
 	AccountSid        string `json:"account_sid" split_words:"true"`
 	AuthToken         string `json:"auth_token" split_words:"true"`
diff --git a/internal/models/amr.go b/internal/models/amr.go
index 0dff5df8..3d3317e6 100644
--- a/internal/models/amr.go
+++ b/internal/models/amr.go
@@ -21,14 +21,14 @@ func (AMRClaim) TableName() string {
 	return tableName
 }
 
-func AddClaimToSession(tx *storage.Connection, session *Session, authenticationMethod AuthenticationMethod) error {
+func AddClaimToSession(tx *storage.Connection, sessionId uuid.UUID, authenticationMethod AuthenticationMethod) error {
 	id := uuid.Must(uuid.NewV4())
 
 	currentTime := time.Now()
 	return tx.RawQuery("INSERT INTO "+(&pop.Model{Value: AMRClaim{}}).TableName()+
 		`(id, session_id, created_at, updated_at, authentication_method) values (?, ?, ?, ?, ?)
 			ON CONFLICT ON CONSTRAINT mfa_amr_claims_session_id_authentication_method_pkey
-			DO UPDATE SET updated_at = ?;`, id, session.ID, currentTime, currentTime, authenticationMethod.String(), currentTime).Exec()
+			DO UPDATE SET updated_at = ?;`, id, sessionId, currentTime, currentTime, authenticationMethod.String(), currentTime).Exec()
 }
 
 func (a *AMRClaim) GetAuthenticationMethod() string {
diff --git a/internal/models/sessions.go b/internal/models/sessions.go
index 264fae76..d6ea13b9 100644
--- a/internal/models/sessions.go
+++ b/internal/models/sessions.go
@@ -86,6 +86,11 @@ func NewSession() (*Session, error) {
 	return session, nil
 }
 
+// FindSessionByID looks up a Session by the provided id. If forUpdate is set
+// to true, then the SELECT statement used by the query has the form SELECT ...
+// FOR UPDATE SKIP LOCKED. This means that a FOR UPDATE lock will only be
+// acquired if there's no other lock. In case there is a lock, a
+// IsNotFound(err) error will be retured.
 func FindSessionByID(tx *storage.Connection, id uuid.UUID, forUpdate bool) (*Session, error) {
 	session := &Session{}
 
@@ -94,10 +99,12 @@ func FindSessionByID(tx *storage.Connection, id uuid.UUID, forUpdate bool) (*Ses
 		// queries which lock the rows affected by the query from
 		// being accessed by any other transaction that also uses FOR
 		// UPDATE
-		if err := tx.RawQuery(fmt.Sprintf("SELECT id FROM %q WHERE id = ? LIMIT 1 FOR UPDATE;", session.TableName()), id).Exec(); err != nil {
+		if err := tx.RawQuery(fmt.Sprintf("SELECT * FROM %q WHERE id = ? LIMIT 1 FOR UPDATE SKIP LOCKED;", session.TableName()), id).First(session); err != nil {
 			if errors.Cause(err) == sql.ErrNoRows {
 				return nil, SessionNotFoundError{}
 			}
+
+			return nil, err
 		}
 	}
 
diff --git a/internal/models/user.go b/internal/models/user.go
index 2bab520a..1ae44814 100644
--- a/internal/models/user.go
+++ b/internal/models/user.go
@@ -435,7 +435,11 @@ func FindUserByEmailChangeToken(tx *storage.Connection, token string) (*User, er
 	return findUser(tx, "is_sso_user = false and (email_change_token_current = ? or email_change_token_new = ?)", token, token)
 }
 
-// FindUserWithRefreshToken finds a user from the provided refresh token.
+// FindUserWithRefreshToken finds a user from the provided refresh token. If
+// forUpdate is set to true, then the SELECT statement used by the query has
+// the form SELECT ... FOR UPDATE SKIP LOCKED. This means that a FOR UPDATE
+// lock will only be acquired if there's no other lock. In case there is a
+// lock, a IsNotFound(err) error will be returned.
 func FindUserWithRefreshToken(tx *storage.Connection, token string, forUpdate bool) (*User, *RefreshToken, *Session, error) {
 	refreshToken := &RefreshToken{}
 
@@ -444,7 +448,7 @@ func FindUserWithRefreshToken(tx *storage.Connection, token string, forUpdate bo
 		// queries which lock the rows affected by the query from
 		// being accessed by any other transaction that also uses FOR
 		// UPDATE
-		if err := tx.RawQuery(fmt.Sprintf("SELECT id FROM %q WHERE token = ? LIMIT 1 FOR UPDATE;", refreshToken.TableName()), token).Exec(); err != nil {
+		if err := tx.RawQuery(fmt.Sprintf("SELECT * FROM %q WHERE token = ? LIMIT 1 FOR UPDATE SKIP LOCKED;", refreshToken.TableName()), token).First(refreshToken); err != nil {
 			if errors.Cause(err) == sql.ErrNoRows {
 				return nil, nil, nil, RefreshTokenNotFoundError{}
 			}
@@ -474,6 +478,10 @@ func FindUserWithRefreshToken(tx *storage.Connection, token string, forUpdate bo
 		if sessionId != uuid.Nil {
 			session, err = FindSessionByID(tx, sessionId, forUpdate)
 			if err != nil {
+				if forUpdate {
+					return nil, nil, nil, err
+				}
+
 				if !IsNotFoundError(err) {
 					return nil, nil, nil, errors.Wrap(err, "error finding session from refresh token")
 				}
@@ -573,10 +581,9 @@ func IsDuplicatedEmail(tx *storage.Connection, email, aud string, currentUser *U
 
 	for _, userID := range userIDs {
 		user, err := FindUserByID(tx, userID)
-		if err != nil && !IsNotFoundError(err) {
+		if err != nil {
 			return nil, errors.Wrap(err, "unable to find user from email identity for duplicates")
 		}
-
 		if user.Aud == aud {
 			return user, nil
 		}
@@ -586,7 +593,7 @@ func IsDuplicatedEmail(tx *storage.Connection, email, aud string, currentUser *U
 	// identities table we also do a final check on the users table
 	user, err := FindUserByEmailAndAudience(tx, email, aud)
 	if err != nil && !IsNotFoundError(err) {
-		return nil, errors.Wrap(err, "unable to find user email addres for duplicates")
+		return nil, errors.Wrap(err, "unable to find user email address for duplicates")
 	}
 
 	return user, nil


Test patch:
diff --git a/internal/api/phone_test.go b/internal/api/phone_test.go
index 93a7e7d6..7047de89 100644
--- a/internal/api/phone_test.go
+++ b/internal/api/phone_test.go
@@ -28,9 +28,12 @@ type PhoneTestSuite struct {
 
 type TestSmsProvider struct {
 	mock.Mock
+
+	SentMessages int
 }
 
 func (t *TestSmsProvider) SendMessage(phone string, message string, channel string) (string, error) {
+	t.SentMessages += 1
 	return "", nil
 }
 
@@ -66,7 +69,7 @@ func (ts *PhoneTestSuite) TestFormatPhoneNumber() {
 	assert.Equal(ts.T(), "123456789", actual)
 }
 
-func (ts *PhoneTestSuite) TestSendPhoneConfirmation() {
+func doTestSendPhoneConfirmation(ts *PhoneTestSuite, useTestOTP bool) {
 	u, err := models.FindUserByPhoneAndAudience(ts.API.db, "123456789", ts.Config.JWT.Aud)
 	require.NoError(ts.T(), err)
 	ctx := context.Background()
@@ -97,13 +100,31 @@ func (ts *PhoneTestSuite) TestSendPhoneConfirmation() {
 		},
 	}
 
+	if useTestOTP {
+		ts.API.config.Sms.TestOTP = map[string]string{
+			"123456789": "123456",
+		}
+	} else {
+		ts.API.config.Sms.TestOTP = nil
+	}
+
 	for _, c := range cases {
 		ts.Run(c.desc, func() {
-			_, err = ts.API.sendPhoneConfirmation(ctx, ts.API.db, u, "123456789", c.otpType, &TestSmsProvider{}, sms_provider.SMSProvider)
+			provider := &TestSmsProvider{}
+
+			_, err = ts.API.sendPhoneConfirmation(ctx, ts.API.db, u, "123456789", c.otpType, provider, sms_provider.SMSProvider)
 			require.Equal(ts.T(), c.expected, err)
 			u, err = models.FindUserByPhoneAndAudience(ts.API.db, "123456789", ts.Config.JWT.Aud)
 			require.NoError(ts.T(), err)
 
+			if c.expected == nil {
+				if useTestOTP {
+					require.Equal(ts.T(), provider.SentMessages, 0)
+				} else {
+					require.Equal(ts.T(), provider.SentMessages, 1)
+				}
+			}
+
 			switch c.otpType {
 			case phoneConfirmationOtp:
 				require.NotEmpty(ts.T(), u.ConfirmationToken)
@@ -120,6 +141,14 @@ func (ts *PhoneTestSuite) TestSendPhoneConfirmation() {
 	}
 }
 
+func (ts *PhoneTestSuite) TestSendPhoneConfirmation() {
+	doTestSendPhoneConfirmation(ts, false)
+}
+
+func (ts *PhoneTestSuite) TestSendPhoneConfirmationWithTestOTP() {
+	doTestSendPhoneConfirmation(ts, true)
+}
+
 func (ts *PhoneTestSuite) TestMissingSmsProviderConfig() {
 	u, err := models.FindUserByPhoneAndAudience(ts.API.db, "123456789", ts.Config.JWT.Aud)
 	require.NoError(ts.T(), err)
@@ -198,6 +227,7 @@ func (ts *PhoneTestSuite) TestMissingSmsProviderConfig() {
 	ts.Config.Sms.Messagebird.AccessKey = ""
 	ts.Config.Sms.Textlocal.ApiKey = ""
 	ts.Config.Sms.Vonage.ApiKey = ""
+
 	for _, c := range cases {
 		for _, provider := range smsProviders {
 			ts.Config.Sms.Provider = provider
diff --git a/internal/models/sessions_test.go b/internal/models/sessions_test.go
index f7e9d7b8..7e72bf87 100644
--- a/internal/models/sessions_test.go
+++ b/internal/models/sessions_test.go
@@ -63,11 +63,11 @@ func (ts *SessionsTestSuite) TestCalculateAALAndAMR() {
 	session.UserID = u.ID
 	require.NoError(ts.T(), ts.db.Create(session))
 
-	err = AddClaimToSession(ts.db, session, PasswordGrant)
+	err = AddClaimToSession(ts.db, session.ID, PasswordGrant)
 	require.NoError(ts.T(), err)
 
 	firstClaimAddedTime := time.Now()
-	err = AddClaimToSession(ts.db, session, TOTPSignIn)
+	err = AddClaimToSession(ts.db, session.ID, TOTPSignIn)
 	require.NoError(ts.T(), err)
 	session, err = FindSessionByID(ts.db, session.ID, false)
 	require.NoError(ts.T(), err)
@@ -77,7 +77,7 @@ func (ts *SessionsTestSuite) TestCalculateAALAndAMR() {
 	require.Equal(ts.T(), AAL2.String(), aal)
 	require.Equal(ts.T(), totalDistinctClaims, len(amr))
 
-	err = AddClaimToSession(ts.db, session, TOTPSignIn)
+	err = AddClaimToSession(ts.db, session.ID, TOTPSignIn)
 	require.NoError(ts.T(), err)
 
 	session, err = FindSessionByID(ts.db, session.ID, false)