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

Improve "must-change-password" logic and document #30472

Merged
merged 2 commits into from
Apr 14, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
14 changes: 5 additions & 9 deletions cmd/admin_user_change_password.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ var microcmdUserChangePassword = &cli.Command{
&cli.BoolFlag{
Name: "must-change-password",
Usage: "User must change password",
Value: true,
},
},
}
Expand All @@ -57,23 +58,18 @@ func runChangePassword(c *cli.Context) error {
return err
}

var mustChangePassword optional.Option[bool]
if c.IsSet("must-change-password") {
mustChangePassword = optional.Some(c.Bool("must-change-password"))
}

opts := &user_service.UpdateAuthOptions{
Password: optional.Some(c.String("password")),
MustChangePassword: mustChangePassword,
MustChangePassword: optional.Some(c.Bool("must-change-password")),
}
if err := user_service.UpdateAuth(ctx, user, opts); err != nil {
switch {
case errors.Is(err, password.ErrMinLength):
return fmt.Errorf("Password is not long enough. Needs to be at least %d", setting.MinPasswordLength)
return fmt.Errorf("password is not long enough, needs to be at least %d characters", setting.MinPasswordLength)
case errors.Is(err, password.ErrComplexity):
return errors.New("Password does not meet complexity requirements")
return errors.New("password does not meet complexity requirements")
case errors.Is(err, password.ErrIsPwned):
return errors.New("The password you chose is on a list of stolen passwords previously exposed in public data breaches. Please try again with a different password.\nFor more details, see https://haveibeenpwned.com/Passwords")
return errors.New("the password is in a list of stolen passwords previously exposed in public data breaches, please try again with a different password, to see more details: https://haveibeenpwned.com/Passwords")
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these errors shown to the user?
So, shouldn't they be correct English instead of Go error messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message is outputed here, so it should be a valid Go error message. And it is correct English.

image

default:
return err
}
Expand Down
38 changes: 22 additions & 16 deletions cmd/admin_user_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"

auth_model "code.gitea.io/gitea/models/auth"
"code.gitea.io/gitea/models/db"
user_model "code.gitea.io/gitea/models/user"
pwd "code.gitea.io/gitea/modules/auth/password"
"code.gitea.io/gitea/modules/optional"
Expand Down Expand Up @@ -46,8 +47,9 @@ var microcmdUserCreate = &cli.Command{
Usage: "Generate a random password for the user",
},
&cli.BoolFlag{
Name: "must-change-password",
Usage: "Set this option to false to prevent forcing the user to change their password after initial login, (Default: true)",
Name: "must-change-password",
Usage: "Set to false to prevent forcing the user to change their password after initial login",
DisableDefaultText: true,
},
&cli.IntFlag{
Name: "random-password-length",
Expand All @@ -71,10 +73,10 @@ func runCreateUser(c *cli.Context) error {
}

if c.IsSet("name") && c.IsSet("username") {
return errors.New("Cannot set both --name and --username flags")
return errors.New("cannot set both --name and --username flags")
}
if !c.IsSet("name") && !c.IsSet("username") {
return errors.New("One of --name or --username flags must be set")
return errors.New("one of --name or --username flags must be set")
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
}

if c.IsSet("password") && c.IsSet("random-password") {
Expand Down Expand Up @@ -110,17 +112,21 @@ func runCreateUser(c *cli.Context) error {
return errors.New("must set either password or random-password flag")
}

// always default to true
changePassword := true

// If this is the first user being created.
// Take it as the admin and don't force a password update.
if n := user_model.CountUsers(ctx, nil); n == 0 {
changePassword = false
}

isAdmin := c.Bool("admin")
mustChangePassword := true // always default to true
if c.IsSet("must-change-password") {
changePassword = c.Bool("must-change-password")
// if the flag is set, use the value provided by the user
mustChangePassword = c.Bool("must-change-password")
} else {
// check whether there are users in the database
hasUserRecord, err := db.IsTableNotEmpty(&user_model.User{})
if err != nil {
return fmt.Errorf("IsTableNotEmpty: %w", err)
}
if !hasUserRecord && isAdmin {
// if this is the first admin being created, don't force to change password (keep the old behavior)
mustChangePassword = false
}
}

restricted := optional.None[bool]()
Expand All @@ -136,8 +142,8 @@ func runCreateUser(c *cli.Context) error {
Name: username,
Email: c.String("email"),
Passwd: password,
IsAdmin: c.Bool("admin"),
MustChangePassword: changePassword,
IsAdmin: isAdmin,
MustChangePassword: mustChangePassword,
Visibility: visibility,
}

Expand Down
5 changes: 2 additions & 3 deletions docs/content/administration/command-line.en-us.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ Admin operations:
- `--email value`: Email. Required.
- `--admin`: If provided, this makes the user an admin. Optional.
- `--access-token`: If provided, an access token will be created for the user. Optional. (default: false).
- `--must-change-password`: If provided, the created user will be required to choose a newer password after the
initial login. Optional. (default: true).
- `--must-change-password`: The created user will be required to set a new password after the initial login, default: true. It could be disabled by `--must-change-password=false`.
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
- `--random-password`: If provided, a randomly generated password will be used as the password of the created
user. The value of `--password` will be discarded. Optional.
- `--random-password-length`: If provided, it will be used to configure the length of the randomly generated
Expand All @@ -95,7 +94,7 @@ Admin operations:
- Options:
- `--username value`, `-u value`: Username. Required.
- `--password value`, `-p value`: New password. Required.
- `--must-change-password`: If provided, the user is required to choose a new password after the login. Optional.
- `--must-change-password`: The user is required to set a new password after the login, default: true. It could be disabled by `--must-change-password=false`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `--must-change-password`: The user is required to set a new password after the login, default: true. It could be disabled by `--must-change-password=false`.
- `--must-change-password`: The user must set a new password after login, default: true. Can be disabled using `--must-change-password=false`.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Apr 14, 2024

Choose a reason for hiding this comment

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

Can be disabled using ... doesn't seem correct English 🤣

I think it should be "be disabled by something"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GPT also agrees with me:

image

- Examples:
- `gitea admin user change-password --username myname --password asecurepassword`
- `must-change-password`:
Expand Down
4 changes: 2 additions & 2 deletions models/db/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,8 @@ func MaxBatchInsertSize(bean any) int {
}

// IsTableNotEmpty returns true if table has at least one record
func IsTableNotEmpty(tableName string) (bool, error) {
return x.Table(tableName).Exist()
func IsTableNotEmpty(beanOrTableName any) (bool, error) {
return x.Table(beanOrTableName).Exist()
}

// DeleteAllRecords will delete all the records of this table
Expand Down