From c26e0dd75698315cc293a10e61201704da332c80 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 18 Jan 2020 16:47:12 +0000 Subject: [PATCH 1/3] Ensure that 2fa is checked on reset-password --- routers/user/auth.go | 56 ++++++++++++++++++++++++--- templates/user/auth/reset_passwd.tmpl | 23 ++++++++++- 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/routers/user/auth.go b/routers/user/auth.go index 258b5e03d79a..d09cf59497f3 100644 --- a/routers/user/auth.go +++ b/routers/user/auth.go @@ -1284,7 +1284,7 @@ func ForgotPasswdPost(ctx *context.Context) { ctx.HTML(200, tplForgotPassword) } -func commonResetPassword(ctx *context.Context) *models.User { +func commonResetPassword(ctx *context.Context) (*models.User, *models.TwoFactor) { code := ctx.Query("code") ctx.Data["Title"] = ctx.Tr("auth.reset_password") @@ -1296,14 +1296,25 @@ func commonResetPassword(ctx *context.Context) *models.User { if len(code) == 0 { ctx.Flash.Error(ctx.Tr("auth.invalid_code")) - return nil + return nil, nil } // Fail early, don't frustrate the user u := models.VerifyUserActiveCode(code) if u == nil { ctx.Flash.Error(ctx.Tr("auth.invalid_code")) - return nil + return nil, nil + } + + twofa, err := models.GetTwoFactorByUID(u.ID) + if err != nil { + if !models.IsErrTwoFactorNotEnrolled(err) { + ctx.Error(http.StatusInternalServerError, "CommonResetPassword", err.Error()) + return nil, nil + } + } else { + ctx.Data["has_two_factor"] = true + ctx.Data["scratch_code"] = ctx.QueryBool("scratch_code") } // Show the user that they are affecting the account that they intended to @@ -1311,10 +1322,10 @@ func commonResetPassword(ctx *context.Context) *models.User { if nil != ctx.User && u.ID != ctx.User.ID { ctx.Flash.Error(ctx.Tr("auth.reset_password_wrong_user", ctx.User.Email, u.Email)) - return nil + return nil, nil } - return u + return u, twofa } // ResetPasswd render the account recovery page @@ -1322,13 +1333,19 @@ func ResetPasswd(ctx *context.Context) { ctx.Data["IsResetForm"] = true commonResetPassword(ctx) + if ctx.Written() { + return + } ctx.HTML(200, tplResetPassword) } // ResetPasswdPost response from account recovery request func ResetPasswdPost(ctx *context.Context) { - u := commonResetPassword(ctx) + u, twofa := commonResetPassword(ctx) + if ctx.Written() { + return + } if u == nil { // Flash error has been set @@ -1336,6 +1353,33 @@ func ResetPasswdPost(ctx *context.Context) { return } + if twofa != nil { + useScratch := ctx.QueryBool("scratch_code") + if useScratch { + token := ctx.Query("token") + ok := twofa.VerifyScratchToken(token) + if !ok { + ctx.Data["IsResetForm"] = true + ctx.Data["Err_Token"] = true + ctx.RenderWithErr(ctx.Tr("auth.twofa_scratch_token_incorrect"), tplResetPassword, nil) + return + } + } else { + code := ctx.Query("passcode") + ok, err := twofa.ValidateTOTP(code) + if err != nil { + ctx.Error(http.StatusInternalServerError, "ValidateTOTP", err.Error()) + return + } + if !ok { + ctx.Data["IsResetForm"] = true + ctx.Data["Err_Passcode"] = true + ctx.RenderWithErr(ctx.Tr("auth.twofa_passcode_incorrect"), tplResetPassword, nil) + return + } + } + } + // Validate password length. passwd := ctx.Query("password") if len(passwd) < setting.MinPasswordLength { diff --git a/templates/user/auth/reset_passwd.tmpl b/templates/user/auth/reset_passwd.tmpl index e7d939294e3f..e5d38bc32e15 100644 --- a/templates/user/auth/reset_passwd.tmpl +++ b/templates/user/auth/reset_passwd.tmpl @@ -18,7 +18,7 @@ {{end}} {{if .IsResetForm}}
- +
{{if not .user_signed_in}} @@ -30,10 +30,31 @@ {{end}} + {{if .has_two_factor}} +

+ {{.i18n.Tr "twofa"}} +

+
{{.i18n.Tr "settings.twofa_is_enrolled" | Str2html }}
+ {{if .scratch_code}} +
+ + +
+ + {{else}} +
+ + +
+ {{end}} + {{end}}
+ {{if and .has_two_factor (not .scratch_code)}} + {{.i18n.Tr "auth.use_scratch_code" | Str2html}} + {{end}}
{{else}}

{{.i18n.Tr "auth.invalid_code"}}

From 29512b3131ab54aaee364d8242c7a2c282e64c32 Mon Sep 17 00:00:00 2001 From: zeripath Date: Sun, 19 Jan 2020 08:44:40 +0000 Subject: [PATCH 2/3] Apply suggestions from code review Co-Authored-By: Lauris BH --- routers/user/auth.go | 9 +++------ templates/user/auth/reset_passwd.tmpl | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/routers/user/auth.go b/routers/user/auth.go index d09cf59497f3..52e8cf49bc1b 100644 --- a/routers/user/auth.go +++ b/routers/user/auth.go @@ -1354,19 +1354,16 @@ func ResetPasswdPost(ctx *context.Context) { } if twofa != nil { - useScratch := ctx.QueryBool("scratch_code") - if useScratch { - token := ctx.Query("token") + if ctx.QueryBool("scratch_code") { ok := twofa.VerifyScratchToken(token) - if !ok { + if !twofa.VerifyScratchToken(ctx.Query("token")) { ctx.Data["IsResetForm"] = true ctx.Data["Err_Token"] = true ctx.RenderWithErr(ctx.Tr("auth.twofa_scratch_token_incorrect"), tplResetPassword, nil) return } } else { - code := ctx.Query("passcode") - ok, err := twofa.ValidateTOTP(code) + ok, err := twofa.ValidateTOTP(ctx.Query("passcode")) if err != nil { ctx.Error(http.StatusInternalServerError, "ValidateTOTP", err.Error()) return diff --git a/templates/user/auth/reset_passwd.tmpl b/templates/user/auth/reset_passwd.tmpl index e5d38bc32e15..91d5a5ef88dc 100644 --- a/templates/user/auth/reset_passwd.tmpl +++ b/templates/user/auth/reset_passwd.tmpl @@ -34,7 +34,7 @@

{{.i18n.Tr "twofa"}}

-
{{.i18n.Tr "settings.twofa_is_enrolled" | Str2html }}
+
{{.i18n.Tr "settings.twofa_is_enrolled" | Str2html }}
{{if .scratch_code}}
From dfcdf05c6ee5c12c1cdee666b398bf6c87a44896 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 19 Jan 2020 11:41:29 +0000 Subject: [PATCH 3/3] Properly manage scratch_code regeneration --- routers/user/auth.go | 64 +++++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 19 deletions(-) diff --git a/routers/user/auth.go b/routers/user/auth.go index 52e8cf49bc1b..6395836480ab 100644 --- a/routers/user/auth.go +++ b/routers/user/auth.go @@ -1353,42 +1353,51 @@ func ResetPasswdPost(ctx *context.Context) { return } + // Validate password length. + passwd := ctx.Query("password") + if len(passwd) < setting.MinPasswordLength { + ctx.Data["IsResetForm"] = true + ctx.Data["Err_Password"] = true + ctx.RenderWithErr(ctx.Tr("auth.password_too_short", setting.MinPasswordLength), tplResetPassword, nil) + return + } else if !password.IsComplexEnough(passwd) { + ctx.Data["IsResetForm"] = true + ctx.Data["Err_Password"] = true + ctx.RenderWithErr(password.BuildComplexityError(ctx), tplResetPassword, nil) + return + } + + // Handle two-factor + regenerateScratchToken := false if twofa != nil { if ctx.QueryBool("scratch_code") { - ok := twofa.VerifyScratchToken(token) if !twofa.VerifyScratchToken(ctx.Query("token")) { ctx.Data["IsResetForm"] = true ctx.Data["Err_Token"] = true ctx.RenderWithErr(ctx.Tr("auth.twofa_scratch_token_incorrect"), tplResetPassword, nil) return } + regenerateScratchToken = true } else { - ok, err := twofa.ValidateTOTP(ctx.Query("passcode")) + passcode := ctx.Query("passcode") + ok, err := twofa.ValidateTOTP(passcode) if err != nil { ctx.Error(http.StatusInternalServerError, "ValidateTOTP", err.Error()) return } - if !ok { + if !ok || twofa.LastUsedPasscode == passcode { ctx.Data["IsResetForm"] = true ctx.Data["Err_Passcode"] = true ctx.RenderWithErr(ctx.Tr("auth.twofa_passcode_incorrect"), tplResetPassword, nil) return } - } - } - // Validate password length. - passwd := ctx.Query("password") - if len(passwd) < setting.MinPasswordLength { - ctx.Data["IsResetForm"] = true - ctx.Data["Err_Password"] = true - ctx.RenderWithErr(ctx.Tr("auth.password_too_short", setting.MinPasswordLength), tplResetPassword, nil) - return - } else if !password.IsComplexEnough(passwd) { - ctx.Data["IsResetForm"] = true - ctx.Data["Err_Password"] = true - ctx.RenderWithErr(password.BuildComplexityError(ctx), tplResetPassword, nil) - return + twofa.LastUsedPasscode = passcode + if err = models.UpdateTwoFactor(twofa); err != nil { + ctx.ServerError("ResetPasswdPost: UpdateTwoFactor", err) + return + } + } } var err error @@ -1400,7 +1409,6 @@ func ResetPasswdPost(ctx *context.Context) { ctx.ServerError("UpdateUser", err) return } - u.HashPassword(passwd) u.MustChangePassword = false if err := models.UpdateUserCols(u, "must_change_password", "passwd", "rands", "salt"); err != nil { @@ -1409,9 +1417,27 @@ func ResetPasswdPost(ctx *context.Context) { } log.Trace("User password reset: %s", u.Name) - ctx.Data["IsResetFailed"] = true remember := len(ctx.Query("remember")) != 0 + + if regenerateScratchToken { + // Invalidate the scratch token. + _, err = twofa.GenerateScratchToken() + if err != nil { + ctx.ServerError("UserSignIn", err) + return + } + if err = models.UpdateTwoFactor(twofa); err != nil { + ctx.ServerError("UserSignIn", err) + return + } + + handleSignInFull(ctx, u, remember, false) + ctx.Flash.Info(ctx.Tr("auth.twofa_scratch_used")) + ctx.Redirect(setting.AppSubURL + "/user/settings/security") + return + } + handleSignInFull(ctx, u, remember, true) }