From 6471a44892a121e6680318a9b3dff6d5d9ddf0e6 Mon Sep 17 00:00:00 2001 From: Patrik Segedy Date: Thu, 11 Jul 2024 09:20:29 +0200 Subject: [PATCH 1/2] RHINENG-11257: fix attributeFilter parsing for string value --- base/rbac/rbac.go | 40 ++++++++++++++++++++++++-- base/rbac/rbac_test.go | 64 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 base/rbac/rbac_test.go diff --git a/base/rbac/rbac.go b/base/rbac/rbac.go index 86ccea1a8..bd9d56a74 100644 --- a/base/rbac/rbac.go +++ b/base/rbac/rbac.go @@ -1,5 +1,9 @@ package rbac +import ( + "encoding/json" +) + type AccessPagination struct { Data []Access `json:"data"` } @@ -13,9 +17,11 @@ type ResourceDefinition struct { AttributeFilter AttributeFilter `json:"attributeFilter,omitempty"` } +type AttributeFilterValue []*string + type AttributeFilter struct { - Key string `json:"key"` - Value []*string `json:"value"` + Key string `json:"key"` + Value AttributeFilterValue `json:"value"` } type inventoryGroup struct { @@ -24,3 +30,33 @@ type inventoryGroup struct { } type InventoryGroup []inventoryGroup + +func (a *AttributeFilterValue) UnmarshalJSON(data []byte) error { + var ( + array []*string + value *string + err error + ) + + if err = json.Unmarshal(data, &array); err != nil { + // parsing of AttributeFilter Value into []*string failed + // try to parse it as *string + if err = json.Unmarshal(data, &value); err != nil { + // fail, the value is neither []*string nor *string + return err + } + if value != nil { + // according to RBAC team, value is a single string value + // not comma delimited strings, multiple values are always in array + array = append(array, value) + } + } + if array == nil && value == nil { + // in this case we got `"value": null` + // we should apply the permission to systems with no inventory groups + array = append(array, value) + } + + *a = array + return nil +} diff --git a/base/rbac/rbac_test.go b/base/rbac/rbac_test.go new file mode 100644 index 000000000..44fdd3967 --- /dev/null +++ b/base/rbac/rbac_test.go @@ -0,0 +1,64 @@ +package rbac + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" +) + +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{}}}, + } + + var v Access + err := json.Unmarshal(data, &v) + if assert.NoError(t, err) { + assert.Equal(t, expected, v.ResourceDefinitions) + } +} From 893ec7d2f1e9554fbdd92e254a58c7602d66ccc5 Mon Sep 17 00:00:00 2001 From: Patrik Segedy Date: Thu, 11 Jul 2024 11:42:01 +0200 Subject: [PATCH 2/2] 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}, }, }}, },