From 2e3f9d4b0f4062a2bdfa84cecf9b3707307f0563 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Mon, 10 Jun 2024 22:36:33 +0800 Subject: [PATCH] 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)