From 893ec7d2f1e9554fbdd92e254a58c7602d66ccc5 Mon Sep 17 00:00:00 2001 From: Patrik Segedy Date: Thu, 11 Jul 2024 11:42:01 +0200 Subject: [PATCH] RHINENG-11257: return error when operation is not 'in' for inventory groups --- base/rbac/rbac.go | 5 +- base/rbac/rbac_test.go | 91 +++++++++++++++------------ manager/middlewares/rbac.go | 25 ++++++-- manager/middlewares/rbac_test.go | 105 ++++++++++++++++++++----------- platform/rbac.go | 5 +- 5 files changed, 146 insertions(+), 85 deletions(-) diff --git a/base/rbac/rbac.go b/base/rbac/rbac.go index bd9d56a74..daefe1fd3 100644 --- a/base/rbac/rbac.go +++ b/base/rbac/rbac.go @@ -20,8 +20,9 @@ type ResourceDefinition struct { type AttributeFilterValue []*string type AttributeFilter struct { - Key string `json:"key"` - Value AttributeFilterValue `json:"value"` + Key string `json:"key"` + Value AttributeFilterValue `json:"value"` + Operation string `json:"operation"` } type inventoryGroup struct { diff --git a/base/rbac/rbac_test.go b/base/rbac/rbac_test.go index 44fdd3967..fa5b87fd9 100644 --- a/base/rbac/rbac_test.go +++ b/base/rbac/rbac_test.go @@ -7,53 +7,62 @@ import ( "github.com/stretchr/testify/assert" ) +var data = []byte(` +{ + "resourceDefinitions": [ + {"attributeFilter": { + "key": "single_string", + "operation": "equal", + "value": "string" + }}, + {"attributeFilter": { + "key": "comma_separated", + "operation": "equal", + "value": "comma,separated" + }}, + {"attributeFilter": { + "key": "null", + "operation": "equal", + "value": null + }}, + {"attributeFilter": { + "key": "string_array", + "operation": "in", + "value": ["string", "array"] + }}, + {"attributeFilter": { + "key": "string_array_with_null", + "operation": "in", + "value": ["string", "array", null] + }}, + {"attributeFilter": { + "key": "null_array", + "operation": "in", + "value": [null] + }}, + {"attributeFilter": { + "key": "empty_array", + "operation": "in", + "value": [] + }} + ] +} +`) + func TestParsing(t *testing.T) { - data := []byte(` - { - "resourceDefinitions": [ - {"attributeFilter": { - "key": "single_string", - "value": "string" - }}, - {"attributeFilter": { - "key": "comma_separated", - "value": "comma,separated" - }}, - {"attributeFilter": { - "key": "null", - "value": null - }}, - {"attributeFilter": { - "key": "string_array", - "value": ["string", "array"] - }}, - {"attributeFilter": { - "key": "string_array_with_null", - "value": ["string", "array", null] - }}, - {"attributeFilter": { - "key": "null_array", - "value": [null] - }}, - {"attributeFilter": { - "key": "empty_array", - "value": [] - }} - ] - } - `) stringS := "string" commaS := "comma,separated" arrayS := "array" expected := []ResourceDefinition{ - {AttributeFilter: AttributeFilter{Key: "single_string", Value: []*string{&stringS}}}, - {AttributeFilter: AttributeFilter{Key: "comma_separated", Value: []*string{&commaS}}}, - {AttributeFilter: AttributeFilter{Key: "null", Value: []*string{nil}}}, - {AttributeFilter: AttributeFilter{Key: "string_array", Value: []*string{&stringS, &arrayS}}}, - {AttributeFilter: AttributeFilter{Key: "string_array_with_null", Value: []*string{&stringS, &arrayS, nil}}}, - {AttributeFilter: AttributeFilter{Key: "null_array", Value: []*string{nil}}}, - {AttributeFilter: AttributeFilter{Key: "empty_array", Value: []*string{}}}, + {AttributeFilter: AttributeFilter{Operation: "equal", Key: "single_string", Value: []*string{&stringS}}}, + {AttributeFilter: AttributeFilter{Operation: "equal", Key: "comma_separated", Value: []*string{&commaS}}}, + {AttributeFilter: AttributeFilter{Operation: "equal", Key: "null", Value: []*string{nil}}}, + {AttributeFilter: AttributeFilter{Operation: "in", Key: "string_array", Value: []*string{&stringS, &arrayS}}}, + {AttributeFilter: AttributeFilter{Operation: "in", Key: "string_array_with_null", + Value: []*string{&stringS, &arrayS, nil}}}, + {AttributeFilter: AttributeFilter{Operation: "in", Key: "null_array", Value: []*string{nil}}}, + {AttributeFilter: AttributeFilter{Operation: "in", Key: "empty_array", Value: []*string{}}}, } var v Access diff --git a/manager/middlewares/rbac.go b/manager/middlewares/rbac.go index c6c28fdb0..a7bde75aa 100644 --- a/manager/middlewares/rbac.go +++ b/manager/middlewares/rbac.go @@ -123,16 +123,21 @@ func isAccessGranted(c *gin.Context) bool { granted := checkPermissions(&access, handlerName, c.Request.Method) if granted { // collect inventory groups - c.Set(utils.KeyInventoryGroups, findInventoryGroups(&access)) + groups, err := findInventoryGroups(&access) + if err != nil { + utils.LogError("err", err.Error(), "RBAC") + granted = false + } + c.Set(utils.KeyInventoryGroups, groups) } return granted } -func findInventoryGroups(access *rbac.AccessPagination) map[string]string { +func findInventoryGroups(access *rbac.AccessPagination) (map[string]string, error) { res := make(map[string]string) if len(access.Data) == 0 { - return res + return res, nil } inventoryHostsReadPerms := expandedPermission(inventoryHostsReadPerm) groups := []string{} @@ -144,12 +149,22 @@ func findInventoryGroups(access *rbac.AccessPagination) map[string]string { if len(a.ResourceDefinitions) == 0 { // access to all groups - return nil + return nil, nil } for _, rd := range a.ResourceDefinitions { if rd.AttributeFilter.Key != "group.id" { continue } + + // https://github.com/RedHatInsights/insights-host-inventory/ + // blob/a7c8a7c980012c89e18ec0f7074609e216b37a8d/lib/middleware.py#L124 + if rd.AttributeFilter.Operation != "in" { + err := fmt.Errorf( + "invalid value '%s' for attributeFilter.Operation", + rd.AttributeFilter.Operation, + ) + return nil, err + } for _, v := range rd.AttributeFilter.Value { if v == nil { res[utils.KeyUngrouped] = "[]" @@ -168,7 +183,7 @@ func findInventoryGroups(access *rbac.AccessPagination) map[string]string { if len(groups) > 0 { res[utils.KeyGrouped] = fmt.Sprintf("{%s}", strings.Join(groups, ",")) } - return res + return res, nil } func RBAC() gin.HandlerFunc { diff --git a/manager/middlewares/rbac_test.go b/manager/middlewares/rbac_test.go index ee79658a4..2d339955a 100644 --- a/manager/middlewares/rbac_test.go +++ b/manager/middlewares/rbac_test.go @@ -247,20 +247,23 @@ func TestFindInventoryGroupsGrouped(t *testing.T) { Permission: "inventory:hosts:read", ResourceDefinitions: []rbac.ResourceDefinition{{ AttributeFilter: rbac.AttributeFilter{ - Key: "group.id", - Value: []*string{&group1}, + Key: "group.id", + Value: []*string{&group1}, + Operation: "in", }, }}, }}, } - groups := findInventoryGroups(access) - assert.Equal(t, - `{"[{\"id\":\"df57820e-965c-49a6-b0bc-797b7dd60581\"}]"}`, - groups[utils.KeyGrouped], - ) - val, ok := groups[utils.KeyUngrouped] - assert.Equal(t, "", val) - assert.Equal(t, false, ok) + groups, err := findInventoryGroups(access) + if assert.NoError(t, err) { + assert.Equal(t, + `{"[{\"id\":\"df57820e-965c-49a6-b0bc-797b7dd60581\"}]"}`, + groups[utils.KeyGrouped], + ) + val, ok := groups[utils.KeyUngrouped] + assert.Equal(t, "", val) + assert.Equal(t, false, ok) + } } func TestFindInventoryGroupsUnrouped(t *testing.T) { @@ -269,17 +272,20 @@ func TestFindInventoryGroupsUnrouped(t *testing.T) { Permission: "inventory:hosts:read", ResourceDefinitions: []rbac.ResourceDefinition{{ AttributeFilter: rbac.AttributeFilter{ - Key: "group.id", - Value: []*string{nil}, + Key: "group.id", + Value: []*string{nil}, + Operation: "in", }, }}, }}, } - groups := findInventoryGroups(access) - val, ok := groups[utils.KeyGrouped] - assert.Equal(t, "", val) - assert.Equal(t, false, ok) - assert.Equal(t, "[]", groups[utils.KeyUngrouped]) + groups, err := findInventoryGroups(access) + if assert.NoError(t, err) { + val, ok := groups[utils.KeyGrouped] + assert.Equal(t, "", val) + assert.Equal(t, false, ok) + assert.Equal(t, "[]", groups[utils.KeyUngrouped]) + } } func TestFindInventoryGroups(t *testing.T) { @@ -288,18 +294,21 @@ func TestFindInventoryGroups(t *testing.T) { Permission: "inventory:hosts:read", ResourceDefinitions: []rbac.ResourceDefinition{{ AttributeFilter: rbac.AttributeFilter{ - Key: "group.id", - Value: []*string{&group1, &group2, nil}, + Key: "group.id", + Value: []*string{&group1, &group2, nil}, + Operation: "in", }, }}, }}, } - groups := findInventoryGroups(access) - assert.Equal(t, - `{"[{\"id\":\"df57820e-965c-49a6-b0bc-797b7dd60581\"}]","[{\"id\":\"df3f0efd-c853-41b5-80a1-86881d5343d1\"}]"}`, - groups[utils.KeyGrouped], - ) - assert.Equal(t, "[]", groups[utils.KeyUngrouped]) + groups, err := findInventoryGroups(access) + if assert.NoError(t, err) { + assert.Equal(t, + `{"[{\"id\":\"df57820e-965c-49a6-b0bc-797b7dd60581\"}]","[{\"id\":\"df3f0efd-c853-41b5-80a1-86881d5343d1\"}]"}`, + groups[utils.KeyGrouped], + ) + assert.Equal(t, "[]", groups[utils.KeyUngrouped]) + } } func TestFindInventoryGroupsOverwrite(t *testing.T) { @@ -309,8 +318,9 @@ func TestFindInventoryGroupsOverwrite(t *testing.T) { Permission: "inventory:hosts:read", ResourceDefinitions: []rbac.ResourceDefinition{{ AttributeFilter: rbac.AttributeFilter{ - Key: "group.id", - Value: []*string{&group1, nil}, + Key: "group.id", + Value: []*string{&group1, nil}, + Operation: "in", }, }}, }, @@ -320,9 +330,11 @@ func TestFindInventoryGroupsOverwrite(t *testing.T) { }, }, } - groups := findInventoryGroups(access) - // we expect access to all groups (empty map) - assert.Equal(t, 0, len(groups)) + groups, err := findInventoryGroups(access) + if assert.NoError(t, err) { + // we expect access to all groups (empty map) + assert.Equal(t, 0, len(groups)) + } } func TestFindInventoryGroupsOverwrite2(t *testing.T) { @@ -336,16 +348,39 @@ func TestFindInventoryGroupsOverwrite2(t *testing.T) { Permission: "inventory:hosts:read", ResourceDefinitions: []rbac.ResourceDefinition{{ AttributeFilter: rbac.AttributeFilter{ - Key: "group.id", - Value: []*string{&group1, nil}, + Key: "group.id", + Value: []*string{&group1, nil}, + Operation: "in", + }, + }}, + }, + }, + } + groups, err := findInventoryGroups(access) + if assert.NoError(t, err) { + // we expect access to all groups (empty map) + assert.Equal(t, 0, len(groups)) + } +} + +func TestFindInventoryGroupsInvalidOp(t *testing.T) { + access := &rbac.AccessPagination{ + Data: []rbac.Access{ + { + Permission: "inventory:hosts:read", + ResourceDefinitions: []rbac.ResourceDefinition{{ + AttributeFilter: rbac.AttributeFilter{ + Key: "group.id", + Value: []*string{}, + Operation: "equal", }, }}, }, }, } - groups := findInventoryGroups(access) - // we expect access to all groups (empty map) - assert.Equal(t, 0, len(groups)) + groups, err := findInventoryGroups(access) + assert.Error(t, err) + assert.Nil(t, groups) } func TestMultiplePermissions(t *testing.T) { diff --git a/platform/rbac.go b/platform/rbac.go index f26ef35ae..1dfa09fda 100644 --- a/platform/rbac.go +++ b/platform/rbac.go @@ -20,8 +20,9 @@ func rbacHandler(c *gin.Context) { Permission: "inventory:hosts:read", ResourceDefinitions: []rbac.ResourceDefinition{{ AttributeFilter: rbac.AttributeFilter{ - Key: "group.id", - Value: []*string{&inventoryGroup, nil}, + Key: "group.id", + Operation: "in", + Value: []*string{&inventoryGroup, nil}, }, }}, },