Bucket users test case #773
Conversation
restapi/admin_users_test.go
Outdated
| } | ||
| ] | ||
| }` | ||
| thirdPolicy := `{ |
There was a problem hiding this comment.
rename to policy4 to keep things consistant
restapi/admin_users.go
Outdated
| policyData := &iampolicy.Policy{} | ||
| json.Unmarshal([]byte(parsedPolicy.Policy), policyData) | ||
| if err2 == nil && !deny[k] { | ||
| switch policyAllowsAndMatchesBucket(parsedPolicy, bucket) { |
There was a problem hiding this comment.
instead of passing parsedPolicy pass policy so that you don't need to unmarshal it again inside of this function
restapi/admin_users.go
Outdated
| return listUsersWithAccessToBucket(ctx, adminClient, bucket) | ||
| } | ||
|
|
||
| func policyAllowsAndMatchesBucket(policy *models.Policy, bucket string) int { |
There was a problem hiding this comment.
if you change models.Policy to iampolicy.Policy you don't need to unmarshal twice per policy
restapi/admin_users.go
Outdated
| if resources.Match(bucket, map[string][]string{}) { | ||
| if effect.IsValid() { | ||
| if effect.IsAllowed(true) { | ||
| return 1 |
There was a problem hiding this comment.
if you return constants it would be more clear, for example:
AccessAllow = 1
AccessDeny = -1
AccessNone = 0
restapi/admin_users.go
Outdated
| return nil, prepareError(err) | ||
| } | ||
| var retval []string | ||
| seen := make(map[string]bool) |
There was a problem hiding this comment.
i think akHasAccess (access key has access) would be a more fitting name for this variable, as only the accessKeys with value true are actually allowed
restapi/admin_users.go
Outdated
| if err2 == nil && !deny[k] { | ||
| switch policyAllowsAndMatchesBucket(parsedPolicy, bucket) { | ||
| case 1: | ||
| if !seen[k] { |
There was a problem hiding this comment.
this would flip a deny wouldn't it? if seen['accessKey'] is already false and seen in a previous policy
restapi/admin_users.go
Outdated
| seen := make(map[string]bool) | ||
| for i := 0; i < len(users); i++ { | ||
| for _, policyName := range users[i].Policy { | ||
| deny := make(map[string]bool) |
There was a problem hiding this comment.
I think a more fitting name for this variable would be akIsDenied (access key is denied)
restapi/admin_users_test.go
Outdated
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if got, _ := listUsersWithAccessToBucket(ctx, adminClient, tt.args.bucket); !stringArrayCompare(got, tt.want) { |
There was a problem hiding this comment.
I believe assert.Equal will compare the two arrays, initialize assert via assert := asrt.New(t) and then simply assert.Equal(tt.want, got)
This assert will fail for the empty case as you return an uninitialized array thus []string{} != []string(nil) so you should change what you expect in your test to want: []string(nil),
restapi/admin_users_test.go
Outdated
| minioGetPolicyMock = func(name string) (*iampolicy.Policy, error) { | ||
| var policy string | ||
| if name == "consoleAdmin" { | ||
| policy = policy1 |
There was a problem hiding this comment.
where there is repetition, there can be abstraction, if you put all the policies inside a map, where the key is the policy name, there's no need for a large if/else block and allows for an easy way to add more policies
restapi/admin_users.go
Outdated
| seen[info.Members[j]] = true | ||
| if err2 == nil && !deny[info.Members[j]] { | ||
| switch policyAllowsAndMatchesBucket(parsedPolicy, bucket) { | ||
| case 1: |
There was a problem hiding this comment.
this is where the constants mentioned above make the switch case even more readable
swtich res {
case AccessAllow:
case AccessDeny:
}
No description provided.