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

Allow options to disable user gpg keys configuration from the interface on app.ini #29486

Merged
merged 2 commits into from
Mar 2, 2024
Merged

Allow options to disable user gpg keys configuration from the interface on app.ini #29486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5f0bbb
Allow disable gpg keys
lunny Feb 29, 2024
b4908f5
Merge branch 'main' into lunny/disable_gpg_keys
lafriks Mar 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion custom/conf/app.example.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1480,8 +1480,9 @@ LEVEL = Info
;;
;; Default configuration for email notifications for users (user configurable). Options: enabled, onmention, disabled
;DEFAULT_EMAIL_NOTIFICATIONS = enabled
;; Disabled features for users, could be "deletion", more features can be disabled in future
;; Disabled features for users, could be "deletion","manage_gpg_keys" more features can be disabled in future
;; - deletion: a user cannot delete their own account
;; - manage_gpg_keys: a user cannot configure gpg keys
;USER_DISABLED_FEATURES =

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Expand Down
3 changes: 2 additions & 1 deletion docs/content/administration/config-cheat-sheet.en-us.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,8 +518,9 @@ And the following unique queues:

- `DEFAULT_EMAIL_NOTIFICATIONS`: **enabled**: Default configuration for email notifications for users (user configurable). Options: enabled, onmention, disabled
- `DISABLE_REGULAR_ORG_CREATION`: **false**: Disallow regular (non-admin) users from creating organizations.
- `USER_DISABLED_FEATURES`: **_empty_** Disabled features for users, could be `deletion` and more features can be added in future.
- `USER_DISABLED_FEATURES`: **_empty_** Disabled features for users, could be `deletion`, `manage_gpg_keys` and more features can be added in future.
- `deletion`: User cannot delete their own account.
- `manage_gpg_keys`: User cannot configure gpg keys

## Security (`security`)

Expand Down
3 changes: 2 additions & 1 deletion docs/content/administration/config-cheat-sheet.zh-cn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,8 +497,9 @@ Gitea 创建以下非唯一队列：

- `DEFAULT_EMAIL_NOTIFICATIONS`: **enabled**：用户电子邮件通知的默认配置（用户可配置）。选项：enabled、onmention、disabled
- `DISABLE_REGULAR_ORG_CREATION`: **false**：禁止普通（非管理员）用户创建组织。
- `USER_DISABLED_FEATURES`:**_empty_** 禁用的用户特性，当前允许为空或者 `deletion`， 未来可以增加更多设置。
- `USER_DISABLED_FEATURES`:**_empty_** 禁用的用户特性，当前允许为空或者 `deletion`，`manage_gpg_keys` 未来可以增加更多设置。
- `deletion`: 用户不能通过界面或者API删除他自己。
- `manage_gpg_keys`: 用户不能配置 GPG 密钥

## 安全性 (`security`)

Expand Down
3 changes: 2 additions & 1 deletion modules/setting/admin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,6 @@ func loadAdminFrom(rootCfg ConfigProvider) {
}

const (
UserFeatureDeletion = "deletion"
UserFeatureDeletion = "deletion"
UserFeatureManageGPGKeys = "manage_gpg_keys"
)
11 changes: 11 additions & 0 deletions routers/api/v1/user/gpg_key.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

asymkey_model "code.gitea.io/gitea/models/asymkey"
"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/routers/api/v1/utils"
Expand Down Expand Up @@ -132,6 +133,11 @@ 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) {
ctx.NotFound("Not Found", fmt.Errorf("gpg keys setting is not allowed to be visited"))
return
}
Comment on lines +136 to +139
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this check into the middleware to keep the logic in the router on topic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that on #20549 but @wxiaoguang think it's better to move them into the router function body.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean this comment? #29275 (comment)

At that time:

  1. Only one handler needs such check
  2. It bloats the "web.go", do we really want to fill more and more function into "web.go"?

For this time: it is still the only way: put checks into routers/web/user/setting/keys.go, it is impossible to "move into middleware" for it.

My reviews never means "it must do so", I am always open for better ideas.

So, for this problem: what is the consistent and maintainable approach in your mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this for the middleware:

func featureEnabled(feature /*...*/string) func(ctx *context.Context) {
	if setting.Admin.UserDisabledFeatures.Contains(feature) {
		ctx.Error(http.StatusNotFound)
		return
	}
}

But that does not work well with the method with the type parameter. Repeating the same code multiple times is not really good either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And by the way, I think my concern for "testing" is still not addressed there #29275 (comment)

Copy link
Contributor

@wxiaoguang wxiaoguang Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that does not work well with the method with the type parameter. Repeating the same code multiple times is not really good either.

There is no consistent way at the moment (if there is no refactoring). You can see that there is no way to use your middleware for the "web.go" now.

Update: I didn't mean blocker, and I don't have other ideas besides "refactoring to a clearer approach".

So feel free to use any style people prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really no way but we need to use one unpleasant way.


token := asymkey_model.VerificationToken(ctx.Doer, 1)
lastToken := asymkey_model.VerificationToken(ctx.Doer, 0)

Expand Down Expand Up @@ -268,6 +274,11 @@ func DeleteGPGKey(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageGPGKeys) {
ctx.NotFound("Not Found", fmt.Errorf("gpg keys setting is not allowed to be visited"))
return
}

if err := asymkey_model.DeleteGPGKey(ctx, ctx.Doer, ctx.ParamsInt64(":id")); err != nil {
if asymkey_model.IsErrGPGKeyAccessDenied(err) {
ctx.Error(http.StatusForbidden, "", "You do not have access to this key")
Expand Down
10 changes: 10 additions & 0 deletions routers/web/user/setting/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package setting

import (
"fmt"
"net/http"

asymkey_model "code.gitea.io/gitea/models/asymkey"
Expand Down Expand Up @@ -77,6 +78,11 @@ 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) {
ctx.NotFound("Not Found", fmt.Errorf("gpg keys setting is not allowed to be visited"))
return
}

token := asymkey_model.VerificationToken(ctx.Doer, 1)
lastToken := asymkey_model.VerificationToken(ctx.Doer, 0)

Expand Down Expand Up @@ -224,6 +230,10 @@ func KeysPost(ctx *context.Context) {
func DeleteKey(ctx *context.Context) {
switch ctx.FormString("type") {
case "gpg":
if setting.Admin.UserDisabledFeatures.Contains(setting.UserFeatureManageGPGKeys) {
ctx.NotFound("Not Found", fmt.Errorf("gpg keys setting is not allowed to be visited"))
return
}
if err := asymkey_model.DeleteGPGKey(ctx, ctx.Doer, ctx.FormInt64("id")); err != nil {
ctx.Flash.Error("DeleteGPGKey: " + err.Error())
} else {
Expand Down
2 changes: 2 additions & 0 deletions templates/user/settings/keys.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
<div class="user-setting-content">
{{template "user/settings/keys_ssh" .}}
{{template "user/settings/keys_principal" .}}
{{if not ($.UserDisabledFeatures.Contains "manage_gpg_keys")}}
{{template "user/settings/keys_gpg" .}}
{{end}}
</div>
{{template "user/settings/layout_footer" .}}
Loading