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

Add must-change-password cli parameter #27626

Merged
merged 8 commits into from
Feb 3, 2024

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Oct 14, 2023

This PR adds a new must-change-password parameter to the change-password cli command.
We already have the must-change-password command but it feels natural to have this integrated into the change-password cli command.

@KN4CK3R KN4CK3R added the type/enhancement An improvement of existing functionality label Oct 14, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 14, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 14, 2023
@github-actions github-actions bot added modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/docs labels Oct 14, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 14, 2023
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

we use it on the other user option that way ... reverting this in that way will just make it more difficult as we have now two similar options but they work the other way

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 20, 2023
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 20, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Oct 20, 2023
@6543
Copy link
Member

6543 commented Oct 20, 2023

@KN4CK3R I did make it the same as the other option ... if you disagree please force push that change away :)

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Oct 20, 2023

@6543 So, what's the command line for changing a users password without requesting a change from them?

@6543
Copy link
Member

6543 commented Oct 20, 2023

gitea admin user change-password -u a_user -p a_pwd --must-change-password false

@6543 6543 added this to the 1.22.0 milestone Oct 20, 2023
@KN4CK3R
Copy link
Member Author

KN4CK3R commented Oct 20, 2023

Yep, and that's wrong:

Command Result
gitea admin user change-password --username x --password y MustChangePassword = true
gitea admin user change-password --username x --password y --must-change-password MustChangePassword = true
gitea admin user change-password --username x --password y --must-change-password false MustChangePassword = true this is unexpected
gitea admin user change-password --username x --password y --must-change-password=false MustChangePassword = false ok, but nobody knows that they need to use =

inverted command:

Command Result
gitea admin user change-password --username x --password y MustChangePassword = true
gitea admin user change-password --username x --password y --must-not-change-password MustChangePassword = false

@6543
Copy link
Member

6543 commented Oct 20, 2023

ah now I get your concerns ... let me fix that issue ...

@6543
Copy link
Member

6543 commented Oct 20, 2023

diff --git a/cmd/admin_user_change_password.go b/cmd/admin_user_change_password.go
index f1b08c197e..1a7d96a920 100644
--- a/cmd/admin_user_change_password.go
+++ b/cmd/admin_user_change_password.go
@@ -32,11 +32,7 @@ var microcmdUserChangePassword = &cli.Command{
                        Value:   "",
                        Usage:   "New password to set for user",
                },
-               &cli.BoolFlag{
-                       Name:  "must-change-password",
-                       Value: true,
-                       Usage: "User must change password",
-               },
+               mustChangePasswordFlag,
        },
 }
 
@@ -74,7 +70,7 @@ func runChangePassword(c *cli.Context) error {
                return err
        }
 
-       user.MustChangePassword = c.Bool("must-change-password")
+       user.MustChangePassword = mustChangePassword(c)
 
        if err = user_model.UpdateUserCols(ctx, user, "must_change_password", "passwd", "passwd_hash_algo", "salt"); err != nil {
                return err
diff --git a/cmd/admin_user_create.go b/cmd/admin_user_create.go
index fefe18d39c..2f4a1d0843 100644
--- a/cmd/admin_user_create.go
+++ b/cmd/admin_user_create.go
@@ -45,10 +45,7 @@ var microcmdUserCreate = &cli.Command{
                        Name:  "random-password",
                        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)",
-               },
+               mustChangePasswordFlag,
                &cli.IntFlag{
                        Name:  "random-password-length",
                        Usage: "Length of the random password to be generated",
@@ -110,19 +107,14 @@ func runCreateUser(c *cli.Context) error {
                return errors.New("must set either password or random-password flag")
        }
 
-       // always default to true
-       changePassword := true
+       changePassword := mustChangePassword(c)
 
        // 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 {
+       if !c.IsSet("must-change-password") && user_model.CountUsers(ctx, nil) == 0 {
                changePassword = false
        }
 
-       if c.IsSet("must-change-password") {
-               changePassword = c.Bool("must-change-password")
-       }
-
        restricted := util.OptionalBoolNone
 
        if c.IsSet("restricted") {

@KN4CK3R

@6543
Copy link
Member

6543 commented Oct 20, 2023

cat cmd/admin_user_flags.go

// Copyright 2023 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package cmd

import (
	"strconv"

	"github.com/urfave/cli/v2"
)

var mustChangePasswordFlag = &cli.StringFlag{
	Name:  "must-change-password",
	Value: "true",
	Usage: "Set this option to false to prevent forcing the user to change their password after initial login, (Default: true)",
}

// return bool of must-change-password flag
func mustChangePassword(c *cli.Context) bool {
	v, _ := strconv.ParseBool(c.String("must-change-password"))
	return v
}

@6543
Copy link
Member

6543 commented Oct 20, 2023

what do you think?

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Nov 12, 2023

Personally I would not want this workaround because it's different to every other command.

@@ -95,6 +95,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. (default: true).
Copy link
Contributor

@wxiaoguang wxiaoguang Nov 17, 2023

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`: If provided, the user is required to choose a new password after the login. Optional. (default: true).
- `--must-change-password`, `--must-change-password=true/false`: If provided, the user is required to choose a new password after the login. Optional.

The (default: true) and optional might cause misunderstanding IMO.

I think everyone could understand that --must-change-password means --must-change-password=true


Or just follow the wording above, like - `--admin`: If provided, this makes the user an admin. Optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any other suggestion? If no, could I commit this suggestion and approve?

Copy link
Member

Choose a reason for hiding this comment

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

The suggestion sounds good

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the wording. Now the default is false if the flag is not set. I still think this should default to true.

Copy link
Contributor

@wxiaoguang wxiaoguang Feb 3, 2024

Choose a reason for hiding this comment

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

Sure, but I think if it defaults to true, then it might needs some new wording. (the old one makes me think for quite a long time to understand what is "optional" and what is "default: true" ....)

Maybe like this?


- `--must-change-password`: Default to true, the user is required to choose a new password after login. Use `--must-change-password=false` to disable this option.

Anyway, no better idea from my side. Either is fine to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I just used the wording from here:

- `--must-change-password`: If provided, the created user will be required to choose a newer password after the
initial login. Optional. (default: true).

Maybe we should have a common format for optional and required parameters.

@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 3, 2024
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 3, 2024
@lafriks lafriks merged commit 9bea276 into go-gitea:main Feb 3, 2024
25 checks passed
@KN4CK3R KN4CK3R deleted the enhancement-cli-password branch February 3, 2024 21:21
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 4, 2024
* giteaofficial/main:
  Add `must-change-password` cli parameter (go-gitea#27626)
  Include username in email headers (go-gitea#28981)
  Update tool dependencies (go-gitea#29030)
  Add artifacts v4 jwt to job message and accept it (go-gitea#28885)
  Pass es2020 to esbuild-loader as well (go-gitea#29027)
  Fix default avatar image size in PR diff page (go-gitea#28971)
  Update JS and PY dependencies, build for `es2020` browsers (go-gitea#28977)
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
This PR adds a new `must-change-password` parameter to the
`change-password` cli command.
We already have the `must-change-password` command but it feels natural
to have this integrated into the `change-password` cli command.

---------

Co-authored-by: 6543 <6543@obermui.de>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
@wxiaoguang
Copy link
Contributor

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

@techknowlogick techknowlogick added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Aug 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/docs pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants