Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force user to change password #4489

Merged
merged 39 commits into from
Sep 13, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
0bef325
redirect to login page after successfully activating account
adelowo Jul 15, 2018
68d57a1
Merge remote-tracking branch 'origin' into force_user_password_change…
adelowo Jul 21, 2018
23acb29
force users to change password if account was created by an admin
adelowo Jul 21, 2018
ff42bfd
force users to change password if account was created by an admin
adelowo Jul 21, 2018
26fccdf
fixed build
adelowo Jul 21, 2018
4562460
fixed build
adelowo Jul 21, 2018
5a2ea86
fix pending issues with translation and wrong routes
adelowo Jul 21, 2018
3b87fef
make sure path check is safe
adelowo Jul 21, 2018
7e67ecc
remove unneccessary newline
adelowo Jul 21, 2018
59432fa
make sure users that don't have to view the form get redirected
adelowo Jul 21, 2018
741ef66
move route to use /settings prefix so as to make sure unauthenticated…
adelowo Jul 21, 2018
9b4f70f
update as per @lafriks review
adelowo Jul 21, 2018
845c00b
add necessary comment
adelowo Jul 21, 2018
3b7258e
Merge branch 'master' into force_user_to_change_password
lafriks Jul 22, 2018
b6b39d3
remove unrelated changes
adelowo Jul 22, 2018
4ebdfc1
Merge branch 'force_user_to_change_password' of github.com:adelowo/gi…
adelowo Jul 22, 2018
e0f8fd8
support redirecting to location the user actually want to go to befor…
adelowo Jul 23, 2018
3e5ed18
Merge branch 'master' into force_user_to_change_password
adelowo Jul 27, 2018
1a75475
Fix merge connflicts
adelowo Jul 28, 2018
4402c56
Merge branch 'master' of https://github.com/go-gitea/gitea into force…
adelowo Jul 28, 2018
f7e1e08
run make fmt
adelowo Jul 28, 2018
4d96ba5
added tests
adelowo Jul 30, 2018
95f035d
improve assertions
adelowo Jul 30, 2018
8ea7cdc
add assertion
adelowo Jul 30, 2018
0181ebf
fix copyright year
adelowo Aug 1, 2018
d083410
Merge branch 'master' into force_user_to_change_password
adelowo Aug 1, 2018
d3febca
Merge branch 'master' of https://github.com/go-gitea/gitea into force…
adelowo Aug 6, 2018
dafa9c8
Merge branch 'master' into force_user_to_change_password
adelowo Aug 10, 2018
8c6e9c6
Merge branch 'master' into force_user_to_change_password
adelowo Aug 14, 2018
454cd3a
Merge branch 'master' into force_user_to_change_password
adelowo Aug 16, 2018
d7ee5f8
Merge branch 'master' into force_user_to_change_password
adelowo Aug 21, 2018
5441645
Merge branch 'master' into force_user_to_change_password
adelowo Aug 21, 2018
98933c6
Merge branch 'master' into force_user_to_change_password
adelowo Aug 23, 2018
d2ddc69
Merge branch 'master' into force_user_to_change_password
adelowo Aug 24, 2018
4c6658a
Merge branch 'master' into force_user_to_change_password
adelowo Aug 30, 2018
e2ef29b
Merge branch 'master' into force_user_to_change_password
adelowo Sep 4, 2018
16b5b61
Merge branch 'master' into force_user_to_change_password
techknowlogick Sep 5, 2018
a4152f2
Merge branch 'master' into force_user_to_change_password
adelowo Sep 10, 2018
a03c9a3
Merge branch 'master' into force_user_to_change_password
lafriks Sep 13, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions models/migrations/v71.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2018 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package migrations

import (
"github.com/go-xorm/xorm"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same new line as v70 should also go between these two imports.

)

func addMustChangePassword(x *xorm.Engine) error {
type User struct {
ID int64 `xorm:"pk autoincr"`
MustChangePassword bool `xorm:"NOT NULL DEFAULT false"`
}

return x.Sync2(new(User))
}
30 changes: 18 additions & 12 deletions models/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"errors"
"fmt"
"image"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for newline here

// Needed for jpeg support
_ "image/jpeg"
"image/png"
Expand Down Expand Up @@ -83,18 +84,23 @@ type User struct {
Email string `xorm:"NOT NULL"`
KeepEmailPrivate bool
Passwd string `xorm:"NOT NULL"`
LoginType LoginType
LoginSource int64 `xorm:"NOT NULL DEFAULT 0"`
LoginName string
Type UserType
OwnedOrgs []*User `xorm:"-"`
Orgs []*User `xorm:"-"`
Repos []*Repository `xorm:"-"`
Location string
Website string
Rands string `xorm:"VARCHAR(10)"`
Salt string `xorm:"VARCHAR(10)"`
Language string `xorm:"VARCHAR(5)"`

// MustChangePassword is an attribute that determines if a user
// is to change his/her password after registration.
MustChangePassword bool `xorm:"NOT NULL DEFAULT false"`

LoginType LoginType
LoginSource int64 `xorm:"NOT NULL DEFAULT 0"`
LoginName string
Type UserType
OwnedOrgs []*User `xorm:"-"`
Orgs []*User `xorm:"-"`
Repos []*Repository `xorm:"-"`
Location string
Website string
Rands string `xorm:"VARCHAR(10)"`
Salt string `xorm:"VARCHAR(10)"`
Language string `xorm:"VARCHAR(5)"`

CreatedUnix util.TimeStamp `xorm:"INDEX created"`
UpdatedUnix util.TimeStamp `xorm:"INDEX updated"`
Expand Down
10 changes: 10 additions & 0 deletions modules/auth/user_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,16 @@ func (f *RegisterForm) Validate(ctx *macaron.Context, errs binding.Errors) bindi
return validate(errs, ctx.Data, f, ctx.Locale)
}

type MustChangePasswordForm struct {
Password string `binding:"Required;MaxSize(255)"`
Retype string
}

// Validate valideates the fields
func (f *MustChangePasswordForm) Validate(ctx *macaron.Context, errs binding.Errors) binding.Errors {
return validate(errs, ctx.Data, f, ctx.Locale)
}

// SignInForm form for signing in with user/password
type SignInForm struct {
UserName string `binding:"Required;MaxSize(254)"`
Expand Down
20 changes: 16 additions & 4 deletions modules/context/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,22 @@ func Toggle(options *ToggleOptions) macaron.Handler {
}

// Check prohibit login users.
if ctx.IsSigned && ctx.User.ProhibitLogin {
ctx.Data["Title"] = ctx.Tr("auth.prohibit_login")
ctx.HTML(200, "user/auth/prohibit_login")
return
if ctx.IsSigned {

if ctx.User.ProhibitLogin {
ctx.Data["Title"] = ctx.Tr("auth.prohibit_login")
ctx.HTML(200, "user/auth/prohibit_login")
return
}

if ctx.Req.URL.Path == "/user/change_password" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work if appsuburl is used

Copy link
Member Author

@adelowo adelowo Jul 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lafriks I don't quite get this ... By the way, I fixed the "main" review with 586c080

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adelowo if gitea is hosted as https://example.com/gitea/ as root url than url path would be /gitea/user/change_password

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lafriks Would it just be safe to use strings.HasSuffix ?

Or what is the potential fix here ? other than giving the url path it's own middleware definition so we can safely handle the scenario without the infinite redirection

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use setting.AppSubURL

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strings.HasSuffix(setting.AppSubURL + ctx.Req.URL.Path, "/user/change_password") ???

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just ctx.Req.URL.Path == setting.AppSubURL + "/user/change_password"

return
} else if ctx.User.MustChangePassword {
ctx.Data["Title"] = ctx.Tr("auth.must_change_password")
ctx.Data["ChangePasscodeLink"] = setting.AppSubURL + "/user/change_password"
ctx.Redirect(setting.AppSubURL + "/user/change_password")
return
}
}

// Redirect to dashboard if user tries to visit any non-login page.
Expand Down
11 changes: 6 additions & 5 deletions routers/admin/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,12 @@ func NewUserPost(ctx *context.Context, form auth.AdminCreateUserForm) {
}

u := &models.User{
Name: form.UserName,
Email: form.Email,
Passwd: form.Password,
IsActive: true,
LoginType: models.LoginPlain,
Name: form.UserName,
Email: form.Email,
Passwd: form.Password,
IsActive: true,
LoginType: models.LoginPlain,
MustChangePassword: false,
}

if len(form.LoginType) > 0 {
Expand Down
69 changes: 68 additions & 1 deletion routers/user/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
)

const (
tplMustChangePassword = "user/auth/change_passwd"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comment would be recommended here like it is for other constants below

// tplSignIn template for sign in page
tplSignIn base.TplName = "user/auth/signin"
// tplSignUp template path for sign up page
Expand Down Expand Up @@ -1035,7 +1036,7 @@ func Activate(ctx *context.Context) {

ctx.Session.Set("uid", user.ID)
ctx.Session.Set("uname", user.Name)
ctx.Redirect(setting.AppSubURL + "/")
ctx.Redirect(setting.AppSubURL + "/user/login")
return
}

Expand Down Expand Up @@ -1185,3 +1186,69 @@ func ResetPasswdPost(ctx *context.Context) {
ctx.Data["IsResetFailed"] = true
ctx.HTML(200, tplResetPassword)
}

// MustChangePassword renders the page to change a user's password
func MustChangePassword(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("auth.must_change_password")
ctx.Data["ChangePasscodeLink"] = setting.AppSubURL + "/user/change_password"

ctx.HTML(200, tplMustChangePassword)
}

// MustChangePasswordPost response for updating a user's password after his/her
// account was created by an admin
func MustChangePasswordPost(ctx *context.Context, cpt *captcha.Captcha, form auth.MustChangePasswordForm) {
ctx.Data["Title"] = ctx.Tr("auth.must_change_password")

ctx.Data["ChangePasscodeLink"] = setting.AppSubURL + "/user/sign_up"

if ctx.HasError() {
ctx.HTML(200, tplMustChangePassword)
return
}

u := ctx.User

// Make sure only requests for users who are eligible to change their password via
// this method passes through
if !u.MustChangePassword {
ctx.ServerError("MustUpdatePassword", errors.New("cannot update password.. Please visit the settings page"))
return
}

if form.Password != form.Retype {
ctx.Data["Err_Password"] = true
ctx.RenderWithErr(ctx.Tr("form.password_not_match"), tplMustChangePassword, &form)
return
}

if len(form.Password) < setting.MinPasswordLength {
ctx.Data["Err_Password"] = true
ctx.RenderWithErr(ctx.Tr("auth.password_too_short", setting.MinPasswordLength), tplMustChangePassword, &form)
return
}

var err error
if u.Rands, err = models.GetUserSalt(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be updated when changing password, only if resetting password.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would fix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User should be allowed to access this password change form only when he has must change password set, otherwise if opening this url manually it will be possible to change password without knowing current password

By the way, the above review has been implemented

ctx.ServerError("UpdateUser", err)
return
}

if u.Salt, err = models.GetUserSalt(); err != nil {
ctx.ServerError("UpdateUser", err)
return
}

u.HashPassword(form.Password)
u.MustChangePassword = false

if err := models.UpdateUserCols(u, "must_change_password", "passwd", "rands", "salt"); err != nil {
ctx.ServerError("UpdateUser", err)
return
}

ctx.Flash.Success(ctx.Tr("settings.change_password_success"))

log.Trace("User updated password: %s", u.Name)
ctx.Redirect(setting.AppSubURL + "/")
}
7 changes: 7 additions & 0 deletions templates/user/auth/change_passwd.tmpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{{template "base/head" .}}
<div class="user signin{{if .LinkAccountMode}} icon{{end}}">
<div class="ui container">
{{template "user/auth/change_passwd_inner" .}}
</div>
</div>
{{template "base/footer" .}}
26 changes: 26 additions & 0 deletions templates/user/auth/change_passwd_inner.tmpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{{if or (not .LinkAccountMode) (and .LinkAccountMode .LinkAccountModeSignIn)}}
{{template "base/alert" .}}
{{end}}
<h4 class="ui top attached header center">
{{.i18n.Tr "settings.change_password"}}
</h4>
<div class="ui attached segment">
<form class="ui form" action="{{.ChangePasscodeLink}}" method="post">
{{.CsrfTokenHtml}}
<div class="required inline field {{if and (.Err_Password) (or (not .LinkAccountMode) (and .LinkAccountMode .LinkAccountModeSignIn))}}error{{end}}">
<label for="password">{{.i18n.Tr "password"}}</label>
<input id="password" name="password" type="password" value="{{.password}}" autocomplete="off" required>
</div>


<div class="required inline field {{if and (.Err_Password) (or (not .LinkAccountMode) (and .LinkAccountMode .LinkAccountModeRegister))}}error{{end}}">
<label for="retype">{{.i18n.Tr "re_type"}}</label>
<input id="retype" name="retype" type="password" autocomplete="off" required>
</div>

<div class="inline field">
<label></label>
<button class="ui green button">{{.i18n.Tr "settings.change_password" }}</button>
</div>
</form>
</div>