fix: add group existence check when verify permission

e2e/tests/permission_test.go

@@ -5,10 +5,14 @@ import (
 	"context"
 	"fmt"
 	"math"
+	"strconv"
 	"time"
 
 	sdkmath "cosmossdk.io/math"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
+	govtypesv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
 
 	sdktype "github.com/bnb-chain/greenfield/sdk/types"
 	storageutil "github.com/bnb-chain/greenfield/testutil/storage"
@@ -1560,6 +1564,7 @@ func (s *StorageTestSuite) TestPutPolicy_ObjectWithSlash() {
 	var err error
 	user := s.GenAndChargeAccounts(2, 1000000)
 
+	ctx := context.Background()
 	sp := s.BaseSuite.PickStorageProvider()
 	gvg, found := sp.GetFirstGlobalVirtualGroup()
 	s.Require().True(found)
@@ -1574,7 +1579,6 @@ func (s *StorageTestSuite) TestPutPolicy_ObjectWithSlash() {
 	s.SendTxBlock(user[0], msgCreateBucket)
 
 	// HeadBucket
-	ctx := context.Background()
 	queryHeadBucketRequest := storagetypes.QueryHeadBucketRequest{
 		BucketName: bucketName,
 	}
@@ -1629,3 +1633,217 @@ func (s *StorageTestSuite) TestPutPolicy_ObjectWithSlash() {
 	s.SendTxBlock(user[0], msgPutPolicy)
 
 }
+
+func (s *StorageTestSuite) TestVerifyStaleGroupPermission() {
+	ctx := context.Background()
+
+	// set the params, not to delete stale policy
+	queryParamsRequest := storagetypes.QueryParamsRequest{}
+	queryParamsResponse, err := s.Client.StorageQueryClient.Params(ctx, &queryParamsRequest)
+	s.Require().NoError(err)
+
+	newParams := queryParamsResponse.GetParams()
+	newParams.StalePolicyCleanupMax = 0
+	s.UpdateParams(&newParams)
+
+	defer func() {
+		newParams.StalePolicyCleanupMax = 100
+		s.UpdateParams(&newParams)
+	}()
+
+	user := s.GenAndChargeAccounts(3, 10000)
+	_, owner, bucketName, bucketId, objectName, objectId := s.createObjectWithVisibility(storagetypes.VISIBILITY_TYPE_PUBLIC_READ)
+
+	// Create Group with 3 group member
+	testGroupName := "testGroup"
+	msgCreateGroup := storagetypes.NewMsgCreateGroup(owner.GetAddr(), testGroupName, "")
+	msgUpdateGroupMember := storagetypes.NewMsgUpdateGroupMember(owner.GetAddr(), owner.GetAddr(), testGroupName,
+		[]*storagetypes.MsgGroupMember{
+			{
+				Member:         user[0].GetAddr().String(),
+				ExpirationTime: storagetypes.MaxTimeStamp,
+			}, {
+				Member:         user[1].GetAddr().String(),
+				ExpirationTime: storagetypes.MaxTimeStamp,
+			}, {
+				Member:         user[2].GetAddr().String(),
+				ExpirationTime: storagetypes.MaxTimeStamp,
+			}},
+		[]sdk.AccAddress{})
+	s.SendTxBlock(owner, msgCreateGroup, 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
+	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}, nil)
+	s.SendTxBlock(owner, msgPutBucketPolicy)
+
+	// Put Object policy for group
+	objectStatement := &types.Statement{
+		Actions: []types.ActionType{types.ACTION_DELETE_OBJECT},
+		Effect:  types.EFFECT_ALLOW,
+	}
+	msgPutObjectPolicy := storagetypes.NewMsgPutPolicy(owner.GetAddr(), types2.NewObjectGRN(bucketName, objectName).String(),
+		principal, []*types.Statement{objectStatement}, nil)
+	s.SendTxBlock(owner, msgPutObjectPolicy)
+
+	// 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
+
+	// Query object policy for group
+	grn2 := types2.NewObjectGRN(bucketName, objectName)
+	queryPolicyForGroupResp, err = s.Client.QueryPolicyForGroup(ctx, &storagetypes.QueryPolicyForGroupRequest{Resource: grn2.String(),
+		PrincipalGroupId: headGroupResponse.GroupInfo.Id.String()})
+	s.Require().NoError(err)
+	s.Require().Equal(objectId, queryPolicyForGroupResp.Policy.ResourceId)
+	s.Require().Equal(queryPolicyForGroupResp.Policy.ResourceType, resource.RESOURCE_TYPE_OBJECT)
+	s.Require().Equal(types.EFFECT_ALLOW, queryPolicyForGroupResp.Policy.Statements[0].Effect)
+	objectPolicyID := queryPolicyForGroupResp.Policy.Id
+
+	// verify group policy
+	verifyPermResp, err := s.Client.VerifyPermission(ctx, &storagetypes.QueryVerifyPermissionRequest{
+		Operator:   user[2].GetAddr().String(),
+		BucketName: bucketName,
+		ActionType: types.ACTION_DELETE_BUCKET,
+	})
+	s.T().Logf("Verify Bucket Permission, %s", verifyPermResp.String())
+	s.Require().NoError(err)
+	s.Require().Equal(types.EFFECT_ALLOW, verifyPermResp.Effect)
+	// verify group policy
+	verifyPermResp, err = s.Client.VerifyPermission(ctx, &storagetypes.QueryVerifyPermissionRequest{
+		Operator:   user[2].GetAddr().String(),
+		BucketName: bucketName,
+		ObjectName: objectName,
+		ActionType: types.ACTION_DELETE_OBJECT,
+	})
+	s.T().Logf("Verify Object Permission, %s", verifyPermResp.String())
+	s.Require().NoError(err)
+	s.Require().Equal(types.EFFECT_ALLOW, verifyPermResp.Effect)
+
+	// user1 deletes the group
+	msgDeleteGroup := storagetypes.NewMsgDeleteGroup(owner.GetAddr(), testGroupName)
+	s.SendTxBlock(owner, msgDeleteGroup)
+
+	// group don't exist after deletion
+	_, err = s.Client.HeadGroup(ctx, &storagetypes.QueryHeadGroupRequest{
+		GroupOwner: owner.GetAddr().String(),
+		GroupName:  testGroupName,
+	})
+	s.Require().Error(err)
+	s.Require().ErrorContains(err, "No such group")
+
+	// stale permission is still exist
+	queryPolicyByIDResp, err := s.Client.QueryPolicyById(ctx, &storagetypes.QueryPolicyByIdRequest{PolicyId: bucketPolicyID.String()})
+	s.T().Logf("Qyery policy by id resp: %s", queryPolicyByIDResp)
+	s.Require().NoError(err)
+
+	queryPolicyByIDResp, err = s.Client.QueryPolicyById(ctx, &storagetypes.QueryPolicyByIdRequest{PolicyId: objectPolicyID.String()})
+	s.T().Logf("Qyery policy by id resp: %s", queryPolicyByIDResp)
+	s.Require().NoError(err)
+
+	// verify group policy
+	verifyPermResp, err = s.Client.VerifyPermission(ctx, &storagetypes.QueryVerifyPermissionRequest{
+		Operator:   user[2].GetAddr().String(),
+		BucketName: bucketName,
+		ActionType: types.ACTION_DELETE_BUCKET,
+	})
+	s.T().Logf("Verify Bucket Permission, %s", verifyPermResp.String())
+	s.Require().NoError(err)
+	s.Require().Equal(types.EFFECT_DENY, verifyPermResp.Effect)
+	// verify group policy
+	verifyPermResp, err = s.Client.VerifyPermission(ctx, &storagetypes.QueryVerifyPermissionRequest{
+		Operator:   user[2].GetAddr().String(),
+		BucketName: bucketName,
+		ObjectName: objectName,
+		ActionType: types.ACTION_DELETE_OBJECT,
+	})
+	s.T().Logf("Verify Object Permission, %s", verifyPermResp.String())
+	s.Require().NoError(err)
+	s.Require().Equal(types.EFFECT_DENY, verifyPermResp.Effect)
+}
+
+func (s *StorageTestSuite) UpdateParams(newParams *storagetypes.Params) {
+	var err error
+	validator := s.Validator.GetAddr()
+
+	ctx := context.Background()
+
+	msgUpdateParams := &storagetypes.MsgUpdateParams{
+		Authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(),
+		Params:    *newParams,
+	}
+
+	msgProposal, err := govtypesv1.NewMsgSubmitProposal(
+		[]sdk.Msg{msgUpdateParams},
+		sdk.Coins{sdk.NewCoin(s.BaseSuite.Config.Denom, sdktype.NewIntFromInt64WithDecimal(100, sdktype.DecimalBNB))},
+		validator.String(),
+		"test", "test", "test",
+	)
+	s.Require().NoError(err)
+
+	txRes := s.SendTxBlock(s.Validator, msgProposal)
+	s.Require().Equal(txRes.Code, uint32(0))
+
+	// 3. query proposal and get proposal ID
+	var proposalId uint64
+	for _, event := range txRes.Logs[0].Events {
+		if event.Type == "submit_proposal" {
+			for _, attr := range event.Attributes {
+				if attr.Key == "proposal_id" {
+					proposalId, err = strconv.ParseUint(attr.Value, 10, 0)
+					s.Require().NoError(err)
+					break
+				}
+			}
+			break
+		}
+	}
+	s.Require().True(proposalId != 0)
+
+	queryProposal := &govtypesv1.QueryProposalRequest{ProposalId: proposalId}
+	_, err = s.Client.GovQueryClientV1.Proposal(ctx, queryProposal)
+	s.Require().NoError(err)
+
+	// 4. submit MsgVote and wait the proposal exec
+	msgVote := govtypesv1.NewMsgVote(validator, proposalId, govtypesv1.OptionYes, "test")
+	txRes = s.SendTxBlock(s.Validator, msgVote)
+	s.Require().Equal(txRes.Code, uint32(0))
+
+	queryVoteParamsReq := govtypesv1.QueryParamsRequest{ParamsType: "voting"}
+	queryVoteParamsResp, err := s.Client.GovQueryClientV1.Params(ctx, &queryVoteParamsReq)
+	s.Require().NoError(err)
+
+	// 5. wait a voting period and confirm that the proposal success.
+	s.T().Logf("voting period %s", *queryVoteParamsResp.Params.VotingPeriod)
+	time.Sleep(*queryVoteParamsResp.Params.VotingPeriod)
+	time.Sleep(1 * time.Second)
+	proposalRes, err := s.Client.GovQueryClientV1.Proposal(ctx, queryProposal)
+	s.Require().NoError(err)
+	s.Require().Equal(proposalRes.Proposal.Status, govtypesv1.ProposalStatus_PROPOSAL_STATUS_PASSED)
+
+	queryParamsResponse, err := s.Client.StorageQueryClient.Params(ctx, &storagetypes.QueryParamsRequest{})
+	s.Require().NoError(err)
+	s.T().Logf("QueryParmas: %s", queryParamsResponse.Params.String())
+	s.Require().Equal(queryParamsResponse.Params, *newParams)
+}

x/permission/keeper/keeper.go

@@ -237,6 +237,20 @@ func (k Keeper) GetPolicyForAccount(ctx sdk.Context, resourceID math.Uint,
 	return k.GetPolicyByID(ctx, k.policySeq.DecodeSequence(bz))
 }
 
+func (k Keeper) GetPolicyGroupForResource(ctx sdk.Context, resourceID math.Uint, resourceType resource.ResourceType) (*types.PolicyGroup, bool) {
+	store := ctx.KVStore(k.storeKey)
+	policyGroupKey := types.GetPolicyForGroupKey(resourceID, resourceType)
+
+	bz := store.Get(policyGroupKey)
+	if bz == nil {
+		return nil, false
+	}
+
+	var policyGroup types.PolicyGroup
+	k.cdc.MustUnmarshal(bz, &policyGroup)
+	return &policyGroup, true
+}
+
 func (k Keeper) GetPolicyForGroup(ctx sdk.Context, resourceID math.Uint,
 	resourceType resource.ResourceType, groupID math.Uint) (policy *types.Policy,
 	isFound bool) {
@@ -260,69 +274,6 @@ func (k Keeper) GetPolicyForGroup(ctx sdk.Context, resourceID math.Uint,
 	return nil, false
 }
 
-func (k Keeper) VerifyPolicy(ctx sdk.Context, resourceID math.Uint, resourceType resource.ResourceType,
-	operator sdk.AccAddress, action types.ActionType, opts *types.VerifyOptions) types.Effect {
-	// verify policy which grant permission to account
-	policy, found := k.GetPolicyForAccount(ctx, resourceID, resourceType, operator)
-	if found {
-		effect, newPolicy := policy.Eval(action, ctx.BlockTime(), opts)
-		k.Logger(ctx).Info(fmt.Sprintf("CreateObject LimitSize update: %s, effect: %s, ctx.TxBytes : %d",
-			newPolicy.String(), effect, ctx.TxSize()))
-		if effect != types.EFFECT_UNSPECIFIED {
-			if effect == types.EFFECT_ALLOW && action == types.ACTION_CREATE_OBJECT && newPolicy != nil && ctx.TxBytes() != nil {
-				_, err := k.PutPolicy(ctx, newPolicy)
-				if err != nil {
-					panic(fmt.Sprintf("Update policy error, %s", err))
-				}
-			}
-			return effect
-		}
-	}
-
-	// verify policy which grant permission to group
-	store := ctx.KVStore(k.storeKey)
-	bz := store.Get(types.GetPolicyForGroupKey(resourceID, resourceType))
-	if bz != nil {
-		policyGroup := types.PolicyGroup{}
-		k.cdc.MustUnmarshal(bz, &policyGroup)
-		allowed := false
-		var (
-			newPolicy *types.Policy
-			effect    types.Effect
-		)
-		for _, item := range policyGroup.Items {
-			// check the group has the right permission of this resource
-			p := k.MustGetPolicyByID(ctx, item.PolicyId)
-			effect, newPolicy = p.Eval(action, ctx.BlockTime(), opts)
-			if effect != types.EFFECT_UNSPECIFIED {
-				// check the operator is the member of this group
-				groupMember, memberFound := k.GetGroupMember(ctx, item.GroupId, operator)
-				if memberFound && groupMember.ExpirationTime.After(ctx.BlockTime().UTC()) {
-					// check if the operator has been revoked
-					if effect == types.EFFECT_ALLOW {
-						allowed = true
-					} else if effect == types.EFFECT_DENY {
-						return types.EFFECT_DENY
-					}
-				}
-			}
-		}
-		if allowed {
-			if action == types.ACTION_CREATE_OBJECT && newPolicy != nil && ctx.TxBytes() != nil {
-				if effect == types.EFFECT_ALLOW && action == types.ACTION_CREATE_OBJECT && newPolicy != nil && ctx.TxBytes() != nil {
-					_, err := k.PutPolicy(ctx, newPolicy)
-					if err != nil {
-						panic(fmt.Sprintf("Update policy error, %s", err))
-					}
-				}
-			}
-			return types.EFFECT_ALLOW
-		}
-	}
-
-	return types.EFFECT_UNSPECIFIED
-}
-
 func (k Keeper) DeletePolicy(ctx sdk.Context, principal *types.Principal, resourceType resource.ResourceType,
 	resourceID math.Uint) (math.Uint, error) {
 	store := ctx.KVStore(k.storeKey)

x/storage/keeper/keeper.go

@@ -2131,3 +2131,9 @@ func (k Keeper) SetInternalBucketInfo(ctx sdk.Context, bucketID sdkmath.Uint, in
 func (k Keeper) fromSpMaintenanceAcct(sp *sptypes.StorageProvider, operatorAddr sdk.AccAddress) bool {
 	return sp.Status == sptypes.STATUS_IN_MAINTENANCE && operatorAddr.Equals(sdk.MustAccAddressFromHex(sp.MaintenanceAddress))
 }
+
+func (k Keeper) hasGroup(ctx sdk.Context, groupID sdkmath.Uint) bool {
+	store := ctx.KVStore(k.storeKey)
+
+	return store.Has(types.GetGroupByIDKey(groupID))
+}