From 49e482674700e184aa84806acfb7edaae0554291 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Mon, 26 Feb 2024 05:55:00 +0800
Subject: [PATCH] Refactor "user/active" related logic (#29390)
And add more tests. Remove a lot of fragile "if" blocks.
The old logic is kept as-is.
---
routers/web/auth/auth.go | 127 ++++++++++++-----------
templates/user/auth/activate.tmpl | 48 +++------
templates/user/auth/activate_prompt.tmpl | 15 +++
tests/integration/signup_test.go | 50 ++++++++-
4 files changed, 146 insertions(+), 94 deletions(-)
create mode 100644 templates/user/auth/activate_prompt.tmpl
diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go
index 3de1f3373dc1..a30ee0ce542f 100644
--- a/routers/web/auth/auth.go
+++ b/routers/web/auth/auth.go
@@ -7,6 +7,7 @@ package auth
import (
"errors"
"fmt"
+ "html/template"
"net/http"
"strings"
@@ -37,12 +38,10 @@ import (
)
const (
- // tplSignIn template for sign in page
- tplSignIn base.TplName = "user/auth/signin"
- // tplSignUp template path for sign up page
- tplSignUp base.TplName = "user/auth/signup"
- // TplActivate template path for activate user
- TplActivate base.TplName = "user/auth/activate"
+ tplSignIn base.TplName = "user/auth/signin" // for sign in page
+ tplSignUp base.TplName = "user/auth/signup" // for sign up page
+ TplActivate base.TplName = "user/auth/activate" // for activate user
+ TplActivatePrompt base.TplName = "user/auth/activate_prompt" // for showing a message for user activation
)
// autoSignIn reads cookie and try to auto-login.
@@ -613,72 +612,83 @@ func handleUserCreated(ctx *context.Context, u *user_model.User, gothUser *goth.
}
}
- // Send confirmation email
- if !u.IsActive && u.ID > 1 {
- if setting.Service.RegisterManualConfirm {
- ctx.Data["ManualActivationOnly"] = true
- ctx.HTML(http.StatusOK, TplActivate)
- return false
- }
+ // for active user or the first (admin) user, we don't need to send confirmation email
+ if u.IsActive || u.ID == 1 {
+ return true
+ }
- mailer.SendActivateAccountMail(ctx.Locale, u)
-
- ctx.Data["IsSendRegisterMail"] = true
- ctx.Data["Email"] = u.Email
- ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale)
- ctx.HTML(http.StatusOK, TplActivate)
-
- if err := ctx.Cache.Put("MailResendLimit_"+u.LowerName, u.LowerName, 180); err != nil {
- log.Error("Set cache(MailResendLimit) fail: %v", err)
- }
+ if setting.Service.RegisterManualConfirm {
+ renderActivationPromptMessage(ctx, ctx.Locale.Tr("auth.manual_activation_only"))
return false
}
- return true
+ sendActivateEmail(ctx, u)
+ return false
+}
+
+func renderActivationPromptMessage(ctx *context.Context, msg template.HTML) {
+ ctx.Data["ActivationPromptMessage"] = msg
+ ctx.HTML(http.StatusOK, TplActivatePrompt)
+}
+
+func sendActivateEmail(ctx *context.Context, u *user_model.User) {
+ if ctx.Cache.IsExist("MailResendLimit_" + u.LowerName) {
+ renderActivationPromptMessage(ctx, ctx.Locale.Tr("auth.resent_limit_prompt"))
+ return
+ }
+
+ if err := ctx.Cache.Put("MailResendLimit_"+u.LowerName, u.LowerName, 180); err != nil {
+ log.Error("Set cache(MailResendLimit) fail: %v", err)
+ renderActivationPromptMessage(ctx, ctx.Locale.Tr("auth.resent_limit_prompt"))
+ return
+ }
+
+ mailer.SendActivateAccountMail(ctx.Locale, u)
+
+ activeCodeLives := timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale)
+ msgHTML := ctx.Locale.Tr("auth.confirmation_mail_sent_prompt", u.Email, activeCodeLives)
+ renderActivationPromptMessage(ctx, msgHTML)
+}
+
+func renderActivationVerifyPassword(ctx *context.Context, code string) {
+ ctx.Data["ActivationCode"] = code
+ ctx.Data["NeedVerifyLocalPassword"] = true
+ ctx.HTML(http.StatusOK, TplActivate)
}
// Activate render activate user page
func Activate(ctx *context.Context) {
code := ctx.FormString("code")
- if len(code) == 0 {
- ctx.Data["IsActivatePage"] = true
- if ctx.Doer == nil || ctx.Doer.IsActive {
- ctx.NotFound("invalid user", nil)
+ if code == "" {
+ if ctx.Doer == nil {
+ ctx.Redirect(setting.AppSubURL + "/user/login")
+ return
+ } else if ctx.Doer.IsActive {
+ ctx.Redirect(setting.AppSubURL + "/")
return
}
- // Resend confirmation email.
- if setting.Service.RegisterEmailConfirm {
- if ctx.Cache.IsExist("MailResendLimit_" + ctx.Doer.LowerName) {
- ctx.Data["ResendLimited"] = true
- } else {
- ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale)
- mailer.SendActivateAccountMail(ctx.Locale, ctx.Doer)
- if err := ctx.Cache.Put("MailResendLimit_"+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil {
- log.Error("Set cache(MailResendLimit) fail: %v", err)
- }
- }
- } else {
- ctx.Data["ServiceNotEnabled"] = true
+ if setting.MailService == nil || !setting.Service.RegisterEmailConfirm {
+ renderActivationPromptMessage(ctx, ctx.Tr("auth.disable_register_mail"))
+ return
}
- ctx.HTML(http.StatusOK, TplActivate)
+
+ // Resend confirmation email.
+ sendActivateEmail(ctx, ctx.Doer)
return
}
+ // TODO: ctx.Doer/ctx.Data["SignedUser"] could be nil or not the same user as the one being activated
user := user_model.VerifyUserActiveCode(ctx, code)
- // if code is wrong
- if user == nil {
- ctx.Data["IsCodeInvalid"] = true
- ctx.HTML(http.StatusOK, TplActivate)
+ if user == nil { // if code is wrong
+ renderActivationPromptMessage(ctx, ctx.Locale.Tr("auth.invalid_code"))
return
}
// if account is local account, verify password
if user.LoginSource == 0 {
- ctx.Data["Code"] = code
- ctx.Data["NeedsPassword"] = true
- ctx.HTML(http.StatusOK, TplActivate)
+ renderActivationVerifyPassword(ctx, code)
return
}
@@ -688,31 +698,28 @@ func Activate(ctx *context.Context) {
// ActivatePost handles account activation with password check
func ActivatePost(ctx *context.Context) {
code := ctx.FormString("code")
- if len(code) == 0 {
+ if code == "" || (ctx.Doer != nil && ctx.Doer.IsActive) {
ctx.Redirect(setting.AppSubURL + "/user/activate")
return
}
+ // TODO: ctx.Doer/ctx.Data["SignedUser"] could be nil or not the same user as the one being activated
user := user_model.VerifyUserActiveCode(ctx, code)
- // if code is wrong
- if user == nil {
- ctx.Data["IsCodeInvalid"] = true
- ctx.HTML(http.StatusOK, TplActivate)
+ if user == nil { // if code is wrong
+ renderActivationPromptMessage(ctx, ctx.Locale.Tr("auth.invalid_code"))
return
}
// if account is local account, verify password
if user.LoginSource == 0 {
password := ctx.FormString("password")
- if len(password) == 0 {
- ctx.Data["Code"] = code
- ctx.Data["NeedsPassword"] = true
- ctx.HTML(http.StatusOK, TplActivate)
+ if password == "" {
+ renderActivationVerifyPassword(ctx, code)
return
}
if !user.ValidatePassword(password) {
- ctx.Data["IsPasswordInvalid"] = true
- ctx.HTML(http.StatusOK, TplActivate)
+ ctx.Flash.Error(ctx.Locale.Tr("auth.invalid_password"), true)
+ renderActivationVerifyPassword(ctx, code)
return
}
}
diff --git a/templates/user/auth/activate.tmpl b/templates/user/auth/activate.tmpl
index 9cd1712275f8..51dc1eb6a6bf 100644
--- a/templates/user/auth/activate.tmpl
+++ b/templates/user/auth/activate.tmpl
@@ -9,40 +9,22 @@
</h2>
<div class="ui attached segment">
{{template "base/alert" .}}
- {{if .IsActivatePage}}
- {{if .ServiceNotEnabled}}
- <p class="center">{{ctx.Locale.Tr "auth.disable_register_mail"}}</p>
- {{else if .ResendLimited}}
- <p class="center">{{ctx.Locale.Tr "auth.resent_limit_prompt"}}</p>
- {{else}}
- <p>{{ctx.Locale.Tr "auth.confirmation_mail_sent_prompt" .SignedUser.Email .ActiveCodeLives}}</p>
- {{end}}
+ {{if .NeedVerifyLocalPassword}}
+ <div class="required inline field">
+ <label for="verify-password">{{ctx.Locale.Tr "password"}}</label>
+ <input id="verify-password" name="password" type="password" autocomplete="off" required>
+ </div>
+ <div class="inline field">
+ <label></label>
+ <button class="ui primary button">{{ctx.Locale.Tr "install.confirm_password"}}</button>
+ </div>
+ <input name="code" type="hidden" value="{{.ActivationCode}}">
{{else}}
- {{if .NeedsPassword}}
- <div class="required inline field">
- <label for="password">{{ctx.Locale.Tr "password"}}</label>
- <input id="password" name="password" type="password" autocomplete="off" required>
- </div>
- <div class="inline field">
- <label></label>
- <button class="ui primary button">{{ctx.Locale.Tr "install.confirm_password"}}</button>
- </div>
- <input id="code" name="code" type="hidden" value="{{.Code}}">
- {{else if .IsSendRegisterMail}}
- <p>{{ctx.Locale.Tr "auth.confirmation_mail_sent_prompt" .Email .ActiveCodeLives}}</p>
- {{else if .IsCodeInvalid}}
- <p>{{ctx.Locale.Tr "auth.invalid_code"}}</p>
- {{else if .IsPasswordInvalid}}
- <p>{{ctx.Locale.Tr "auth.invalid_password"}}</p>
- {{else if .ManualActivationOnly}}
- <p class="center">{{ctx.Locale.Tr "auth.manual_activation_only"}}</p>
- {{else}}
- <p>{{ctx.Locale.Tr "auth.has_unconfirmed_mail" .SignedUser.Name .SignedUser.Email}}</p>
- <div class="divider"></div>
- <div class="text right">
- <button class="ui primary button">{{ctx.Locale.Tr "auth.resend_mail"}}</button>
- </div>
- {{end}}
+ <p>{{ctx.Locale.Tr "auth.has_unconfirmed_mail" .SignedUser.Name .SignedUser.Email}}</p>
+ <div class="divider"></div>
+ <div class="text right">
+ <button class="ui primary button">{{ctx.Locale.Tr "auth.resend_mail"}}</button>
+ </div>
{{end}}
</div>
</form>
diff --git a/templates/user/auth/activate_prompt.tmpl b/templates/user/auth/activate_prompt.tmpl
new file mode 100644
index 000000000000..237244df8c39
--- /dev/null
+++ b/templates/user/auth/activate_prompt.tmpl
@@ -0,0 +1,15 @@
+{{template "base/head" .}}
+<div role="main" aria-label="{{.Title}}" class="page-content user activate">
+ <div class="ui middle very relaxed page grid">
+ <div class="column">
+ <h2 class="ui top attached header">
+ {{ctx.Locale.Tr "auth.active_your_account"}}
+ </h2>
+ <div class="ui attached segment">
+ {{template "base/alert" .}}
+ <p>{{.ActivationPromptMessage}}</p>
+ </div>
+ </div>
+ </div>
+</div>
+{{template "base/footer" .}}
diff --git a/tests/integration/signup_test.go b/tests/integration/signup_test.go
index 859f873f8518..fbf586f6964f 100644
--- a/tests/integration/signup_test.go
+++ b/tests/integration/signup_test.go
@@ -12,6 +12,7 @@ import (
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/setting"
+ "code.gitea.io/gitea/modules/test"
"code.gitea.io/gitea/modules/translation"
"code.gitea.io/gitea/tests"
@@ -58,7 +59,7 @@ func TestSignupAsRestricted(t *testing.T) {
assert.True(t, user2.IsRestricted)
}
-func TestSignupEmail(t *testing.T) {
+func TestSignupEmailValidation(t *testing.T) {
defer tests.PrepareTestEnv(t)()
setting.Service.EnableCaptcha = false
@@ -91,3 +92,50 @@ func TestSignupEmail(t *testing.T) {
}
}
}
+
+func TestSignupEmailActive(t *testing.T) {
+ defer tests.PrepareTestEnv(t)()
+ defer test.MockVariableValue(&setting.Service.RegisterEmailConfirm, true)()
+
+ // try to sign up and send the activation email
+ req := NewRequestWithValues(t, "POST", "/user/sign_up", map[string]string{
+ "user_name": "test-user-1",
+ "email": "email-1@example.com",
+ "password": "password1",
+ "retype": "password1",
+ })
+ resp := MakeRequest(t, req, http.StatusOK)
+ assert.Contains(t, resp.Body.String(), `A new confirmation email has been sent to <b>email-1@example.com</b>.`)
+
+ // access "user/active" means trying to re-send the activation email
+ session := loginUserWithPassword(t, "test-user-1", "password1")
+ resp = session.MakeRequest(t, NewRequest(t, "GET", "/user/activate"), http.StatusOK)
+ assert.Contains(t, resp.Body.String(), "You have already requested an activation email recently")
+
+ // access "user/active" with a valid activation code, then get the "verify password" page
+ user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "test-user-1"})
+ activationCode := user.GenerateEmailActivateCode(user.Email)
+ resp = session.MakeRequest(t, NewRequest(t, "GET", "/user/activate?code="+activationCode), http.StatusOK)
+ assert.Contains(t, resp.Body.String(), `<input id="verify-password"`)
+
+ // try to use a wrong password, it should fail
+ req = NewRequestWithValues(t, "POST", "/user/activate", map[string]string{
+ "code": activationCode,
+ "password": "password-wrong",
+ })
+ resp = session.MakeRequest(t, req, http.StatusOK)
+ assert.Contains(t, resp.Body.String(), `Your password does not match`)
+ assert.Contains(t, resp.Body.String(), `<input id="verify-password"`)
+ user = unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "test-user-1"})
+ assert.False(t, user.IsActive)
+
+ // then use a correct password, the user should be activated
+ req = NewRequestWithValues(t, "POST", "/user/activate", map[string]string{
+ "code": activationCode,
+ "password": "password1",
+ })
+ resp = session.MakeRequest(t, req, http.StatusSeeOther)
+ assert.Equal(t, "/", test.RedirectURL(resp))
+ user = unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "test-user-1"})
+ assert.True(t, user.IsActive)
+}