add group check when verify permission

e2e/tests/permission_test.go

@@ -5,18 +5,21 @@ import (
 	"context"
 	"fmt"
 	"math"
+	"strconv"
 	"time"
 
 	sdkmath "cosmossdk.io/math"
-	sdk "github.com/cosmos/cosmos-sdk/types"
-
 	sdktype "github.com/bnb-chain/greenfield/sdk/types"
 	storageutil "github.com/bnb-chain/greenfield/testutil/storage"
 	types2 "github.com/bnb-chain/greenfield/types"
 	"github.com/bnb-chain/greenfield/types/common"
 	"github.com/bnb-chain/greenfield/types/resource"
 	"github.com/bnb-chain/greenfield/x/permission/types"
 	storagetypes "github.com/bnb-chain/greenfield/x/storage/types"
+	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"
 )
 
 func (s *StorageTestSuite) TestDeleteBucketPermission() {
@@ -1434,6 +1437,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)
@@ -1448,7 +1452,6 @@ func (s *StorageTestSuite) TestPutPolicy_ObjectWithSlash() {
 	s.SendTxBlock(user[0], msgCreateBucket)
 
 	// HeadBucket
-	ctx := context.Background()
 	queryHeadBucketRequest := storagetypes.QueryHeadBucketRequest{
 		BucketName: bucketName,
 	}
@@ -1506,6 +1509,21 @@ func (s *StorageTestSuite) TestPutPolicy_ObjectWithSlash() {
 
 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)
 
@@ -1565,7 +1583,7 @@ func (s *StorageTestSuite) TestVerifyStaleGroupPermission() {
 
 	// verify group policy
 	verifyPermResp, err := s.Client.VerifyPermission(ctx, &storagetypes.QueryVerifyPermissionRequest{
-		Operator:   user[1].GetAddr().String(),
+		Operator:   user[2].GetAddr().String(),
 		BucketName: bucketName,
 		ActionType: types.ACTION_DELETE_BUCKET,
 	})
@@ -1574,7 +1592,7 @@ func (s *StorageTestSuite) TestVerifyStaleGroupPermission() {
 	s.Require().Equal(types.EFFECT_ALLOW, verifyPermResp.Effect)
 	// verify group policy
 	verifyPermResp, err = s.Client.VerifyPermission(ctx, &storagetypes.QueryVerifyPermissionRequest{
-		Operator:   user[1].GetAddr().String(),
+		Operator:   user[2].GetAddr().String(),
 		BucketName: bucketName,
 		ObjectName: objectName,
 		ActionType: types.ACTION_DELETE_OBJECT,
@@ -1587,24 +1605,26 @@ func (s *StorageTestSuite) TestVerifyStaleGroupPermission() {
 	msgDeleteGroup := storagetypes.NewMsgDeleteGroup(owner.GetAddr(), testGroupName)
 	s.SendTxBlock(owner, msgDeleteGroup)
 
-	// stale permission is still exist
-	_, err = s.Client.QueryPolicyById(ctx, &storagetypes.QueryPolicyByIdRequest{PolicyId: bucketPolicyID.String()})
-	s.Require().NoError(err)
-
-	_, err = s.Client.QueryPolicyById(ctx, &storagetypes.QueryPolicyByIdRequest{PolicyId: objectPolicyID.String()})
-	s.Require().NoError(err)
-
 	// group don't exist after deletion
 	_, err = s.Client.HeadGroup(ctx, &storagetypes.QueryHeadGroupRequest{
-		GroupOwner: user[1].GetAddr().String(),
+		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[1].GetAddr().String(),
+		Operator:   user[2].GetAddr().String(),
 		BucketName: bucketName,
 		ActionType: types.ACTION_DELETE_BUCKET,
 	})
@@ -1613,7 +1633,7 @@ func (s *StorageTestSuite) TestVerifyStaleGroupPermission() {
 	s.Require().Equal(types.EFFECT_DENY, verifyPermResp.Effect)
 	// verify group policy
 	verifyPermResp, err = s.Client.VerifyPermission(ctx, &storagetypes.QueryVerifyPermissionRequest{
-		Operator:   user[1].GetAddr().String(),
+		Operator:   user[2].GetAddr().String(),
 		BucketName: bucketName,
 		ObjectName: objectName,
 		ActionType: types.ACTION_DELETE_OBJECT,
@@ -1622,3 +1642,68 @@ func (s *StorageTestSuite) TestVerifyStaleGroupPermission() {
 	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

@@ -4,16 +4,15 @@ import (
 	"fmt"
 
 	"cosmossdk.io/math"
+	"github.com/bnb-chain/greenfield/internal/sequence"
+	"github.com/bnb-chain/greenfield/types/resource"
+	"github.com/bnb-chain/greenfield/x/permission/types"
+	storagetypes "github.com/bnb-chain/greenfield/x/storage/types"
 	"github.com/cometbft/cometbft/libs/log"
 	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/store/prefix"
 	storetypes "github.com/cosmos/cosmos-sdk/store/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-
-	"github.com/bnb-chain/greenfield/internal/sequence"
-	"github.com/bnb-chain/greenfield/types/resource"
-	"github.com/bnb-chain/greenfield/x/permission/types"
-	storagetypes "github.com/bnb-chain/greenfield/x/storage/types"
 )
 
 type (
@@ -248,7 +247,7 @@ func (k Keeper) GetPolicyForGroup(ctx sdk.Context, resourceID math.Uint,
 }
 
 func (k Keeper) VerifyPolicy(ctx sdk.Context, resourceID math.Uint, resourceType resource.ResourceType,
-	operator sdk.AccAddress, action types.ActionType, opts *types.VerifyOptions) types.Effect {
+	operator sdk.AccAddress, action types.ActionType, opts *types.VerifyOptions) (types.Effect, math.Uint) {
 	// verify policy which grant permission to account
 	policy, found := k.GetPolicyForAccount(ctx, resourceID, resourceType, operator)
 	if found {
@@ -262,12 +261,13 @@ func (k Keeper) VerifyPolicy(ctx sdk.Context, resourceID math.Uint, resourceType
 					panic(fmt.Sprintf("Update policy error, %s", err))
 				}
 			}
-			return effect
+			return effect, math.ZeroUint()
 		}
 	}
 
 	// verify policy which grant permission to group
 	store := ctx.KVStore(k.storeKey)
+	withGroupID := math.ZeroUint()
 	bz := store.Get(types.GetPolicyForGroupKey(resourceID, resourceType))
 	if bz != nil {
 		policyGroup := types.PolicyGroup{}
@@ -287,8 +287,9 @@ func (k Keeper) VerifyPolicy(ctx sdk.Context, resourceID math.Uint, resourceType
 				if memberFound {
 					if effect == types.EFFECT_ALLOW {
 						allowed = true
+						withGroupID = item.GroupId
 					} else if effect == types.EFFECT_DENY {
-						return types.EFFECT_DENY
+						return types.EFFECT_DENY, math.ZeroUint()
 					}
 				}
 			}
@@ -302,11 +303,11 @@ func (k Keeper) VerifyPolicy(ctx sdk.Context, resourceID math.Uint, resourceType
 					}
 				}
 			}
-			return types.EFFECT_ALLOW
+			return types.EFFECT_ALLOW, withGroupID
 		}
 	}
 
-	return types.EFFECT_UNSPECIFIED
+	return types.EFFECT_UNSPECIFIED, withGroupID
 }
 
 func (k Keeper) DeletePolicy(ctx sdk.Context, principal *types.Principal, resourceType resource.ResourceType,

x/storage/keeper/keeper.go

@@ -2051,3 +2051,9 @@ func (k Keeper) SetInternalBucketInfo(ctx sdk.Context, bucketID sdkmath.Uint, in
 	bz := k.cdc.MustMarshal(internalBucketInfo)
 	store.Set(types.GetInternalBucketInfoKey(bucketID), bz)
 }
+
+func (k Keeper) hasGroup(ctx sdk.Context, groupID sdkmath.Uint) bool {
+	store := ctx.KVStore(k.storeKey)
+
+	return store.Has(types.GetGroupByIDKey(groupID))
+}

x/storage/keeper/permission.go

@@ -28,6 +28,16 @@ var (
 	}
 )
 
+func (k Keeper) checkIfGroupPolicy(ctx sdk.Context, groupID math.Uint) bool {
+	// if the groupID is zero, it means that the permission verification path does not go through the group
+	if groupID.Equal(math.ZeroUint()) {
+		return true
+	}
+
+	// if the permission verification path go through the group, we should check whether the group exist.
+	return k.hasGroup(ctx, groupID)
+}
+
 // VerifyBucketPermission Bucket permissions checks are divided into three steps:
 // First, if the bucket is a public bucket and the action is a read-only action, it returns "allow".
 // Second, if the operator is the owner of the bucket, it returns "allow", as the owner has the highest permission.
@@ -49,8 +59,8 @@ func (k Keeper) VerifyBucketPermission(ctx sdk.Context, bucketInfo *types.Bucket
 		return permtypes.EFFECT_ALLOW
 	}
 	// verify policy
-	effect := k.permKeeper.VerifyPolicy(ctx, bucketInfo.Id, gnfdresource.RESOURCE_TYPE_BUCKET, operator, action, options)
-	if effect == permtypes.EFFECT_ALLOW {
+	effect, withGroupID := k.permKeeper.VerifyPolicy(ctx, bucketInfo.Id, gnfdresource.RESOURCE_TYPE_BUCKET, operator, action, options)
+	if effect == permtypes.EFFECT_ALLOW && k.checkIfGroupPolicy(ctx, withGroupID) {
 		return permtypes.EFFECT_ALLOW
 	}
 	return permtypes.EFFECT_DENY
@@ -94,18 +104,19 @@ func (k Keeper) VerifyObjectPermission(ctx sdk.Context, bucketInfo *types.Bucket
 	opts := &permtypes.VerifyOptions{
 		Resource: types2.NewObjectGRN(objectInfo.BucketName, objectInfo.ObjectName).String(),
 	}
-	bucketEffect := k.permKeeper.VerifyPolicy(ctx, bucketInfo.Id, gnfdresource.RESOURCE_TYPE_BUCKET, operator, action, opts)
+	bucketEffect, bucketWithGroupID := k.permKeeper.VerifyPolicy(ctx, bucketInfo.Id, gnfdresource.RESOURCE_TYPE_BUCKET, operator, action, opts)
 	if bucketEffect == permtypes.EFFECT_DENY {
 		return permtypes.EFFECT_DENY
 	}
 
-	objectEffect := k.permKeeper.VerifyPolicy(ctx, objectInfo.Id, gnfdresource.RESOURCE_TYPE_OBJECT, operator, action,
+	objectEffect, objectWithGroupID := k.permKeeper.VerifyPolicy(ctx, objectInfo.Id, gnfdresource.RESOURCE_TYPE_OBJECT, operator, action,
 		nil)
 	if objectEffect == permtypes.EFFECT_DENY {
 		return permtypes.EFFECT_DENY
 	}
 
-	if bucketEffect == permtypes.EFFECT_ALLOW || objectEffect == permtypes.EFFECT_ALLOW {
+	if (bucketEffect == permtypes.EFFECT_ALLOW && k.checkIfGroupPolicy(ctx, bucketWithGroupID)) ||
+		(objectEffect == permtypes.EFFECT_ALLOW && k.checkIfGroupPolicy(ctx, objectWithGroupID)) {
 		return permtypes.EFFECT_ALLOW
 	}
 	return permtypes.EFFECT_DENY
@@ -119,8 +130,8 @@ func (k Keeper) VerifyGroupPermission(ctx sdk.Context, groupInfo *types.GroupInf
 	}
 
 	// verify policy
-	effect := k.permKeeper.VerifyPolicy(ctx, groupInfo.Id, gnfdresource.RESOURCE_TYPE_GROUP, operator, action, nil)
-	if effect == permtypes.EFFECT_ALLOW {
+	effect, withGroupID := k.permKeeper.VerifyPolicy(ctx, groupInfo.Id, gnfdresource.RESOURCE_TYPE_GROUP, operator, action, nil)
+	if effect == permtypes.EFFECT_ALLOW && k.checkIfGroupPolicy(ctx, withGroupID) {
 		return permtypes.EFFECT_ALLOW
 	}
 

x/storage/types/expected_keepers.go

@@ -57,7 +57,7 @@ type PermissionKeeper interface {
 	DeletePolicy(ctx sdk.Context, principal *permtypes.Principal, resourceType resource.ResourceType,
 		resourceID math.Uint) (math.Uint, error)
 	VerifyPolicy(ctx sdk.Context, resourceID math.Uint, resourceType resource.ResourceType, operator sdk.AccAddress,
-		action permtypes.ActionType, opts *permtypes.VerifyOptions) permtypes.Effect
+		action permtypes.ActionType, opts *permtypes.VerifyOptions) (permtypes.Effect, math.Uint)
 	AddGroupMember(ctx sdk.Context, groupID math.Uint, member sdk.AccAddress) error
 	RemoveGroupMember(ctx sdk.Context, groupID math.Uint, member sdk.AccAddress) error
 	GetPolicyByID(ctx sdk.Context, policyID math.Uint) (*permtypes.Policy, bool)