From d6065c64c485151702232a5f8ff7cd9f85e602d7 Mon Sep 17 00:00:00 2001 From: Alex Vanin Date: Wed, 6 Jul 2022 16:28:28 +0300 Subject: [PATCH] [#553] Check group grantee based on stored list of users Signed-off-by: Alex Vanin --- api/handler/acl.go | 59 +++++++------ api/handler/acl_test.go | 178 +++++++++++++++++++--------------------- 2 files changed, 114 insertions(+), 123 deletions(-) diff --git a/api/handler/acl.go b/api/handler/acl.go index e3ef3cc..95bc1fd 100644 --- a/api/handler/acl.go +++ b/api/handler/acl.go @@ -122,10 +122,13 @@ func (r *resourceInfo) IsBucket() bool { } type astOperation struct { - Users []string - IsGroupGrantee bool - Op eacl.Operation - Action eacl.Action + Users []string + Op eacl.Operation + Action eacl.Action +} + +func (a astOperation) IsGroupGrantee() bool { + return len(a.Users) == 0 } func (h *handler) GetBucketACLHandler(w http.ResponseWriter, r *http.Request) { @@ -618,14 +621,14 @@ func mergeAst(parent, child *ast) (*ast, bool) { switch len(ops) { case 2: // potential inconsistency - if astOp.IsGroupGrantee { + if groupGrantee := astOp.IsGroupGrantee(); groupGrantee { // it is not likely (such state must be detected early) // inconsistency action := eacl.ActionAllow if astOp.Action == eacl.ActionAllow { action = eacl.ActionDeny } - removeAstOp(parentResource, astOp.IsGroupGrantee, astOp.Op, action) + removeAstOp(parentResource, groupGrantee, astOp.Op, action) updated = true continue } @@ -641,15 +644,12 @@ func mergeAst(parent, child *ast) (*ast, bool) { if handleRemoveOperations(parentResource, astOp, opToDelete) { updated = true } - if !opToDelete.IsGroupGrantee && len(opToDelete.Users) == 0 { - removeAstOp(parentResource, opToDelete.IsGroupGrantee, opToDelete.Op, opToDelete.Action) - } case 1: if astOp.Action != ops[0].Action { // potential inconsistency - if astOp.IsGroupGrantee { + if groupGrantee := astOp.IsGroupGrantee(); groupGrantee { // inconsistency - removeAstOp(parentResource, astOp.IsGroupGrantee, astOp.Op, ops[0].Action) + removeAstOp(parentResource, groupGrantee, astOp.Op, ops[0].Action) parentResource.Operations = append(parentResource.Operations, astOp) updated = true continue @@ -658,9 +658,6 @@ func mergeAst(parent, child *ast) (*ast, bool) { if handleRemoveOperations(parentResource, astOp, ops[0]) { updated = true } - if !ops[0].IsGroupGrantee && len(ops[0].Users) == 0 { - removeAstOp(parentResource, ops[0].IsGroupGrantee, ops[0].Op, ops[0].Action) - } parentResource.Operations = append(parentResource.Operations, astOp) continue } @@ -723,7 +720,7 @@ func containsStr(list []string, element string) bool { func getAstOps(resource *astResource, childOp *astOperation) []*astOperation { var res []*astOperation for _, astOp := range resource.Operations { - if astOp.IsGroupGrantee == childOp.IsGroupGrantee && astOp.Op == childOp.Op { + if astOp.IsGroupGrantee() == childOp.IsGroupGrantee() && astOp.Op == childOp.Op { res = append(res, astOp) } } @@ -732,7 +729,7 @@ func getAstOps(resource *astResource, childOp *astOperation) []*astOperation { func removeAstOp(resource *astResource, group bool, op eacl.Operation, action eacl.Action) { for i, astOp := range resource.Operations { - if astOp.IsGroupGrantee == group && astOp.Op == op && astOp.Action == action { + if astOp.IsGroupGrantee() == group && astOp.Op == op && astOp.Action == action { resource.Operations = append(resource.Operations[:i], resource.Operations[i+1:]...) return } @@ -741,7 +738,7 @@ func removeAstOp(resource *astResource, group bool, op eacl.Operation, action ea func addUsers(resource *astResource, astO *astOperation, users []string) { for _, astOp := range resource.Operations { - if astOp.IsGroupGrantee == astO.IsGroupGrantee && astOp.Op == astO.Op && astOp.Action == astO.Action { + if astOp.IsGroupGrantee() == astO.IsGroupGrantee() && astOp.Op == astO.Op && astOp.Action == astO.Action { astOp.Users = append(astO.Users, users...) return } @@ -749,15 +746,19 @@ func addUsers(resource *astResource, astO *astOperation, users []string) { } func removeUsers(resource *astResource, astOperation *astOperation, users []string) { - for _, astOp := range resource.Operations { - if astOp.IsGroupGrantee == astOperation.IsGroupGrantee && astOp.Op == astOperation.Op && astOp.Action == astOperation.Action { + for ind, astOp := range resource.Operations { + if !astOp.IsGroupGrantee() && astOp.Op == astOperation.Op && astOp.Action == astOperation.Action { filteredUsers := astOp.Users[:0] // new slice without allocation for _, user := range astOp.Users { if !containsStr(users, user) { filteredUsers = append(filteredUsers, user) } } - astOp.Users = filteredUsers + if len(filteredUsers) == 0 { // remove ast resource + resource.Operations = append(resource.Operations[:ind], resource.Operations[ind+1:]...) + } else { + astOp.Users = filteredUsers + } return } } @@ -796,7 +797,7 @@ func formRecords(operations []*astOperation, resource *astResource) ([]*eacl.Rec record := eacl.NewRecord() record.SetOperation(astOp.Op) record.SetAction(astOp.Action) - if astOp.IsGroupGrantee { + if astOp.IsGroupGrantee() { eacl.AddFormedTarget(record, eacl.RoleOthers) } else { // TODO(av): optimize target @@ -832,7 +833,7 @@ func addToList(operations []*astOperation, rec eacl.Record, target eacl.Target) ) for _, astOp := range operations { - if astOp.Op == rec.Operation() && astOp.IsGroupGrantee == groupTarget { + if astOp.Op == rec.Operation() && astOp.IsGroupGrantee() == groupTarget { found = astOp } } @@ -845,9 +846,8 @@ func addToList(operations []*astOperation, rec eacl.Record, target eacl.Target) } } else { astOperation := &astOperation{ - IsGroupGrantee: groupTarget, - Op: rec.Operation(), - Action: rec.Action(), + Op: rec.Operation(), + Action: rec.Action(), } if !groupTarget { for _, key := range target.BinaryKeys() { @@ -922,7 +922,7 @@ func handleResourceOperations(bktPolicy *bucketPolicy, list []*astOperation, eac userOpsMap := make(map[string][]eacl.Operation) for _, op := range list { - if !op.IsGroupGrantee { + if !op.IsGroupGrantee() { for _, user := range op.Users { userOps := userOpsMap[user] userOps = append(userOps, op.Op) @@ -976,7 +976,7 @@ func triageOperations(operations []*astOperation) ([]*astOperation, []*astOperat func addTo(list []*astOperation, userID string, op eacl.Operation, groupGrantee bool, action eacl.Action) []*astOperation { var found *astOperation for _, astop := range list { - if astop.Op == op && astop.IsGroupGrantee == groupGrantee { + if astop.Op == op && astop.IsGroupGrantee() == groupGrantee { found = astop } } @@ -987,9 +987,8 @@ func addTo(list []*astOperation, userID string, op eacl.Operation, groupGrantee } } else { astoperation := &astOperation{ - IsGroupGrantee: groupGrantee, - Op: op, - Action: action, + Op: op, + Action: action, } if !groupGrantee { astoperation.Users = append(astoperation.Users, userID) diff --git a/api/handler/acl_test.go b/api/handler/acl_test.go index b34d70d..cae9e15 100644 --- a/api/handler/acl_test.go +++ b/api/handler/acl_test.go @@ -48,9 +48,8 @@ func TestTableToAst(t *testing.T) { { resourceInfo: resourceInfo{Bucket: "bucketName"}, Operations: []*astOperation{{ - IsGroupGrantee: true, - Op: eacl.OperationGet, - Action: eacl.ActionAllow, + Op: eacl.OperationGet, + Action: eacl.ActionAllow, }}}, { resourceInfo: resourceInfo{ @@ -63,9 +62,8 @@ func TestTableToAst(t *testing.T) { hex.EncodeToString(key.PublicKey().Bytes()), hex.EncodeToString(key2.PublicKey().Bytes()), }, - IsGroupGrantee: false, - Op: eacl.OperationPut, - Action: eacl.ActionDeny, + Op: eacl.OperationPut, + Action: eacl.ActionDeny, }}}, }, } @@ -111,9 +109,8 @@ func TestPolicyToAst(t *testing.T) { Bucket: "bucketName", }, Operations: []*astOperation{{ - IsGroupGrantee: true, - Op: eacl.OperationPut, - Action: eacl.ActionAllow, + Op: eacl.OperationPut, + Action: eacl.ActionAllow, }}, }, { @@ -139,14 +136,19 @@ func TestPolicyToAst(t *testing.T) { } func getReadOps(key *keys.PrivateKey, groupGrantee bool, action eacl.Action) []*astOperation { - var result []*astOperation + var ( + result []*astOperation + users []string + ) + if !groupGrantee { + users = append(users, hex.EncodeToString(key.PublicKey().Bytes())) + } for _, op := range readOps { result = append(result, &astOperation{ - Users: []string{hex.EncodeToString(key.PublicKey().Bytes())}, - IsGroupGrantee: groupGrantee, - Op: op, - Action: action, + Users: users, + Op: op, + Action: action, }) } @@ -165,10 +167,9 @@ func TestMergeAstUnModified(t *testing.T) { Object: "objectName", }, Operations: []*astOperation{{ - Users: []string{hex.EncodeToString(key.PublicKey().Bytes())}, - IsGroupGrantee: false, - Op: eacl.OperationPut, - Action: eacl.ActionDeny, + Users: []string{hex.EncodeToString(key.PublicKey().Bytes())}, + Op: eacl.OperationPut, + Action: eacl.ActionDeny, }}, }, }, @@ -181,9 +182,8 @@ func TestMergeAstUnModified(t *testing.T) { Bucket: "bucket", }, Operations: []*astOperation{{ - IsGroupGrantee: true, - Op: eacl.OperationGet, - Action: eacl.ActionAllow, + Op: eacl.OperationGet, + Action: eacl.ActionAllow, }}, }, child.Resources[0], @@ -204,14 +204,12 @@ func TestMergeAstModified(t *testing.T) { Object: "objectName", }, Operations: []*astOperation{{ - IsGroupGrantee: true, - Op: eacl.OperationPut, - Action: eacl.ActionDeny, + Op: eacl.OperationPut, + Action: eacl.ActionDeny, }, { - Users: []string{"user2"}, - IsGroupGrantee: false, - Op: eacl.OperationGet, - Action: eacl.ActionDeny, + Users: []string{"user2"}, + Op: eacl.OperationGet, + Action: eacl.ActionDeny, }}, }, }, @@ -225,10 +223,9 @@ func TestMergeAstModified(t *testing.T) { Object: "objectName", }, Operations: []*astOperation{{ - Users: []string{"user1"}, - IsGroupGrantee: false, - Op: eacl.OperationGet, - Action: eacl.ActionDeny, + Users: []string{"user1"}, + Op: eacl.OperationGet, + Action: eacl.ActionDeny, }}, }, }, @@ -244,10 +241,9 @@ func TestMergeAstModified(t *testing.T) { Operations: []*astOperation{ child.Resources[0].Operations[0], { - Users: []string{"user1", "user2"}, - IsGroupGrantee: false, - Op: eacl.OperationGet, - Action: eacl.ActionDeny, + Users: []string{"user1", "user2"}, + Op: eacl.OperationGet, + Action: eacl.ActionDeny, }, }, }, @@ -268,15 +264,13 @@ func TestMergeAstModifiedConflict(t *testing.T) { Object: "objectName", }, Operations: []*astOperation{{ - Users: []string{"user1"}, - IsGroupGrantee: false, - Op: eacl.OperationPut, - Action: eacl.ActionDeny, + Users: []string{"user1"}, + Op: eacl.OperationPut, + Action: eacl.ActionDeny, }, { - Users: []string{"user3"}, - IsGroupGrantee: false, - Op: eacl.OperationGet, - Action: eacl.ActionAllow, + Users: []string{"user3"}, + Op: eacl.OperationGet, + Action: eacl.ActionAllow, }}, }, }, @@ -290,20 +284,17 @@ func TestMergeAstModifiedConflict(t *testing.T) { Object: "objectName", }, Operations: []*astOperation{{ - Users: []string{"user1"}, - IsGroupGrantee: false, - Op: eacl.OperationPut, - Action: eacl.ActionAllow, + Users: []string{"user1"}, + Op: eacl.OperationPut, + Action: eacl.ActionAllow, }, { - Users: []string{"user2"}, - IsGroupGrantee: false, - Op: eacl.OperationPut, - Action: eacl.ActionDeny, + Users: []string{"user2"}, + Op: eacl.OperationPut, + Action: eacl.ActionDeny, }, { - Users: []string{"user3"}, - IsGroupGrantee: false, - Op: eacl.OperationGet, - Action: eacl.ActionDeny, + Users: []string{"user3"}, + Op: eacl.OperationGet, + Action: eacl.ActionDeny, }}, }, }, @@ -318,15 +309,13 @@ func TestMergeAstModifiedConflict(t *testing.T) { }, Operations: []*astOperation{ { - Users: []string{"user2", "user1"}, - IsGroupGrantee: false, - Op: eacl.OperationPut, - Action: eacl.ActionDeny, + Users: []string{"user2", "user1"}, + Op: eacl.OperationPut, + Action: eacl.ActionDeny, }, { - Users: []string{"user3"}, - IsGroupGrantee: false, - Op: eacl.OperationGet, - Action: eacl.ActionAllow, + Users: []string{"user3"}, + Op: eacl.OperationGet, + Action: eacl.ActionAllow, }, }, }, @@ -349,10 +338,9 @@ func TestAstToTable(t *testing.T) { Bucket: "bucketName", }, Operations: []*astOperation{{ - Users: []string{hex.EncodeToString(key.PublicKey().Bytes())}, - IsGroupGrantee: false, - Op: eacl.OperationPut, - Action: eacl.ActionAllow, + Users: []string{hex.EncodeToString(key.PublicKey().Bytes())}, + Op: eacl.OperationPut, + Action: eacl.ActionAllow, }}, }, { @@ -361,9 +349,8 @@ func TestAstToTable(t *testing.T) { Object: "objectName", }, Operations: []*astOperation{{ - IsGroupGrantee: true, - Op: eacl.OperationGet, - Action: eacl.ActionDeny, + Op: eacl.OperationGet, + Action: eacl.ActionDeny, }}, }, }, @@ -393,20 +380,29 @@ func TestRemoveUsers(t *testing.T) { Bucket: "bucket", }, Operations: []*astOperation{{ - Users: []string{"user1", "user3", "user4"}, - IsGroupGrantee: false, - Op: eacl.OperationPut, - Action: eacl.ActionAllow, - }}, + Users: []string{"user1", "user3", "user4"}, + Op: eacl.OperationPut, + Action: eacl.ActionAllow, + }, + { + Users: []string{"user5"}, + Op: eacl.OperationGet, + Action: eacl.ActionDeny, + }, + }, } - op := &astOperation{ - IsGroupGrantee: false, - Op: eacl.OperationPut, - Action: eacl.ActionAllow, + op1 := &astOperation{ + Op: eacl.OperationPut, + Action: eacl.ActionAllow, + } + op2 := &astOperation{ + Op: eacl.OperationGet, + Action: eacl.ActionDeny, } - removeUsers(resource, op, []string{"user1", "user2", "user4"}) + removeUsers(resource, op1, []string{"user1", "user2", "user4"}) // modify astOperation + removeUsers(resource, op2, []string{"user5"}) // remove astOperation require.Equal(t, len(resource.Operations), 1) require.Equal(t, []string{"user3"}, resource.Operations[0].Users) @@ -782,9 +778,8 @@ func TestObjectAclToAst(t *testing.T) { hex.EncodeToString(key.PublicKey().Bytes()), hex.EncodeToString(key2.PublicKey().Bytes()), }, - IsGroupGrantee: false, - Op: op, - Action: eacl.ActionAllow, + Op: op, + Action: eacl.ActionAllow, } operations = append(operations, astOp) } @@ -845,9 +840,8 @@ func TestBucketAclToAst(t *testing.T) { astOp := &astOperation{Users: []string{ hex.EncodeToString(key.PublicKey().Bytes()), }, - IsGroupGrantee: false, - Op: op, - Action: eacl.ActionAllow, + Op: op, + Action: eacl.ActionAllow, } operations = append(operations, astOp) } @@ -856,17 +850,15 @@ func TestBucketAclToAst(t *testing.T) { hex.EncodeToString(key.PublicKey().Bytes()), hex.EncodeToString(key2.PublicKey().Bytes()), }, - IsGroupGrantee: false, - Op: op, - Action: eacl.ActionAllow, + Op: op, + Action: eacl.ActionAllow, } operations = append(operations, astOp) } for _, op := range readOps { astOp := &astOperation{ - IsGroupGrantee: true, - Op: op, - Action: eacl.ActionAllow, + Op: op, + Action: eacl.ActionAllow, } operations = append(operations, astOp) }