From a5462ac1ca8fdce5bda56cf3fd5b26634ed8c06a Mon Sep 17 00:00:00 2001 From: "joel@joellee.org" Date: Sun, 3 Sep 2023 21:49:41 +0200 Subject: [PATCH 1/4] fix: add flag for singleConfirmationResponse --- internal/api/verify.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index 1589225fa..6ab4e0cbe 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -231,6 +231,7 @@ func (a *API) verifyPost(w http.ResponseWriter, r *http.Request, params *VerifyP grantParams models.GrantParams token *AccessTokenResponse ) + var isSingleConfirmationResponse = false err := db.Transaction(func(tx *storage.Connection) error { var terr error @@ -253,10 +254,8 @@ func (a *API) verifyPost(w http.ResponseWriter, r *http.Request, params *VerifyP case emailChangeVerification: user, terr = a.emailChangeVerify(r, ctx, tx, params, user) if user == nil && terr == nil { - return sendJSON(w, http.StatusOK, map[string]string{ - "msg": singleConfirmationAccepted, - "code": strconv.Itoa(http.StatusOK), - }) + isSingleConfirmationResponse = true + return nil } case smsVerification, phoneChangeVerification: user, terr = a.smsVerify(r, ctx, tx, user, params.Type) @@ -280,6 +279,12 @@ func (a *API) verifyPost(w http.ResponseWriter, r *http.Request, params *VerifyP if err != nil { return err } + if isSingleConfirmationResponse { + return sendJSON(w, http.StatusOK, map[string]string{ + "msg": singleConfirmationAccepted, + "code": strconv.Itoa(http.StatusOK), + }) + } return sendJSON(w, http.StatusOK, token) } From 4c820a144be831037056e2b4aac2ee598f7fae4c Mon Sep 17 00:00:00 2001 From: "joel@joellee.org" Date: Mon, 4 Sep 2023 12:56:34 +0400 Subject: [PATCH 2/4] chore: add initial test --- internal/api/verify_test.go | 63 +++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/internal/api/verify_test.go b/internal/api/verify_test.go index af1fcfd1d..941f3d894 100644 --- a/internal/api/verify_test.go +++ b/internal/api/verify_test.go @@ -622,7 +622,6 @@ func (ts *VerifyTestSuite) TestVerifySignupWithredirectURLContainedPath() { } func (ts *VerifyTestSuite) TestVerifyPKCEOTP() { - u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) u.ConfirmationToken = "pkce_confirmation_token" @@ -769,8 +768,8 @@ func (ts *VerifyTestSuite) TestVerifyValidOtp() { u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) u.EmailChange = "new@example.com" - u.Phone = "12345678" - u.PhoneChange = "1234567890" + u.Phone = "12345677" + u.PhoneChange = "1234567888" require.NoError(ts.T(), ts.API.db.Update(u)) type expected struct { @@ -935,6 +934,64 @@ func (ts *VerifyTestSuite) TestVerifyValidOtp() { } } +func (ts *VerifyTestSuite) TestSecureEmailChangeWithTokenHash() { + ts.Config.Mailer.SecureEmailChangeEnabled = true + u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + u.EmailChange = "new@example.com" + u.Phone = "12345677" + u.PhoneChange = "1234567888" + require.NoError(ts.T(), ts.API.db.Update(u)) + + cases := []struct { + desc string + emailChangeTokenNew string + emailChangeTokenCurrent string + shouldBeSuccessful bool + }{ + { + desc: "Secure Email Change with Token Hash. Calling Token hash with the two respective token hashes should return token", + emailChangeTokenNew: "TODO: to fill", + emailChangeTokenCurrent: "TODO: to fill", + shouldBeSuccessful: true, + }, + { + desc: "Secure Email Change with Token Hash. Using the same token hash twice should fail.", + emailChangeTokenNew: "TODO: to fill", + emailChangeTokenCurrent: "TODO: same as firstTokenHash", + shouldBeSuccessful: false, + }, + } + for _, c := range cases { + ts.Run(c.desc, func() { + // Set the corresponding email change tokens + u.EmailChangeSentAt = &c.sentTime + u.EmailChangeTokenNew = c.tokenHash + require.NoError(ts.T(), ts.API.db.Update(u)) + + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.body)) + + // Setup request + req := httptest.NewRequest(http.MethodPost, "http://localhost/verify", &buffer) + req.Header.Set("Content-Type", "application/json") + + // Setup response recorder + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + assert.Equal(ts.T(), c.expected.code, w.Code) + // Check that response is adequate + // + + // Make another request here + // if it shouldBeSuccessful, check that it returns a token. + // Else make sure that it properly returns an error. + }) + + } + +} + func (ts *VerifyTestSuite) TestPrepRedirectURL() { escapedMessage := url.QueryEscape(singleConfirmationAccepted) cases := []struct { From 576b08ea39e43ef36e339561bd50505f8b6d5941 Mon Sep 17 00:00:00 2001 From: "joel@joellee.org" Date: Mon, 4 Sep 2023 17:25:33 +0400 Subject: [PATCH 3/4] chore: move over token hash fix --- internal/api/verify.go | 4 +-- internal/api/verify_test.go | 72 +++++++++++++++++++++++++------------ 2 files changed, 52 insertions(+), 24 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index 6ab4e0cbe..ce3a32b1c 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -453,9 +453,9 @@ func (a *API) emailChangeVerify(r *http.Request, ctx context.Context, conn *stor if config.Mailer.SecureEmailChangeEnabled && user.EmailChangeConfirmStatus == zeroConfirmation && user.GetEmail() != "" { err := conn.Transaction(func(tx *storage.Connection) error { user.EmailChangeConfirmStatus = singleConfirmation - if params.Token == user.EmailChangeTokenCurrent { + if params.Token == user.EmailChangeTokenCurrent || params.TokenHash == user.EmailChangeTokenCurrent { user.EmailChangeTokenCurrent = "" - } else if params.Token == user.EmailChangeTokenNew { + } else if params.Token == user.EmailChangeTokenNew || params.TokenHash == user.EmailChangeTokenNew { user.EmailChangeTokenNew = "" } if terr := tx.UpdateOnly(user, "email_change_confirm_status", "email_change_token_current", "email_change_token_new"); terr != nil { diff --git a/internal/api/verify_test.go b/internal/api/verify_test.go index 941f3d894..c44b441ad 100644 --- a/internal/api/verify_test.go +++ b/internal/api/verify_test.go @@ -939,38 +939,55 @@ func (ts *VerifyTestSuite) TestSecureEmailChangeWithTokenHash() { u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) u.EmailChange = "new@example.com" - u.Phone = "12345677" - u.PhoneChange = "1234567888" require.NoError(ts.T(), ts.API.db.Update(u)) + currentEmailChangeToken := crypto.GenerateTokenHash(string(u.Email), "123456") + newEmailChangeToken := crypto.GenerateTokenHash(u.EmailChange, "123456") + cases := []struct { - desc string - emailChangeTokenNew string - emailChangeTokenCurrent string - shouldBeSuccessful bool + desc string + firstVerificationBody map[string]interface{} + secondVerificationBody map[string]interface{} + shouldBeSuccessful bool }{ { - desc: "Secure Email Change with Token Hash. Calling Token hash with the two respective token hashes should return token", - emailChangeTokenNew: "TODO: to fill", - emailChangeTokenCurrent: "TODO: to fill", - shouldBeSuccessful: true, + desc: "Secure Email Change with Token Hash. Calling Token hash with the two respective token hashes should return token", + firstVerificationBody: map[string]interface{}{ + "type": emailChangeVerification, + "token_hash": currentEmailChangeToken, + }, + secondVerificationBody: map[string]interface{}{ + "type": emailChangeVerification, + "token_hash": newEmailChangeToken, + }, + shouldBeSuccessful: true, }, { - desc: "Secure Email Change with Token Hash. Using the same token hash twice should fail.", - emailChangeTokenNew: "TODO: to fill", - emailChangeTokenCurrent: "TODO: same as firstTokenHash", - shouldBeSuccessful: false, + desc: "Secure Email Change with Token Hash. Using the same token hash twice should fail.", + firstVerificationBody: map[string]interface{}{ + "type": emailChangeVerification, + "token_hash": currentEmailChangeToken, + }, + secondVerificationBody: map[string]interface{}{ + "type": emailChangeVerification, + "token_hash": currentEmailChangeToken, + }, + shouldBeSuccessful: false, }, } for _, c := range cases { ts.Run(c.desc, func() { // Set the corresponding email change tokens - u.EmailChangeSentAt = &c.sentTime - u.EmailChangeTokenNew = c.tokenHash + + u.EmailChangeTokenCurrent = currentEmailChangeToken + u.EmailChangeTokenNew = newEmailChangeToken + + currentTime := time.Now() + u.EmailChangeSentAt = ¤tTime require.NoError(ts.T(), ts.API.db.Update(u)) var buffer bytes.Buffer - require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.body)) + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.firstVerificationBody)) // Setup request req := httptest.NewRequest(http.MethodPost, "http://localhost/verify", &buffer) @@ -979,13 +996,24 @@ func (ts *VerifyTestSuite) TestSecureEmailChangeWithTokenHash() { // Setup response recorder w := httptest.NewRecorder() ts.API.handler.ServeHTTP(w, req) - assert.Equal(ts.T(), c.expected.code, w.Code) - // Check that response is adequate - // + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.secondVerificationBody)) + + // Setup request + req = httptest.NewRequest(http.MethodPost, "http://localhost/verify", &buffer) + req.Header.Set("Content-Type", "application/json") + + // Setup response recorder + w = httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) - // Make another request here // if it shouldBeSuccessful, check that it returns a token. - // Else make sure that it properly returns an error. + if c.shouldBeSuccessful { + assert.Equal(ts.T(), http.StatusOK, w.Code) + + } else { + assert.NotEqual(ts.T(), http.StatusOK, w.Code) + + } }) } From 40ba86fe9c1d6d8e438c510bb7bc1c2e9d51ff0b Mon Sep 17 00:00:00 2001 From: "joel@joellee.org" Date: Tue, 5 Sep 2023 13:16:41 +0400 Subject: [PATCH 4/4] refactor: update descriptions --- internal/api/verify_test.go | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/internal/api/verify_test.go b/internal/api/verify_test.go index c44b441ad..6c93e07c7 100644 --- a/internal/api/verify_test.go +++ b/internal/api/verify_test.go @@ -768,8 +768,8 @@ func (ts *VerifyTestSuite) TestVerifyValidOtp() { u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) u.EmailChange = "new@example.com" - u.Phone = "12345677" - u.PhoneChange = "1234567888" + u.Phone = "12345678" + u.PhoneChange = "1234567890" require.NoError(ts.T(), ts.API.db.Update(u)) type expected struct { @@ -948,10 +948,10 @@ func (ts *VerifyTestSuite) TestSecureEmailChangeWithTokenHash() { desc string firstVerificationBody map[string]interface{} secondVerificationBody map[string]interface{} - shouldBeSuccessful bool + expectedStatus int }{ { - desc: "Secure Email Change with Token Hash. Calling Token hash with the two respective token hashes should return token", + desc: "Secure Email Change with Token Hash (Success)", firstVerificationBody: map[string]interface{}{ "type": emailChangeVerification, "token_hash": currentEmailChangeToken, @@ -960,10 +960,10 @@ func (ts *VerifyTestSuite) TestSecureEmailChangeWithTokenHash() { "type": emailChangeVerification, "token_hash": newEmailChangeToken, }, - shouldBeSuccessful: true, + expectedStatus: http.StatusOK, }, { - desc: "Secure Email Change with Token Hash. Using the same token hash twice should fail.", + desc: "Secure Email Change with Token Hash. Reusing a token hash twice should fail", firstVerificationBody: map[string]interface{}{ "type": emailChangeVerification, "token_hash": currentEmailChangeToken, @@ -972,13 +972,12 @@ func (ts *VerifyTestSuite) TestSecureEmailChangeWithTokenHash() { "type": emailChangeVerification, "token_hash": currentEmailChangeToken, }, - shouldBeSuccessful: false, + expectedStatus: http.StatusUnauthorized, }, } for _, c := range cases { ts.Run(c.desc, func() { // Set the corresponding email change tokens - u.EmailChangeTokenCurrent = currentEmailChangeToken u.EmailChangeTokenNew = newEmailChangeToken @@ -998,22 +997,14 @@ func (ts *VerifyTestSuite) TestSecureEmailChangeWithTokenHash() { ts.API.handler.ServeHTTP(w, req) require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.secondVerificationBody)) - // Setup request + // Setup second request req = httptest.NewRequest(http.MethodPost, "http://localhost/verify", &buffer) req.Header.Set("Content-Type", "application/json") - // Setup response recorder + // Setup second response recorder w = httptest.NewRecorder() ts.API.handler.ServeHTTP(w, req) - - // if it shouldBeSuccessful, check that it returns a token. - if c.shouldBeSuccessful { - assert.Equal(ts.T(), http.StatusOK, w.Code) - - } else { - assert.NotEqual(ts.T(), http.StatusOK, w.Code) - - } + assert.Equal(ts.T(), c.expectedStatus, w.Code) }) }