Respect DefaultUserIsRestricted system default when creating new user #19310

jpraet · 2022-04-02T10:08:10Z

The DefaultUserIsRestricted system configuration options is not always enforced when creating a new user.

This PR configures the system defaults in user_model.CreateUser, and also enhances the CreateUserOverwriteOptions to allow overwriting the system defaults when needed.

Note that the RegisterEmailConfirm and RegisterManualConfirm are also not enforced in most places.
Normalization of these settings has been taken out of the scope of this PR.

Allow for overwrites with CreateUserOverwriteOptions

jpraet · 2022-04-02T11:20:20Z

There are a lot of places where IsActive is currently set to true instead of
!(setting.Service.RegisterEmailConfirm || setting.Service.RegisterManualConfirm).
I'm not sure which of these need to remain true by design?

codecov-commenter · 2022-04-02T17:02:20Z

Codecov Report

Merging #19310 (a8dc6ec) into main (d242511) will decrease coverage by 0.11%.
The diff coverage is 41.66%.

❗ Current head a8dc6ec differs from pull request most recent head ee95d3e. Consider uploading reports for the commit ee95d3e to get more accurate results

@@            Coverage Diff             @@
##             main   #19310      +/-   ##
==========================================
- Coverage   47.51%   47.39%   -0.12%     
==========================================
  Files         944      949       +5     
  Lines      131549   132139     +590     
==========================================
+ Hits        62500    62623     +123     
- Misses      61541    61974     +433     
- Partials     7508     7542      +34

Impacted Files	Coverage Δ
cmd/admin.go	`0.00% <0.00%> (ø)`
cmd/serv.go	`2.33% <0.00%> (-0.06%)`	⬇️
models/action.go	`48.61% <0.00%> (ø)`
models/error.go	`36.46% <ø> (-0.03%)`	⬇️
models/issue_label.go	`68.02% <0.00%> (ø)`
models/issue_lock.go	`0.00% <0.00%> (ø)`
models/issue_project.go	`31.70% <0.00%> (ø)`
models/pull.go	`55.81% <0.00%> (ø)`
models/statistic.go	`0.00% <0.00%> (ø)`
models/unittest/testdb.go	`15.50% <0.00%> (-0.13%)`	⬇️
... and 121 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72479bf...ee95d3e. Read the comment docs.

cmd/admin.go

routers/api/v1/admin/user.go

routers/web/admin/users.go

routers/web/auth/oauth.go

services/auth/reverseproxy.go

services/auth/source/ldap/source_authenticate.go

services/auth/source/ldap/source_sync.go

services/auth/source/pam/source_authenticate.go

services/auth/source/smtp/source_authenticate.go

jpraet · 2022-04-09T17:11:41Z

Could anyone advise on where setting.Service.RegisterEmailConfirm and/or setting.Service.RegisterManualConfirm can be bypassed and where they cannot?

lunny · 2022-04-10T04:09:41Z

Could anyone advise on where setting.Service.RegisterEmailConfirm and/or setting.Service.RegisterManualConfirm can be bypassed and where they cannot?

I think gitea command line could ignore these options. Otherwise they should be obey.

…ice.RegisterManualConfirm where needed

lafriks · 2022-04-10T08:01:46Z

I don't really think external authentication sources should obey to these rules also, probably they could have option to them to added then to activate user or not but currently I don't think that's a good idea to have to activate them

jpraet · 2022-04-10T11:40:21Z

My initial reason for this PR was that DefaultUserIsRestricted is currently not taken into account in multiple places where new user accounts are created.

When discussing this on discord @zeripath mentioned "you might wanna check isManualConfirm too - appears I may have missed some", so I broadened the scope to also look into RegisterEmailConfirm and RegisterManualConfirm as well.

But it looks like there's no consensus as to where these settings should apply?

jpraet · 2022-04-26T09:56:38Z

Should I maybe leave the RegisterEmailConfirm and RegisterManualConfirm settings AS-IS, and only address DefaultUserIsRestricted in this PR?

lunny · 2022-04-26T11:31:16Z

Should I maybe leave the RegisterEmailConfirm and RegisterManualConfirm settings AS-IS, and only address DefaultUserIsRestricted in this PR?

I'm OK for that.

…ing.Service.RegisterManualConfirm where needed" This reverts commit ee95d3e.

lafriks · 2022-04-26T18:08:40Z

cmd/admin.go

 		IsAdmin:            c.Bool("admin"),
 		MustChangePassword: changePassword,
-		Theme:              setting.UI.DefaultTheme,


Why default theme is removed?

Already applied by default in CreateUser:

gitea/models/user/user.go

Line 646 in 5574ad1

u.Theme = setting.UI.DefaultTheme

services/auth/sspi_windows.go

…go-gitea#19310 )

…#19310 ) (#19560)

…go-gitea#19310) * Apply DefaultUserIsRestricted in CreateUser * Enforce system defaults in CreateUser Allow for overwrites with CreateUserOverwriteOptions * Fix compilation errors * Add "restricted" option to create user command * Add "restricted" option to create user admin api * Respect default setting.Service.RegisterEmailConfirm and setting.Service.RegisterManualConfirm where needed * Revert "Respect default setting.Service.RegisterEmailConfirm and setting.Service.RegisterManualConfirm where needed" This reverts commit ee95d3e.

* giteaofficial/main: Avoid MoreThanOne Error (go-gitea#19557) [skip ci] Updated licenses and gitignores Simplify loops to copy (go-gitea#19569) Use middleware to open gitRepo (go-gitea#19559) Added X-Mailer header to outgoing emails (go-gitea#19562) fix go-gitea#19545 (go-gitea#19563) [skip ci] Updated translations via Crowdin Respect DefaultUserIsRestricted system default when creating new user (go-gitea#19310) Mute link in diff header (go-gitea#19556) Add API to query collaborators permission for a repository (go-gitea#18761) Permalink files In PR diff (go-gitea#19534) Fix Pull Request comment filename word breaks (go-gitea#19535) Don't error when branch's commit doesn't exist (go-gitea#19547)

…go-gitea#19310) * Apply DefaultUserIsRestricted in CreateUser * Enforce system defaults in CreateUser Allow for overwrites with CreateUserOverwriteOptions * Fix compilation errors * Add "restricted" option to create user command * Add "restricted" option to create user admin api * Respect default setting.Service.RegisterEmailConfirm and setting.Service.RegisterManualConfirm where needed * Revert "Respect default setting.Service.RegisterEmailConfirm and setting.Service.RegisterManualConfirm where needed" This reverts commit ee95d3e.

jpraet added 3 commits April 2, 2022 12:00

Apply DefaultUserIsRestricted in CreateUser

9020faa

Enforce system defaults in CreateUser

1f8710e

Allow for overwrites with CreateUserOverwriteOptions

Fix compilation errors

4172f3f

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

jpraet added 2 commits April 2, 2022 18:34

Add "restricted" option to create user command

f747cdd

Add "restricted" option to create user admin api

a795e55

jpraet commented Apr 4, 2022

View reviewed changes

jpraet changed the title ~~[WIP] Respect restricted and account confirmation system defaults when creating new user~~ Respect restricted and account confirmation system defaults when creating new user Apr 4, 2022

jpraet marked this pull request as ready for review April 4, 2022 17:19

Merge branch 'main' into create-user-system-defaults

5ceff35

jpraet added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Apr 5, 2022

jpraet added 2 commits April 10, 2022 08:43

Merge branch 'main' into create-user-system-defaults

a8dc6ec

Respect default setting.Service.RegisterEmailConfirm and setting.Serv…

ee95d3e

…ice.RegisterManualConfirm where needed

jpraet added 2 commits April 22, 2022 16:00

Merge branch 'main' into create-user-system-defaults

c481b65

Merge branch 'main' into create-user-system-defaults

dd8f66d

jpraet added 2 commits April 26, 2022 19:59

Merge branch 'master' into create-user-system-defaults

dc26370

Revert "Respect default setting.Service.RegisterEmailConfirm and sett…

5574ad1

…ing.Service.RegisterManualConfirm where needed" This reverts commit ee95d3e.

lafriks reviewed Apr 26, 2022

View reviewed changes

jpraet changed the title ~~Respect restricted and account confirmation system defaults when creating new user~~ Respect DefaultUserIsRestricted system default when creating new user Apr 26, 2022

jpraet commented Apr 26, 2022

View reviewed changes

services/auth/sspi_windows.go Show resolved Hide resolved

jpraet added 2 commits April 27, 2022 16:35

Merge branch 'main' into create-user-system-defaults

a2e3b13

Merge branch 'main' into create-user-system-defaults

1a086b8

Merge branch 'main' into create-user-system-defaults

fb9ba55

zeripath approved these changes Apr 29, 2022

View reviewed changes

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 29, 2022

wxiaoguang approved these changes Apr 29, 2022

View reviewed changes

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 29, 2022

techknowlogick merged commit 5aebc4f into go-gitea:main Apr 29, 2022

techknowlogick added this to the 1.17.0 milestone Apr 29, 2022

jpraet added a commit to jpraet/gitea that referenced this pull request Apr 29, 2022

Respect DefaultUserIsRestricted system default when creating new user (…

1ac86cb

…go-gitea#19310 )

jpraet mentioned this pull request Apr 29, 2022

Respect DefaultUserIsRestricted system default when creating new user (#19310) #19560

Merged

techknowlogick added backport/v1.16 backport/done All backports for this PR have been created labels Apr 29, 2022

6543 pushed a commit that referenced this pull request Apr 30, 2022

Respect DefaultUserIsRestricted system default when creating new user (…

7c0bf06

…#19310 ) (#19560)

This was referenced Apr 21, 2023

service.REGISTER_MANUAL_CONFIRM is ignored if oauth2_client.ENABLE_AUTO_REGISTRATION is enabled #24138

Open

Respect the REGISTER_MANUAL_CONFIRM setting when registering via OIDC #24035

Merged

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect DefaultUserIsRestricted system default when creating new user #19310

Respect DefaultUserIsRestricted system default when creating new user #19310

jpraet commented Apr 2, 2022 •

edited

Loading

jpraet commented Apr 2, 2022

codecov-commenter commented Apr 2, 2022 •

edited

Loading

jpraet commented Apr 9, 2022

lunny commented Apr 10, 2022

lafriks commented Apr 10, 2022

jpraet commented Apr 10, 2022

jpraet commented Apr 26, 2022

lunny commented Apr 26, 2022

lafriks Apr 26, 2022

jpraet Apr 26, 2022

Respect DefaultUserIsRestricted system default when creating new user #19310

Respect DefaultUserIsRestricted system default when creating new user #19310

Conversation

jpraet commented Apr 2, 2022 • edited Loading

jpraet commented Apr 2, 2022

codecov-commenter commented Apr 2, 2022 • edited Loading

Codecov Report

jpraet commented Apr 9, 2022

lunny commented Apr 10, 2022

lafriks commented Apr 10, 2022

jpraet commented Apr 10, 2022

jpraet commented Apr 26, 2022

lunny commented Apr 26, 2022

lafriks Apr 26, 2022

Choose a reason for hiding this comment

jpraet Apr 26, 2022

Choose a reason for hiding this comment

jpraet commented Apr 2, 2022 •

edited

Loading

codecov-commenter commented Apr 2, 2022 •

edited

Loading