feature/11-support_notprincipal #12

dkirillov · 2023-10-30T13:36:08Z

dkirillov commented

2023-10-30 13:36:08 +00:00

close #11

dkirillov self-assigned this 2023-10-30 13:36:08 +00:00

dkirillov requested review from storage-core-committers 2023-10-30 13:36:15 +00:00

dkirillov requested review from storage-core-developers 2023-10-30 13:36:15 +00:00

dkirillov requested review from storage-services-committers 2023-10-30 13:36:16 +00:00

dkirillov requested review from storage-services-developers 2023-10-30 13:36:16 +00:00

dstepanov-yadro reviewed 2023-10-31 17:01:35 +00:00

iam/converter.go Outdated

													
				@ -72,2 +69,3 @@

						var op policyengine.ConditionType

						if _, ok := statement.Principal[Wildcard]; ok {

						statementPrincipal, inverted := statement.principal()

						if _, ok := statementPrincipal[Wildcard]; ok { // this can be true only if 'inverted' false

dstepanov-yadro commented

2023-10-31 17:01:35 +00:00

I didn't catch the thought. Please explain.

dkirillov commented

2023-11-01 14:38:14 +00:00

Poster

Only Principal can be *:

{
	"Statement": {
		"Principal": "*",
	}
}

The following json is invalid according to spec

{
	"Statement": {
		"NotPrincipal": "*",
	}
}

Only `Principal` can be `*`: ```json { "Statement": { "Principal": "*", } } ``` The following json is invalid according to [spec](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_notprincipal.html) ```json { "Statement": { "NotPrincipal": "*", } } ```

👍 1

dstepanov-yadro marked this conversation as resolved

aarifullin reviewed 2023-10-31 20:15:36 +00:00

chain.go Outdated

													
				@ -86,0 +128,4 @@

					case CondNumericGreaterThanEquals:

						return "NumericGreaterThanEquals"

					default:

						return "unknown condition type"

aarifullin commented

2023-10-31 20:15:36 +00:00

Didn't you consdier to panic here? I am afraid that this can be taken for fine invocation and may cause many problems for the side that invokes this convertation

Didn't you consdier to _panic_ here? I am afraid that this can be taken for fine invocation and may cause many problems for the side that invokes this convertation

dkirillov commented

2023-11-01 14:42:02 +00:00

Poster

I supposed in our case it's enough to have panic here

I supposed in our case it's enough to have panic [here](https://git.frostfs.info/dkirillov/policy-engine/src/commit/08deadaa4cae569609540c58491ddb3d47d76922/chain.go#L148)

aarifullin marked this conversation as resolved

aarifullin reviewed 2023-10-31 20:47:39 +00:00

iam/policy.go Outdated

													
				@ -178,0 +229,4 @@

						if len(statement.Action) != 0 && len(statement.NotAction) != 0 {

							return errors.New("'Actions' and 'NotAction' are mutually exclusive")

						}

						if len(statement.Resource) != 0 && len(statement.NotResource) != 0 {

aarifullin commented

2023-10-31 20:47:39 +00:00

It is good that you intend to check mutual exclusion of these fields and this semantically correct but:

I would insist on statement.Resource != nil check instead length checking. If these types weren't be slices or maps you would define a pointer to a type and use check for nil-ness
Is it right that "one of" must surely defined? Are there situtation when neither "Resource" nor "NotResource" are defined?

It is good that you intend to check mutual exclusion of these fields and this semantically correct but: 1. I would insist on `statement.Resource != nil` check instead length checking. If these types weren't be slices or maps you would define a pointer to a type and use check for `nil`-ness 2. Is it right that "one of" must surely defined? Are there situtation when neither "Resource" nor "NotResource" are defined?

dkirillov commented

2023-11-01 14:53:53 +00:00

Poster