Skip to content

Commit

Permalink
WebAdmin and REST API: remove too granular permissions
Browse files Browse the repository at this point in the history
Our permissions system for admin users is too granular and some
permissions overlap. For example, you can define an administrator
with the "manage_system" permission and not with the "manage_admins"
or "manage_user" permission, but the "manage_system" permission
allows you to restore a backup and then create users and
administrators. The following permissions will be removed:
"manage_admins", "manage_apikeys", "manage_system", "retention_checks",
"manage_event_rules", "manage_roles", "manage_ip_lists". Now you
need to add the "*" permission to replace the removed granular
permissions because the removed permissions allow actions that
should only be allowed to super administrators.
There is no point in having separate, overlapping permissions.

Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
  • Loading branch information
drakkan committed Nov 10, 2024
1 parent ef98ee7 commit 3dd412f
Show file tree
Hide file tree
Showing 15 changed files with 127 additions and 162 deletions.
29 changes: 5 additions & 24 deletions internal/dataprovider/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,12 @@ const (
PermAdminViewConnections = "view_conns"
PermAdminCloseConnections = "close_conns"
PermAdminViewServerStatus = "view_status"
PermAdminManageAdmins = "manage_admins"
PermAdminManageGroups = "manage_groups"
PermAdminManageFolders = "manage_folders"
PermAdminManageAPIKeys = "manage_apikeys"
PermAdminQuotaScans = "quota_scans"
PermAdminManageSystem = "manage_system"
PermAdminManageDefender = "manage_defender"
PermAdminViewDefender = "view_defender"
PermAdminRetentionChecks = "retention_checks"
PermAdminViewEvents = "view_events"
PermAdminManageEventRules = "manage_event_rules"
PermAdminManageRoles = "manage_roles"
PermAdminManageIPLists = "manage_ip_lists"
PermAdminDisableMFA = "disable_mfa"
)

Expand All @@ -73,12 +66,9 @@ const (
var (
validAdminPerms = []string{PermAdminAny, PermAdminAddUsers, PermAdminChangeUsers, PermAdminDeleteUsers,
PermAdminViewUsers, PermAdminManageFolders, PermAdminManageGroups, PermAdminViewConnections,
PermAdminCloseConnections, PermAdminViewServerStatus, PermAdminManageAdmins, PermAdminManageRoles,
PermAdminManageEventRules, PermAdminManageAPIKeys, PermAdminQuotaScans, PermAdminManageSystem,
PermAdminManageDefender, PermAdminViewDefender, PermAdminManageIPLists, PermAdminRetentionChecks,
PermAdminViewEvents, PermAdminDisableMFA}
forbiddenPermsForRoleAdmins = []string{PermAdminAny, PermAdminManageAdmins, PermAdminManageSystem,
PermAdminManageEventRules, PermAdminManageIPLists, PermAdminManageRoles}
PermAdminCloseConnections, PermAdminViewServerStatus, PermAdminQuotaScans,
PermAdminManageDefender, PermAdminViewDefender, PermAdminViewEvents, PermAdminDisableMFA}
forbiddenPermsForRoleAdmins = []string{PermAdminAny}
)

// AdminTOTPConfig defines the time-based one time password configuration
Expand Down Expand Up @@ -266,12 +256,7 @@ type Admin struct {
// Last login as unix timestamp in milliseconds
LastLogin int64 `json:"last_login"`
// Role name. If set the admin can only administer users with the same role.
// Role admins cannot have the following permissions:
// - manage_admins
// - manage_apikeys
// - manage_system
// - manage_event_rules
// - manage_roles
// Role admins cannot be super administrators
Role string `json:"role,omitempty"`
}

Expand Down Expand Up @@ -347,13 +332,9 @@ func (a *Admin) validatePermissions() error {
}
if a.Role != "" {
if slices.Contains(forbiddenPermsForRoleAdmins, perm) {
deniedPerms := strings.Join(forbiddenPermsForRoleAdmins, ",")
return util.NewI18nError(
util.NewValidationError(fmt.Sprintf("a role admin cannot have the following permissions: %q", deniedPerms)),
util.NewValidationError("a role admin cannot be a super admin"),
util.I18nErrorRoleAdminPerms,
util.I18nErrorArgs(map[string]any{
"val": deniedPerms,
}),
)
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/httpd/api_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ func getProtocolFromRequest(r *http.Request) string {
}

func hideConfidentialData(claims *jwtTokenClaims, r *http.Request) bool {
if !claims.hasPerm(dataprovider.PermAdminManageSystem) {
if !claims.hasPerm(dataprovider.PermAdminAny) {
return true
}
return r.URL.Query().Get("confidential_data") != "1"
Expand Down
6 changes: 3 additions & 3 deletions internal/httpd/httpd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,7 @@ func TestRoleRelations(t *testing.T) {
a.Role = role.Name
_, resp, err = httpdtest.AddAdmin(a, http.StatusBadRequest)
assert.NoError(t, err)
assert.Contains(t, string(resp), "a role admin cannot have the following permissions")
assert.Contains(t, string(resp), "a role admin cannot be a super admin")

a.Permissions = []string{dataprovider.PermAdminAddUsers, dataprovider.PermAdminChangeUsers,
dataprovider.PermAdminDeleteUsers, dataprovider.PermAdminViewUsers}
Expand Down Expand Up @@ -11801,7 +11801,7 @@ func TestUpdateAdminMock(t *testing.T) {
assert.Error(t, err)
admin := getTestAdmin()
admin.Username = altAdminUsername
admin.Permissions = []string{dataprovider.PermAdminManageAdmins}
admin.Permissions = []string{dataprovider.PermAdminAny}
asJSON, err := json.Marshal(admin)
assert.NoError(t, err)
req, _ := http.NewRequest(http.MethodPost, adminPath, bytes.NewBuffer(asJSON))
Expand Down Expand Up @@ -11858,7 +11858,7 @@ func TestUpdateAdminMock(t *testing.T) {
altToken, err := getJWTAPITokenFromTestServer(altAdminUsername, defaultTokenAuthPass)
assert.NoError(t, err)
admin.Password = "" // it must remain unchanged
admin.Permissions = []string{dataprovider.PermAdminManageAdmins}
admin.Permissions = []string{dataprovider.PermAdminAny}
asJSON, err = json.Marshal(admin)
assert.NoError(t, err)
req, _ = http.NewRequest(http.MethodPut, path.Join(adminPath, altAdminUsername), bytes.NewBuffer(asJSON))
Expand Down
Loading

0 comments on commit 3dd412f

Please sign in to comment.