DISABLE_LOCAL_USER_MANAGEMENT mode added #18466

pboguslawski · 2022-01-30T14:42:52Z

Added parameter DISABLE_LOCAL_USER_MANAGEMENT (false by default) in
app.ini [service] section; when true blocks access to local user
management options that are not required and possibly problematic
in scenario when users are managed in external database (like LDAP)
and should not be managed separately in gitea. Options specific
to gitea (like restricted users) are still managed in this app.

This mod disables local management for all users in gitea not only
users created from specific backend (i.e. LDAP).

Author-Change-Id: IB#1105051

Added parameter DISABLE_LOCAL_USER_MANAGEMENT (false by default) in app.ini [service] section; when true disables local modifications of username, fullname and e-mail fields in user Settings. Author-Change-Id: IB#1105051

This patch blocks access to local user management options that are not required and possibly problematic in scenario when users are managed in external database (like LDAP) and should not be managed separately in gitea. Options specific to gitea (like restricted users) are still managed in this app. Author-Change-Id: IB#1105051

This fixes external user syncing when local user management is disabled. Fixes: eca3563 Author-Change-Id: IB#1105051

Hide message about changing username when local user modifications are disabled. Author-Change-Id: IB#1105051

Adopted repos screens didn't hide menu tabs. This mod fixes it and simplifies configuration for templates. It also removes unnecessarry comment. Author-Change-Id: IB#1105051 Fixes: eca3563

Fixes 500 on organization name change in DISABLE_LOCAL_USER_MANAGEMENT mode. Fixes: eca3563 Author-Change-Id: IB#1105051

pboguslawski · 2022-01-30T15:03:53Z

This is #13068 updated to current main.

Gusted · 2022-01-31T20:36:46Z

models/user/user.go

@@ -872,16 +908,21 @@ func UpdateUser(u *User, emailChanged bool) error {
 }

 // UpdateUserCols update user according special columns
-func UpdateUserCols(ctx context.Context, u *User, cols ...string) error {
-	return updateUserCols(db.GetEngine(ctx), u, cols...)
+func UpdateUserCols(ctx context.Context, u *User, force bool, cols ...string) error {


I'd mostly see that force = false, I think it's better if the exported function doesn't have this as a argument but rather a new function UpdateForceUserCols, the unexported function is fine to have such argument IMO

Fixes: f92d339 Related: go-gitea#18466 (comment) Author-Change-Id: IB#1105051

Gusted · 2022-02-01T17:19:16Z

models/user/user.go

+func updateUserAllowed(u *User) error {
+	// Don't allow changes of selected user fields if local user management is disabled.
+	if setting.Service.DisableLocalUserManagement && (u.Type == UserTypeIndividual) {
+		if currUser, err := GetUserByID(u.ID); err == nil {


More a nitpick: first check if err != nil to then return err so we can avoid a extra indent.

Current syntax is ok (currUser is defined only for if scope).

Gusted · 2022-02-01T17:20:03Z

models/user/user.go

+// updateUserAllowed is used to block updating selected user fields when local user managemement is disabled.
+func updateUserAllowed(u *User) error {
+	// Don't allow changes of selected user fields if local user management is disabled.
+	if setting.Service.DisableLocalUserManagement && (u.Type == UserTypeIndividual) {


The linter doesn't like unnecessary parentheses(issue is also present in other places in this PR)

Suggested change

if setting.Service.DisableLocalUserManagement && (u.Type == UserTypeIndividual) {

if setting.Service.DisableLocalUserManagement && u.Type == UserTypeIndividual {

Done in 25673a9.

Gusted · 2022-02-01T17:25:59Z

routers/web/admin/emails.go

@@ -109,6 +116,13 @@ func isKeywordValid(keyword string) bool {

 // ActivateEmail serves a POST request for activating/deactivating a user's email
 func ActivateEmail(ctx *context.Context) {
+


Suggested change

Done in 25673a9.

Gusted · 2022-02-01T17:26:34Z

routers/web/admin/emails.go

@@ -109,6 +116,13 @@ func isKeywordValid(keyword string) bool {

 // ActivateEmail serves a POST request for activating/deactivating a user's email
 func ActivateEmail(ctx *context.Context) {
+


Suggested change

CirnoT · 2022-02-01T22:45:07Z

What is the reasoning and desired use-case of this configuration? Instance can be have users from mixed sources and external sources are not editable already. Why anyone would want to explicitly configure this option (potentially when there are existing local users)?

In addition, why would this configuration option disable access to configuration of new external sources? As far as I am aware they have nothing to do with "local user management".

pboguslawski · 2022-02-02T06:57:31Z

What is the reasoning and desired use-case of this configuration?

To allow use gitea in corpo environments where user db is centrally managed (i.e. user data, e-mail in LDAP) and authenticated using reverse proxy (SSO with or without passwords) and local admin should not try to create nor change user data in the app itself.

Why anyone would want to explicitly configure this option (potentially when there are existing local users)?

There are no local users in described scenarios.

In addition, why would this configuration option disable access to configuration of new external sources?

Because in environments as above there is fixed number of user backends and after initial config there should be a switch to disable changes to avoid mistakes. Backend configuration should be moved from DB to app.ini IHMO but that's another (already discussed topic).

Related: go-gitea#18466 (review) Author-Change-Id: IB#1105051

CirnoT · 2022-02-02T15:15:16Z

There are no local users in described scenarios.

But there can be, isn't that right? For example after installation initial administrator is local one.

pboguslawski · 2022-02-02T15:33:12Z

But there can be, isn't that right?

Sure; system owner (not gitea developer) should be allowed to decide to enable/disable access to local user management with DISABLE_LOCAL_USER_MANAGEMENT; that's what this PR is for.

For example after installation initial administrator is local one.

Only if is created during installation. Installation using cli fortunatelly does not force it.

Fixes: cda4a8a Related: go-gitea#18466 Author-Change-Id: IB#1105051

…1105051

codecov-commenter · 2022-02-03T09:46:02Z

Codecov Report

Merging #18466 (2691234) into main (3349fd8) will decrease coverage by 0.00%.
The diff coverage is 18.39%.

@@            Coverage Diff             @@
##             main   #18466      +/-   ##
==========================================
- Coverage   46.03%   46.03%   -0.01%     
==========================================
  Files         840      846       +6     
  Lines       92856    93275     +419     
==========================================
+ Hits        42746    42937     +191     
- Misses      43323    43519     +196     
- Partials     6787     6819      +32

Impacted Files	Coverage Δ
routers/web/admin/emails.go	`0.00% <0.00%> (ø)`
routers/web/admin/users.go	`39.18% <0.00%> (-1.23%)`	⬇️
routers/web/user/setting/account.go	`25.38% <0.00%> (-2.10%)`	⬇️
routers/web/user/setting/profile.go	`36.22% <0.00%> (-0.28%)`	⬇️
services/auth/source/ldap/source_authenticate.go	`46.29% <0.00%> (ø)`
services/auth/source/ldap/source_sync.go	`37.06% <0.00%> (ø)`
services/forms/user_form.go	`40.90% <ø> (ø)`
models/user/user.go	`60.10% <20.00%> (-2.45%)`	⬇️
routers/web/admin/auths.go	`44.88% <33.33%> (-2.84%)`	⬇️
modules/setting/service.go	`76.40% <100.00%> (+0.26%)`	⬆️
... and 67 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 3349fd8...2691234. Read the comment docs.

When local user management is disabled, active user login should not be prohibited. Only primary user e-mail should be available when local user management is enabled (only this mail is synchronized from LDAP). Related: go-gitea#18466 Author-Change-Id: IB#1105051

…1105051

pboguslawski · 2022-02-04T10:22:06Z

Fixed problem with prohibition and extra e-mails when local user management is disabled:

e45a831
38e5a4d

Fixes: e45a831 Related: go-gitea#18466 Author-Change-Id: IB#1105051

…1105051

stale · 2022-04-17T12:06:56Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

wxiaoguang · 2023-05-10T07:36:44Z

It has been stale for a long time, I think it could be closed.

ps: I also agree that usually it should avoid "hiding" UI details, which makes the UI fragile and difficult to test.

Especially for this PR, a lot of logic couldn't be tested easily, and there are too many if blocks .... really fragile.

A whole page could be hidden totally if a feature is disabled, but hiding UI elements again and again seems whack-a-mole game.

pboguslawski · 2023-05-20T11:29:37Z

I think it could be closed.

Allowing local user management when using external user backend makes no sense and should not be ignored but fixed. Consider creating separate issue if you don't like proposed solution.

I also agree that usually it should avoid "hiding" UI details, which makes the UI fragile and difficult to test.

So you leave bugs just to avoid difficult testing?

A whole page could be hidden totally if a feature is disabled

No because it contains user gitea specific settings also. Only data coming from external must not be changed in gitea.

Feel free to adapt to your vision but please don't ignore such shortcoming - should be fixed if gitea wants to be serious solution for enterprise environments.

wxiaoguang · 2023-05-20T11:34:18Z

There are too many legacy problem for the auth system, not only this "disable DISABLE_LOCAL_USER_MANAGEMENT", but also includes: Enforce 2FA, default login source (OAuth/SAML), etc.

Without a complete design, the simple "patches" would conflict in the end, and would cause security problems for end users.

So, it needs a complete refactoring, I might start the work in 1.21.

pboguslawski · 2023-05-20T11:36:32Z

it needs a complete refactoring

Agree. Glad to hear there are plans to fix it. If you know any open issue regarding it - may be worth to link it here.

wxiaoguang · 2023-05-20T12:08:30Z

I opened a summary issue to track the problem: [Summary] Improve auth source / login system #24821

(it's really complicated ....)

pboguslawski added 8 commits October 6, 2020 20:13

Parameter DISABLE_LOCAL_USER_MANAGEMENT added

795b75e

Added parameter DISABLE_LOCAL_USER_MANAGEMENT (false by default) in app.ini [service] section; when true disables local modifications of username, fullname and e-mail fields in user Settings. Author-Change-Id: IB#1105051

Fixed external user syncing when local user management is disabled

f92d339

This fixes external user syncing when local user management is disabled. Fixes: eca3563 Author-Change-Id: IB#1105051

Hide message about changing username

70672ee

Hide message about changing username when local user modifications are disabled. Author-Change-Id: IB#1105051

Fixed DISABLE_LOCAL_USER_MANAGEMENT mode

d97a8ea

Adopted repos screens didn't hide menu tabs. This mod fixes it and simplifies configuration for templates. It also removes unnecessarry comment. Author-Change-Id: IB#1105051 Fixes: eca3563

Allow org name change in DISABLE_LOCAL_USER_MANAGEMENT mode

fe6ae27

Fixes 500 on organization name change in DISABLE_LOCAL_USER_MANAGEMENT mode. Fixes: eca3563 Author-Change-Id: IB#1105051

Merge master-IB#1105051 into main-IB#1105051

b3c084c

Merge main into main-IB#1105051

75e58fa

pboguslawski mentioned this pull request Jan 30, 2022

Allow to disable local user management #13068

Closed

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

Gusted reviewed Jan 31, 2022

View reviewed changes

pboguslawski added 3 commits February 1, 2022 12:04

Separate function for forced update added

cda4a8a

Fixes: f92d339 Related: go-gitea#18466 (comment) Author-Change-Id: IB#1105051

Merge main into main-IB#1105051

15a3a5d

Merge remote-tracking branch 'main-IB#1105051' into main-IB#1105051

725a608

pboguslawski mentioned this pull request Feb 1, 2022

Account autocreation from LDAP after reverse proxy authentication #18452

Open

Gusted reviewed Feb 1, 2022

View reviewed changes

pboguslawski added 2 commits February 2, 2022 11:42

Code cleanup

25673a9

Related: go-gitea#18466 (review) Author-Change-Id: IB#1105051

Merge remote-tracking branch 'main-IB#1105051' into main-IB#1105051

2b6990c

pboguslawski added 2 commits February 3, 2022 10:15

User data must be updatable from LDAP on authentication

5c8f905

Fixes: cda4a8a Related: go-gitea#18466 Author-Change-Id: IB#1105051

Merge branch 'main-IB#1105051' of github.com:ibpl/gitea into main-IB#…

2691234

…1105051

pboguslawski added 2 commits February 4, 2022 11:09

Merge branch 'main-IB#1105051' of github.com:ibpl/gitea into main-IB#…

eaecead

…1105051

Option hiding corrected

38e5a4d

Fixes: e45a831 Related: go-gitea#18466 Author-Change-Id: IB#1105051

Merge branch 'main-IB#1105051' of github.com:ibpl/gitea into main-IB#…

2c905e8

…1105051

stale bot added the issue/stale label Apr 17, 2022

wxiaoguang closed this May 10, 2023

wxiaoguang mentioned this pull request May 20, 2023

[Summary] Improve auth source / login system #24821

Open

go-gitea locked as resolved and limited conversation to collaborators Aug 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DISABLE_LOCAL_USER_MANAGEMENT mode added #18466

DISABLE_LOCAL_USER_MANAGEMENT mode added #18466

pboguslawski commented Jan 30, 2022

pboguslawski commented Jan 30, 2022

Gusted Jan 31, 2022

pboguslawski Feb 1, 2022

Gusted Feb 1, 2022

pboguslawski Feb 2, 2022 •

edited

Loading

Gusted Feb 1, 2022

pboguslawski Feb 2, 2022

Gusted Feb 1, 2022

pboguslawski Feb 2, 2022

Gusted Feb 1, 2022

CirnoT commented Feb 1, 2022

pboguslawski commented Feb 2, 2022 •

edited

Loading

CirnoT commented Feb 2, 2022

pboguslawski commented Feb 2, 2022 •

edited

Loading

codecov-commenter commented Feb 3, 2022

pboguslawski commented Feb 4, 2022 •

edited

Loading

stale bot commented Apr 17, 2022

wxiaoguang commented May 10, 2023

pboguslawski commented May 20, 2023 •

edited

Loading

wxiaoguang commented May 20, 2023

pboguslawski commented May 20, 2023

wxiaoguang commented May 20, 2023

	if setting.Service.DisableLocalUserManagement && (u.Type == UserTypeIndividual) {
	if setting.Service.DisableLocalUserManagement && u.Type == UserTypeIndividual {

		@@ -109,6 +116,13 @@ func isKeywordValid(keyword string) bool {

		// ActivateEmail serves a POST request for activating/deactivating a user's email
		func ActivateEmail(ctx *context.Context) {

DISABLE_LOCAL_USER_MANAGEMENT mode added #18466

DISABLE_LOCAL_USER_MANAGEMENT mode added #18466

Conversation

pboguslawski commented Jan 30, 2022

pboguslawski commented Jan 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pboguslawski Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CirnoT commented Feb 1, 2022

pboguslawski commented Feb 2, 2022 • edited Loading

CirnoT commented Feb 2, 2022

pboguslawski commented Feb 2, 2022 • edited Loading

codecov-commenter commented Feb 3, 2022

Codecov Report

pboguslawski commented Feb 4, 2022 • edited Loading

stale bot commented Apr 17, 2022

wxiaoguang commented May 10, 2023

pboguslawski commented May 20, 2023 • edited Loading

wxiaoguang commented May 20, 2023

pboguslawski commented May 20, 2023

wxiaoguang commented May 20, 2023

pboguslawski Feb 2, 2022 •

edited

Loading

pboguslawski commented Feb 2, 2022 •

edited

Loading

pboguslawski commented Feb 2, 2022 •

edited

Loading

pboguslawski commented Feb 4, 2022 •

edited

Loading

pboguslawski commented May 20, 2023 •

edited

Loading