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

Admin should not delete himself #19423

Merged
merged 3 commits into from
May 8, 2022
Merged

Admin should not delete himself #19423

merged 3 commits into from
May 8, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Apr 19, 2022 •
edited
Loading

Also partially fix #15449

@wxiaoguang
Copy link
Contributor

However, it doesn't resolve the issue fundamentally.

IIRC the issue author meant that he did the dumb operation by setting himself as non-admin.

And beside such operation, there could be more situations that the only admin can do dumb operations to get stuck:

  1. The only admin set himself as inactive
  2. The only admin set himself as restricted
  3. The only admin set himself as prohibited
  4. The only admin changed the Authentication Source to an incorrect one
  5. The only admin changed the password and then forgot the password
  6. The only admin set OTP and then forgot the scratch code and lose the OTP token.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 19, 2022
@delvh
Copy link
Member

delvh commented Apr 19, 2022 •
edited
Loading

I agree with 1...3,
but should we really handle 4...6?
Because it is a stupid idea to disallow admins to change their authentication.
I think the only logical thing, in this case, is to either add special gitea admin commands so that they can restore their account from the CLI, or to let them modify the database directly.

delvh
delvh approved these changes Apr 19, 2022
View reviewed changes
routers/web/admin/users.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 19, 2022
@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 20, 2022
relnod
relnod reviewed Apr 20, 2022
View reviewed changes
routers/web/admin/users.go Outdated Show resolved Hide resolved
routers/api/v1/admin/user.go Outdated Show resolved Hide resolved
@6543
Copy link
Member

6543 commented Apr 20, 2022

I think we should add a check if CurrentUser is the only admin left ...

@zeripath
Copy link
Contributor

I think this is more enhancement than bug so I'm not sure that this should be backported.

@Gusted Gusted modified the milestones: 1.16.7, 1.17.0 Apr 28, 2022
@lunny lunny force-pushed the lunny/admin_not_delete_self branch from 3930b63 to b5facd0 Compare May 8, 2022 16:07
@lunny lunny added type/enhancement An improvement of existing functionality and removed backport/v1.16 type/bug labels May 8, 2022
@lunny
Copy link
Member Author

lunny commented May 8, 2022

It's ready.

@zeripath
Copy link
Contributor

zeripath commented May 8, 2022

make lgtm work

@zeripath zeripath merged commit 9efa471 into go-gitea:main May 8, 2022
@lunny lunny deleted the lunny/admin_not_delete_self branch May 9, 2022 01:01
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 9, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
fdb41d4 
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Calculate filename hash only once (go-gitea#19654)
  Admin should not delete himself (go-gitea#19423)
  Restore reviewed-on message  (go-gitea#19657)
  Move some helper files out of models (go-gitea#19355)
  Repository level enable package or disable (go-gitea#19323)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@lunny @AbdulrhmnGhanem
Admin should not delete himself (go-gitea#19423)
e480930 
Admin should not be able to delete themselves.

Also partially fix go-gitea#15449
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@a1012112796 a1012112796 a1012112796 approved these changes

@relnod relnod relnod left review comments

@zeripath zeripath zeripath left review comments

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

Ensure there is always one user with admin privileges
9 participants
@lunny @wxiaoguang @delvh @6543 @zeripath @relnod @a1012112796 @GiteaBot @Gusted