From 6e5d8fbc934397f02831a22ba803bcdd5b13b680 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Tue, 28 May 2024 23:46:59 +0800 Subject: [PATCH 01/11] fix: check if ott exists in isOtpValid --- internal/api/reauthenticate.go | 8 +++-- internal/api/verify.go | 53 ++++++++++++++++++++-------------- 2 files changed, 38 insertions(+), 23 deletions(-) diff --git a/internal/api/reauthenticate.go b/internal/api/reauthenticate.go index 89e434514..e0793b7bf 100644 --- a/internal/api/reauthenticate.go +++ b/internal/api/reauthenticate.go @@ -84,9 +84,10 @@ func (a *API) verifyReauthentication(nonce string, tx *storage.Connection, confi return unprocessableEntityError(ErrorCodeReauthenticationNotValid, InvalidNonceMessage) } var isValid bool + var otpErr error if user.GetEmail() != "" { tokenHash := crypto.GenerateTokenHash(user.GetEmail(), nonce) - isValid = isOtpValid(tokenHash, user.ReauthenticationToken, user.ReauthenticationSentAt, config.Mailer.OtpExp) + isValid, otpErr = isOtpValid(tx, tokenHash, config.Mailer.OtpExp, models.ReauthenticationToken) } else if user.GetPhone() != "" { if config.Sms.IsTwilioVerifyProvider() { smsProvider, _ := sms_provider.GetSmsProvider(*config) @@ -96,11 +97,14 @@ func (a *API) verifyReauthentication(nonce string, tx *storage.Connection, confi return nil } else { tokenHash := crypto.GenerateTokenHash(user.GetPhone(), nonce) - isValid = isOtpValid(tokenHash, user.ReauthenticationToken, user.ReauthenticationSentAt, config.Sms.OtpExp) + isValid, otpErr = isOtpValid(tx, tokenHash, config.Sms.OtpExp, models.ReauthenticationToken) } } else { return unprocessableEntityError(ErrorCodeReauthenticationNotValid, "Reauthentication requires an email or a phone number") } + if otpErr != nil { + return internalServerError("Database error finding token").WithInternalError(otpErr) + } if !isValid { return unprocessableEntityError(ErrorCodeReauthenticationNotValid, InvalidNonceMessage) } diff --git a/internal/api/verify.go b/internal/api/verify.go index 5badfc77e..fb75e5d68 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -659,38 +659,36 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, } var isValid bool - + var otpErr error smsProvider, _ := sms_provider.GetSmsProvider(*config) switch params.Type { case mail.EmailOTPVerification: // if the type is emailOTPVerification, we'll check both the confirmation_token and recovery_token columns - if isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) { - isValid = true + // if isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) { + isValid, otpErr = isOtpValid(conn, tokenHash, config.Mailer.OtpExp, models.ConfirmationToken) + if otpErr == nil { params.Type = mail.SignupVerification - } else if isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) { - isValid = true + break + } + isValid, otpErr = isOtpValid(conn, tokenHash, config.Mailer.OtpExp, models.RecoveryToken) + if otpErr == nil { params.Type = mail.MagicLinkVerification - } else { - isValid = false + break } + // need some way to figure out whether to set the param type to signup or magiclink case mail.SignupVerification, mail.InviteVerification: - isValid = isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) + isValid, otpErr = isOtpValid(conn, tokenHash, config.Mailer.OtpExp, models.ConfirmationToken) case mail.RecoveryVerification, mail.MagicLinkVerification: - isValid = isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) + isValid, otpErr = isOtpValid(conn, tokenHash, config.Mailer.OtpExp, models.RecoveryToken) case mail.EmailChangeVerification: - isValid = isOtpValid(tokenHash, user.EmailChangeTokenCurrent, user.EmailChangeSentAt, config.Mailer.OtpExp) || - isOtpValid(tokenHash, user.EmailChangeTokenNew, user.EmailChangeSentAt, config.Mailer.OtpExp) + isValid, otpErr = isOtpValid(conn, tokenHash, config.Mailer.OtpExp, models.EmailChangeTokenCurrent, models.EmailChangeTokenNew) case phoneChangeVerification, smsVerification: phone := params.Phone - sentAt := user.ConfirmationSentAt - expectedToken := user.ConfirmationToken if params.Type == phoneChangeVerification { phone = user.PhoneChange - sentAt = user.PhoneChangeSentAt - expectedToken = user.PhoneChangeToken } if config.Sms.IsTwilioVerifyProvider() { - if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok { + if testOTP, ok := config.Sms.GetTestOTP(phone, time.Now()); ok { if params.Token == testOTP { return user, nil } @@ -700,7 +698,11 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, } return user, nil } - isValid = isOtpValid(tokenHash, expectedToken, sentAt, config.Sms.OtpExp) + isValid, otpErr = isOtpValid(conn, tokenHash, config.Sms.OtpExp, models.ConfirmationToken, models.PhoneChangeToken) + } + + if otpErr != nil { + return nil, internalServerError("Database error finding token").WithInternalError(err) } if !isValid { @@ -710,11 +712,20 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, } // isOtpValid checks the actual otp sent against the expected otp and ensures that it's within the valid window -func isOtpValid(actual, expected string, sentAt *time.Time, otpExp uint) bool { - if expected == "" || sentAt == nil { - return false +func isOtpValid(tx *storage.Connection, tokenHash string, otpExp uint, tokenTypes ...models.OneTimeTokenType) (bool, error) { + token, err := models.FindOneTimeToken(tx, tokenHash, tokenTypes...) + if err != nil { + if models.IsNotFoundError(err) { + return false, nil + } + return false, err } - return !isOtpExpired(sentAt, otpExp) && ((actual == expected) || ("pkce_"+actual == expected)) + if isOtpExpired(&token.CreatedAt, otpExp) { + return false, nil + } + + // return !isOtpExpired(sentAt, otpExp) && ((actual == expected) || ("pkce_"+actual == expected)) + return true, nil } func isOtpExpired(sentAt *time.Time, otpExp uint) bool { From 738eb12482897d9c044d78360f4c11d42695997e Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Wed, 29 May 2024 18:11:46 +0800 Subject: [PATCH 02/11] chore: CreateOneTimeToken should return ott --- internal/api/admin_test.go | 15 ++++++---- internal/api/external_test.go | 3 +- internal/api/invite_test.go | 3 +- internal/api/mail.go | 24 ++++++++-------- internal/api/phone.go | 6 ++-- internal/api/resend_test.go | 12 +++++--- internal/api/signup_test.go | 3 +- internal/api/verify_test.go | 48 ++++++++++++++++++++----------- internal/models/one_time_token.go | 9 +++--- internal/models/user_test.go | 6 ++-- 10 files changed, 80 insertions(+), 49 deletions(-) diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go index c57659414..d16bc2079 100644 --- a/internal/api/admin_test.go +++ b/internal/api/admin_test.go @@ -594,11 +594,16 @@ func (ts *AdminTestSuite) TestAdminUserSoftDeletion() { "provider": "email", } require.NoError(ts.T(), ts.API.db.Create(u)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenNew, models.EmailChangeTokenNew)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetPhone(), u.PhoneChangeToken, models.PhoneChangeToken)) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenNew, models.EmailChangeTokenNew) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetPhone(), u.PhoneChangeToken, models.PhoneChangeToken) + require.NoError(ts.T(), err) // create user identities _, err = ts.API.createNewIdentity(ts.API.db, u, "email", map[string]interface{}{ diff --git a/internal/api/external_test.go b/internal/api/external_test.go index 09fdcc433..d693c536a 100644 --- a/internal/api/external_test.go +++ b/internal/api/external_test.go @@ -57,7 +57,8 @@ func (ts *ExternalTestSuite) createUser(providerId string, email string, name st ts.Require().NoError(ts.API.db.Create(u), "Error creating user") if confirmationToken != "" { - ts.Require().NoError(models.CreateOneTimeToken(ts.API.db, u.ID, email, u.ConfirmationToken, models.ConfirmationToken), "Error creating one-time confirmation/invite token") + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, email, u.ConfirmationToken, models.ConfirmationToken) + ts.Require().NoError(err, "Error creating one-time confirmation/invite token") } i, err := models.NewIdentity(u, "email", map[string]interface{}{ diff --git a/internal/api/invite_test.go b/internal/api/invite_test.go index 1d502adc8..f713510c8 100644 --- a/internal/api/invite_test.go +++ b/internal/api/invite_test.go @@ -211,7 +211,8 @@ func (ts *InviteTestSuite) TestVerifyInvite() { user.ConfirmationToken = crypto.GenerateTokenHash(c.email, c.requestBody["token"].(string)) require.NoError(ts.T(), err) require.NoError(ts.T(), ts.API.db.Create(user)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken)) + _, err = models.CreateOneTimeToken(ts.API.db, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken) + require.NoError(ts.T(), err) // Find test user _, err = models.FindUserByEmailAndAudience(ts.API.db, c.email, ts.Config.JWT.Aud) diff --git a/internal/api/mail.go b/internal/api/mail.go index 86c31f56a..61485b2c8 100644 --- a/internal/api/mail.go +++ b/internal/api/mail.go @@ -127,7 +127,7 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { return terr } - terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.RecoveryToken, models.RecoveryToken) + _, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.RecoveryToken, models.RecoveryToken) if terr != nil { terr = errors.Wrap(terr, "Database error creating recovery token in admin") return terr @@ -180,7 +180,7 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { terr = errors.Wrap(terr, "Database error updating user for invite") return terr } - terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken) + _, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken) if terr != nil { terr = errors.Wrap(terr, "Database error creating confirmation token for invite in admin") return terr @@ -218,7 +218,7 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { terr = errors.Wrap(terr, "Database error updating user for confirmation") return terr } - terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken) + _, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken) if terr != nil { terr = errors.Wrap(terr, "Database error creating confirmation token for signup in admin") return terr @@ -251,14 +251,14 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { return terr } if user.EmailChangeTokenCurrent != "" { - terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent) + _, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent) if terr != nil { terr = errors.Wrap(terr, "Database error creating email change token current in admin") return terr } } if user.EmailChangeTokenNew != "" { - terr = models.CreateOneTimeToken(tx, user.ID, user.EmailChange, user.EmailChangeTokenNew, models.EmailChangeTokenNew) + _, terr = models.CreateOneTimeToken(tx, user.ID, user.EmailChange, user.EmailChangeTokenNew, models.EmailChangeTokenNew) if terr != nil { terr = errors.Wrap(terr, "Database error creating email change token new in admin") return terr @@ -325,7 +325,7 @@ func (a *API) sendConfirmation(r *http.Request, tx *storage.Connection, u *model return errors.Wrap(err, "Database error updating user for confirmation") } - err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) if err != nil { return errors.Wrap(err, "Database error creating confirmation token") } @@ -357,7 +357,7 @@ func (a *API) sendInvite(r *http.Request, tx *storage.Connection, u *models.User return errors.Wrap(err, "Database error updating user for invite") } - err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) if err != nil { return errors.Wrap(err, "Database error creating confirmation token for invite") } @@ -394,7 +394,7 @@ func (a *API) sendPasswordRecovery(r *http.Request, tx *storage.Connection, u *m return errors.Wrap(err, "Database error updating user for recovery") } - err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken) + _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken) if err != nil { return errors.Wrap(err, "Database error creating recovery token") } @@ -431,7 +431,7 @@ func (a *API) sendReauthenticationOtp(r *http.Request, tx *storage.Connection, u return errors.Wrap(err, "Database error updating user for reauthentication") } - err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ReauthenticationToken, models.ReauthenticationToken) + _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ReauthenticationToken, models.ReauthenticationToken) if err != nil { return errors.Wrap(err, "Database error creating reauthentication token") } @@ -471,7 +471,7 @@ func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.U return errors.Wrap(err, "Database error updating user for recovery") } - err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken) + _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken) if err != nil { return errors.Wrap(err, "Database error creating recovery token") } @@ -530,14 +530,14 @@ func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models } if u.EmailChangeTokenCurrent != "" { - err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent) + _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent) if err != nil { return errors.Wrap(err, "Database error creating email change token current") } } if u.EmailChangeTokenNew != "" { - err = models.CreateOneTimeToken(tx, u.ID, u.EmailChange, u.EmailChangeTokenNew, models.EmailChangeTokenNew) + _, err = models.CreateOneTimeToken(tx, u.ID, u.EmailChange, u.EmailChangeTokenNew, models.EmailChangeTokenNew) if err != nil { return errors.Wrap(err, "Database error creating email change token new") } diff --git a/internal/api/phone.go b/internal/api/phone.go index 83caec4af..5117db325 100644 --- a/internal/api/phone.go +++ b/internal/api/phone.go @@ -134,15 +134,15 @@ func (a *API) sendPhoneConfirmation(ctx context.Context, r *http.Request, tx *st switch otpType { case phoneConfirmationOtp: - if err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ConfirmationToken, models.ConfirmationToken); err != nil { + if _, err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ConfirmationToken, models.ConfirmationToken); err != nil { return messageID, errors.Wrap(err, "Database error creating confirmation token for phone") } case phoneChangeVerification: - if err := models.CreateOneTimeToken(tx, user.ID, user.PhoneChange, user.PhoneChangeToken, models.PhoneChangeToken); err != nil { + if _, err := models.CreateOneTimeToken(tx, user.ID, user.PhoneChange, user.PhoneChangeToken, models.PhoneChangeToken); err != nil { return messageID, errors.Wrap(err, "Database error creating phone change token") } case phoneReauthenticationOtp: - if err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ReauthenticationToken, models.ReauthenticationToken); err != nil { + if _, err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ReauthenticationToken, models.ReauthenticationToken); err != nil { return messageID, errors.Wrap(err, "Database error creating reauthentication token for phone") } } diff --git a/internal/api/resend_test.go b/internal/api/resend_test.go index 83c58c4e4..02e898e0d 100644 --- a/internal/api/resend_test.go +++ b/internal/api/resend_test.go @@ -128,8 +128,10 @@ func (ts *ResendTestSuite) TestResendSuccess() { u.EmailChangeSentAt = &now u.EmailChangeTokenNew = "123456" require.NoError(ts.T(), ts.API.db.Create(u), "Error saving new test user") - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.EmailChange, u.EmailChangeTokenNew, models.EmailChangeTokenNew)) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.EmailChange, u.EmailChangeTokenNew, models.EmailChangeTokenNew) + require.NoError(ts.T(), err) phoneUser, err := models.NewUser("1234567890", "", "password", ts.Config.JWT.Aud, nil) require.NoError(ts.T(), err, "Error creating test user model") @@ -137,7 +139,8 @@ func (ts *ResendTestSuite) TestResendSuccess() { phoneUser.EmailChangeSentAt = &now phoneUser.EmailChangeTokenNew = "123456" require.NoError(ts.T(), ts.API.db.Create(phoneUser), "Error saving new test user") - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, phoneUser.ID, phoneUser.EmailChange, phoneUser.EmailChangeTokenNew, models.EmailChangeTokenNew)) + _, err = models.CreateOneTimeToken(ts.API.db, phoneUser.ID, phoneUser.EmailChange, phoneUser.EmailChangeTokenNew, models.EmailChangeTokenNew) + require.NoError(ts.T(), err) emailUser, err := models.NewUser("", "bar@example.com", "password", ts.Config.JWT.Aud, nil) require.NoError(ts.T(), err, "Error creating test user model") @@ -145,7 +148,8 @@ func (ts *ResendTestSuite) TestResendSuccess() { phoneUser.PhoneChangeSentAt = &now phoneUser.PhoneChangeToken = "123456" require.NoError(ts.T(), ts.API.db.Create(emailUser), "Error saving new test user") - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, phoneUser.ID, phoneUser.PhoneChange, phoneUser.PhoneChangeToken, models.PhoneChangeToken)) + _, err = models.CreateOneTimeToken(ts.API.db, phoneUser.ID, phoneUser.PhoneChange, phoneUser.PhoneChangeToken, models.PhoneChangeToken) + require.NoError(ts.T(), err) cases := []struct { desc string diff --git a/internal/api/signup_test.go b/internal/api/signup_test.go index 3f4783261..fc0267021 100644 --- a/internal/api/signup_test.go +++ b/internal/api/signup_test.go @@ -128,7 +128,8 @@ func (ts *SignupTestSuite) TestVerifySignup() { user.ConfirmationSentAt = &now require.NoError(ts.T(), err) require.NoError(ts.T(), ts.API.db.Create(user)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken)) + _, err = models.CreateOneTimeToken(ts.API.db, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken) + require.NoError(ts.T(), err) // Find test user u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) diff --git a/internal/api/verify_test.go b/internal/api/verify_test.go index 8d818b43a..009a84984 100644 --- a/internal/api/verify_test.go +++ b/internal/api/verify_test.go @@ -287,7 +287,8 @@ func (ts *VerifyTestSuite) TestExpiredConfirmationToken() { sentTime := time.Now().Add(-48 * time.Hour) u.ConfirmationSentAt = &sentTime require.NoError(ts.T(), ts.API.db.Update(u)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken)) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + require.NoError(ts.T(), err) // Setup request reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s", mail.SignupVerification, u.ConfirmationToken) @@ -319,8 +320,10 @@ func (ts *VerifyTestSuite) TestInvalidOtp() { u.PhoneChangeToken = "123456" u.PhoneChangeSentAt = &sentTime require.NoError(ts.T(), ts.API.db.Update(u)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.PhoneChange, u.PhoneChangeToken, models.PhoneChangeToken)) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.PhoneChange, u.PhoneChangeToken, models.PhoneChangeToken) + require.NoError(ts.T(), err) type ResponseBody struct { Code int `json:"code"` @@ -643,7 +646,8 @@ func (ts *VerifyTestSuite) TestVerifySignupWithRedirectURLContainedPath() { sendTime := time.Now().Add(time.Hour) u.ConfirmationSentAt = &sendTime require.NoError(ts.T(), ts.API.db.Update(u)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken)) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + require.NoError(ts.T(), err) reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s&redirect_to=%s", "signup", u.ConfirmationToken, redirectURL) req := httptest.NewRequest(http.MethodGet, reqURL, nil) @@ -698,9 +702,11 @@ func (ts *VerifyTestSuite) TestVerifyPKCEOTP() { // since the test user is the same, the tokens are being cleared after each successful verification attempt // so we create them on each run if c.payload.Type == "signup" { - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), c.payload.Token, models.ConfirmationToken)) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), c.payload.Token, models.ConfirmationToken) + require.NoError(ts.T(), err) } else if c.payload.Type == "magiclink" { - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), c.payload.Token, models.RecoveryToken)) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), c.payload.Token, models.RecoveryToken) + require.NoError(ts.T(), err) } require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.payload)) @@ -744,10 +750,14 @@ func (ts *VerifyTestSuite) TestVerifyBannedUser() { t = time.Now().Add(24 * time.Hour) u.BannedUntil = &t require.NoError(ts.T(), ts.API.db.Update(u)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenNew, models.EmailChangeTokenNew)) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenNew, models.EmailChangeTokenNew) + require.NoError(ts.T(), err) cases := []struct { desc string @@ -964,10 +974,14 @@ func (ts *VerifyTestSuite) TestVerifyValidOtp() { u.EmailChangeTokenNew = c.expected.tokenHash u.PhoneChangeToken = c.expected.tokenHash - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.ConfirmationToken, models.ConfirmationToken)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.RecoveryToken, models.RecoveryToken)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.EmailChangeTokenNew, models.EmailChangeTokenNew)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.PhoneChangeToken, models.PhoneChangeToken)) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.ConfirmationToken, models.ConfirmationToken) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.RecoveryToken, models.RecoveryToken) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.EmailChangeTokenNew, models.EmailChangeTokenNew) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.PhoneChangeToken, models.PhoneChangeToken) + require.NoError(ts.T(), err) require.NoError(ts.T(), ts.API.db.Update(u)) @@ -1035,8 +1049,10 @@ func (ts *VerifyTestSuite) TestSecureEmailChangeWithTokenHash() { u.EmailChangeTokenNew = newEmailChangeToken require.NoError(ts.T(), models.ClearAllOneTimeTokensForUser(ts.API.db, u.ID)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", currentEmailChangeToken, models.EmailChangeTokenCurrent)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", newEmailChangeToken, models.EmailChangeTokenNew)) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", currentEmailChangeToken, models.EmailChangeTokenCurrent) + require.NoError(ts.T(), err) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", newEmailChangeToken, models.EmailChangeTokenNew) + require.NoError(ts.T(), err) currentTime := time.Now() u.EmailChangeSentAt = ¤tTime diff --git a/internal/models/one_time_token.go b/internal/models/one_time_token.go index c5a204902..afee99c57 100644 --- a/internal/models/one_time_token.go +++ b/internal/models/one_time_token.go @@ -128,9 +128,9 @@ func ClearOneTimeTokenForUser(tx *storage.Connection, userID uuid.UUID, tokenTyp return nil } -func CreateOneTimeToken(tx *storage.Connection, userID uuid.UUID, relatesTo, tokenHash string, tokenType OneTimeTokenType) error { +func CreateOneTimeToken(tx *storage.Connection, userID uuid.UUID, relatesTo, tokenHash string, tokenType OneTimeTokenType) (*OneTimeToken, error) { if err := ClearOneTimeTokenForUser(tx, userID, tokenType); err != nil { - return err + return nil, err } oneTimeToken := &OneTimeToken{ @@ -139,13 +139,14 @@ func CreateOneTimeToken(tx *storage.Connection, userID uuid.UUID, relatesTo, tok TokenType: tokenType, TokenHash: tokenHash, RelatesTo: strings.ToLower(relatesTo), + CreatedAt: time.Now(), } if err := tx.Eager().Create(oneTimeToken); err != nil { - return err + return nil, err } - return nil + return oneTimeToken, nil } func FindOneTimeToken(tx *storage.Connection, tokenHash string, tokenTypes ...OneTimeTokenType) (*OneTimeToken, error) { diff --git a/internal/models/user_test.go b/internal/models/user_test.go index 6c915f6af..c5b7608b1 100644 --- a/internal/models/user_test.go +++ b/internal/models/user_test.go @@ -84,7 +84,8 @@ func (ts *UserTestSuite) TestUpdateUserMetadata() { func (ts *UserTestSuite) TestFindUserByConfirmationToken() { u := ts.createUser() tokenHash := "test_confirmation_token" - require.NoError(ts.T(), CreateOneTimeToken(ts.db, u.ID, "relates_to not used", tokenHash, ConfirmationToken)) + _, err := CreateOneTimeToken(ts.db, u.ID, "relates_to not used", tokenHash, ConfirmationToken) + require.NoError(ts.T(), err) n, err := FindUserByConfirmationToken(ts.db, tokenHash) require.NoError(ts.T(), err) @@ -139,7 +140,8 @@ func (ts *UserTestSuite) TestFindUserByID() { func (ts *UserTestSuite) TestFindUserByRecoveryToken() { u := ts.createUser() tokenHash := "test_recovery_token" - require.NoError(ts.T(), CreateOneTimeToken(ts.db, u.ID, "relates_to not used", tokenHash, RecoveryToken)) + _, err := CreateOneTimeToken(ts.db, u.ID, "relates_to not used", tokenHash, RecoveryToken) + require.NoError(ts.T(), err) n, err := FindUserByRecoveryToken(ts.db, tokenHash) require.NoError(ts.T(), err) From 6bddb195368e5f2aae3935d49df0024556ee3868 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Thu, 6 Jun 2024 16:50:58 +0800 Subject: [PATCH 03/11] fix: generate link should use ott instead of tokens in user model --- internal/api/mail.go | 64 +++++++++++++++++++------------------ internal/api/mail_test.go | 4 +-- internal/mailer/mailer.go | 2 +- internal/mailer/template.go | 18 +++++++---- 4 files changed, 47 insertions(+), 41 deletions(-) diff --git a/internal/api/mail.go b/internal/api/mail.go index 61485b2c8..3ed0bdb55 100644 --- a/internal/api/mail.go +++ b/internal/api/mail.go @@ -112,6 +112,7 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { } } + var ott *models.OneTimeToken err = db.Transaction(func(tx *storage.Connection) error { var terr error switch params.Type { @@ -119,15 +120,14 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { if terr = models.NewAuditLogEntry(r, tx, user, models.UserRecoveryRequestedAction, "", nil); terr != nil { return terr } - user.RecoveryToken = hashedToken user.RecoverySentAt = &now - terr = tx.UpdateOnly(user, "recovery_token", "recovery_sent_at") + terr = tx.UpdateOnly(user, "recovery_sent_at") if terr != nil { terr = errors.Wrap(terr, "Database error updating user for recovery") return terr } - _, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.RecoveryToken, models.RecoveryToken) + ott, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), hashedToken, models.RecoveryToken) if terr != nil { terr = errors.Wrap(terr, "Database error creating recovery token in admin") return terr @@ -172,15 +172,14 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { }); terr != nil { return terr } - user.ConfirmationToken = hashedToken user.ConfirmationSentAt = &now user.InvitedAt = &now - terr = tx.UpdateOnly(user, "confirmation_token", "confirmation_sent_at", "invited_at") + terr = tx.UpdateOnly(user, "confirmation_sent_at", "invited_at") if terr != nil { terr = errors.Wrap(terr, "Database error updating user for invite") return terr } - _, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken) + ott, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), hashedToken, models.ConfirmationToken) if terr != nil { terr = errors.Wrap(terr, "Database error creating confirmation token for invite in admin") return terr @@ -211,20 +210,19 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { } user.Identities = []models.Identity{*identity} } - user.ConfirmationToken = hashedToken user.ConfirmationSentAt = &now - terr = tx.UpdateOnly(user, "confirmation_token", "confirmation_sent_at") + terr = tx.UpdateOnly(user, "confirmation_sent_at") if terr != nil { terr = errors.Wrap(terr, "Database error updating user for confirmation") return terr } - _, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken) + ott, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), hashedToken, models.ConfirmationToken) if terr != nil { terr = errors.Wrap(terr, "Database error creating confirmation token for signup in admin") return terr } - case mail.EmailChangeCurrentVerification, mail.EmailChangeNewVerification: - if !config.Mailer.SecureEmailChangeEnabled && params.Type == "email_change_current" { + case mail.EmailChangeCurrentVerification: + if !config.Mailer.SecureEmailChangeEnabled { return badRequestError(ErrorCodeValidationFailed, "Enable secure email change to generate link for current email") } params.NewEmail, terr = validateEmail(params.NewEmail) @@ -240,29 +238,33 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { user.EmailChangeSentAt = &now user.EmailChange = params.NewEmail user.EmailChangeConfirmStatus = zeroConfirmation - if params.Type == "email_change_current" { - user.EmailChangeTokenCurrent = hashedToken - } else if params.Type == "email_change_new" { - user.EmailChangeTokenNew = crypto.GenerateTokenHash(params.NewEmail, otp) + if terr := tx.UpdateOnly(user, "email_change", "email_change_sent_at", "email_change_confirm_status"); terr != nil { + return errors.Wrap(terr, "Database error updating user for email change") } - terr = tx.UpdateOnly(user, "email_change_token_current", "email_change_token_new", "email_change", "email_change_sent_at", "email_change_confirm_status") + ott, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), hashedToken, models.EmailChangeTokenCurrent) + if terr != nil { + return errors.Wrap(terr, "Database error creating email change token current in admin") + } + case mail.EmailChangeNewVerification: + params.NewEmail, terr = validateEmail(params.NewEmail) if terr != nil { - terr = errors.Wrap(terr, "Database error updating user for email change") return terr } - if user.EmailChangeTokenCurrent != "" { - _, terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent) - if terr != nil { - terr = errors.Wrap(terr, "Database error creating email change token current in admin") - return terr - } + if duplicateUser, terr := models.IsDuplicatedEmail(tx, params.NewEmail, user.Aud, user); terr != nil { + return internalServerError("Database error checking email").WithInternalError(terr) + } else if duplicateUser != nil { + return unprocessableEntityError(ErrorCodeEmailExists, DuplicateEmailMsg) } - if user.EmailChangeTokenNew != "" { - _, terr = models.CreateOneTimeToken(tx, user.ID, user.EmailChange, user.EmailChangeTokenNew, models.EmailChangeTokenNew) - if terr != nil { - terr = errors.Wrap(terr, "Database error creating email change token new in admin") - return terr - } + now := time.Now() + user.EmailChangeSentAt = &now + user.EmailChange = params.NewEmail + user.EmailChangeConfirmStatus = zeroConfirmation + if terr := tx.UpdateOnly(user, "email_change", "email_change_sent_at", "email_change_confirm_status"); terr != nil { + return errors.Wrap(terr, "Database error updating user for email change") + } + ott, terr = models.CreateOneTimeToken(tx, user.ID, user.EmailChange, crypto.GenerateTokenHash(params.NewEmail, otp), models.EmailChangeTokenNew) + if terr != nil { + return errors.Wrap(terr, "Database error creating email change token new in admin") } default: return badRequestError(ErrorCodeValidationFailed, "Invalid email action link type requested: %v", params.Type) @@ -273,9 +275,9 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { } externalURL := getExternalHost(ctx) - url, terr = mailer.GetEmailActionLink(user, params.Type, referrer, externalURL) + url, terr = mailer.GetEmailActionLink(ott, params.Type, referrer, externalURL) if terr != nil { - return terr + return internalServerError("Error generating email action link").WithInternalError(terr) } return nil }) diff --git a/internal/api/mail_test.go b/internal/api/mail_test.go index 6063a9a6b..dbd5a8d45 100644 --- a/internal/api/mail_test.go +++ b/internal/api/mail_test.go @@ -135,7 +135,7 @@ func (ts *MailTestSuite) TestGenerateLink() { }, }, { - Desc: "Generate email change link", + Desc: "Generate email change link current", Body: GenerateLinkParams{ Email: "test@example.com", NewEmail: "new@example.com", @@ -147,7 +147,7 @@ func (ts *MailTestSuite) TestGenerateLink() { }, }, { - Desc: "Generate email change link", + Desc: "Generate email change link new", Body: GenerateLinkParams{ Email: "test@example.com", NewEmail: "new@example.com", diff --git a/internal/mailer/mailer.go b/internal/mailer/mailer.go index ff19239d8..d579af400 100644 --- a/internal/mailer/mailer.go +++ b/internal/mailer/mailer.go @@ -23,7 +23,7 @@ type Mailer interface { EmailChangeMail(r *http.Request, user *models.User, otpNew, otpCurrent, referrerURL string, externalURL *url.URL) error ReauthenticateMail(r *http.Request, user *models.User, otp string) error ValidateEmail(email string) error - GetEmailActionLink(user *models.User, actionType, referrerURL string, externalURL *url.URL) (string, error) + GetEmailActionLink(ott *models.OneTimeToken, actionType, referrerURL string, externalURL *url.URL) (string, error) } type EmailParams struct { diff --git a/internal/mailer/template.go b/internal/mailer/template.go index cd1aacf95..7ede00f37 100644 --- a/internal/mailer/template.go +++ b/internal/mailer/template.go @@ -312,44 +312,48 @@ func (m TemplateMailer) Send(user *models.User, subject, body string, data map[s } // GetEmailActionLink returns a magiclink, recovery or invite link based on the actionType passed. -func (m TemplateMailer) GetEmailActionLink(user *models.User, actionType, referrerURL string, externalURL *url.URL) (string, error) { +func (m TemplateMailer) GetEmailActionLink(ott *models.OneTimeToken, actionType, referrerURL string, externalURL *url.URL) (string, error) { + if ott == nil { + return "", fmt.Errorf("ott is nil") + } + var err error var path *url.URL switch actionType { case "magiclink": path, err = getPath(m.Config.Mailer.URLPaths.Recovery, &EmailParams{ - Token: user.RecoveryToken, + Token: ott.TokenHash, Type: "magiclink", RedirectTo: referrerURL, }) case "recovery": path, err = getPath(m.Config.Mailer.URLPaths.Recovery, &EmailParams{ - Token: user.RecoveryToken, + Token: ott.TokenHash, Type: "recovery", RedirectTo: referrerURL, }) case "invite": path, err = getPath(m.Config.Mailer.URLPaths.Invite, &EmailParams{ - Token: user.ConfirmationToken, + Token: ott.TokenHash, Type: "invite", RedirectTo: referrerURL, }) case "signup": path, err = getPath(m.Config.Mailer.URLPaths.Confirmation, &EmailParams{ - Token: user.ConfirmationToken, + Token: ott.TokenHash, Type: "signup", RedirectTo: referrerURL, }) case "email_change_current": path, err = getPath(m.Config.Mailer.URLPaths.EmailChange, &EmailParams{ - Token: user.EmailChangeTokenCurrent, + Token: ott.TokenHash, Type: "email_change", RedirectTo: referrerURL, }) case "email_change_new": path, err = getPath(m.Config.Mailer.URLPaths.EmailChange, &EmailParams{ - Token: user.EmailChangeTokenNew, + Token: ott.TokenHash, Type: "email_change", RedirectTo: referrerURL, }) From b42f1b37bbc64643498696938ef6122576d501f0 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Fri, 7 Jun 2024 12:23:08 +0800 Subject: [PATCH 04/11] feat: deprecate token columns in user model --- internal/api/mail.go | 217 +++++++++++++++------------------ internal/api/phone.go | 19 ++- internal/api/reauthenticate.go | 2 +- internal/api/verify.go | 13 +- internal/mailer/mailer.go | 11 +- internal/mailer/template.go | 141 +++++++-------------- internal/models/user.go | 66 +++------- 7 files changed, 184 insertions(+), 285 deletions(-) diff --git a/internal/api/mail.go b/internal/api/mail.go index 3ed0bdb55..683a9a88f 100644 --- a/internal/api/mail.go +++ b/internal/api/mail.go @@ -303,65 +303,51 @@ func (a *API) sendConfirmation(r *http.Request, tx *storage.Connection, u *model maxFrequency := config.SMTP.MaxFrequency otpLength := config.Mailer.OtpLength - var err error if err := validateSentWithinFrequencyLimit(u.ConfirmationSentAt, maxFrequency); err != nil { return err } - oldToken := u.ConfirmationToken otp, err := crypto.GenerateOtp(otpLength) if err != nil { // OTP generation must succeeed panic(err) } - token := crypto.GenerateTokenHash(u.GetEmail(), otp) - u.ConfirmationToken = addFlowPrefixToToken(token, flowType) - now := time.Now() - err = a.sendEmail(r, tx, u, mail.SignupVerification, otp, "", u.ConfirmationToken) + token := addFlowPrefixToToken(crypto.GenerateTokenHash(u.GetEmail(), otp), flowType) + ott, err := models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), token, models.ConfirmationToken) if err != nil { - u.ConfirmationToken = oldToken - return errors.Wrap(err, "Error sending confirmation email") + return errors.Wrap(err, "Database error creating confirmation token") } + now := time.Now() u.ConfirmationSentAt = &now - err = tx.UpdateOnly(u, "confirmation_token", "confirmation_sent_at") - if err != nil { + if err := tx.UpdateOnly(u, "confirmation_sent_at"); err != nil { return errors.Wrap(err, "Database error updating user for confirmation") } - - _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) - if err != nil { - return errors.Wrap(err, "Database error creating confirmation token") + if err := a.sendEmail(r, tx, u, ott, mail.SignupVerification, otp); err != nil { + return errors.Wrap(err, "Error sending confirmation email") } - return nil } func (a *API) sendInvite(r *http.Request, tx *storage.Connection, u *models.User) error { config := a.config otpLength := config.Mailer.OtpLength - var err error - oldToken := u.ConfirmationToken otp, err := crypto.GenerateOtp(otpLength) if err != nil { // OTP generation must succeed panic(err) } - u.ConfirmationToken = crypto.GenerateTokenHash(u.GetEmail(), otp) - now := time.Now() - err = a.sendEmail(r, tx, u, mail.InviteVerification, otp, "", u.ConfirmationToken) + token := crypto.GenerateTokenHash(u.GetEmail(), otp) + ott, err := models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), token, models.ConfirmationToken) if err != nil { - u.ConfirmationToken = oldToken - return errors.Wrap(err, "Error sending invite email") + return errors.Wrap(err, "Database error creating confirmation token for invite") } + now := time.Now() u.InvitedAt = &now u.ConfirmationSentAt = &now - err = tx.UpdateOnly(u, "confirmation_token", "confirmation_sent_at", "invited_at") - if err != nil { + if err := tx.UpdateOnly(u, "confirmation_sent_at", "invited_at"); err != nil { return errors.Wrap(err, "Database error updating user for invite") } - - _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) - if err != nil { - return errors.Wrap(err, "Database error creating confirmation token for invite") + if err := a.sendEmail(r, tx, u, ott, mail.InviteVerification, otp); err != nil { + return errors.Wrap(err, "Error sending invite email") } return nil @@ -371,34 +357,27 @@ func (a *API) sendPasswordRecovery(r *http.Request, tx *storage.Connection, u *m config := a.config maxFrequency := config.SMTP.MaxFrequency otpLength := config.Mailer.OtpLength - var err error if err := validateSentWithinFrequencyLimit(u.RecoverySentAt, maxFrequency); err != nil { return err } - - oldToken := u.RecoveryToken otp, err := crypto.GenerateOtp(otpLength) if err != nil { // OTP generation must succeed panic(err) } - token := crypto.GenerateTokenHash(u.GetEmail(), otp) - u.RecoveryToken = addFlowPrefixToToken(token, flowType) - now := time.Now() - err = a.sendEmail(r, tx, u, mail.RecoveryVerification, otp, "", u.RecoveryToken) + token := addFlowPrefixToToken(crypto.GenerateTokenHash(u.GetEmail(), otp), flowType) + ott, err := models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), token, models.RecoveryToken) if err != nil { - u.RecoveryToken = oldToken - return errors.Wrap(err, "Error sending recovery email") + return errors.Wrap(err, "Database error creating recovery token") } + now := time.Now() u.RecoverySentAt = &now - err = tx.UpdateOnly(u, "recovery_token", "recovery_sent_at") + err = tx.UpdateOnly(u, "recovery_sent_at") if err != nil { return errors.Wrap(err, "Database error updating user for recovery") } - - _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken) - if err != nil { - return errors.Wrap(err, "Database error creating recovery token") + if err = a.sendEmail(r, tx, u, ott, mail.RecoveryVerification, otp); err != nil { + return errors.Wrap(err, "Error sending recovery email") } return nil @@ -408,36 +387,27 @@ func (a *API) sendReauthenticationOtp(r *http.Request, tx *storage.Connection, u config := a.config maxFrequency := config.SMTP.MaxFrequency otpLength := config.Mailer.OtpLength - var err error - if err := validateSentWithinFrequencyLimit(u.ReauthenticationSentAt, maxFrequency); err != nil { return err } - - oldToken := u.ReauthenticationToken otp, err := crypto.GenerateOtp(otpLength) if err != nil { // OTP generation must succeed panic(err) } - u.ReauthenticationToken = crypto.GenerateTokenHash(u.GetEmail(), otp) - now := time.Now() - err = a.sendEmail(r, tx, u, mail.ReauthenticationVerification, otp, "", u.ReauthenticationToken) + token := crypto.GenerateTokenHash(u.GetEmail(), otp) + ott, err := models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), token, models.ReauthenticationToken) if err != nil { - u.ReauthenticationToken = oldToken - return errors.Wrap(err, "Error sending reauthentication email") + return errors.Wrap(err, "Database error creating reauthentication token") } + now := time.Now() u.ReauthenticationSentAt = &now - err = tx.UpdateOnly(u, "reauthentication_token", "reauthentication_sent_at") - if err != nil { + if err := tx.UpdateOnly(u, "reauthentication_sent_at"); err != nil { return errors.Wrap(err, "Database error updating user for reauthentication") } - - _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ReauthenticationToken, models.ReauthenticationToken) - if err != nil { - return errors.Wrap(err, "Database error creating reauthentication token") + if err := a.sendEmail(r, tx, u, ott, mail.ReauthenticationVerification, otp); err != nil { + return errors.Wrap(err, "Error sending reauthentication email") } - return nil } @@ -451,33 +421,24 @@ func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.U if err := validateSentWithinFrequencyLimit(u.RecoverySentAt, maxFrequency); err != nil { return err } - - oldToken := u.RecoveryToken otp, err := crypto.GenerateOtp(otpLength) if err != nil { // OTP generation must succeed panic(err) } - token := crypto.GenerateTokenHash(u.GetEmail(), otp) - u.RecoveryToken = addFlowPrefixToToken(token, flowType) - - now := time.Now() - err = a.sendEmail(r, tx, u, mail.MagicLinkVerification, otp, "", u.RecoveryToken) + token := addFlowPrefixToToken(crypto.GenerateTokenHash(u.GetEmail(), otp), flowType) + ott, err := models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), token, models.RecoveryToken) if err != nil { - u.RecoveryToken = oldToken - return errors.Wrap(err, "Error sending magic link email") + return errors.Wrap(err, "Database error creating recovery token") } + now := time.Now() u.RecoverySentAt = &now - err = tx.UpdateOnly(u, "recovery_token", "recovery_sent_at") - if err != nil { + if err = tx.UpdateOnly(u, "recovery_sent_at"); err != nil { return errors.Wrap(err, "Database error updating user for recovery") } - - _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken) - if err != nil { - return errors.Wrap(err, "Database error creating recovery token") + if err := a.sendEmail(r, tx, u, ott, mail.MagicLinkVerification, otp); err != nil { + return errors.Wrap(err, "Error sending magic link email") } - return nil } @@ -485,7 +446,6 @@ func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.U func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models.User, email string, flowType models.FlowType) error { config := a.config otpLength := config.Mailer.OtpLength - var err error if err := validateSentWithinFrequencyLimit(u.EmailChangeSentAt, config.SMTP.MaxFrequency); err != nil { return err } @@ -496,53 +456,41 @@ func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models panic(err) } u.EmailChange = email - token := crypto.GenerateTokenHash(u.EmailChange, otpNew) - u.EmailChangeTokenNew = addFlowPrefixToToken(token, flowType) + token := addFlowPrefixToToken(crypto.GenerateTokenHash(u.EmailChange, otpNew), flowType) + ottNew, err := models.CreateOneTimeToken(tx, u.ID, u.EmailChange, token, models.EmailChangeTokenNew) + if err != nil { + return errors.Wrap(err, "Database error creating email change token new") + } otpCurrent := "" + var ottCurrent *models.OneTimeToken if config.Mailer.SecureEmailChangeEnabled && u.GetEmail() != "" { otpCurrent, err = crypto.GenerateOtp(otpLength) if err != nil { // OTP generation must succeed panic(err) } - currentToken := crypto.GenerateTokenHash(u.GetEmail(), otpCurrent) - u.EmailChangeTokenCurrent = addFlowPrefixToToken(currentToken, flowType) + currentToken := addFlowPrefixToToken(crypto.GenerateTokenHash(u.GetEmail(), otpCurrent), flowType) + ottCurrent, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), currentToken, models.EmailChangeTokenCurrent) + if err != nil { + return errors.Wrap(err, "Database error creating email change token current") + } } - u.EmailChangeConfirmStatus = zeroConfirmation now := time.Now() - err = a.sendEmail(r, tx, u, mail.EmailChangeVerification, otpCurrent, otpNew, u.EmailChangeTokenNew) - if err != nil { - return err - } - + u.EmailChangeConfirmStatus = zeroConfirmation u.EmailChangeSentAt = &now - err = tx.UpdateOnly( + if err := tx.UpdateOnly( u, - "email_change_token_current", - "email_change_token_new", "email_change", "email_change_sent_at", "email_change_confirm_status", - ) - - if err != nil { + ); err != nil { return errors.Wrap(err, "Database error updating user for email change") } - if u.EmailChangeTokenCurrent != "" { - _, err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent) - if err != nil { - return errors.Wrap(err, "Database error creating email change token current") - } - } - - if u.EmailChangeTokenNew != "" { - _, err = models.CreateOneTimeToken(tx, u.ID, u.EmailChange, u.EmailChangeTokenNew, models.EmailChangeTokenNew) - if err != nil { - return errors.Wrap(err, "Database error creating email change token new") - } + if err = a.sendEmailChangeEmails(r, tx, u, ottNew, ottCurrent, mail.EmailChangeVerification, otpCurrent, otpNew); err != nil { + return err } return nil @@ -565,7 +513,7 @@ func validateSentWithinFrequencyLimit(sentAt *time.Time, frequency time.Duration return nil } -func (a *API) sendEmail(r *http.Request, tx *storage.Connection, u *models.User, emailActionType, otp, otpNew, tokenHashWithPrefix string) error { +func (a *API) sendEmail(r *http.Request, tx *storage.Connection, u *models.User, ott *models.OneTimeToken, emailActionType, otp string) error { mailer := a.Mailer() ctx := r.Context() config := a.config @@ -577,34 +525,71 @@ func (a *API) sendEmail(r *http.Request, tx *storage.Connection, u *models.User, EmailActionType: emailActionType, RedirectTo: referrerURL, SiteURL: externalURL.String(), - TokenHash: tokenHashWithPrefix, - } - if emailActionType == mail.EmailChangeVerification && config.Mailer.SecureEmailChangeEnabled && u.GetEmail() != "" { - emailData.TokenNew = otpNew - emailData.TokenHashNew = u.EmailChangeTokenCurrent + TokenHash: ott.TokenHash, } input := hooks.SendEmailInput{ User: u, EmailData: emailData, } output := hooks.SendEmailOutput{} - return a.invokeHook(tx, r, &input, &output, a.config.Hook.SendEmail.URI) + return a.invokeHook(tx, r, &input, &output, config.Hook.SendEmail.URI) } switch emailActionType { case mail.SignupVerification: - return mailer.ConfirmationMail(r, u, otp, referrerURL, externalURL) + return mailer.ConfirmationMail(r, u, ott, otp, referrerURL, externalURL) case mail.MagicLinkVerification: - return mailer.MagicLinkMail(r, u, otp, referrerURL, externalURL) + return mailer.MagicLinkMail(r, u, ott, otp, referrerURL, externalURL) case mail.ReauthenticationVerification: return mailer.ReauthenticateMail(r, u, otp) case mail.RecoveryVerification: - return mailer.RecoveryMail(r, u, otp, referrerURL, externalURL) + return mailer.RecoveryMail(r, u, ott, otp, referrerURL, externalURL) case mail.InviteVerification: - return mailer.InviteMail(r, u, otp, referrerURL, externalURL) - case mail.EmailChangeVerification: - return mailer.EmailChangeMail(r, u, otpNew, otp, referrerURL, externalURL) + return mailer.InviteMail(r, u, ott, otp, referrerURL, externalURL) default: return errors.New("invalid email action type") } } + +func (a *API) sendEmailChangeEmails(r *http.Request, tx *storage.Connection, u *models.User, ottNew, ottCurrent *models.OneTimeToken, emailActionType, otp, otpNew string) error { + mailer := a.Mailer() + ctx := r.Context() + config := a.config + referrerURL := utilities.GetReferrer(r, config) + externalURL := getExternalHost(ctx) + if config.Hook.SendEmail.Enabled { + emailData := mail.EmailData{ + Token: otpNew, + EmailActionType: emailActionType, + RedirectTo: referrerURL, + SiteURL: externalURL.String(), + TokenHash: ottNew.TokenHash, + } + if config.Mailer.SecureEmailChangeEnabled && u.GetEmail() != "" { + emailData.TokenNew = otp + emailData.TokenHashNew = ottCurrent.TokenHash + } + input := hooks.SendEmailInput{ + User: u, + EmailData: emailData, + } + output := hooks.SendEmailOutput{} + return a.invokeHook(tx, r, &input, &output, config.Hook.SendEmail.URI) + } + + errors := make(chan error) + if config.Mailer.SecureEmailChangeEnabled && u.GetEmail() != "" { + go func(c chan error) { + c <- mailer.EmailChangeMail(r, u, ottCurrent, otp, referrerURL, externalURL) + }(errors) + } + go func(c chan error) { + c <- mailer.EmailChangeMail(r, u, ottNew, otpNew, referrerURL, externalURL) + }(errors) + + // we return the first error that's sent to the channel + if e := <-errors; e != nil { + return e + } + return nil +} diff --git a/internal/api/phone.go b/internal/api/phone.go index 0e0799d55..ce9ca0248 100644 --- a/internal/api/phone.go +++ b/internal/api/phone.go @@ -45,25 +45,20 @@ func formatPhoneNumber(phone string) string { // sendPhoneConfirmation sends an otp to the user's phone number func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, user *models.User, phone, otpType string, smsProvider sms_provider.SmsProvider, channel string) (string, error) { config := a.config - - var token *string var sentAt *time.Time includeFields := []string{} switch otpType { case phoneChangeVerification: - token = &user.PhoneChangeToken sentAt = user.PhoneChangeSentAt user.PhoneChange = phone - includeFields = append(includeFields, "phone_change", "phone_change_token", "phone_change_sent_at") + includeFields = append(includeFields, "phone_change", "phone_change_sent_at") case phoneConfirmationOtp: - token = &user.ConfirmationToken sentAt = user.ConfirmationSentAt - includeFields = append(includeFields, "confirmation_token", "confirmation_sent_at") + includeFields = append(includeFields, "confirmation_sent_at") case phoneReauthenticationOtp: - token = &user.ReauthenticationToken sentAt = user.ReauthenticationSentAt - includeFields = append(includeFields, "reauthentication_token", "reauthentication_sent_at") + includeFields = append(includeFields, "reauthentication_sent_at") default: return "", internalServerError("invalid otp type") } @@ -116,7 +111,7 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use } } - *token = crypto.GenerateTokenHash(phone, otp) + token := crypto.GenerateTokenHash(phone, otp) switch otpType { case phoneConfirmationOtp: @@ -133,15 +128,15 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use switch otpType { case phoneConfirmationOtp: - if _, err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ConfirmationToken, models.ConfirmationToken); err != nil { + if _, err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), token, models.ConfirmationToken); err != nil { return messageID, errors.Wrap(err, "Database error creating confirmation token for phone") } case phoneChangeVerification: - if _, err := models.CreateOneTimeToken(tx, user.ID, user.PhoneChange, user.PhoneChangeToken, models.PhoneChangeToken); err != nil { + if _, err := models.CreateOneTimeToken(tx, user.ID, user.PhoneChange, token, models.PhoneChangeToken); err != nil { return messageID, errors.Wrap(err, "Database error creating phone change token") } case phoneReauthenticationOtp: - if _, err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ReauthenticationToken, models.ReauthenticationToken); err != nil { + if _, err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), token, models.ReauthenticationToken); err != nil { return messageID, errors.Wrap(err, "Database error creating reauthentication token for phone") } } diff --git a/internal/api/reauthenticate.go b/internal/api/reauthenticate.go index b74f0ad28..7e358895b 100644 --- a/internal/api/reauthenticate.go +++ b/internal/api/reauthenticate.go @@ -80,7 +80,7 @@ func (a *API) Reauthenticate(w http.ResponseWriter, r *http.Request) error { // verifyReauthentication checks if the nonce provided is valid func (a *API) verifyReauthentication(nonce string, tx *storage.Connection, config *conf.GlobalConfiguration, user *models.User) error { - if user.ReauthenticationToken == "" || user.ReauthenticationSentAt == nil { + if user.ReauthenticationSentAt == nil { return unprocessableEntityError(ErrorCodeReauthenticationNotValid, InvalidNonceMessage) } var isValid bool diff --git a/internal/api/verify.go b/internal/api/verify.go index fb75e5d68..ae2cb6266 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -501,18 +501,16 @@ func (a *API) emailChangeVerify(r *http.Request, conn *storage.Connection, param user.EmailChangeConfirmStatus = singleConfirmation - if params.Token == user.EmailChangeTokenCurrent || params.TokenHash == user.EmailChangeTokenCurrent || (currentOTT != nil && params.TokenHash == currentOTT.TokenHash) { - user.EmailChangeTokenCurrent = "" + if currentOTT != nil && params.TokenHash == currentOTT.TokenHash { if terr := models.ClearOneTimeTokenForUser(tx, user.ID, models.EmailChangeTokenCurrent); terr != nil { return terr } - } else if params.Token == user.EmailChangeTokenNew || params.TokenHash == user.EmailChangeTokenNew || (newOTT != nil && params.TokenHash == newOTT.TokenHash) { - user.EmailChangeTokenNew = "" + } else if newOTT != nil && params.TokenHash == newOTT.TokenHash { if terr := models.ClearOneTimeTokenForUser(tx, user.ID, models.EmailChangeTokenNew); terr != nil { return terr } } - if terr := tx.UpdateOnly(user, "email_change_confirm_status", "email_change_token_current", "email_change_token_new"); terr != nil { + if terr := tx.UpdateOnly(user, "email_change_confirm_status"); terr != nil { return terr } return nil @@ -606,7 +604,10 @@ func (a *API) verifyTokenHash(conn *storage.Connection, params *VerifyParams) (* case mail.EmailOTPVerification: sentAt := user.ConfirmationSentAt params.Type = "signup" - if user.RecoveryToken == params.TokenHash { + if _, err := models.FindOneTimeToken(conn, params.TokenHash, models.ReauthenticationToken); err != nil { + if !models.IsNotFoundError(err) { + return nil, internalServerError("Database error finding token").WithInternalError(err) + } sentAt = user.RecoverySentAt params.Type = "magiclink" } diff --git a/internal/mailer/mailer.go b/internal/mailer/mailer.go index d579af400..37ffc05da 100644 --- a/internal/mailer/mailer.go +++ b/internal/mailer/mailer.go @@ -15,12 +15,11 @@ import ( // Mailer defines the interface a mailer must implement. type Mailer interface { - Send(user *models.User, subject, body string, data map[string]interface{}) error - InviteMail(r *http.Request, user *models.User, otp, referrerURL string, externalURL *url.URL) error - ConfirmationMail(r *http.Request, user *models.User, otp, referrerURL string, externalURL *url.URL) error - RecoveryMail(r *http.Request, user *models.User, otp, referrerURL string, externalURL *url.URL) error - MagicLinkMail(r *http.Request, user *models.User, otp, referrerURL string, externalURL *url.URL) error - EmailChangeMail(r *http.Request, user *models.User, otpNew, otpCurrent, referrerURL string, externalURL *url.URL) error + InviteMail(r *http.Request, user *models.User, ott *models.OneTimeToken, otp, referrerURL string, externalURL *url.URL) error + ConfirmationMail(r *http.Request, user *models.User, ott *models.OneTimeToken, otp, referrerURL string, externalURL *url.URL) error + RecoveryMail(r *http.Request, user *models.User, ott *models.OneTimeToken, otp, referrerURL string, externalURL *url.URL) error + MagicLinkMail(r *http.Request, user *models.User, ott *models.OneTimeToken, otp, referrerURL string, externalURL *url.URL) error + EmailChangeMail(r *http.Request, user *models.User, ott *models.OneTimeToken, otp, referrerURL string, externalURL *url.URL) error ReauthenticateMail(r *http.Request, user *models.User, otp string) error ValidateEmail(email string) error GetEmailActionLink(ott *models.OneTimeToken, actionType, referrerURL string, externalURL *url.URL) (string, error) diff --git a/internal/mailer/template.go b/internal/mailer/template.go index 7ede00f37..506fbaf21 100644 --- a/internal/mailer/template.go +++ b/internal/mailer/template.go @@ -88,9 +88,9 @@ func (m TemplateMailer) ValidateEmail(email string) error { } // InviteMail sends a invite mail to a new user -func (m *TemplateMailer) InviteMail(r *http.Request, user *models.User, otp, referrerURL string, externalURL *url.URL) error { +func (m *TemplateMailer) InviteMail(r *http.Request, user *models.User, ott *models.OneTimeToken, otp, referrerURL string, externalURL *url.URL) error { path, err := getPath(m.Config.Mailer.URLPaths.Invite, &EmailParams{ - Token: user.ConfirmationToken, + Token: ott.TokenHash, Type: "invite", RedirectTo: referrerURL, }) @@ -102,9 +102,9 @@ func (m *TemplateMailer) InviteMail(r *http.Request, user *models.User, otp, ref data := map[string]interface{}{ "SiteURL": m.Config.SiteURL, "ConfirmationURL": externalURL.ResolveReference(path).String(), - "Email": user.Email, + "Email": user.GetEmail(), "Token": otp, - "TokenHash": user.ConfirmationToken, + "TokenHash": ott.TokenHash, "Data": user.UserMetaData, "RedirectTo": referrerURL, } @@ -119,9 +119,9 @@ func (m *TemplateMailer) InviteMail(r *http.Request, user *models.User, otp, ref } // ConfirmationMail sends a signup confirmation mail to a new user -func (m *TemplateMailer) ConfirmationMail(r *http.Request, user *models.User, otp, referrerURL string, externalURL *url.URL) error { +func (m *TemplateMailer) ConfirmationMail(r *http.Request, user *models.User, ott *models.OneTimeToken, otp, referrerURL string, externalURL *url.URL) error { path, err := getPath(m.Config.Mailer.URLPaths.Confirmation, &EmailParams{ - Token: user.ConfirmationToken, + Token: ott.TokenHash, Type: "signup", RedirectTo: referrerURL, }) @@ -132,9 +132,9 @@ func (m *TemplateMailer) ConfirmationMail(r *http.Request, user *models.User, ot data := map[string]interface{}{ "SiteURL": m.Config.SiteURL, "ConfirmationURL": externalURL.ResolveReference(path).String(), - "Email": user.Email, + "Email": user.GetEmail(), "Token": otp, - "TokenHash": user.ConfirmationToken, + "TokenHash": ott.TokenHash, "Data": user.UserMetaData, "RedirectTo": referrerURL, } @@ -152,7 +152,7 @@ func (m *TemplateMailer) ConfirmationMail(r *http.Request, user *models.User, ot func (m *TemplateMailer) ReauthenticateMail(r *http.Request, user *models.User, otp string) error { data := map[string]interface{}{ "SiteURL": m.Config.SiteURL, - "Email": user.Email, + "Email": user.GetEmail(), "Token": otp, "Data": user.UserMetaData, } @@ -167,84 +167,48 @@ func (m *TemplateMailer) ReauthenticateMail(r *http.Request, user *models.User, } // EmailChangeMail sends an email change confirmation mail to a user -func (m *TemplateMailer) EmailChangeMail(r *http.Request, user *models.User, otpNew, otpCurrent, referrerURL string, externalURL *url.URL) error { - type Email struct { - Address string - Otp string - TokenHash string - Subject string - Template string - } - emails := []Email{ - { - Address: user.EmailChange, - Otp: otpNew, - TokenHash: user.EmailChangeTokenNew, - Subject: withDefault(m.Config.Mailer.Subjects.EmailChange, "Confirm Email Change"), - Template: m.Config.Mailer.Templates.EmailChange, +func (m *TemplateMailer) EmailChangeMail(r *http.Request, user *models.User, ott *models.OneTimeToken, otp, referrerURL string, externalURL *url.URL) error { + path, err := getPath( + m.Config.Mailer.URLPaths.EmailChange, + &EmailParams{ + Token: ott.TokenHash, + Type: "email_change", + RedirectTo: referrerURL, }, + ) + if err != nil { + return err } - currentEmail := user.GetEmail() - if m.Config.Mailer.SecureEmailChangeEnabled && currentEmail != "" { - emails = append(emails, Email{ - Address: currentEmail, - Otp: otpCurrent, - TokenHash: user.EmailChangeTokenCurrent, - Subject: withDefault(m.Config.Mailer.Subjects.Confirmation, "Confirm Email Address"), - Template: m.Config.Mailer.Templates.EmailChange, - }) - } - - errors := make(chan error) - for _, email := range emails { - path, err := getPath( - m.Config.Mailer.URLPaths.EmailChange, - &EmailParams{ - Token: email.TokenHash, - Type: "email_change", - RedirectTo: referrerURL, - }, - ) - if err != nil { - return err - } - go func(address, token, tokenHash, template string) { - data := map[string]interface{}{ - "SiteURL": m.Config.SiteURL, - "ConfirmationURL": externalURL.ResolveReference(path).String(), - "Email": user.GetEmail(), - "NewEmail": user.EmailChange, - "Token": token, - "TokenHash": tokenHash, - "SendingTo": address, - "Data": user.UserMetaData, - "RedirectTo": referrerURL, - } - errors <- m.Mailer.Mail( - address, - withDefault(m.Config.Mailer.Subjects.EmailChange, "Confirm Email Change"), - template, - defaultEmailChangeMail, - data, - ) - }(email.Address, email.Otp, email.TokenHash, email.Template) + sendingTo := user.EmailChange + if ott.TokenType == models.EmailChangeTokenCurrent { + sendingTo = user.GetEmail() } - for i := 0; i < len(emails); i++ { - e := <-errors - if e != nil { - return e - } + data := map[string]interface{}{ + "SiteURL": m.Config.SiteURL, + "ConfirmationURL": externalURL.ResolveReference(path).String(), + "Email": user.GetEmail(), + "NewEmail": user.EmailChange, + "Token": otp, + "TokenHash": ott.TokenHash, + "SendingTo": sendingTo, + "Data": user.UserMetaData, + "RedirectTo": referrerURL, } - - return nil + return m.Mailer.Mail( + sendingTo, + withDefault(m.Config.Mailer.Subjects.EmailChange, "Confirm Email Change"), + m.Config.Mailer.Templates.EmailChange, + defaultEmailChangeMail, + data, + ) } // RecoveryMail sends a password recovery mail -func (m *TemplateMailer) RecoveryMail(r *http.Request, user *models.User, otp, referrerURL string, externalURL *url.URL) error { +func (m *TemplateMailer) RecoveryMail(r *http.Request, user *models.User, ott *models.OneTimeToken, otp, referrerURL string, externalURL *url.URL) error { path, err := getPath(m.Config.Mailer.URLPaths.Recovery, &EmailParams{ - Token: user.RecoveryToken, + Token: ott.TokenHash, Type: "recovery", RedirectTo: referrerURL, }) @@ -254,9 +218,9 @@ func (m *TemplateMailer) RecoveryMail(r *http.Request, user *models.User, otp, r data := map[string]interface{}{ "SiteURL": m.Config.SiteURL, "ConfirmationURL": externalURL.ResolveReference(path).String(), - "Email": user.Email, + "Email": user.GetEmail(), "Token": otp, - "TokenHash": user.RecoveryToken, + "TokenHash": ott.TokenHash, "Data": user.UserMetaData, "RedirectTo": referrerURL, } @@ -271,9 +235,9 @@ func (m *TemplateMailer) RecoveryMail(r *http.Request, user *models.User, otp, r } // MagicLinkMail sends a login link mail -func (m *TemplateMailer) MagicLinkMail(r *http.Request, user *models.User, otp, referrerURL string, externalURL *url.URL) error { +func (m *TemplateMailer) MagicLinkMail(r *http.Request, user *models.User, ott *models.OneTimeToken, otp, referrerURL string, externalURL *url.URL) error { path, err := getPath(m.Config.Mailer.URLPaths.Recovery, &EmailParams{ - Token: user.RecoveryToken, + Token: ott.TokenHash, Type: "magiclink", RedirectTo: referrerURL, }) @@ -284,9 +248,9 @@ func (m *TemplateMailer) MagicLinkMail(r *http.Request, user *models.User, otp, data := map[string]interface{}{ "SiteURL": m.Config.SiteURL, "ConfirmationURL": externalURL.ResolveReference(path).String(), - "Email": user.Email, + "Email": user.GetEmail(), "Token": otp, - "TokenHash": user.RecoveryToken, + "TokenHash": ott.TokenHash, "Data": user.UserMetaData, "RedirectTo": referrerURL, } @@ -300,17 +264,6 @@ func (m *TemplateMailer) MagicLinkMail(r *http.Request, user *models.User, otp, ) } -// Send can be used to send one-off emails to users -func (m TemplateMailer) Send(user *models.User, subject, body string, data map[string]interface{}) error { - return m.Mailer.Mail( - user.GetEmail(), - subject, - "", - body, - data, - ) -} - // GetEmailActionLink returns a magiclink, recovery or invite link based on the actionType passed. func (m TemplateMailer) GetEmailActionLink(ott *models.OneTimeToken, actionType, referrerURL string, externalURL *url.URL) (string, error) { if ott == nil { diff --git a/internal/models/user.go b/internal/models/user.go index 270484e08..aaf3e18be 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -32,27 +32,19 @@ type User struct { Phone storage.NullString `json:"phone" db:"phone"` PhoneConfirmedAt *time.Time `json:"phone_confirmed_at,omitempty" db:"phone_confirmed_at"` - ConfirmationToken string `json:"-" db:"confirmation_token"` ConfirmationSentAt *time.Time `json:"confirmation_sent_at,omitempty" db:"confirmation_sent_at"` // For backward compatibility only. Use EmailConfirmedAt or PhoneConfirmedAt instead. - ConfirmedAt *time.Time `json:"confirmed_at,omitempty" db:"confirmed_at" rw:"r"` - - RecoveryToken string `json:"-" db:"recovery_token"` + ConfirmedAt *time.Time `json:"confirmed_at,omitempty" db:"confirmed_at" rw:"r"` RecoverySentAt *time.Time `json:"recovery_sent_at,omitempty" db:"recovery_sent_at"` - EmailChangeTokenCurrent string `json:"-" db:"email_change_token_current"` - EmailChangeTokenNew string `json:"-" db:"email_change_token_new"` + ReauthenticationSentAt *time.Time `json:"reauthentication_sent_at,omitempty" db:"reauthentication_sent_at"` + EmailChange string `json:"new_email,omitempty" db:"email_change"` EmailChangeSentAt *time.Time `json:"email_change_sent_at,omitempty" db:"email_change_sent_at"` EmailChangeConfirmStatus int `json:"-" db:"email_change_confirm_status"` - - PhoneChangeToken string `json:"-" db:"phone_change_token"` - PhoneChange string `json:"new_phone,omitempty" db:"phone_change"` - PhoneChangeSentAt *time.Time `json:"phone_change_sent_at,omitempty" db:"phone_change_sent_at"` - - ReauthenticationToken string `json:"-" db:"reauthentication_token"` - ReauthenticationSentAt *time.Time `json:"reauthentication_sent_at,omitempty" db:"reauthentication_sent_at"` + PhoneChange string `json:"new_phone,omitempty" db:"phone_change"` + PhoneChangeSentAt *time.Time `json:"phone_change_sent_at,omitempty" db:"phone_change_sent_at"` LastSignInAt *time.Time `json:"last_sign_in_at,omitempty" db:"last_sign_in_at"` @@ -69,6 +61,14 @@ type User struct { IsAnonymous bool `json:"is_anonymous" db:"is_anonymous"` DONTUSEINSTANCEID uuid.UUID `json:"-" db:"instance_id"` + + // Deprecated: use the one_time_tokens model instead + ConfirmationToken string `json:"-" db:"confirmation_token"` + RecoveryToken string `json:"-" db:"recovery_token"` + EmailChangeTokenCurrent string `json:"-" db:"email_change_token_current"` + EmailChangeTokenNew string `json:"-" db:"email_change_token_new"` + PhoneChangeToken string `json:"-" db:"phone_change_token"` + ReauthenticationToken string `json:"-" db:"reauthentication_token"` } // NewUser initializes a new user from an email, password and user data. @@ -302,19 +302,13 @@ func (u *User) SetPassword(ctx context.Context, password string) error { // UpdatePassword updates the user's password. Use SetPassword outside of a transaction first! func (u *User) UpdatePassword(tx *storage.Connection, sessionID *uuid.UUID) error { // These need to be reset because password change may mean the user no longer trusts the actions performed by the previous password. - u.ConfirmationToken = "" u.ConfirmationSentAt = nil - u.RecoveryToken = "" u.RecoverySentAt = nil - u.EmailChangeTokenCurrent = "" - u.EmailChangeTokenNew = "" u.EmailChangeSentAt = nil - u.PhoneChangeToken = "" u.PhoneChangeSentAt = nil - u.ReauthenticationToken = "" u.ReauthenticationSentAt = nil - if err := tx.UpdateOnly(u, "encrypted_password", "confirmation_token", "confirmation_sent_at", "recovery_token", "recovery_sent_at", "email_change_token_current", "email_change_token_new", "email_change_sent_at", "phone_change_token", "phone_change_sent_at", "reauthentication_token", "reauthentication_sent_at"); err != nil { + if err := tx.UpdateOnly(u, "encrypted_password", "confirmation_sent_at", "recovery_sent_at", "email_change_sent_at", "phone_change_sent_at", "reauthentication_sent_at"); err != nil { return err } @@ -339,11 +333,6 @@ func (u *User) Authenticate(ctx context.Context, password string) bool { // ConfirmReauthentication resets the reauthentication token func (u *User) ConfirmReauthentication(tx *storage.Connection) error { - u.ReauthenticationToken = "" - if err := tx.UpdateOnly(u, "reauthentication_token"); err != nil { - return err - } - if err := ClearAllOneTimeTokensForUser(tx, u.ID); err != nil { return err } @@ -353,11 +342,10 @@ func (u *User) ConfirmReauthentication(tx *storage.Connection) error { // Confirm resets the confimation token and sets the confirm timestamp func (u *User) Confirm(tx *storage.Connection) error { - u.ConfirmationToken = "" now := time.Now() u.EmailConfirmedAt = &now - if err := tx.UpdateOnly(u, "confirmation_token", "email_confirmed_at"); err != nil { + if err := tx.UpdateOnly(u, "email_confirmed_at"); err != nil { return err } @@ -370,10 +358,9 @@ func (u *User) Confirm(tx *storage.Connection) error { // ConfirmPhone resets the confimation token and sets the confirm timestamp func (u *User) ConfirmPhone(tx *storage.Connection) error { - u.ConfirmationToken = "" now := time.Now() u.PhoneConfirmedAt = &now - if err := tx.UpdateOnly(u, "confirmation_token", "phone_confirmed_at"); err != nil { + if err := tx.UpdateOnly(u, "phone_confirmed_at"); err != nil { return nil } @@ -391,16 +378,12 @@ func (u *User) ConfirmEmailChange(tx *storage.Connection, status int) error { u.Email = storage.NullString(email) u.EmailChange = "" - u.EmailChangeTokenCurrent = "" - u.EmailChangeTokenNew = "" u.EmailChangeConfirmStatus = status if err := tx.UpdateOnly( u, "email", "email_change", - "email_change_token_current", - "email_change_token_new", "email_change_confirm_status", ); err != nil { return err @@ -442,14 +425,12 @@ func (u *User) ConfirmPhoneChange(tx *storage.Connection) error { u.Phone = storage.NullString(phone) u.PhoneChange = "" - u.PhoneChangeToken = "" u.PhoneConfirmedAt = &now if err := tx.UpdateOnly( u, "phone", "phone_change", - "phone_change_token", "phone_confirmed_at", ); err != nil { return err @@ -482,11 +463,6 @@ func (u *User) ConfirmPhoneChange(tx *storage.Connection) error { // Recover resets the recovery token func (u *User) Recover(tx *storage.Connection) error { - u.RecoveryToken = "" - if err := tx.UpdateOnly(u, "recovery_token"); err != nil { - return err - } - return ClearAllOneTimeTokensForUser(tx, u.ID) } @@ -735,11 +711,6 @@ func (u *User) SoftDeleteUser(tx *storage.Connection) error { u.EmailChange = obfuscateEmail(u, u.EmailChange) u.PhoneChange = obfuscatePhone(u, u.PhoneChange) u.EncryptedPassword = "" - u.ConfirmationToken = "" - u.RecoveryToken = "" - u.EmailChangeTokenCurrent = "" - u.EmailChangeTokenNew = "" - u.PhoneChangeToken = "" // set deleted_at time now := time.Now() @@ -752,11 +723,6 @@ func (u *User) SoftDeleteUser(tx *storage.Connection) error { "encrypted_password", "email_change", "phone_change", - "confirmation_token", - "recovery_token", - "email_change_token_current", - "email_change_token_new", - "phone_change_token", "deleted_at", ); err != nil { return err From 0782fc7f0d5bdb051f3bc170d5baa0c2c28c45db Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Fri, 7 Jun 2024 12:51:46 +0800 Subject: [PATCH 05/11] chore: add comment to indicate deprecated columns --- internal/models/user.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/internal/models/user.go b/internal/models/user.go index aaf3e18be..39021bd82 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -62,13 +62,20 @@ type User struct { DONTUSEINSTANCEID uuid.UUID `json:"-" db:"instance_id"` + OneTimeTokens []OneTimeToken `json:"-" has_many:"one_time_tokens"` + + // Deprecated: use the one_time_tokens model instead + ConfirmationToken string `json:"-" db:"confirmation_token"` + // Deprecated: use the one_time_tokens model instead + RecoveryToken string `json:"-" db:"recovery_token"` // Deprecated: use the one_time_tokens model instead - ConfirmationToken string `json:"-" db:"confirmation_token"` - RecoveryToken string `json:"-" db:"recovery_token"` EmailChangeTokenCurrent string `json:"-" db:"email_change_token_current"` - EmailChangeTokenNew string `json:"-" db:"email_change_token_new"` - PhoneChangeToken string `json:"-" db:"phone_change_token"` - ReauthenticationToken string `json:"-" db:"reauthentication_token"` + // Deprecated: use the one_time_tokens model instead + EmailChangeTokenNew string `json:"-" db:"email_change_token_new"` + // Deprecated: use the one_time_tokens model instead + PhoneChangeToken string `json:"-" db:"phone_change_token"` + // Deprecated: use the one_time_tokens model instead + ReauthenticationToken string `json:"-" db:"reauthentication_token"` } // NewUser initializes a new user from an email, password and user data. From 61182b8e0a2a2385db5e918cecf85b43dfda3136 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Fri, 7 Jun 2024 12:51:58 +0800 Subject: [PATCH 06/11] chore: fix verify tests --- internal/api/verify_test.go | 100 ++++++++++++++++++------------------ 1 file changed, 49 insertions(+), 51 deletions(-) diff --git a/internal/api/verify_test.go b/internal/api/verify_test.go index 009a84984..8031c3ef0 100644 --- a/internal/api/verify_test.go +++ b/internal/api/verify_test.go @@ -108,7 +108,7 @@ func (ts *VerifyTestSuite) TestVerifyPasswordRecovery() { assert.WithinDuration(ts.T(), time.Now(), *u.RecoverySentAt, 1*time.Second) assert.False(ts.T(), u.IsConfirmed()) - recoveryToken := u.RecoveryToken + recoveryToken := u.OneTimeTokens[0].TokenHash reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s", mail.RecoveryVerification, recoveryToken) req = httptest.NewRequest(http.MethodGet, reqURL, nil) @@ -174,8 +174,6 @@ func (ts *VerifyTestSuite) TestVerifySecureEmailChange() { // reset user u.EmailChangeSentAt = nil - u.EmailChangeTokenCurrent = "" - u.EmailChangeTokenNew = "" require.NoError(ts.T(), ts.API.db.Update(u)) require.NoError(ts.T(), models.ClearAllOneTimeTokensForUser(ts.API.db, u.ID)) @@ -205,8 +203,14 @@ func (ts *VerifyTestSuite) TestVerifySecureEmailChange() { u, err = models.FindUserByEmailAndAudience(ts.API.db, c.currentEmail, ts.Config.JWT.Aud) require.NoError(ts.T(), err) - currentTokenHash := u.EmailChangeTokenCurrent - newTokenHash := u.EmailChangeTokenNew + var currentTokenHash, newTokenHash string + for _, ott := range u.OneTimeTokens { + if ott.TokenType == models.EmailChangeTokenCurrent { + currentTokenHash = ott.TokenHash + } else if ott.TokenType == models.EmailChangeTokenNew { + newTokenHash = ott.TokenHash + } + } u, err = models.FindUserByEmailAndAudience(ts.API.db, c.currentEmail, ts.Config.JWT.Aud) require.NoError(ts.T(), err) @@ -283,15 +287,15 @@ func (ts *VerifyTestSuite) TestExpiredConfirmationToken() { u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) - u.ConfirmationToken = "asdf3" + token := "asdf3" sentTime := time.Now().Add(-48 * time.Hour) u.ConfirmationSentAt = &sentTime require.NoError(ts.T(), ts.API.db.Update(u)) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + ott, err := models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), token, models.ConfirmationToken) require.NoError(ts.T(), err) // Setup request - reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s", mail.SignupVerification, u.ConfirmationToken) + reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s", mail.SignupVerification, ott.TokenHash) req := httptest.NewRequest(http.MethodGet, reqURL, nil) // Setup response recorder @@ -314,15 +318,13 @@ func (ts *VerifyTestSuite) TestInvalidOtp() { u, err := models.FindUserByPhoneAndAudience(ts.API.db, "12345678", ts.Config.JWT.Aud) require.NoError(ts.T(), err) sentTime := time.Now().Add(-48 * time.Hour) - u.ConfirmationToken = "123456" u.ConfirmationSentAt = &sentTime u.PhoneChange = "22222222" - u.PhoneChangeToken = "123456" u.PhoneChangeSentAt = &sentTime require.NoError(ts.T(), ts.API.db.Update(u)) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + confirmationOtt, err := models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "123456", models.ConfirmationToken) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.PhoneChange, u.PhoneChangeToken, models.PhoneChangeToken) + phoneChangeOtt, err := models.CreateOneTimeToken(ts.API.db, u.ID, u.PhoneChange, "123456", models.PhoneChangeToken) require.NoError(ts.T(), err) type ResponseBody struct { @@ -346,7 +348,17 @@ func (ts *VerifyTestSuite) TestInvalidOtp() { sentTime: time.Now().Add(-48 * time.Hour), body: map[string]interface{}{ "type": smsVerification, - "token": u.ConfirmationToken, + "token": confirmationOtt.TokenHash, + "phone": u.GetPhone(), + }, + expected: expectedResponse, + }, + { + desc: "Expired Phone Change OTP", + sentTime: time.Now().Add(-48 * time.Hour), + body: map[string]interface{}{ + "type": smsVerification, + "token": phoneChangeOtt.TokenHash, "phone": u.GetPhone(), }, expected: expectedResponse, @@ -416,18 +428,15 @@ func (ts *VerifyTestSuite) TestInvalidOtp() { } func (ts *VerifyTestSuite) TestExpiredRecoveryToken() { - // verify variant testing not necessary in this test as it's testing - // the RecoverySentAt behavior, not the RecoveryToken behavior - + // Since we are testing the RecoverySentAt behavior, not the RecoveryToken behavior, we don't need to create a recovery token u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) - u.RecoveryToken = "asdf3" sentTime := time.Now().Add(-48 * time.Hour) u.RecoverySentAt = &sentTime require.NoError(ts.T(), ts.API.db.Update(u)) // Setup request - reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s", "signup", u.RecoveryToken) + reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s", "signup", "asdf3") req := httptest.NewRequest(http.MethodGet, reqURL, nil) // Setup response recorder @@ -470,7 +479,8 @@ func (ts *VerifyTestSuite) TestVerifyPermitedCustomUri() { redirectURL, _ := url.Parse(ts.Config.URIAllowList[0]) - reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s&redirect_to=%s", "recovery", u.RecoveryToken, redirectURL.String()) + recoveryToken := u.OneTimeTokens[0].TokenHash + reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s&redirect_to=%s", "recovery", recoveryToken, redirectURL.String()) req = httptest.NewRequest(http.MethodGet, reqURL, nil) w = httptest.NewRecorder() @@ -517,7 +527,8 @@ func (ts *VerifyTestSuite) TestVerifyNotPermitedCustomUri() { fakeredirectURL, _ := url.Parse("http://custom-url.com") siteURL, _ := url.Parse(ts.Config.SiteURL) - reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s&redirect_to=%s", "recovery", u.RecoveryToken, fakeredirectURL.String()) + recoveryToken := u.OneTimeTokens[0].TokenHash + reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s&redirect_to=%s", "recovery", recoveryToken, fakeredirectURL.String()) req = httptest.NewRequest(http.MethodGet, reqURL, nil) w = httptest.NewRecorder() @@ -642,14 +653,13 @@ func (ts *VerifyTestSuite) TestVerifySignupWithRedirectURLContainedPath() { // set verify token to user as it actual do in magic link method u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) - u.ConfirmationToken = "someToken" sendTime := time.Now().Add(time.Hour) u.ConfirmationSentAt = &sendTime require.NoError(ts.T(), ts.API.db.Update(u)) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + ott, err := models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "test-confirmation-token", models.ConfirmationToken) require.NoError(ts.T(), err) - reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s&redirect_to=%s", "signup", u.ConfirmationToken, redirectURL) + reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s&redirect_to=%s", "signup", ott.TokenHash, redirectURL) req := httptest.NewRequest(http.MethodGet, reqURL, nil) w := httptest.NewRecorder() @@ -727,6 +737,9 @@ func (ts *VerifyTestSuite) TestVerifyPKCEOTP() { require.NoError(ts.T(), err) assert.True(ts.T(), u.IsConfirmed()) + // one time tokens should be cleared after successful verification + assert.Empty(ts.T(), u.OneTimeTokens) + f, err := url.ParseQuery(rURL.RawQuery) require.NoError(ts.T(), err) assert.NotEmpty(ts.T(), f.Get("code")) @@ -738,10 +751,6 @@ func (ts *VerifyTestSuite) TestVerifyPKCEOTP() { func (ts *VerifyTestSuite) TestVerifyBannedUser() { u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) - u.ConfirmationToken = "confirmation_token" - u.RecoveryToken = "recovery_token" - u.EmailChangeTokenCurrent = "current_email_change_token" - u.EmailChangeTokenNew = "new_email_change_token" t := time.Now() u.ConfirmationSentAt = &t u.RecoverySentAt = &t @@ -750,13 +759,11 @@ func (ts *VerifyTestSuite) TestVerifyBannedUser() { t = time.Now().Add(24 * time.Hour) u.BannedUntil = &t require.NoError(ts.T(), ts.API.db.Update(u)) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + confirmationOtt, err := models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "test-confirmation-token", models.ConfirmationToken) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken) + recoveryOtt, err := models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "test-recovery-token", models.RecoveryToken) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent) - require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenNew, models.EmailChangeTokenNew) + emailChangeCurrentOtt, err := models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "test-email-change-token-current", models.EmailChangeTokenCurrent) require.NoError(ts.T(), err) cases := []struct { @@ -767,35 +774,35 @@ func (ts *VerifyTestSuite) TestVerifyBannedUser() { desc: "Verify banned user on signup", payload: &VerifyParams{ Type: "signup", - Token: u.ConfirmationToken, + Token: confirmationOtt.TokenHash, }, }, { desc: "Verify banned user on invite", payload: &VerifyParams{ Type: "invite", - Token: u.ConfirmationToken, + Token: confirmationOtt.TokenHash, }, }, { desc: "Verify banned user on recover", payload: &VerifyParams{ Type: "recovery", - Token: u.RecoveryToken, + Token: recoveryOtt.TokenHash, }, }, { desc: "Verify banned user on magiclink", payload: &VerifyParams{ Type: "magiclink", - Token: u.RecoveryToken, + Token: recoveryOtt.TokenHash, }, }, { desc: "Verify banned user on email change", payload: &VerifyParams{ Type: "email_change", - Token: u.EmailChangeTokenCurrent, + Token: emailChangeCurrentOtt.TokenHash, }, }, } @@ -961,26 +968,19 @@ func (ts *VerifyTestSuite) TestVerifyValidOtp() { for _, caseItem := range cases { c := caseItem ts.Run(c.desc, func() { - // create user - require.NoError(ts.T(), models.ClearAllOneTimeTokensForUser(ts.API.db, u.ID)) - u.ConfirmationSentAt = &c.sentTime u.RecoverySentAt = &c.sentTime u.EmailChangeSentAt = &c.sentTime u.PhoneChangeSentAt = &c.sentTime + assert.NoError(ts.T(), ts.API.db.Update(u)) - u.ConfirmationToken = c.expected.tokenHash - u.RecoveryToken = c.expected.tokenHash - u.EmailChangeTokenNew = c.expected.tokenHash - u.PhoneChangeToken = c.expected.tokenHash - - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.ConfirmationToken, models.ConfirmationToken) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", c.expected.tokenHash, models.ConfirmationToken) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.RecoveryToken, models.RecoveryToken) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", c.expected.tokenHash, models.RecoveryToken) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.EmailChangeTokenNew, models.EmailChangeTokenNew) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", c.expected.tokenHash, models.EmailChangeTokenNew) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", u.PhoneChangeToken, models.PhoneChangeToken) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", c.expected.tokenHash, models.PhoneChangeToken) require.NoError(ts.T(), err) require.NoError(ts.T(), ts.API.db.Update(u)) @@ -1045,8 +1045,6 @@ func (ts *VerifyTestSuite) TestSecureEmailChangeWithTokenHash() { for _, c := range cases { ts.Run(c.desc, func() { // Set the corresponding email change tokens - u.EmailChangeTokenCurrent = currentEmailChangeToken - u.EmailChangeTokenNew = newEmailChangeToken require.NoError(ts.T(), models.ClearAllOneTimeTokensForUser(ts.API.db, u.ID)) _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", currentEmailChangeToken, models.EmailChangeTokenCurrent) From 521c55dec131b614a94f9ea61c8e282ca49d8cb7 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Fri, 7 Jun 2024 16:51:46 +0800 Subject: [PATCH 07/11] chore: fix tests to check against otts --- internal/api/admin_test.go | 19 +++++++++---------- internal/api/anonymous_test.go | 3 ++- internal/api/external_test.go | 6 +----- internal/api/invite_test.go | 9 +++++---- internal/api/otp_test.go | 10 ++++++---- internal/api/phone_test.go | 11 ++++++++--- internal/api/resend_test.go | 34 ++++++++++++++++++++++++---------- internal/api/signup_test.go | 6 +++--- internal/api/token_test.go | 3 ++- internal/api/user_test.go | 11 +++++++---- 10 files changed, 67 insertions(+), 45 deletions(-) diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go index d16bc2079..515c0c26e 100644 --- a/internal/api/admin_test.go +++ b/internal/api/admin_test.go @@ -585,24 +585,19 @@ func (ts *AdminTestSuite) TestAdminUserSoftDeletion() { // create user u, err := models.NewUser("123456789", "test@example.com", "secret", ts.Config.JWT.Aud, map[string]interface{}{"name": "test"}) require.NoError(ts.T(), err) - u.ConfirmationToken = "some_token" - u.RecoveryToken = "some_token" - u.EmailChangeTokenCurrent = "some_token" - u.EmailChangeTokenNew = "some_token" - u.PhoneChangeToken = "some_token" u.AppMetaData = map[string]interface{}{ "provider": "email", } require.NoError(ts.T(), ts.API.db.Create(u)) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "some_token", models.ConfirmationToken) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "some_token", models.RecoveryToken) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "some_token", models.EmailChangeTokenCurrent) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenNew, models.EmailChangeTokenNew) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "some_token", models.EmailChangeTokenNew) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetPhone(), u.PhoneChangeToken, models.PhoneChangeToken) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetPhone(), "some_token", models.PhoneChangeToken) require.NoError(ts.T(), err) // create user identities @@ -633,12 +628,16 @@ func (ts *AdminTestSuite) TestAdminUserSoftDeletion() { deletedUser, err := models.FindUserByID(ts.API.db, u.ID) require.NoError(ts.T(), err) + // TODO(km): remove once the db columns are removed require.Empty(ts.T(), deletedUser.ConfirmationToken) require.Empty(ts.T(), deletedUser.RecoveryToken) require.Empty(ts.T(), deletedUser.EmailChangeTokenCurrent) require.Empty(ts.T(), deletedUser.EmailChangeTokenNew) require.Empty(ts.T(), deletedUser.EncryptedPassword) require.Empty(ts.T(), deletedUser.PhoneChangeToken) + + // one time tokens table should be empty after a soft deletion + require.Empty(ts.T(), u.OneTimeTokens) require.Empty(ts.T(), deletedUser.UserMetaData) require.Empty(ts.T(), deletedUser.AppMetaData) require.NotEmpty(ts.T(), deletedUser.DeletedAt) diff --git a/internal/api/anonymous_test.go b/internal/api/anonymous_test.go index fdee4cc07..b0aa6fbf8 100644 --- a/internal/api/anonymous_test.go +++ b/internal/api/anonymous_test.go @@ -142,8 +142,9 @@ func (ts *AnonymousTestSuite) TestConvertAnonymousUserToPermanent() { switch c.verificationType { case mail.EmailChangeVerification: + emailChangeToken := user.OneTimeTokens[0].TokenHash require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ - "token_hash": user.EmailChangeTokenNew, + "token_hash": emailChangeToken, "type": c.verificationType, })) case phoneChangeVerification: diff --git a/internal/api/external_test.go b/internal/api/external_test.go index d693c536a..1e1f24a83 100644 --- a/internal/api/external_test.go +++ b/internal/api/external_test.go @@ -49,15 +49,11 @@ func (ts *ExternalTestSuite) createUser(providerId string, email string, name st userData["avatar_url"] = avatar } u, err := models.NewUser("", email, "test", ts.Config.JWT.Aud, userData) - - if confirmationToken != "" { - u.ConfirmationToken = confirmationToken - } ts.Require().NoError(err, "Error making new user") ts.Require().NoError(ts.API.db.Create(u), "Error creating user") if confirmationToken != "" { - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, email, u.ConfirmationToken, models.ConfirmationToken) + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, email, confirmationToken, models.ConfirmationToken) ts.Require().NoError(err, "Error creating one-time confirmation/invite token") } diff --git a/internal/api/invite_test.go b/internal/api/invite_test.go index f713510c8..6e6d38d22 100644 --- a/internal/api/invite_test.go +++ b/internal/api/invite_test.go @@ -208,10 +208,9 @@ func (ts *InviteTestSuite) TestVerifyInvite() { user.InvitedAt = &now user.ConfirmationSentAt = &now user.EncryptedPassword = "" - user.ConfirmationToken = crypto.GenerateTokenHash(c.email, c.requestBody["token"].(string)) require.NoError(ts.T(), err) require.NoError(ts.T(), ts.API.db.Create(user)) - _, err = models.CreateOneTimeToken(ts.API.db, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken) + _, err = models.CreateOneTimeToken(ts.API.db, user.ID, user.GetEmail(), crypto.GenerateTokenHash(c.email, c.requestBody["token"].(string)), models.ConfirmationToken) require.NoError(ts.T(), err) // Find test user @@ -280,9 +279,10 @@ func (ts *InviteTestSuite) TestInviteExternalGitlab() { // Find test user user, err := models.FindUserByEmailAndAudience(ts.API.db, "gitlab@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) + inviteToken := user.OneTimeTokens[0].TokenHash // get redirect url w/ state - req = httptest.NewRequest(http.MethodGet, "http://localhost/authorize?provider=gitlab&invite_token="+user.ConfirmationToken, nil) + req = httptest.NewRequest(http.MethodGet, "http://localhost/authorize?provider=gitlab&invite_token="+inviteToken, nil) w = httptest.NewRecorder() ts.API.handler.ServeHTTP(w, req) ts.Require().Equal(http.StatusFound, w.Code) @@ -372,9 +372,10 @@ func (ts *InviteTestSuite) TestInviteExternalGitlab_MismatchedEmails() { // Find test user user, err := models.FindUserByEmailAndAudience(ts.API.db, "gitlab@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) + inviteToken := user.OneTimeTokens[0].TokenHash // get redirect url w/ state - req = httptest.NewRequest(http.MethodGet, "http://localhost/authorize?provider=gitlab&invite_token="+user.ConfirmationToken, nil) + req = httptest.NewRequest(http.MethodGet, "http://localhost/authorize?provider=gitlab&invite_token="+inviteToken, nil) w = httptest.NewRecorder() ts.API.handler.ServeHTTP(w, req) ts.Require().Equal(http.StatusFound, w.Code) diff --git a/internal/api/otp_test.go b/internal/api/otp_test.go index c72fbc361..2b980d838 100644 --- a/internal/api/otp_test.go +++ b/internal/api/otp_test.go @@ -280,9 +280,10 @@ func (ts *OtpTestSuite) TestSubsequentOtp() { newUser, err := models.FindUserByEmailAndAudience(ts.API.db, userEmail, ts.Config.JWT.Aud) require.NoError(ts.T(), err) - require.NotEmpty(ts.T(), newUser.ConfirmationToken) + + require.Len(ts.T(), newUser.OneTimeTokens, 1) + require.Equal(ts.T(), models.ConfirmationToken, newUser.OneTimeTokens[0].TokenType) require.NotEmpty(ts.T(), newUser.ConfirmationSentAt) - require.Empty(ts.T(), newUser.RecoveryToken) require.Empty(ts.T(), newUser.RecoverySentAt) require.Empty(ts.T(), newUser.EmailConfirmedAt) @@ -303,9 +304,10 @@ func (ts *OtpTestSuite) TestSubsequentOtp() { user, err := models.FindUserByEmailAndAudience(ts.API.db, userEmail, ts.Config.JWT.Aud) require.NoError(ts.T(), err) - require.NotEmpty(ts.T(), user.ConfirmationToken) + + require.Len(ts.T(), user.OneTimeTokens, 1) + require.Equal(ts.T(), models.ConfirmationToken, user.OneTimeTokens[0].TokenType) require.NotEmpty(ts.T(), user.ConfirmationSentAt) - require.Empty(ts.T(), user.RecoveryToken) require.Empty(ts.T(), user.RecoverySentAt) require.Empty(ts.T(), user.EmailConfirmedAt) } diff --git a/internal/api/phone_test.go b/internal/api/phone_test.go index 38daa4941..a48576d38 100644 --- a/internal/api/phone_test.go +++ b/internal/api/phone_test.go @@ -110,6 +110,7 @@ func doTestSendPhoneConfirmation(ts *PhoneTestSuite, useTestOTP bool) { for _, c := range cases { ts.Run(c.desc, func() { + require.NoError(ts.T(), models.ClearAllOneTimeTokensForUser(ts.API.db, u.ID)) provider := &TestSmsProvider{} _, err = ts.API.sendPhoneConfirmation(req, ts.API.db, u, "123456789", c.otpType, provider, sms_provider.SMSProvider) @@ -127,15 +128,19 @@ func doTestSendPhoneConfirmation(ts *PhoneTestSuite, useTestOTP bool) { switch c.otpType { case phoneConfirmationOtp: - require.NotEmpty(ts.T(), u.ConfirmationToken) + require.NotEmpty(ts.T(), u.OneTimeTokens[0].TokenHash) + require.Equal(ts.T(), u.OneTimeTokens[0].TokenType, models.ConfirmationToken) require.NotEmpty(ts.T(), u.ConfirmationSentAt) case phoneChangeVerification: - require.NotEmpty(ts.T(), u.PhoneChangeToken) + require.NotEmpty(ts.T(), u.OneTimeTokens[0].TokenHash) + require.Equal(ts.T(), u.OneTimeTokens[0].TokenType, models.PhoneChangeToken) require.NotEmpty(ts.T(), u.PhoneChangeSentAt) case phoneReauthenticationOtp: - require.NotEmpty(ts.T(), u.ReauthenticationToken) + require.NotEmpty(ts.T(), u.OneTimeTokens[0].TokenHash) + require.Equal(ts.T(), u.OneTimeTokens[0].TokenType, models.ReauthenticationToken) require.NotEmpty(ts.T(), u.ReauthenticationSentAt) default: + require.Empty(ts.T(), u.OneTimeTokens) } }) } diff --git a/internal/api/resend_test.go b/internal/api/resend_test.go index 02e898e0d..0a8526193 100644 --- a/internal/api/resend_test.go +++ b/internal/api/resend_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "github.com/supabase/auth/internal/conf" + "github.com/supabase/auth/internal/crypto" mail "github.com/supabase/auth/internal/mailer" "github.com/supabase/auth/internal/models" ) @@ -122,33 +123,38 @@ func (ts *ResendTestSuite) TestResendSuccess() { // disable secure email change ts.Config.Mailer.SecureEmailChangeEnabled = false - u.ConfirmationToken = "123456" u.ConfirmationSentAt = &now u.EmailChange = "bar@example.com" u.EmailChangeSentAt = &now - u.EmailChangeTokenNew = "123456" require.NoError(ts.T(), ts.API.db.Create(u), "Error saving new test user") - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken) + + // see sendConfirmation + token := addFlowPrefixToToken(crypto.GenerateTokenHash(u.GetEmail(), "123456"), models.ImplicitFlow) + confirmationOtt, err := models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), token, models.ConfirmationToken) require.NoError(ts.T(), err) - _, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.EmailChange, u.EmailChangeTokenNew, models.EmailChangeTokenNew) + emailChangeNewOtt, err := models.CreateOneTimeToken(ts.API.db, u.ID, u.EmailChange, token, models.EmailChangeTokenNew) require.NoError(ts.T(), err) phoneUser, err := models.NewUser("1234567890", "", "password", ts.Config.JWT.Aud, nil) require.NoError(ts.T(), err, "Error creating test user model") phoneUser.EmailChange = "bar@example.com" phoneUser.EmailChangeSentAt = &now - phoneUser.EmailChangeTokenNew = "123456" require.NoError(ts.T(), ts.API.db.Create(phoneUser), "Error saving new test user") - _, err = models.CreateOneTimeToken(ts.API.db, phoneUser.ID, phoneUser.EmailChange, phoneUser.EmailChangeTokenNew, models.EmailChangeTokenNew) + + // see sendEmailChange + token = addFlowPrefixToToken(crypto.GenerateTokenHash(phoneUser.EmailChange, "123456"), models.ImplicitFlow) + phoneUserEmailChangeOtt, err := models.CreateOneTimeToken(ts.API.db, phoneUser.ID, phoneUser.EmailChange, token, models.EmailChangeTokenNew) require.NoError(ts.T(), err) emailUser, err := models.NewUser("", "bar@example.com", "password", ts.Config.JWT.Aud, nil) require.NoError(ts.T(), err, "Error creating test user model") phoneUser.PhoneChange = "1234567890" phoneUser.PhoneChangeSentAt = &now - phoneUser.PhoneChangeToken = "123456" require.NoError(ts.T(), ts.API.db.Create(emailUser), "Error saving new test user") - _, err = models.CreateOneTimeToken(ts.API.db, phoneUser.ID, phoneUser.PhoneChange, phoneUser.PhoneChangeToken, models.PhoneChangeToken) + + // see sendPhoneChange + token = crypto.GenerateTokenHash(phoneUser.PhoneChange, "123456") + phoneChangeOtt, err := models.CreateOneTimeToken(ts.API.db, phoneUser.ID, phoneUser.PhoneChange, token, models.PhoneChangeToken) require.NoError(ts.T(), err) cases := []struct { @@ -156,6 +162,7 @@ func (ts *ResendTestSuite) TestResendSuccess() { params map[string]interface{} // expected map[string]interface{} user *models.User + ott *models.OneTimeToken }{ { desc: "Resend signup confirmation", @@ -164,6 +171,7 @@ func (ts *ResendTestSuite) TestResendSuccess() { "email": u.GetEmail(), }, user: u, + ott: confirmationOtt, }, { desc: "Resend email change", @@ -172,6 +180,7 @@ func (ts *ResendTestSuite) TestResendSuccess() { "email": u.GetEmail(), }, user: u, + ott: emailChangeNewOtt, }, { desc: "Resend email change for phone user", @@ -180,6 +189,7 @@ func (ts *ResendTestSuite) TestResendSuccess() { "phone": phoneUser.GetPhone(), }, user: phoneUser, + ott: phoneUserEmailChangeOtt, }, { desc: "Resend phone change for email user", @@ -188,6 +198,7 @@ func (ts *ResendTestSuite) TestResendSuccess() { "email": emailUser.GetEmail(), }, user: emailUser, + ott: phoneChangeOtt, }, } @@ -195,6 +206,9 @@ func (ts *ResendTestSuite) TestResendSuccess() { ts.Run(c.desc, func() { var buffer bytes.Buffer require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.params)) + + // resend endpoint is expected to generate a new token to send to the user + // we need to check that the original token doesn't match the new token req := httptest.NewRequest(http.MethodPost, "http://localhost/resend", &buffer) req.Header.Set("Content-Type", "application/json") @@ -209,10 +223,10 @@ func (ts *ResendTestSuite) TestResendSuccess() { require.NotEmpty(ts.T(), dbUser) if c.params["type"] == mail.SignupVerification { - require.NotEqual(ts.T(), dbUser.ConfirmationToken, c.user.ConfirmationToken) + require.NotContains(ts.T(), dbUser.OneTimeTokens, c.ott) require.NotEqual(ts.T(), dbUser.ConfirmationSentAt, c.user.ConfirmationSentAt) } else if c.params["type"] == mail.EmailChangeVerification { - require.NotEqual(ts.T(), dbUser.EmailChangeTokenNew, c.user.EmailChangeTokenNew) + require.NotContains(ts.T(), dbUser.OneTimeTokens, c.ott) require.NotEqual(ts.T(), dbUser.EmailChangeSentAt, c.user.EmailChangeSentAt) } } diff --git a/internal/api/signup_test.go b/internal/api/signup_test.go index fc0267021..4cdd716b3 100644 --- a/internal/api/signup_test.go +++ b/internal/api/signup_test.go @@ -123,20 +123,20 @@ func (ts *SignupTestSuite) TestSignupTwice() { func (ts *SignupTestSuite) TestVerifySignup() { user, err := models.NewUser("123456789", "test@example.com", "testing", ts.Config.JWT.Aud, nil) - user.ConfirmationToken = "asdf3" now := time.Now() user.ConfirmationSentAt = &now require.NoError(ts.T(), err) require.NoError(ts.T(), ts.API.db.Create(user)) - _, err = models.CreateOneTimeToken(ts.API.db, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken) + ott, err := models.CreateOneTimeToken(ts.API.db, user.ID, user.GetEmail(), "test-confirmation-token", models.ConfirmationToken) require.NoError(ts.T(), err) // Find test user u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) + require.NotEmpty(ts.T(), u) // Setup request - reqUrl := fmt.Sprintf("http://localhost/verify?type=%s&token=%s", mail.SignupVerification, u.ConfirmationToken) + reqUrl := fmt.Sprintf("http://localhost/verify?type=%s&token=%s", mail.SignupVerification, ott.TokenHash) req := httptest.NewRequest(http.MethodGet, reqUrl, nil) // Setup response recorder diff --git a/internal/api/token_test.go b/internal/api/token_test.go index 53a492b11..b6dc62c28 100644 --- a/internal/api/token_test.go +++ b/internal/api/token_test.go @@ -557,8 +557,9 @@ func (ts *TokenTestSuite) TestMagicLinkPKCESignIn() { u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) + recoveryToken := u.OneTimeTokens[0].TokenHash // Verify OTP - requestUrl := fmt.Sprintf("http://localhost/verify?type=%v&token=%v", "magiclink", u.RecoveryToken) + requestUrl := fmt.Sprintf("http://localhost/verify?type=%v&token=%v", "magiclink", recoveryToken) req = httptest.NewRequest(http.MethodGet, requestUrl, &buffer) req.Header.Set("Content-Type", "application/json") diff --git a/internal/api/user_test.go b/internal/api/user_test.go index ac97d9c24..ecaf4a352 100644 --- a/internal/api/user_test.go +++ b/internal/api/user_test.go @@ -395,14 +395,17 @@ func (ts *UserTestSuite) TestUserUpdatePasswordReauthentication() { ts.API.handler.ServeHTTP(w, req) require.Equal(ts.T(), w.Code, http.StatusOK) + // check if reauthentication token is created after calling /reauthenticate u, err = models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) - require.NotEmpty(ts.T(), u.ReauthenticationToken) + reauthenticationToken := u.OneTimeTokens[0].TokenHash + require.NotEmpty(ts.T(), reauthenticationToken) require.NotEmpty(ts.T(), u.ReauthenticationSentAt) // update reauthentication token to a known token - u.ReauthenticationToken = crypto.GenerateTokenHash(u.GetEmail(), "123456") - require.NoError(ts.T(), ts.API.db.Update(u)) + newReauthenticationToken := crypto.GenerateTokenHash(u.GetEmail(), "123456") + _, err = models.CreateOneTimeToken(ts.API.db, u.ID, "relates-to-not-used", newReauthenticationToken, models.ReauthenticationToken) + require.NoError(ts.T(), err) // update password with reauthentication token var buffer bytes.Buffer @@ -425,7 +428,7 @@ func (ts *UserTestSuite) TestUserUpdatePasswordReauthentication() { require.NoError(ts.T(), err) require.True(ts.T(), u.Authenticate(context.Background(), "newpass")) - require.Empty(ts.T(), u.ReauthenticationToken) + require.Empty(ts.T(), u.OneTimeTokens) require.Nil(ts.T(), u.ReauthenticationSentAt) } From fdfbfd5bd24e9c1b2e95ef6d95f169403b92ddb8 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Fri, 7 Jun 2024 16:52:04 +0800 Subject: [PATCH 08/11] chore: format test logs to use json --- hack/test.env | 2 +- internal/api/api_test.go | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/hack/test.env b/hack/test.env index f4f3d0e6e..fcdd6f36b 100644 --- a/hack/test.env +++ b/hack/test.env @@ -11,7 +11,7 @@ GOTRUE_API_HOST=localhost PORT=9999 API_EXTERNAL_URL="http://localhost:9999" GOTRUE_LOG_SQL=none -GOTRUE_LOG_LEVEL=warn +GOTRUE_LOG_LEVEL=info GOTRUE_SITE_URL=https://example.netlify.com GOTRUE_URI_ALLOW_LIST="http://localhost:3000" GOTRUE_OPERATOR_TOKEN=foobar diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 87639a09c..c1ca6a120 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" "github.com/supabase/auth/internal/conf" "github.com/supabase/auth/internal/crypto" + "github.com/supabase/auth/internal/observability" "github.com/supabase/auth/internal/storage" "github.com/supabase/auth/internal/storage/test" ) @@ -32,6 +33,10 @@ func setupAPIForTestWithCallback(cb func(*conf.GlobalConfiguration, *storage.Con return nil, nil, err } + if err := observability.ConfigureLogging(&config.Logging); err != nil { + return nil, nil, err + } + if cb != nil { cb(config, nil) } From b7d4a766106ee4bc9c693b9dcbf61863ef060ddc Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Fri, 7 Jun 2024 17:12:10 +0800 Subject: [PATCH 09/11] chore: ignore lint for deprecated fields --- internal/api/admin_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go index 515c0c26e..893df2cb7 100644 --- a/internal/api/admin_test.go +++ b/internal/api/admin_test.go @@ -628,13 +628,17 @@ func (ts *AdminTestSuite) TestAdminUserSoftDeletion() { deletedUser, err := models.FindUserByID(ts.API.db, u.ID) require.NoError(ts.T(), err) - // TODO(km): remove once the db columns are removed + //lint:ignore SA1019 to be removed once the db columns are removed require.Empty(ts.T(), deletedUser.ConfirmationToken) + //lint:ignore SA1019 to be removed once the db columns are removed require.Empty(ts.T(), deletedUser.RecoveryToken) + //lint:ignore SA1019 to be removed once the db columns are removed require.Empty(ts.T(), deletedUser.EmailChangeTokenCurrent) + //lint:ignore SA1019 to be removed once the db columns are removed require.Empty(ts.T(), deletedUser.EmailChangeTokenNew) - require.Empty(ts.T(), deletedUser.EncryptedPassword) + //lint:ignore SA1019 to be removed once the db columns are removed require.Empty(ts.T(), deletedUser.PhoneChangeToken) + require.Empty(ts.T(), deletedUser.EncryptedPassword) // one time tokens table should be empty after a soft deletion require.Empty(ts.T(), u.OneTimeTokens) From ac75297b1dfa3218f8f9e10de28af0d326a21215 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Mon, 10 Jun 2024 16:29:52 +0800 Subject: [PATCH 10/11] chore: resolve pr comments --- internal/api/admin_test.go | 11 ----------- internal/api/mail.go | 18 +++++++----------- internal/models/user.go | 13 ------------- 3 files changed, 7 insertions(+), 35 deletions(-) diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go index 893df2cb7..d230878e9 100644 --- a/internal/api/admin_test.go +++ b/internal/api/admin_test.go @@ -627,17 +627,6 @@ func (ts *AdminTestSuite) TestAdminUserSoftDeletion() { // get soft-deleted user from db deletedUser, err := models.FindUserByID(ts.API.db, u.ID) require.NoError(ts.T(), err) - - //lint:ignore SA1019 to be removed once the db columns are removed - require.Empty(ts.T(), deletedUser.ConfirmationToken) - //lint:ignore SA1019 to be removed once the db columns are removed - require.Empty(ts.T(), deletedUser.RecoveryToken) - //lint:ignore SA1019 to be removed once the db columns are removed - require.Empty(ts.T(), deletedUser.EmailChangeTokenCurrent) - //lint:ignore SA1019 to be removed once the db columns are removed - require.Empty(ts.T(), deletedUser.EmailChangeTokenNew) - //lint:ignore SA1019 to be removed once the db columns are removed - require.Empty(ts.T(), deletedUser.PhoneChangeToken) require.Empty(ts.T(), deletedUser.EncryptedPassword) // one time tokens table should be empty after a soft deletion diff --git a/internal/api/mail.go b/internal/api/mail.go index 683a9a88f..d0013116a 100644 --- a/internal/api/mail.go +++ b/internal/api/mail.go @@ -7,6 +7,7 @@ import ( "github.com/supabase/auth/internal/hooks" mail "github.com/supabase/auth/internal/mailer" + "golang.org/x/sync/errgroup" "github.com/badoux/checkmail" "github.com/fatih/structs" @@ -577,19 +578,14 @@ func (a *API) sendEmailChangeEmails(r *http.Request, tx *storage.Connection, u * return a.invokeHook(tx, r, &input, &output, config.Hook.SendEmail.URI) } - errors := make(chan error) + wg := new(errgroup.Group) if config.Mailer.SecureEmailChangeEnabled && u.GetEmail() != "" { - go func(c chan error) { - c <- mailer.EmailChangeMail(r, u, ottCurrent, otp, referrerURL, externalURL) - }(errors) + go mailer.EmailChangeMail(r, u, ottCurrent, otp, referrerURL, externalURL) } - go func(c chan error) { - c <- mailer.EmailChangeMail(r, u, ottNew, otpNew, referrerURL, externalURL) - }(errors) - - // we return the first error that's sent to the channel - if e := <-errors; e != nil { - return e + go mailer.EmailChangeMail(r, u, ottNew, otpNew, referrerURL, externalURL) + if err := wg.Wait(); err != nil { + // we return the first not-nil error observed + return err } return nil } diff --git a/internal/models/user.go b/internal/models/user.go index 39021bd82..3ac808f08 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -63,19 +63,6 @@ type User struct { DONTUSEINSTANCEID uuid.UUID `json:"-" db:"instance_id"` OneTimeTokens []OneTimeToken `json:"-" has_many:"one_time_tokens"` - - // Deprecated: use the one_time_tokens model instead - ConfirmationToken string `json:"-" db:"confirmation_token"` - // Deprecated: use the one_time_tokens model instead - RecoveryToken string `json:"-" db:"recovery_token"` - // Deprecated: use the one_time_tokens model instead - EmailChangeTokenCurrent string `json:"-" db:"email_change_token_current"` - // Deprecated: use the one_time_tokens model instead - EmailChangeTokenNew string `json:"-" db:"email_change_token_new"` - // Deprecated: use the one_time_tokens model instead - PhoneChangeToken string `json:"-" db:"phone_change_token"` - // Deprecated: use the one_time_tokens model instead - ReauthenticationToken string `json:"-" db:"reauthentication_token"` } // NewUser initializes a new user from an email, password and user data. From 2e3f9d4b0f4062a2bdfa84cecf9b3707307f0563 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Mon, 10 Jun 2024 22:36:33 +0800 Subject: [PATCH 11/11] chore: remove unnecessary comments & check for pkce prefix in verify --- hack/test.env | 1 + internal/api/verify.go | 19 +++++++------ internal/api/verify_test.go | 55 +++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 8 deletions(-) diff --git a/hack/test.env b/hack/test.env index fcdd6f36b..295680b25 100644 --- a/hack/test.env +++ b/hack/test.env @@ -15,6 +15,7 @@ GOTRUE_LOG_LEVEL=info GOTRUE_SITE_URL=https://example.netlify.com GOTRUE_URI_ALLOW_LIST="http://localhost:3000" GOTRUE_OPERATOR_TOKEN=foobar +GOTRUE_API_MAX_REQUEST_DURATION="0" GOTRUE_EXTERNAL_APPLE_ENABLED=true GOTRUE_EXTERNAL_APPLE_CLIENT_ID=testclientid GOTRUE_EXTERNAL_APPLE_SECRET=testsecret diff --git a/internal/api/verify.go b/internal/api/verify.go index ae2cb6266..a59391550 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -604,7 +604,7 @@ func (a *API) verifyTokenHash(conn *storage.Connection, params *VerifyParams) (* case mail.EmailOTPVerification: sentAt := user.ConfirmationSentAt params.Type = "signup" - if _, err := models.FindOneTimeToken(conn, params.TokenHash, models.ReauthenticationToken); err != nil { + if _, err := models.FindOneTimeToken(conn, params.TokenHash, models.RecoveryToken); err != nil { if !models.IsNotFoundError(err) { return nil, internalServerError("Database error finding token").WithInternalError(err) } @@ -665,7 +665,6 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, switch params.Type { case mail.EmailOTPVerification: // if the type is emailOTPVerification, we'll check both the confirmation_token and recovery_token columns - // if isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) { isValid, otpErr = isOtpValid(conn, tokenHash, config.Mailer.OtpExp, models.ConfirmationToken) if otpErr == nil { params.Type = mail.SignupVerification @@ -676,7 +675,6 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, params.Type = mail.MagicLinkVerification break } - // need some way to figure out whether to set the param type to signup or magiclink case mail.SignupVerification, mail.InviteVerification: isValid, otpErr = isOtpValid(conn, tokenHash, config.Mailer.OtpExp, models.ConfirmationToken) case mail.RecoveryVerification, mail.MagicLinkVerification: @@ -716,16 +714,21 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, func isOtpValid(tx *storage.Connection, tokenHash string, otpExp uint, tokenTypes ...models.OneTimeTokenType) (bool, error) { token, err := models.FindOneTimeToken(tx, tokenHash, tokenTypes...) if err != nil { - if models.IsNotFoundError(err) { - return false, nil + if !models.IsNotFoundError(err) { + return false, err + } + // try again with the pkce prefix + token, err = models.FindOneTimeToken(tx, "pkce_"+tokenHash, tokenTypes...) + if err != nil { + if models.IsNotFoundError(err) { + return false, nil + } + return false, err } - return false, err } if isOtpExpired(&token.CreatedAt, otpExp) { return false, nil } - - // return !isOtpExpired(sentAt, otpExp) && ((actual == expected) || ("pkce_"+actual == expected)) return true, nil } diff --git a/internal/api/verify_test.go b/internal/api/verify_test.go index 8031c3ef0..4e192bd2e 100644 --- a/internal/api/verify_test.go +++ b/internal/api/verify_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/gofrs/uuid" mail "github.com/supabase/auth/internal/mailer" "github.com/stretchr/testify/assert" @@ -748,6 +749,60 @@ func (ts *VerifyTestSuite) TestVerifyPKCEOTP() { } +func (ts *VerifyTestSuite) TestVerifyPostPKCE() { + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "email": "test@example.com", + "password": "test-password", + "code_challenge": "codechallengecodechallengcodechallengcodechallengcodechallenge", + "code_challenge_method": "plain", + })) + req := httptest.NewRequest(http.MethodPost, "http://localhost/signup", &buffer) + req.Header.Set("Content-Type", "application/json") + + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + require.Equal(ts.T(), http.StatusOK, w.Code) + + var data map[string]interface{} + require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data)) + userId, err := uuid.FromString(data["id"].(string)) + require.NoError(ts.T(), err) + u, err := models.FindUserByID(ts.API.db, userId) + require.NoError(ts.T(), err) + require.NotEmpty(ts.T(), u.OneTimeTokens) + + // update the token hash to a known value for subsequent verification + ott := u.OneTimeTokens[0] + ott.TokenHash = addFlowPrefixToToken(crypto.GenerateTokenHash("test@example.com", "123456"), models.PKCEFlow) + require.NoError(ts.T(), ts.API.db.Update(&ott)) + + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "email": "test@example.com", + "token": "123456", + "type": "email", + })) + req = httptest.NewRequest(http.MethodPost, "http://localhost/verify", &buffer) + req.Header.Set("Content-Type", "application/json") + + w = httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + require.Equal(ts.T(), http.StatusOK, w.Code) + + // check that the response contains an access and refresh token + require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data)) + require.Contains(ts.T(), data, "access_token") + require.Contains(ts.T(), data, "refresh_token") + + // check that the user in the db is confirmed + u, err = models.FindUserByID(ts.API.db, userId) + require.NoError(ts.T(), err) + require.True(ts.T(), u.IsConfirmed()) + + // // one time tokens should be cleared after successful verification + require.Empty(ts.T(), u.OneTimeTokens) +} + func (ts *VerifyTestSuite) TestVerifyBannedUser() { u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err)