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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Apr 14, 2024
Merged

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

merged 2 commits into from
Apr 14, 2024

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Apr 14, 2024 •
edited
Loading

Follow #27626 (comment)

Unify the behaviors of "user create" and "user change-password". The details are commented in code.

And fix a strange output in old code:

   --must-change-password          Set this option to false to prevent forcing the user to change their password after initial login, (Default: true) (default: false)

@wxiaoguang wxiaoguang added the backport/v1.22 This PR should be backported to Gitea 1.22 label Apr 14, 2024
@wxiaoguang wxiaoguang added this to the 1.23.0 milestone Apr 14, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 14, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 14, 2024
@wxiaoguang wxiaoguang requested a review from KN4CK3R April 14, 2024 09:23
@wxiaoguang wxiaoguang mentioned this pull request Apr 14, 2024
Merged
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/docs labels Apr 14, 2024
delvh
delvh reviewed Apr 14, 2024
View reviewed changes
cmd/admin_user_change_password.go
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

docs/content/administration/command-line.en-us.md
@@ -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 •
edited
Loading

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

docs/content/administration/command-line.en-us.md Show resolved Hide resolved
cmd/admin_user_create.go Show resolved Hide resolved
@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 Apr 14, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 14, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) April 14, 2024 16:57
@wxiaoguang wxiaoguang added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 14, 2024
@wxiaoguang wxiaoguang merged commit 4c6e2da into go-gitea:main Apr 14, 2024
26 checks passed
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 14, 2024
@wxiaoguang @KN4CK3R @GiteaBot
Improve "must-change-password" logic and document (go-gitea#30472)
079b16f 
Unify the behaviors of "user create" and "user change-password".

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
@GiteaBot GiteaBot mentioned this pull request Apr 14, 2024
Merged
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Apr 14, 2024
wxiaoguang added a commit that referenced this pull request Apr 14, 2024
@GiteaBot @wxiaoguang @KN4CK3R
Improve "must-change-password" logic and document (#30472) (#30478)
b266c78 
Backport #30472 by wxiaoguang

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 14, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into rmapi
db2e80c 
* origin/main: (35 commits)
  Remove fomantic button module (go-gitea#30475)
  Improve "must-change-password" logic and document (go-gitea#30472)
  Fix commitstatus summary (go-gitea#30431)
  Remove fomantic menu module (go-gitea#30325)
  Use `flex-container` for dashboard layout (go-gitea#30214)
  Rewrite and restyle reaction selector and enable no-sizzle eslint rule (go-gitea#30453)
  Pulse page improvements (go-gitea#30149)
  Fix JS error when opening to expanded code comment (go-gitea#30463)
  fix: Fix to delete cookie when AppSubURL is non-empty (go-gitea#30375)
  Add `interface{}` to `any` replacement to `make fmt`, exclude `*.pb.go` (go-gitea#30461)
  Fix network error when open/close organization/individual projects and redirect to project page (go-gitea#30387)
  Avoid losing token when updating mirror settings (go-gitea#30429)
  Fix label rendering (go-gitea#30456)
  Add comment for ContainsRedirectURI about the exact match (go-gitea#30457)
  Update JS and PY deps, lock eslint and related plugins (go-gitea#30452)
  Refactor cache and disable go-chi cache (go-gitea#30417)
  Fix admin notice view-detail (go-gitea#30450)
  Fix mirror error when mirror repo is empty (go-gitea#30432)
  Add `/public/assets/img/webpack` to ignore files again (go-gitea#30451)
  Lock a few tool dependencies to major versions (go-gitea#30439)
  ...
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 14, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into longbranch
efa7943 
* origin/main:
  Improve flex ellipsis (go-gitea#30479)
  Remove fomantic button module (go-gitea#30475)
  Improve "must-change-password" logic and document (go-gitea#30472)
  Fix commitstatus summary (go-gitea#30431)
  Remove fomantic menu module (go-gitea#30325)
  Use `flex-container` for dashboard layout (go-gitea#30214)
  Rewrite and restyle reaction selector and enable no-sizzle eslint rule (go-gitea#30453)
  Pulse page improvements (go-gitea#30149)
  Fix JS error when opening to expanded code comment (go-gitea#30463)
  fix: Fix to delete cookie when AppSubURL is non-empty (go-gitea#30375)
  Add `interface{}` to `any` replacement to `make fmt`, exclude `*.pb.go` (go-gitea#30461)
  Fix network error when open/close organization/individual projects and redirect to project page (go-gitea#30387)
  Avoid losing token when updating mirror settings (go-gitea#30429)
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 15, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
4e4c21e 
* giteaofficial/main:
  [skip ci] Updated licenses and gitignores
  Revert 100% label max-width (go-gitea#30481)
  Improve flex ellipsis (go-gitea#30479)
  Remove fomantic button module (go-gitea#30475)
  Improve "must-change-password" logic and document (go-gitea#30472)
  Fix commitstatus summary (go-gitea#30431)
  Remove fomantic menu module (go-gitea#30325)
  Use `flex-container` for dashboard layout (go-gitea#30214)
  Rewrite and restyle reaction selector and enable no-sizzle eslint rule (go-gitea#30453)
  Pulse page improvements (go-gitea#30149)
@wxiaoguang wxiaoguang deleted the fix-must-change-password branch April 15, 2024 14:27
@wxiaoguang wxiaoguang added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Apr 23, 2024
@wxiaoguang wxiaoguang mentioned this pull request Apr 25, 2024
Merged
@wxiaoguang
Copy link
Contributor Author

Added some tests: Add some tests to clarify the "must-change-password" behavior #30693

And a question: (ps: does it need to keep the old behavior before #30472?)

@wxiaoguang wxiaoguang removed the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Apr 25, 2024
wxiaoguang added a commit that referenced this pull request Apr 27, 2024
@wxiaoguang
Add some tests to clarify the "must-change-password" behavior (#30693)
d3cdef8 
Follow  #30472:

When a user is created by command line `./gitea admin user create`:

Old behavior before #30472: the first user (admin or non-admin) doesn't
need to change password.

Revert to the old behavior before #30472
@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Apr 27, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh approved these changes

@KN4CK3R KN4CK3R KN4CK3R approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.22 This PR should be backported to Gitea 1.22 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 modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.
4 participants
@wxiaoguang @KN4CK3R @delvh @GiteaBot