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

@@ -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"          // for sign in page
-	tplSignIn base.TplName = "user/auth/signin"
+	tplSignUp         base.TplName = "user/auth/signup"          // for sign up page
-	// tplSignUp template path for sign up page
+	TplActivate       base.TplName = "user/auth/activate"        // for activate user
-	tplSignUp base.TplName = "user/auth/signup"
+	TplActivatePrompt base.TplName = "user/auth/activate_prompt" // for showing a message for user activation
-	// TplActivate template path for activate user
-	TplActivate base.TplName = "user/auth/activate"
 )
 
 // 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
+	// for active user or the first (admin) user, we don't need to send confirmation email
-	if !u.IsActive && u.ID > 1 {
+	if u.IsActive || u.ID == 1 {
+		return true
+	}
+
 	if setting.Service.RegisterManualConfirm {
-			ctx.Data["ManualActivationOnly"] = true
+		renderActivationPromptMessage(ctx, ctx.Locale.Tr("auth.manual_activation_only"))
-			ctx.HTML(http.StatusOK, TplActivate)
 		return false
 	}
 
+	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)
 
-		ctx.Data["IsSendRegisterMail"] = true
+	activeCodeLives := timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale)
-		ctx.Data["Email"] = u.Email
+	msgHTML := ctx.Locale.Tr("auth.confirmation_mail_sent_prompt", u.Email, activeCodeLives)
-		ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale)
+	renderActivationPromptMessage(ctx, msgHTML)
+}
+
+func renderActivationVerifyPassword(ctx *context.Context, code string) {
+	ctx.Data["ActivationCode"] = code
+	ctx.Data["NeedVerifyLocalPassword"] = true
 	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)
-		}
-		return false
-	}
-
-	return true
 }
 
 // Activate render activate user page
 func Activate(ctx *context.Context) {
 	code := ctx.FormString("code")
 
-	if len(code) == 0 {
+	if code == "" {
-		ctx.Data["IsActivatePage"] = true
+		if ctx.Doer == nil {
-		if ctx.Doer == nil || ctx.Doer.IsActive {
+			ctx.Redirect(setting.AppSubURL + "/user/login")
-			ctx.NotFound("invalid user", nil)
+			return
+		} else if ctx.Doer.IsActive {
+			ctx.Redirect(setting.AppSubURL + "/")
 			return
 		}
+
+		if setting.MailService == nil || !setting.Service.RegisterEmailConfirm {
+			renderActivationPromptMessage(ctx, ctx.Tr("auth.disable_register_mail"))
+			return
+		}
+
 		// Resend confirmation email.
-		if setting.Service.RegisterEmailConfirm {
+		sendActivateEmail(ctx, ctx.Doer)
-			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
-		}
-		ctx.HTML(http.StatusOK, TplActivate)
 		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 { // if code is wrong
-	if user == nil {
+		renderActivationPromptMessage(ctx, ctx.Locale.Tr("auth.invalid_code"))
-		ctx.Data["IsCodeInvalid"] = true
-		ctx.HTML(http.StatusOK, TplActivate)
 		return
 	}
 
 	// if account is local account, verify password
 	if user.LoginSource == 0 {
-		ctx.Data["Code"] = code
+		renderActivationVerifyPassword(ctx, code)
-		ctx.Data["NeedsPassword"] = true
-		ctx.HTML(http.StatusOK, TplActivate)
 		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 { // if code is wrong
-	if user == nil {
+		renderActivationPromptMessage(ctx, ctx.Locale.Tr("auth.invalid_code"))
-		ctx.Data["IsCodeInvalid"] = true
-		ctx.HTML(http.StatusOK, TplActivate)
 		return
 	}
 
 	// if account is local account, verify password
 	if user.LoginSource == 0 {
 		password := ctx.FormString("password")
-		if len(password) == 0 {
+		if password == "" {
-			ctx.Data["Code"] = code
+			renderActivationVerifyPassword(ctx, code)
-			ctx.Data["NeedsPassword"] = true
-			ctx.HTML(http.StatusOK, TplActivate)
 			return
 		}
 		if !user.ValidatePassword(password) {
-			ctx.Data["IsPasswordInvalid"] = true
+			ctx.Flash.Error(ctx.Locale.Tr("auth.invalid_password"), true)
-			ctx.HTML(http.StatusOK, TplActivate)
+			renderActivationVerifyPassword(ctx, code)
 			return
 		}
 	}

templates/user/auth/activate.tmpl

@@ -9,33 +9,16 @@
 				</h2>
 				<div class="ui attached segment">
 					{{template "base/alert" .}}
-					{{if .IsActivatePage}}
+					{{if .NeedVerifyLocalPassword}}
-						{{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}}
-					{{else}}
-						{{if .NeedsPassword}}
 						<div class="required inline field">
-								<label for="password">{{ctx.Locale.Tr "password"}}</label>
+							<label for="verify-password">{{ctx.Locale.Tr "password"}}</label>
-								<input id="password" name="password" type="password" autocomplete="off" required>
+							<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 id="code" name="code" type="hidden" value="{{.Code}}">
+						<input name="code" type="hidden" value="{{.ActivationCode}}">
-						{{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>
@@ -43,7 +26,6 @@
 							<button class="ui primary button">{{ctx.Locale.Tr "auth.resend_mail"}}</button>
 						</div>
 					{{end}}
-					{{end}}
 				</div>
 			</form>
 		</div>

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" .}}

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)
+}