diff options
author | wxiaoguang <wxiaoguang@gmail.com> | 2024-02-26 05:55:00 +0800 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-02-25 21:55:00 +0000 |
commit | 49e482674700e184aa84806acfb7edaae0554291 (patch) | |
tree | 26b14bcdb2d348fc2f7e34a884205af1fa491d9a | |
parent | ed3892d8430652c2bc8e2af21844d14769825e8a (diff) | |
download | gitea-49e482674700e184aa84806acfb7edaae0554291.tar.gz gitea-49e482674700e184aa84806acfb7edaae0554291.zip |
Refactor "user/active" related logic (#29390)
And add more tests. Remove a lot of fragile "if" blocks.
The old logic is kept as-is.
-rw-r--r-- | routers/web/auth/auth.go | 125 | ||||
-rw-r--r-- | templates/user/auth/activate.tmpl | 48 | ||||
-rw-r--r-- | templates/user/auth/activate_prompt.tmpl | 15 | ||||
-rw-r--r-- | tests/integration/signup_test.go | 50 |
4 files changed, 145 insertions, 93 deletions
diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 3de1f3373d..a30ee0ce54 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) + if setting.Service.RegisterManualConfirm { + renderActivationPromptMessage(ctx, ctx.Locale.Tr("auth.manual_activation_only")) + return false + } - ctx.Data["IsSendRegisterMail"] = true - ctx.Data["Email"] = u.Email - ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale) - ctx.HTML(http.StatusOK, TplActivate) + sendActivateEmail(ctx, u) + return false +} - if err := ctx.Cache.Put("MailResendLimit_"+u.LowerName, u.LowerName, 180); err != nil { - log.Error("Set cache(MailResendLimit) fail: %v", err) - } - 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 } - return true + 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 9cd1712275..51dc1eb6a6 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 0000000000..237244df8c --- /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 859f873f85..fbf586f696 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) +} |