From 59d4aadba5c15d02f3b9f0e61abb7476870c20a5 Mon Sep 17 00:00:00 2001 From: Jack Hay Date: Fri, 29 Mar 2024 11:05:41 -0400 Subject: [PATCH] Add setting to disable user features when user login type is not plain (#29615) ## Changes - Adds setting `EXTERNAL_USER_DISABLE_FEATURES` to disable any supported user features when login type is not plain - In general, this is necessary for SSO implementations to avoid inconsistencies between the external account management and the linked account - Adds helper functions to encourage correct use --- custom/conf/app.example.ini | 5 +++ .../config-cheat-sheet.en-us.md | 4 +++ models/user/user.go | 18 ++++++++++ models/user/user_test.go | 35 +++++++++++++++++++ modules/setting/admin.go | 12 ++++--- routers/api/v1/user/gpg_key.go | 5 +-- routers/api/v1/user/key.go | 4 +-- routers/web/user/setting/account.go | 4 +-- routers/web/user/setting/keys.go | 13 +++---- 9 files changed, 84 insertions(+), 16 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index e2723bd8ae..1584b10301 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1485,6 +1485,11 @@ LEVEL = Info ;; - manage_ssh_keys: a user cannot configure ssh keys ;; - manage_gpg_keys: a user cannot configure gpg keys ;USER_DISABLED_FEATURES = +;; Comma separated list of disabled features ONLY if the user has an external login type (eg. LDAP, Oauth, etc.), could be `deletion`, `manage_ssh_keys`, `manage_gpg_keys`. This setting is independent from `USER_DISABLED_FEATURES` and supplements its behavior. +;; - deletion: a user cannot delete their own account +;; - manage_ssh_keys: a user cannot configure ssh keys +;; - manage_gpg_keys: a user cannot configure gpg keys +;;EXTERNAL_USER_DISABLE_FEATURES = ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/docs/content/administration/config-cheat-sheet.en-us.md b/docs/content/administration/config-cheat-sheet.en-us.md index 03ab517d80..f2209d269a 100644 --- a/docs/content/administration/config-cheat-sheet.en-us.md +++ b/docs/content/administration/config-cheat-sheet.en-us.md @@ -522,6 +522,10 @@ And the following unique queues: - `deletion`: User cannot delete their own account. - `manage_ssh_keys`: User cannot configure ssh keys. - `manage_gpg_keys`: User cannot configure gpg keys. +- `EXTERNAL_USER_DISABLE_FEATURES`: **_empty_**: Comma separated list of disabled features ONLY if the user has an external login type (eg. LDAP, Oauth, etc.), could be `deletion`, `manage_ssh_keys`, `manage_gpg_keys`. This setting is independent from `USER_DISABLED_FEATURES` and supplements its behavior. + - `deletion`: User cannot delete their own account. + - `manage_ssh_keys`: User cannot configure ssh keys. + - `manage_gpg_keys`: User cannot configure gpg keys. ## Security (`security`) diff --git a/models/user/user.go b/models/user/user.go index 22a3099643..d459ec239e 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -1232,3 +1232,21 @@ func GetOrderByName() string { } return "name" } + +// IsFeatureDisabledWithLoginType checks if a user feature is disabled, taking into account the login type of the +// user if applicable +func IsFeatureDisabledWithLoginType(user *User, feature string) bool { + // NOTE: in the long run it may be better to check the ExternalLoginUser table rather than user.LoginType + return (user != nil && user.LoginType > auth.Plain && setting.Admin.ExternalUserDisableFeatures.Contains(feature)) || + setting.Admin.UserDisabledFeatures.Contains(feature) +} + +// DisabledFeaturesWithLoginType returns the set of user features disabled, taking into account the login type +// of the user if applicable +func DisabledFeaturesWithLoginType(user *User) *container.Set[string] { + // NOTE: in the long run it may be better to check the ExternalLoginUser table rather than user.LoginType + if user != nil && user.LoginType > auth.Plain { + return &setting.Admin.ExternalUserDisableFeatures + } + return &setting.Admin.UserDisabledFeatures +} diff --git a/models/user/user_test.go b/models/user/user_test.go index f4efd071ea..a4550fa655 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/auth/password/hash" + "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" @@ -526,3 +527,37 @@ func Test_NormalizeUserFromEmail(t *testing.T) { } } } + +func TestDisabledUserFeatures(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + testValues := container.SetOf(setting.UserFeatureDeletion, + setting.UserFeatureManageSSHKeys, + setting.UserFeatureManageGPGKeys) + + oldSetting := setting.Admin.ExternalUserDisableFeatures + defer func() { + setting.Admin.ExternalUserDisableFeatures = oldSetting + }() + setting.Admin.ExternalUserDisableFeatures = testValues + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + assert.Len(t, setting.Admin.UserDisabledFeatures.Values(), 0) + + // no features should be disabled with a plain login type + assert.LessOrEqual(t, user.LoginType, auth.Plain) + assert.Len(t, user_model.DisabledFeaturesWithLoginType(user).Values(), 0) + for _, f := range testValues.Values() { + assert.False(t, user_model.IsFeatureDisabledWithLoginType(user, f)) + } + + // check disabled features with external login type + user.LoginType = auth.OAuth2 + + // all features should be disabled + assert.NotEmpty(t, user_model.DisabledFeaturesWithLoginType(user).Values()) + for _, f := range testValues.Values() { + assert.True(t, user_model.IsFeatureDisabledWithLoginType(user, f)) + } +} diff --git a/modules/setting/admin.go b/modules/setting/admin.go index be214a58ce..8aebc76154 100644 --- a/modules/setting/admin.go +++ b/modules/setting/admin.go @@ -3,13 +3,16 @@ package setting -import "code.gitea.io/gitea/modules/container" +import ( + "code.gitea.io/gitea/modules/container" +) // Admin settings var Admin struct { - DisableRegularOrgCreation bool - DefaultEmailNotification string - UserDisabledFeatures container.Set[string] + DisableRegularOrgCreation bool + DefaultEmailNotification string + UserDisabledFeatures container.Set[string] + ExternalUserDisableFeatures container.Set[string] } func loadAdminFrom(rootCfg ConfigProvider) { @@ -17,6 +20,7 @@ func loadAdminFrom(rootCfg ConfigProvider) { Admin.DisableRegularOrgCreation = sec.Key("DISABLE_REGULAR_ORG_CREATION").MustBool(false) Admin.DefaultEmailNotification = sec.Key("DEFAULT_EMAIL_NOTIFICATIONS").MustString("enabled") Admin.UserDisabledFeatures = container.SetOf(sec.Key("USER_DISABLED_FEATURES").Strings(",")...) + Admin.ExternalUserDisableFeatures = container.SetOf(sec.Key("EXTERNAL_USER_DISABLE_FEATURES").Strings(",")...) } const ( diff --git a/routers/api/v1/user/gpg_key.go b/routers/api/v1/user/gpg_key.go index dcf5da0b2e..5a2f995e1b 100644 --- a/routers/api/v1/user/gpg_key.go +++ b/routers/api/v1/user/gpg_key.go @@ -10,6 +10,7 @@ import ( asymkey_model "code.gitea.io/gitea/models/asymkey" "code.gitea.io/gitea/models/db" + user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/web" @@ -133,7 +134,7 @@ func GetGPGKey(ctx *context.APIContext) { // CreateUserGPGKey creates new GPG key to given user by ID. func CreateUserGPGKey(ctx *context.APIContext, form api.CreateGPGKeyOption, uid int64) { - if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageGPGKeys) { + if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageGPGKeys) { ctx.NotFound("Not Found", fmt.Errorf("gpg keys setting is not allowed to be visited")) return } @@ -274,7 +275,7 @@ func DeleteGPGKey(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageGPGKeys) { + if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageGPGKeys) { ctx.NotFound("Not Found", fmt.Errorf("gpg keys setting is not allowed to be visited")) return } diff --git a/routers/api/v1/user/key.go b/routers/api/v1/user/key.go index bcbfd93bd3..d9456e7ec6 100644 --- a/routers/api/v1/user/key.go +++ b/routers/api/v1/user/key.go @@ -199,7 +199,7 @@ func GetPublicKey(ctx *context.APIContext) { // CreateUserPublicKey creates new public key to given user by ID. func CreateUserPublicKey(ctx *context.APIContext, form api.CreateKeyOption, uid int64) { - if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageSSHKeys) { + if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageSSHKeys) { ctx.NotFound("Not Found", fmt.Errorf("ssh keys setting is not allowed to be visited")) return } @@ -269,7 +269,7 @@ func DeletePublicKey(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageSSHKeys) { + if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageSSHKeys) { ctx.NotFound("Not Found", fmt.Errorf("ssh keys setting is not allowed to be visited")) return } diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index d69bda6663..c93b70af76 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -235,7 +235,7 @@ func DeleteEmail(ctx *context.Context) { // DeleteAccount render user suicide page and response for delete user himself func DeleteAccount(ctx *context.Context) { - if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureDeletion) { + if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureDeletion) { ctx.Error(http.StatusNotFound) return } @@ -319,7 +319,7 @@ func loadAccountData(ctx *context.Context) { ctx.Data["EmailNotificationsPreference"] = ctx.Doer.EmailNotificationsPreference ctx.Data["ActivationsPending"] = pendingActivation ctx.Data["CanAddEmails"] = !pendingActivation || !setting.Service.RegisterEmailConfirm - ctx.Data["UserDisabledFeatures"] = &setting.Admin.UserDisabledFeatures + ctx.Data["UserDisabledFeatures"] = user_model.DisabledFeaturesWithLoginType(ctx.Doer) if setting.Service.UserDeleteWithCommentsMaxTime != 0 { ctx.Data["UserDeleteWithCommentsMaxTime"] = setting.Service.UserDeleteWithCommentsMaxTime.String() diff --git a/routers/web/user/setting/keys.go b/routers/web/user/setting/keys.go index 056fcc0ace..9e969e045d 100644 --- a/routers/web/user/setting/keys.go +++ b/routers/web/user/setting/keys.go @@ -10,6 +10,7 @@ import ( asymkey_model "code.gitea.io/gitea/models/asymkey" "code.gitea.io/gitea/models/db" + user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/web" @@ -78,7 +79,7 @@ func KeysPost(ctx *context.Context) { ctx.Flash.Success(ctx.Tr("settings.add_principal_success", form.Content)) ctx.Redirect(setting.AppSubURL + "/user/settings/keys") case "gpg": - if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageGPGKeys) { + if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageGPGKeys) { ctx.NotFound("Not Found", fmt.Errorf("gpg keys setting is not allowed to be visited")) return } @@ -159,7 +160,7 @@ func KeysPost(ctx *context.Context) { ctx.Flash.Success(ctx.Tr("settings.verify_gpg_key_success", keyID)) ctx.Redirect(setting.AppSubURL + "/user/settings/keys") case "ssh": - if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageSSHKeys) { + if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageSSHKeys) { ctx.NotFound("Not Found", fmt.Errorf("ssh keys setting is not allowed to be visited")) return } @@ -203,7 +204,7 @@ func KeysPost(ctx *context.Context) { ctx.Flash.Success(ctx.Tr("settings.add_key_success", form.Title)) ctx.Redirect(setting.AppSubURL + "/user/settings/keys") case "verify_ssh": - if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageSSHKeys) { + if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageSSHKeys) { ctx.NotFound("Not Found", fmt.Errorf("ssh keys setting is not allowed to be visited")) return } @@ -240,7 +241,7 @@ func KeysPost(ctx *context.Context) { func DeleteKey(ctx *context.Context) { switch ctx.FormString("type") { case "gpg": - if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageGPGKeys) { + if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageGPGKeys) { ctx.NotFound("Not Found", fmt.Errorf("gpg keys setting is not allowed to be visited")) return } @@ -250,7 +251,7 @@ func DeleteKey(ctx *context.Context) { ctx.Flash.Success(ctx.Tr("settings.gpg_key_deletion_success")) } case "ssh": - if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageSSHKeys) { + if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageSSHKeys) { ctx.NotFound("Not Found", fmt.Errorf("ssh keys setting is not allowed to be visited")) return } @@ -333,5 +334,5 @@ func loadKeysData(ctx *context.Context) { ctx.Data["VerifyingID"] = ctx.FormString("verify_gpg") ctx.Data["VerifyingFingerprint"] = ctx.FormString("verify_ssh") - ctx.Data["UserDisabledFeatures"] = &setting.Admin.UserDisabledFeatures + ctx.Data["UserDisabledFeatures"] = user_model.DisabledFeaturesWithLoginType(ctx.Doer) }