[#60] chain: Support numeric conditions #60

Merged
dkirillov merged 1 commit from mbiryukova/policy-engine:feature/numeric_conditions into master 2024-04-08 14:32:14 +00:00
5 changed files with 270 additions and 2 deletions

View file

@ -10,6 +10,7 @@ import (
"git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain"
"git.frostfs.info/TrueCloudLab/policy-engine/schema/common" "git.frostfs.info/TrueCloudLab/policy-engine/schema/common"
"github.com/nspcc-dev/neo-go/pkg/encoding/fixedn"
) )
const ( const (
@ -233,8 +234,7 @@ func getConditionTypeAndConverter(op string) (chain.ConditionType, convertFuncti
return 0, nil, fmt.Errorf("unsupported condition operator: '%s'", op) return 0, nil, fmt.Errorf("unsupported condition operator: '%s'", op)
} }
case strings.HasPrefix(op, "Numeric"): case strings.HasPrefix(op, "Numeric"):
// TODO return numericConditionTypeAndConverter(op)
return 0, nil, fmt.Errorf("currently nummeric conditions unsupported: '%s'", op)
case strings.HasPrefix(op, "Date"): case strings.HasPrefix(op, "Date"):
switch op { switch op {
case CondDateEquals: case CondDateEquals:
@ -269,12 +269,39 @@ func getConditionTypeAndConverter(op string) (chain.ConditionType, convertFuncti
} }
} }
func numericConditionTypeAndConverter(op string) (chain.ConditionType, convertFunction, error) {
switch op {
case CondNumericEquals:
return chain.CondNumericEquals, numericConvertFunction, nil
case CondNumericNotEquals:
return chain.CondNumericNotEquals, numericConvertFunction, nil
case CondNumericLessThan:
return chain.CondNumericLessThan, numericConvertFunction, nil
case CondNumericLessThanEquals:
return chain.CondNumericLessThanEquals, numericConvertFunction, nil
case CondNumericGreaterThan:
return chain.CondNumericGreaterThan, numericConvertFunction, nil
case CondNumericGreaterThanEquals:
return chain.CondNumericGreaterThanEquals, numericConvertFunction, nil
default:
return 0, nil, fmt.Errorf("unsupported condition operator: '%s'", op)
}
}
type convertFunction func(string) (string, error) type convertFunction func(string) (string, error)
func noConvertFunction(val string) (string, error) { func noConvertFunction(val string) (string, error) {
return val, nil return val, nil
} }
func numericConvertFunction(val string) (string, error) {

+Inf, inf, NaN, nan are valid float strings, see https://pkg.go.dev/strconv#example-ParseFloat
Does IAM supports it?

`+Inf`, `inf`, `NaN`, `nan` are valid float strings, see https://pkg.go.dev/strconv#example-ParseFloat Does IAM supports it?

No. But now we use Fixed8 and it doesn't support it.

Probably we should include this +Inf, inf, NaN, nan in test cases

No. But now we use [Fixed8](https://github.com/nspcc-dev/neo-go/blob/master/pkg/encoding/fixedn/fixed8.go) and it doesn't support it. Probably we should include this `+Inf, inf, NaN, nan` in test cases
if _, err := fixedn.Fixed8FromString(val); err == nil {
return val, nil
}
return "", fmt.Errorf("invalid numeric value: '%s'", val)
}
func dateConvertFunction(val string) (string, error) { func dateConvertFunction(val string) (string, error) {
if _, err := strconv.ParseInt(val, 10, 64); err == nil { if _, err := strconv.ParseInt(val, 10, 64); err == nil {
return val, nil return val, nil

View file

@ -385,6 +385,12 @@ func TestConvertToChainCondition(t *testing.T) {
CondArnLike: {condKeyAWSPrincipalARN: {principal}}, CondArnLike: {condKeyAWSPrincipalARN: {principal}},
CondArnNotEquals: {"key18": {"val18"}}, CondArnNotEquals: {"key18": {"val18"}},
CondArnNotLike: {"key19": {"val19"}}, CondArnNotLike: {"key19": {"val19"}},
CondNumericEquals: {"key20": {"-20"}},

negative, zero, hex, binary values tests should be added.
also values with spaces, with '+' sign.

negative, zero, hex, binary values tests should be added. also values with spaces, with '+' sign.

AWS supports only integer and decimals. So here (in converter_test.go) I wouldn't add hex/binary values.
If this is required we can support such values in native condition types and add tests there.

AWS supports only integer and decimals. So here (in `converter_test.go`) I wouldn't add hex/binary values. If this is required we can support such values in native condition types and add tests there.

Tests should not be only positive. I was expecting to see a negative binary and hex test. Also we need to support decimals)

Tests should not be only positive. I was expecting to see a negative binary and hex test. Also we need to support decimals)
CondNumericNotEquals: {"key21": {"+21"}},
CondNumericLessThan: {"key22": {"0"}},
CondNumericLessThanEquals: {"key23": {"23.23"}},
CondNumericGreaterThan: {"key24": {"-24.24"}},
CondNumericGreaterThanEquals: {"key25": {"+25.25"}},
} }
expectedCondition := []GroupedConditions{ expectedCondition := []GroupedConditions{
@ -549,11 +555,76 @@ func TestConvertToChainCondition(t *testing.T) {
Value: "val19", Value: "val19",
}}, }},
}, },
{
Conditions: []chain.Condition{{
Op: chain.CondNumericEquals,
Object: chain.ObjectRequest,
Key: "key20",
Value: "-20",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericNotEquals,
Object: chain.ObjectRequest,
Key: "key21",
Value: "+21",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericLessThan,
Object: chain.ObjectRequest,
Key: "key22",
Value: "0",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericLessThanEquals,
Object: chain.ObjectRequest,
Key: "key23",
Value: "23.23",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericGreaterThan,
Object: chain.ObjectRequest,
Key: "key24",
Value: "-24.24",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericGreaterThanEquals,
Object: chain.ObjectRequest,
Key: "key25",
Value: "+25.25",
}},
},
} }
actualCondition, err := convertToChainCondition(conditions) actualCondition, err := convertToChainCondition(conditions)
require.NoError(t, err) require.NoError(t, err)
require.ElementsMatch(t, expectedCondition, actualCondition) require.ElementsMatch(t, expectedCondition, actualCondition)
invalidConditions := []Condition{
{"key1": {"invalid"}},
{"key2": {"1 2"}},
{"key3": {"0x12f"}},
{"key4": {"0b1010"}},
{"key5": {"+Inf"}},
{"key6": {"-Inf"}},
{"key7": {"inf"}},
{"key8": {"NaN"}},
{"key9": {"nan"}},
}
for _, cond := range invalidConditions {
_, err = convertToChainCondition(Conditions{CondNumericEquals: cond})
require.Error(t, err)
}
} }
func TestParsePrincipalARN(t *testing.T) { func TestParsePrincipalARN(t *testing.T) {

View file

@ -6,6 +6,7 @@ import (
"git.frostfs.info/TrueCloudLab/policy-engine/pkg/resource" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/resource"
"git.frostfs.info/TrueCloudLab/policy-engine/util" "git.frostfs.info/TrueCloudLab/policy-engine/util"
"github.com/nspcc-dev/neo-go/pkg/encoding/fixedn"
"golang.org/x/exp/slices" "golang.org/x/exp/slices"
) )
@ -186,6 +187,38 @@ func (c *Condition) Match(req resource.Request) bool {
return val >= c.Value return val >= c.Value
case CondSliceContains: case CondSliceContains:
return slices.Contains(strings.Split(val, condSliceContainsDelimiter), c.Value) return slices.Contains(strings.Split(val, condSliceContainsDelimiter), c.Value)
case CondNumericEquals, CondNumericNotEquals, CondNumericLessThan, CondNumericLessThanEquals, CondNumericGreaterThan,
CondNumericGreaterThanEquals:
return c.matchNumeric(val)
}
}
func (c *Condition) matchNumeric(val string) bool {
valDecimal, err := fixedn.Fixed8FromString(val)

Probably we should try parse float if parsing integer failed because spec says Numeric condition operators let you construct Condition elements that restrict access based on comparing a key to an integer or decimal value.

Probably we should try parse float if parsing integer failed because [spec](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition_operators.html#Conditions_Numeric) says `Numeric condition operators let you construct Condition elements that restrict access based on comparing a key to an integer or decimal value.`

Can we parse only float in this case?

Can we parse only float in this case?

No. In general we cannot do this because equals for float can have some error:

	tmp := 0.1
	tmp2 := 0.2

	fmt.Println(tmp+tmp2 == 0.3) // false

Or we have to write custom equal function and not use ==

No. In general we cannot do this because equals for float can have some error: ```golang tmp := 0.1 tmp2 := 0.2 fmt.Println(tmp+tmp2 == 0.3) // false ``` Or we have to write custom equal function and not use `==`

What is the length of a decimal?
neo-go has a fixed decimal package https://github.com/nspcc-dev/neo-go/blob/master/pkg/encoding/fixedn/decimal.go (and we already depend on neo-go)

What is the length of a decimal? neo-go has a fixed decimal package https://github.com/nspcc-dev/neo-go/blob/master/pkg/encoding/fixedn/decimal.go (and we already depend on neo-go)

Do we have any compatibility tests here? The description at the link you have provided is vague.

Do we have any compatibility tests here? The description at the link you have provided is vague.

I couldn't find more precise description of this conditions. So we decided to use 1e-8 precision. We can use decimal from neo-go I think

I couldn't find more precise description of this conditions. So we decided to use `1e-8` precision. We can use decimal from neo-go I think
if err != nil {
return c.Op == CondNumericNotEquals
}
condVal, err := fixedn.Fixed8FromString(c.Value)
if err != nil {
return c.Op == CondNumericNotEquals
}
switch c.Op {
default:
panic(fmt.Sprintf("unimplemented: %d", c.Op))
case CondNumericEquals:
return valDecimal.Equal(condVal)
case CondNumericNotEquals:
return !valDecimal.Equal(condVal)
case CondNumericLessThan:
return valDecimal.LessThan(condVal)
case CondNumericLessThanEquals:
return valDecimal.LessThan(condVal) || valDecimal.Equal(condVal)
case CondNumericGreaterThan:
aarifullin marked this conversation as resolved Outdated

I suppose the correct way is to compare them like this

valFloat < condVal || equalFloat(valFloat, condVal)
I suppose the correct way is to compare them like this ```go valFloat < condVal || equalFloat(valFloat, condVal) ```
return valDecimal.GreaterThan(condVal)
case CondNumericGreaterThanEquals:
return valDecimal.GreaterThan(condVal) || valDecimal.Equal(condVal)
} }
} }

View file

@ -6,6 +6,7 @@ import (
"git.frostfs.info/TrueCloudLab/policy-engine/pkg/resource/testutil" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/resource/testutil"
"git.frostfs.info/TrueCloudLab/policy-engine/schema/common" "git.frostfs.info/TrueCloudLab/policy-engine/schema/common"
"git.frostfs.info/TrueCloudLab/policy-engine/schema/native" "git.frostfs.info/TrueCloudLab/policy-engine/schema/native"
"git.frostfs.info/TrueCloudLab/policy-engine/schema/s3"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
) )
@ -149,3 +150,138 @@ func TestCondSliceContainsMatch(t *testing.T) {
}) })
} }
} }
func TestNumericConditionsMatch(t *testing.T) {
propKey := s3.PropertyKeyMaxKeys
for _, tc := range []struct {
name string
conditions []Condition
value string
status Status
}{
{
name: "value from interval",
conditions: []Condition{
{
Op: CondNumericLessThan,
Object: ObjectRequest,
Key: propKey,
Value: "100",
},
{
Op: CondNumericGreaterThan,
Object: ObjectRequest,
Key: propKey,
Value: "80",
},
{
Op: CondNumericNotEquals,
Object: ObjectRequest,
Key: propKey,
Value: "91",
},
},
value: "90",
status: Allow,
},
{
name: "border value",
conditions: []Condition{
{
Op: CondNumericEquals,
Object: ObjectRequest,
Key: propKey,
Value: "50",
},
{
Op: CondNumericLessThanEquals,
Object: ObjectRequest,
Key: propKey,
Value: "50",
},
{
Op: CondNumericGreaterThanEquals,
Object: ObjectRequest,
Key: propKey,
Value: "50",
},
},
value: "50",
status: Allow,
},
} {
t.Run(tc.name, func(t *testing.T) {
resource := testutil.NewResource(native.ResourceFormatRootContainers, nil)
request := testutil.NewRequest(native.MethodPutObject, resource, map[string]string{propKey: tc.value})
ch := Chain{Rules: []Rule{{
Status: Allow,
Actions: Actions{Names: []string{native.MethodPutObject}},
Resources: Resources{Names: []string{native.ResourceFormatRootContainers}},
Condition: tc.conditions,
}}}
st, _ := ch.Match(request)
require.Equal(t, tc.status.String(), st.String())
})
}
}
func TestInvalidNumericValues(t *testing.T) {
propKey := s3.PropertyKeyMaxKeys
propValues := []string{"", "invalid"}
for _, tc := range []struct {
name string
conditionType ConditionType
match bool
}{
{
name: "NumericEquals condition",
conditionType: CondNumericEquals,
match: false,
},
{
name: "NumericNotEquals condition",
conditionType: CondNumericNotEquals,
match: true,
},
{
name: "NumericLessThan condition",
conditionType: CondNumericLessThan,
match: false,
},
{
name: "NumericLessThanEquals condition",
conditionType: CondNumericLessThanEquals,
match: false,
},
{
name: "NumericGreaterThan condition",
conditionType: CondNumericGreaterThan,
match: false,
},
{
name: "NumericGreaterThanEquals condition",
conditionType: CondNumericGreaterThanEquals,
match: false,
},
} {
t.Run(tc.name, func(t *testing.T) {
resource := testutil.NewResource(native.ResourceFormatRootContainers, nil)
condition := Condition{
Op: tc.conditionType,
Object: ObjectRequest,
Key: propKey,
Value: "50",
}
for _, propValue := range propValues {
request := testutil.NewRequest(native.MethodPutObject, resource, map[string]string{propKey: propValue})
match := condition.Match(request)
require.Equal(t, tc.match, match)
}
})
}
}

View file

@ -6,6 +6,7 @@ const (
PropertyKeyDelimiter = "s3:delimiter" PropertyKeyDelimiter = "s3:delimiter"
PropertyKeyPrefix = "s3:prefix" PropertyKeyPrefix = "s3:prefix"
PropertyKeyVersionID = "s3:VersionId" PropertyKeyVersionID = "s3:VersionId"
PropertyKeyMaxKeys = "s3:max-keys"
ResourceFormatS3All = "arn:aws:s3:::*" ResourceFormatS3All = "arn:aws:s3:::*"
ResourceFormatS3Bucket = "arn:aws:s3:::%s" ResourceFormatS3Bucket = "arn:aws:s3:::%s"