From 0dcdb4430479b715cd062dc30d8d3b958e0b850c Mon Sep 17 00:00:00 2001 From: Alexgao001 Date: Fri, 27 Oct 2023 16:44:52 +0800 Subject: [PATCH 1/5] fix policy key is not GC when a policy expiration time comes --- e2e/tests/permission_test.go | 52 +++++++++++++++++++++++++++++++++++ x/permission/keeper/keeper.go | 11 ++++++++ 2 files changed, 63 insertions(+) diff --git a/e2e/tests/permission_test.go b/e2e/tests/permission_test.go index 5252b61db..b0eb745fb 100644 --- a/e2e/tests/permission_test.go +++ b/e2e/tests/permission_test.go @@ -1910,3 +1910,55 @@ func (s *StorageTestSuite) TestGrantsPermissionToObjectWithWildcardInName() { _, err := s.Client.HeadObject(context.Background(), &storagetypes.QueryHeadObjectRequest{BucketName: bucketName, ObjectName: objectName}) s.Require().True(strings.Contains(err.Error(), "No such object")) } + +func (s *StorageTestSuite) TestExpiredPolicyGCAndRePut() { + var err error + ctx := context.Background() + user1 := s.GenAndChargeAccounts(1, 1000000)[0] + + _, owner, bucketName, _, _, objectId := s.createObjectWithVisibility(storagetypes.VISIBILITY_TYPE_PUBLIC_READ) + + principal := types.NewPrincipalWithAccount(user1.GetAddr()) + + // Put bucket policy + bucketStatement := &types.Statement{ + Actions: []types.ActionType{types.ACTION_DELETE_BUCKET}, + Effect: types.EFFECT_ALLOW, + } + expirationTime := time.Now().Add(5 * time.Second) + + msgPutBucketPolicy := storagetypes.NewMsgPutPolicy(owner.GetAddr(), types2.NewBucketGRN(bucketName).String(), + principal, []*types.Statement{bucketStatement}, &expirationTime) + s.SendTxBlock(owner, msgPutBucketPolicy) + + // Query the policy which is enforced on bucket + grn1 := types2.NewBucketGRN(bucketName) + queryPolicyForAccountResp, err := s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ + Resource: grn1.String(), + PrincipalAddress: user1.GetAddr().String(), + }) + s.Require().NoError(err) + s.Require().Equal(objectId, queryPolicyForAccountResp.Policy.ResourceId) + + // wait for policy expired + time.Sleep(5 * time.Second) + // Query the policy which is enforced on bucket and object + queryPolicyForAccountResp, err = s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ + Resource: grn1.String(), + PrincipalAddress: user1.GetAddr().String(), + }) + s.Require().Error(err) + + // the user should be able to re-put policy for the resource. + msgPutBucketPolicy = storagetypes.NewMsgPutPolicy(owner.GetAddr(), types2.NewBucketGRN(bucketName).String(), + principal, []*types.Statement{bucketStatement}, nil) + s.SendTxBlock(owner, msgPutBucketPolicy) + + // Query the policy which is enforced on bucket + queryPolicyForAccountResp, err = s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ + Resource: grn1.String(), + PrincipalAddress: user1.GetAddr().String(), + }) + s.Require().NoError(err) + s.Require().Equal(objectId, queryPolicyForAccountResp.Policy.ResourceId) +} diff --git a/x/permission/keeper/keeper.go b/x/permission/keeper/keeper.go index dc1f618d5..80a7bb640 100644 --- a/x/permission/keeper/keeper.go +++ b/x/permission/keeper/keeper.go @@ -531,8 +531,19 @@ func (k Keeper) RemoveExpiredPolicies(ctx sdk.Context) { } store.Delete(iterator.Key()) + // delete policyId -> policy policyId := types.ParsePolicyIdFromQueueKey(iterator.Key()) + var policy types.Policy + k.cdc.MustUnmarshal(store.Get(types.GetPolicyByIDKey(policyId)), &policy) store.Delete(types.GetPolicyByIDKey(policyId)) + + // delete policyKey -> policyId + if ctx.IsUpgraded(upgradetypes.Pampas) { + policyKey := types.GetPolicyForAccountKey(policy.ResourceId, policy.ResourceType, + policy.Principal.MustGetAccountAddress()) + store.Delete(policyKey) + } + ctx.EventManager().EmitTypedEvents(&types.EventDeletePolicy{PolicyId: policyId}) //nolint: errcheck count++ From 82c06b6e7ad11c5f5ad2bf2952df2a5cdf5d0217 Mon Sep 17 00:00:00 2001 From: Alexgao001 Date: Fri, 27 Oct 2023 17:04:50 +0800 Subject: [PATCH 2/5] fix policy key is not GC when a policy expiration time comes --- e2e/tests/permission_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/e2e/tests/permission_test.go b/e2e/tests/permission_test.go index b0eb745fb..13671b99e 100644 --- a/e2e/tests/permission_test.go +++ b/e2e/tests/permission_test.go @@ -1942,19 +1942,20 @@ func (s *StorageTestSuite) TestExpiredPolicyGCAndRePut() { // wait for policy expired time.Sleep(5 * time.Second) - // Query the policy which is enforced on bucket and object - queryPolicyForAccountResp, err = s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ + + // query the policy, which is already GC, should get err. + _, err = s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ Resource: grn1.String(), PrincipalAddress: user1.GetAddr().String(), }) s.Require().Error(err) - // the user should be able to re-put policy for the resource. + // the user should be able to re-put policy for the bucket. msgPutBucketPolicy = storagetypes.NewMsgPutPolicy(owner.GetAddr(), types2.NewBucketGRN(bucketName).String(), principal, []*types.Statement{bucketStatement}, nil) s.SendTxBlock(owner, msgPutBucketPolicy) - // Query the policy which is enforced on bucket + // Query the policy which is enforced on bucket. queryPolicyForAccountResp, err = s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ Resource: grn1.String(), PrincipalAddress: user1.GetAddr().String(), From e8381138375763c5cfbc345de990c72d051a2c89 Mon Sep 17 00:00:00 2001 From: Alexgao001 Date: Fri, 27 Oct 2023 17:59:07 +0800 Subject: [PATCH 3/5] add group policy case --- e2e/tests/permission_test.go | 55 ++++++++++++++++++++++++++++++++++- x/permission/keeper/keeper.go | 37 ++++++++++++++++++----- 2 files changed, 83 insertions(+), 9 deletions(-) diff --git a/e2e/tests/permission_test.go b/e2e/tests/permission_test.go index 13671b99e..f64318904 100644 --- a/e2e/tests/permission_test.go +++ b/e2e/tests/permission_test.go @@ -1911,7 +1911,60 @@ func (s *StorageTestSuite) TestGrantsPermissionToObjectWithWildcardInName() { s.Require().True(strings.Contains(err.Error(), "No such object")) } -func (s *StorageTestSuite) TestExpiredPolicyGCAndRePut() { +func (s *StorageTestSuite) TestExpiredAccountPolicyGCAndRePut() { + var err error + ctx := context.Background() + user1 := s.GenAndChargeAccounts(1, 1000000)[0] + + _, owner, bucketName, _, _, objectId := s.createObjectWithVisibility(storagetypes.VISIBILITY_TYPE_PUBLIC_READ) + + principal := types.NewPrincipalWithAccount(user1.GetAddr()) + + // Put bucket policy + bucketStatement := &types.Statement{ + Actions: []types.ActionType{types.ACTION_DELETE_BUCKET}, + Effect: types.EFFECT_ALLOW, + } + expirationTime := time.Now().Add(5 * time.Second) + + msgPutBucketPolicy := storagetypes.NewMsgPutPolicy(owner.GetAddr(), types2.NewBucketGRN(bucketName).String(), + principal, []*types.Statement{bucketStatement}, &expirationTime) + s.SendTxBlock(owner, msgPutBucketPolicy) + + // Query the policy which is enforced on bucket + grn1 := types2.NewBucketGRN(bucketName) + queryPolicyForAccountResp, err := s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ + Resource: grn1.String(), + PrincipalAddress: user1.GetAddr().String(), + }) + s.Require().NoError(err) + s.Require().Equal(objectId, queryPolicyForAccountResp.Policy.ResourceId) + + // wait for policy expired + time.Sleep(5 * time.Second) + + // query the policy, which is already GC, should get err. + _, err = s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ + Resource: grn1.String(), + PrincipalAddress: user1.GetAddr().String(), + }) + s.Require().Error(err) + + // the user should be able to re-put policy for the bucket. + msgPutBucketPolicy = storagetypes.NewMsgPutPolicy(owner.GetAddr(), types2.NewBucketGRN(bucketName).String(), + principal, []*types.Statement{bucketStatement}, nil) + s.SendTxBlock(owner, msgPutBucketPolicy) + + // Query the policy which is enforced on bucket. + queryPolicyForAccountResp, err = s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ + Resource: grn1.String(), + PrincipalAddress: user1.GetAddr().String(), + }) + s.Require().NoError(err) + s.Require().Equal(objectId, queryPolicyForAccountResp.Policy.ResourceId) +} + +func (s *StorageTestSuite) TestExpiredGroupPolicyGCAndRePut() { var err error ctx := context.Background() user1 := s.GenAndChargeAccounts(1, 1000000)[0] diff --git a/x/permission/keeper/keeper.go b/x/permission/keeper/keeper.go index 80a7bb640..7916d3c7c 100644 --- a/x/permission/keeper/keeper.go +++ b/x/permission/keeper/keeper.go @@ -535,17 +535,38 @@ func (k Keeper) RemoveExpiredPolicies(ctx sdk.Context) { policyId := types.ParsePolicyIdFromQueueKey(iterator.Key()) var policy types.Policy k.cdc.MustUnmarshal(store.Get(types.GetPolicyByIDKey(policyId)), &policy) + store.Delete(types.GetPolicyByIDKey(policyId)) - // delete policyKey -> policyId + //1. the policy is an account policy, delete policyKey -> policyId. + //2. the policy is group policy within a policy group, delete the index in the policy group if ctx.IsUpgraded(upgradetypes.Pampas) { - policyKey := types.GetPolicyForAccountKey(policy.ResourceId, policy.ResourceType, - policy.Principal.MustGetAccountAddress()) - store.Delete(policyKey) + if policy.Principal.Type == types.PRINCIPAL_TYPE_GNFD_ACCOUNT { + policyKey := types.GetPolicyForAccountKey(policy.ResourceId, policy.ResourceType, + policy.Principal.MustGetAccountAddress()) + store.Delete(policyKey) + } else if policy.Principal.Type == types.PRINCIPAL_TYPE_GNFD_GROUP { + policyGroupKey := types.GetPolicyForGroupKey(policy.ResourceId, policy.ResourceType) + bz := store.Get(policyGroupKey) + if bz != nil { + policyGroup := types.PolicyGroup{} + k.cdc.MustUnmarshal(bz, &policyGroup) + for i := 0; i < len(policyGroup.Items); i++ { + if policyGroup.Items[i].PolicyId.Equal(policyId) { + policyGroup.Items = append(policyGroup.Items[:i], policyGroup.Items[i+1:]...) + break + } + } + if len(policyGroup.Items) == 0 { + // delete the key if no item left + store.Delete(policyGroupKey) + } else { + store.Set(policyGroupKey, k.cdc.MustMarshal(&policyGroup)) + } + } + } + ctx.EventManager().EmitTypedEvents(&types.EventDeletePolicy{PolicyId: policyId}) //nolint: errcheck + count++ } - - ctx.EventManager().EmitTypedEvents(&types.EventDeletePolicy{PolicyId: policyId}) //nolint: errcheck - - count++ } } From b984d4c73160defd2ce4d0d2e31fcf70eeca0cf3 Mon Sep 17 00:00:00 2001 From: Alexgao001 Date: Fri, 27 Oct 2023 18:13:17 +0800 Subject: [PATCH 4/5] add test --- e2e/tests/permission_test.go | 67 ++++++++++++++++++++++------------- x/permission/keeper/keeper.go | 4 +-- 2 files changed, 44 insertions(+), 27 deletions(-) diff --git a/e2e/tests/permission_test.go b/e2e/tests/permission_test.go index f64318904..92eec103b 100644 --- a/e2e/tests/permission_test.go +++ b/e2e/tests/permission_test.go @@ -1965,54 +1965,71 @@ func (s *StorageTestSuite) TestExpiredAccountPolicyGCAndRePut() { } func (s *StorageTestSuite) TestExpiredGroupPolicyGCAndRePut() { - var err error ctx := context.Background() - user1 := s.GenAndChargeAccounts(1, 1000000)[0] + user := s.GenAndChargeAccounts(3, 10000) + _, owner, bucketName, bucketId, _, _ := s.createObjectWithVisibility(storagetypes.VISIBILITY_TYPE_PUBLIC_READ) - _, owner, bucketName, _, _, objectId := s.createObjectWithVisibility(storagetypes.VISIBILITY_TYPE_PUBLIC_READ) + // Create Group + testGroupName := "testGroup" + msgCreateGroup := storagetypes.NewMsgCreateGroup(owner.GetAddr(), testGroupName, "") + s.SendTxBlock(owner, msgCreateGroup) + membersToAdd := []*storagetypes.MsgGroupMember{ + {Member: user[1].GetAddr().String()}, + } + membersToDelete := []sdk.AccAddress{} + msgUpdateGroupMember := storagetypes.NewMsgUpdateGroupMember(owner.GetAddr(), owner.GetAddr(), testGroupName, membersToAdd, membersToDelete) + s.SendTxBlock(owner, msgUpdateGroupMember) - principal := types.NewPrincipalWithAccount(user1.GetAddr()) + // Head Group + headGroupRequest := storagetypes.QueryHeadGroupRequest{GroupOwner: owner.GetAddr().String(), GroupName: testGroupName} + headGroupResponse, err := s.Client.HeadGroup(ctx, &headGroupRequest) + s.Require().NoError(err) + s.Require().Equal(headGroupResponse.GroupInfo.GroupName, testGroupName) + s.Require().True(owner.GetAddr().Equals(sdk.MustAccAddressFromHex(headGroupResponse.GroupInfo.Owner))) + s.T().Logf("GroupInfo: %s", headGroupResponse.GetGroupInfo().String()) + + principal := types.NewPrincipalWithGroupId(headGroupResponse.GroupInfo.Id) + // Put bucket policy for group + expirationTime := time.Now().Add(5 * time.Second) - // Put bucket policy bucketStatement := &types.Statement{ Actions: []types.ActionType{types.ACTION_DELETE_BUCKET}, Effect: types.EFFECT_ALLOW, } - expirationTime := time.Now().Add(5 * time.Second) - msgPutBucketPolicy := storagetypes.NewMsgPutPolicy(owner.GetAddr(), types2.NewBucketGRN(bucketName).String(), principal, []*types.Statement{bucketStatement}, &expirationTime) s.SendTxBlock(owner, msgPutBucketPolicy) - // Query the policy which is enforced on bucket - grn1 := types2.NewBucketGRN(bucketName) - queryPolicyForAccountResp, err := s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ - Resource: grn1.String(), - PrincipalAddress: user1.GetAddr().String(), - }) + // Query bucket policy for group + grn := types2.NewBucketGRN(bucketName) + queryPolicyForGroupReq := storagetypes.QueryPolicyForGroupRequest{ + Resource: grn.String(), + PrincipalGroupId: headGroupResponse.GroupInfo.Id.String(), + } + + queryPolicyForGroupResp, err := s.Client.QueryPolicyForGroup(ctx, &queryPolicyForGroupReq) s.Require().NoError(err) - s.Require().Equal(objectId, queryPolicyForAccountResp.Policy.ResourceId) + s.Require().Equal(bucketId, queryPolicyForGroupResp.Policy.ResourceId) + s.Require().Equal(queryPolicyForGroupResp.Policy.ResourceType, resource.RESOURCE_TYPE_BUCKET) + s.Require().Equal(types.EFFECT_ALLOW, queryPolicyForGroupResp.Policy.Statements[0].Effect) + bucketPolicyId := queryPolicyForGroupResp.Policy.Id // wait for policy expired time.Sleep(5 * time.Second) - // query the policy, which is already GC, should get err. - _, err = s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ - Resource: grn1.String(), - PrincipalAddress: user1.GetAddr().String(), - }) + // policy is GC + _, err = s.Client.QueryPolicyById(ctx, &storagetypes.QueryPolicyByIdRequest{PolicyId: bucketPolicyId.String()}) s.Require().Error(err) + s.Require().ErrorContains(err, "No such Policy") // the user should be able to re-put policy for the bucket. msgPutBucketPolicy = storagetypes.NewMsgPutPolicy(owner.GetAddr(), types2.NewBucketGRN(bucketName).String(), principal, []*types.Statement{bucketStatement}, nil) s.SendTxBlock(owner, msgPutBucketPolicy) - // Query the policy which is enforced on bucket. - queryPolicyForAccountResp, err = s.Client.QueryPolicyForAccount(ctx, &storagetypes.QueryPolicyForAccountRequest{ - Resource: grn1.String(), - PrincipalAddress: user1.GetAddr().String(), - }) + queryPolicyForGroupResp, err = s.Client.QueryPolicyForGroup(ctx, &queryPolicyForGroupReq) s.Require().NoError(err) - s.Require().Equal(objectId, queryPolicyForAccountResp.Policy.ResourceId) + s.Require().Equal(bucketId, queryPolicyForGroupResp.Policy.ResourceId) + s.Require().Equal(queryPolicyForGroupResp.Policy.ResourceType, resource.RESOURCE_TYPE_BUCKET) + s.Require().Equal(types.EFFECT_ALLOW, queryPolicyForGroupResp.Policy.Statements[0].Effect) } diff --git a/x/permission/keeper/keeper.go b/x/permission/keeper/keeper.go index 7916d3c7c..3c105514a 100644 --- a/x/permission/keeper/keeper.go +++ b/x/permission/keeper/keeper.go @@ -537,6 +537,8 @@ func (k Keeper) RemoveExpiredPolicies(ctx sdk.Context) { k.cdc.MustUnmarshal(store.Get(types.GetPolicyByIDKey(policyId)), &policy) store.Delete(types.GetPolicyByIDKey(policyId)) + ctx.EventManager().EmitTypedEvents(&types.EventDeletePolicy{PolicyId: policyId}) //nolint: errcheck + count++ //1. the policy is an account policy, delete policyKey -> policyId. //2. the policy is group policy within a policy group, delete the index in the policy group @@ -565,8 +567,6 @@ func (k Keeper) RemoveExpiredPolicies(ctx sdk.Context) { } } } - ctx.EventManager().EmitTypedEvents(&types.EventDeletePolicy{PolicyId: policyId}) //nolint: errcheck - count++ } } } From d3a065b96336033a8593e3f89da16718f2214705 Mon Sep 17 00:00:00 2001 From: Alexgao001 Date: Fri, 27 Oct 2023 18:55:10 +0800 Subject: [PATCH 5/5] fix test --- e2e/tests/permission_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/tests/permission_test.go b/e2e/tests/permission_test.go index 92eec103b..699d4f1b8 100644 --- a/e2e/tests/permission_test.go +++ b/e2e/tests/permission_test.go @@ -1916,7 +1916,7 @@ func (s *StorageTestSuite) TestExpiredAccountPolicyGCAndRePut() { ctx := context.Background() user1 := s.GenAndChargeAccounts(1, 1000000)[0] - _, owner, bucketName, _, _, objectId := s.createObjectWithVisibility(storagetypes.VISIBILITY_TYPE_PUBLIC_READ) + _, owner, bucketName, bucketId, _, _ := s.createObjectWithVisibility(storagetypes.VISIBILITY_TYPE_PUBLIC_READ) principal := types.NewPrincipalWithAccount(user1.GetAddr()) @@ -1938,7 +1938,7 @@ func (s *StorageTestSuite) TestExpiredAccountPolicyGCAndRePut() { PrincipalAddress: user1.GetAddr().String(), }) s.Require().NoError(err) - s.Require().Equal(objectId, queryPolicyForAccountResp.Policy.ResourceId) + s.Require().Equal(bucketId, queryPolicyForAccountResp.Policy.ResourceId) // wait for policy expired time.Sleep(5 * time.Second) @@ -1961,7 +1961,7 @@ func (s *StorageTestSuite) TestExpiredAccountPolicyGCAndRePut() { PrincipalAddress: user1.GetAddr().String(), }) s.Require().NoError(err) - s.Require().Equal(objectId, queryPolicyForAccountResp.Policy.ResourceId) + s.Require().Equal(bucketId, queryPolicyForAccountResp.Policy.ResourceId) } func (s *StorageTestSuite) TestExpiredGroupPolicyGCAndRePut() {