fix: policy key is not GC while a expired policy is GC (#511)

* fix policy key is not GC when a policy expiration time comes

* fix policy key is not GC when a policy expiration time comes

* add group policy case

* add test

* fix test

e2e/tests/permission_test.go

@@ -1910,3 +1910,126 @@ 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) TestExpiredAccountPolicyGCAndRePut() {
+	var err error
+	ctx := context.Background()
+	user1 := s.GenAndChargeAccounts(1, 1000000)[0]
+
+	_, owner, bucketName, bucketId, _, _ := 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(bucketId, 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(bucketId, queryPolicyForAccountResp.Policy.ResourceId)
+}
+
+func (s *StorageTestSuite) TestExpiredGroupPolicyGCAndRePut() {
+	ctx := context.Background()
+	user := s.GenAndChargeAccounts(3, 10000)
+	_, owner, bucketName, bucketId, _, _ := 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)
+
+	// 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)
+
+	bucketStatement := &types.Statement{
+		Actions: []types.ActionType{types.ACTION_DELETE_BUCKET},
+		Effect:  types.EFFECT_ALLOW,
+	}
+	msgPutBucketPolicy := storagetypes.NewMsgPutPolicy(owner.GetAddr(), types2.NewBucketGRN(bucketName).String(),
+		principal, []*types.Statement{bucketStatement}, &expirationTime)
+	s.SendTxBlock(owner, msgPutBucketPolicy)
+
+	// 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(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)
+
+	// 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)
+
+	queryPolicyForGroupResp, err = s.Client.QueryPolicyForGroup(ctx, &queryPolicyForGroupReq)
+	s.Require().NoError(err)
+	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)
+}

x/permission/keeper/keeper.go

@@ -531,10 +531,42 @@ 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))
 		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
+		if ctx.IsUpgraded(upgradetypes.Pampas) {
+			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))
+					}
+				}
+			}
+		}
 	}
 }