From 385ea800a4bb6ffbfdfe7b5931232e3b5700a7bf Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Mon, 9 Jan 2023 00:50:41 +0100 Subject: [PATCH] don't verify subscription email once more for email users Previous behaviour is preserved for query parameters way of requesting the subscription. The new behaviour with the possibility to confirm the email right away without a separate /email/confirm call is enabled only with request params sent in the request body, which was not a thing before 27fc339e, which was merged just now and is not part of any tagged version yet. --- backend/app/rest/api/rest_private.go | 42 ++++++--- backend/app/rest/api/rest_private_test.go | 101 +++++++++++++++++++++- backend/app/rest/api/rest_test.go | 2 + backend/remark.rest | 2 + 4 files changed, 132 insertions(+), 15 deletions(-) diff --git a/backend/app/rest/api/rest_private.go b/backend/app/rest/api/rest_private.go index 4a8cd27b..f1930232 100644 --- a/backend/app/rest/api/rest_private.go +++ b/backend/app/rest/api/rest_private.go @@ -307,6 +307,8 @@ func (s *private) getEmailCtrl(w http.ResponseWriter, r *http.Request) { } // sendEmailConfirmationCtrl gets address and siteID from query, makes confirmation token and sends it to user. +// In case user is logged in with the same email, and auto_confirm is true, confirm it right away. +// In case of quick confirmation, "updated" is set to true, otherwise - to false. // POST /email/subscribe with site and address in json body // //nolint:dupl // too hard to deduplicate that logic, as then it's tricky to use SendErrorJSON @@ -314,17 +316,19 @@ func (s *private) sendEmailConfirmationCtrl(w http.ResponseWriter, r *http.Reque user := rest.MustGetUserInfo(r) subscribe := struct { - Site string - Address string - }{} + Site string + Address string + autoConfirm bool + }{autoConfirm: true} if err := render.DecodeJSON(http.MaxBytesReader(w, r.Body, hardBodyLimit), &subscribe); err != nil { if err != io.EOF { rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't parse request body", rest.ErrDecode) return } - // old behavior fallback, reading from the query params + // old behavior fallback, reading from the query params. Auto confirm is false in this case. subscribe.Address = r.URL.Query().Get("address") subscribe.Site = r.URL.Query().Get("site") + subscribe.autoConfirm = false } if subscribe.Address == "" { @@ -332,15 +336,28 @@ func (s *private) sendEmailConfirmationCtrl(w http.ResponseWriter, r *http.Reque fmt.Errorf("missing parameter"), "address parameter is required", rest.ErrInternal) return } - existingAddress, err := s.dataService.GetUserEmail(subscribe.Site, user.ID) - if err != nil { - log.Printf("[WARN] can't read email for %s, %v", user.ID, err) + existingAddress, getErr := s.dataService.GetUserEmail(subscribe.Site, user.ID) + if getErr != nil { + log.Printf("[WARN] can't read email for %s, %v", user.ID, getErr) } if subscribe.Address == existingAddress { rest.SendErrorJSON(w, r, http.StatusConflict, fmt.Errorf("already verified"), "email address is already verified for this user", rest.ErrInternal) return } + + // in case the user logged in with the same email as they try to subscribe with, confirm it right away + // this behavior is different from the previous one and is hidden behind the autoConfirm flag, + // which is true for the new API, and false for the old one + // + // nolint:gosec // this is not used for security purposes + if subscribe.autoConfirm && + strings.HasPrefix(user.ID, "email_") && + strings.TrimPrefix(user.ID, "email_") == token.HashID(sha1.New(), subscribe.Address) { + s.setEmail(w, r, user.ID, subscribe.Site, subscribe.Address) + return + } + claims := token.Claims{ Handshake: &token.Handshake{ID: user.ID + "::" + subscribe.Address}, StandardClaims: jwt.StandardClaims{ @@ -366,7 +383,7 @@ func (s *private) sendEmailConfirmationCtrl(w http.ResponseWriter, r *http.Reque }, ) - render.JSON(w, r, R.JSON{"user": user, "address": subscribe.Address}) + render.JSON(w, r, R.JSON{"user": user, "address": subscribe.Address, "updated": false}) } // telegramSubscribeCtrl generates and verifies telegram notification request @@ -470,17 +487,20 @@ func (s *private) setConfirmedEmailCtrl(w http.ResponseWriter, r *http.Request) return } address := elems[1] + s.setEmail(w, r, user.ID, confirm.Site, address) +} - log.Printf("[DEBUG] set email for user %s", user.ID) +func (s *private) setEmail(w http.ResponseWriter, r *http.Request, userID, siteID, address string) { + log.Printf("[DEBUG] set email for user %s", userID) - val, err := s.dataService.SetUserEmail(confirm.Site, user.ID, address) + val, err := s.dataService.SetUserEmail(siteID, userID, address) if err != nil { code := parseError(err, rest.ErrInternal) rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't set email for user", code) return } - // update User.Email from the token + // update User.Email field claims, _, err := s.authenticator.TokenService().Get(r) if err != nil { rest.SendErrorJSON(w, r, http.StatusForbidden, err, "failed to verify confirmation token", rest.ErrInternal) diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index 5cb66d46..0e412863 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -849,7 +849,7 @@ func TestRest_EmailNotification(t *testing.T) { // send confirmation token for email req, err = http.NewRequest( http.MethodPost, - ts.URL+"/api/v1/email/subscribe?site=remark42&address=", + ts.URL+"/api/v1/email/subscribe", io.NopCloser(strings.NewReader(`{"site": "remark42", "address": "good@example.com"}`)), ) require.NoError(t, err) @@ -866,6 +866,25 @@ func TestRest_EmailNotification(t *testing.T) { assert.Equal(t, "good@example.com", mockDestination.GetVerify()[0].Email) verificationToken := mockDestination.GetVerify()[0].Token + // get user information to verify lack of the subscription + req, err = http.NewRequest( + http.MethodGet, + ts.URL+"/api/v1/user?site=remark42", + http.NoBody) + require.NoError(t, err) + req.Header.Add("X-JWT", devToken) + resp, err = client.Do(req) + require.NoError(t, err) + body, err = io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusOK, resp.StatusCode, string(body)) + var clearUser store.User + err = json.Unmarshal(body, &clearUser) + assert.NoError(t, err) + assert.Equal(t, store.User{Name: "developer one", ID: "dev", EmailSubscription: false, + Picture: "http://example.com/pic.png", IP: "127.0.0.1", SiteID: "remark42"}, clearUser) + // verify email req, err = http.NewRequest( http.MethodPost, @@ -894,11 +913,11 @@ func TestRest_EmailNotification(t *testing.T) { require.NoError(t, err) require.NoError(t, resp.Body.Close()) require.Equal(t, http.StatusOK, resp.StatusCode, string(body)) - var user store.User - err = json.Unmarshal(body, &user) + var subscribedUser store.User + err = json.Unmarshal(body, &subscribedUser) assert.NoError(t, err) assert.Equal(t, store.User{Name: "developer one", ID: "dev", EmailSubscription: true, - Picture: "http://example.com/pic.png", IP: "127.0.0.1", SiteID: "remark42"}, user) + Picture: "http://example.com/pic.png", IP: "127.0.0.1", SiteID: "remark42"}, subscribedUser) // create child comment from another user, email notification expected req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader(fmt.Sprintf( @@ -949,6 +968,80 @@ func TestRest_EmailNotification(t *testing.T) { time.Sleep(time.Millisecond * 30) require.Equal(t, 4, len(mockDestination.Get())) assert.Empty(t, mockDestination.Get()[3].Emails) + + // confirm email via subscribe call with query params, old behavior, email notification is expected + req, err = http.NewRequest( + http.MethodPost, + ts.URL+"/api/v1/email/subscribe?site=remark42&address=good@example.com", + http.NoBody, + ) + require.NoError(t, err) + req.Header.Add("X-JWT", emailUserToken) + resp, err = client.Do(req) + require.NoError(t, err) + body, err = io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusOK, resp.StatusCode, string(body)) + // wait for mock notification Submit to kick off + time.Sleep(time.Millisecond * 30) + require.Equal(t, 2, len(mockDestination.GetVerify()), "verification email was sent") + + // get email user information to verify there is no subscription yet + req, err = http.NewRequest( + http.MethodGet, + ts.URL+"/api/v1/user?site=remark42", + http.NoBody) + require.NoError(t, err) + req.Header.Add("X-JWT", emailUserToken) + resp, err = client.Do(req) + require.NoError(t, err) + body, err = io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusOK, resp.StatusCode, string(body)) + var unsubscribedEmailUser store.User + err = json.Unmarshal(body, &unsubscribedEmailUser) + assert.NoError(t, err) + assert.Equal(t, store.User{Name: "good@example.com test user", ID: "email_f5dfe9d2e6bd75fc74ea5fabf273b45b5baeb195", EmailSubscription: false, + Picture: "http://example.com/pic.png", IP: "127.0.0.1", SiteID: "remark42"}, unsubscribedEmailUser) + + // confirm email via subscribe call, no email notification is expected + req, err = http.NewRequest( + http.MethodPost, + ts.URL+"/api/v1/email/subscribe", + io.NopCloser(strings.NewReader(`{"site": "remark42", "address": "good@example.com"}`)), + ) + require.NoError(t, err) + req.Header.Add("X-JWT", emailUserToken) + resp, err = client.Do(req) + require.NoError(t, err) + body, err = io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusOK, resp.StatusCode, string(body)) + // wait for mock notification Submit to kick off + time.Sleep(time.Millisecond * 30) + require.Equal(t, 2, len(mockDestination.GetVerify()), "no new verification email was sent") + + // get email user information to verify the subscription happened without the confirmation call + req, err = http.NewRequest( + http.MethodGet, + ts.URL+"/api/v1/user?site=remark42", + http.NoBody) + require.NoError(t, err) + req.Header.Add("X-JWT", emailUserToken) + resp, err = client.Do(req) + require.NoError(t, err) + body, err = io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusOK, resp.StatusCode, string(body)) + var subscribedEmailUser store.User + err = json.Unmarshal(body, &subscribedEmailUser) + assert.NoError(t, err) + assert.Equal(t, store.User{Name: "good@example.com test user", ID: "email_f5dfe9d2e6bd75fc74ea5fabf273b45b5baeb195", EmailSubscription: true, + Picture: "http://example.com/pic.png", IP: "127.0.0.1", SiteID: "remark42"}, subscribedEmailUser) } func TestRest_TelegramNotification(t *testing.T) { diff --git a/backend/app/rest/api/rest_test.go b/backend/app/rest/api/rest_test.go index 0667da53..80280052 100644 --- a/backend/app/rest/api/rest_test.go +++ b/backend/app/rest/api/rest_test.go @@ -41,6 +41,8 @@ var devToken = `eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJyZW1hcms0MiIsImV var anonToken = `eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJyZW1hcms0MiIsImV4cCI6Mzc4OTE5MTgyMiwianRpIjoicmFuZG9tIGlkIiwiaXNzIjoicmVtYXJrNDIiLCJuYmYiOjE1MjE4ODQyMjIsInVzZXIiOnsibmFtZSI6ImFub255bW91cyB0ZXN0IHVzZXIiLCJpZCI6ImFub255bW91c190ZXN0X3VzZXIiLCJwaWN0dXJlIjoiaHR0cDovL2V4YW1wbGUuY29tL3BpYy5wbmciLCJpcCI6IjEyNy4wLjAuMSIsImVtYWlsIjoiYW5vbkBleGFtcGxlLmNvbSJ9fQ.gAae2WMxZNZE5ebVboptPEyQ7Nk6EQxciNnGJ_mPOuU` +var emailUserToken = `eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJyZW1hcms0MiIsImV4cCI6Mzc4OTE5MTgyMiwianRpIjoicmFuZG9tIGlkIiwiaXNzIjoicmVtYXJrNDIiLCJuYmYiOjE1MjE4ODQyMjIsInVzZXIiOnsibmFtZSI6Imdvb2RAZXhhbXBsZS5jb20gdGVzdCB1c2VyIiwiaWQiOiJlbWFpbF9mNWRmZTlkMmU2YmQ3NWZjNzRlYTVmYWJmMjczYjQ1YjViYWViMTk1IiwicGljdHVyZSI6Imh0dHA6Ly9leGFtcGxlLmNvbS9waWMucG5nIiwiaXAiOiIxMjcuMC4wLjEiLCJlbWFpbCI6Imdvb2RAZXhhbXBsZS5jb20ifX0.vH2HN1JpuXL8okTJq1A-zGHQ-l2ILcwxvDDEmu2zwks` + var devTokenBadAud = `eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJyZW1hcms0Ml9iYWQiLCJleHAiOjM3ODkxOTE4MjIsImp0aSI6InJhbmRvbSBpZCIsImlzcyI6InJlbWFyazQyIiwibmJmIjoxNTIxODg0MjIyLCJ1c2VyIjp7Im5hbWUiOiJkZXZlbG9wZXIgb25lIiwiaWQiOiJkZXYiLCJwaWN0dXJlIjoiaHR0cDovL2V4YW1wbGUuY29tL3BpYy5wbmciLCJpcCI6IjEyNy4wLjAuMSIsImVtYWlsIjoibWVAZXhhbXBsZS5jb20ifX0.FuTTocVtcxr4VjpfIICvU2yOb3su28VkDzj94H9Q3xY` var adminUmputunToken = `eyJhbGciOiJIUzI1NiJ9.eyJhdWQiOiJyZW1hcms0MiIsImV4cCI6MTk1NDU5Nzk4MCwianRpIjoiOTdhMmUwYWM0ZGM3ZDVmNjkyNmQ1ZTg2MjBhY2VmOWE0MGMwIiwiaWF0IjoxNDU0NTk3NjgwLCJpc3MiOiJyZW1hcms0MiIsInVzZXIiOnsibmFtZSI6IlVtcHV0dW4iLCJpZCI6ImdpdGh1Yl9lZjBmNzA2YTciLCJwaWN0dXJlIjoiaHR0cHM6Ly9yZW1hcms0Mi5yYWRpby10LmNvbS9hcGkvdjEvYXZhdGFyL2NiNDJmZjQ5M2FkZTY5NmQ4OGEzYTU5MGYxMzZhZTllMzRkZTdjMWIuaW1hZ2UiLCJhdHRycyI6eyJhZG1pbiI6dHJ1ZSwiYmxvY2tlZCI6ZmFsc2V9fX0.dZiOjWHguo9f42XCMooMcv4EmYFzifl_-LEvPZHCtks` diff --git a/backend/remark.rest b/backend/remark.rest index eca6c6d1..7febc218 100644 --- a/backend/remark.rest +++ b/backend/remark.rest @@ -119,6 +119,8 @@ GET {{host}}/api/v1/rss/reply?site={{site}}&user={{user}} GET {{host}}/api/v1/avatar/blah ### send confirmation token for current user to specified email. auth token for dev user for secret=12345. +### in case the user logged in with the same email, it will be confirmed right away with "updated" set to "true" in the response, +### and no email will be sent. POST {{host}}/api/v1/email/subscribe X-JWT: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJyZW1hcmsiLCJleHAiOjE5NzYwNTY3NTYsImp0aSI6IjJlOGJmMTE5OTI0MjQxMDRjYjFhZGRlODllMWYwNGFiMTg4YWZjMzQiLCJpYXQiOjE1NzYwNTY0NTYsImlzcyI6InJlbWFyazQyIiwidXNlciI6eyJuYW1lIjoiZGV2X3VzZXIiLCJpZCI6ImRldl91c2VyIiwicGljdHVyZSI6Imh0dHA6Ly8xMjcuMC4wLjE6ODA4MC9hcGkvdjEvYXZhdGFyL2NjZmEyYWJkMDE2Njc2MDViNGUxZmM0ZmNiOTFiMWUxYWYzMjMyNDAuaW1hZ2UiLCJhdHRycyI6eyJhZG1pbiI6dHJ1ZSwiYmxvY2tlZCI6ZmFsc2V9fX0.6Qt5s2enBMRC-Jmsua01yViVYI95Dx6BPBMaNjj36d4 Content-Type: application/json