From ca94755fb18deb551d780012c36d6f2ad345d77a Mon Sep 17 00:00:00 2001 From: Fynn Date: Wed, 2 Aug 2023 15:41:56 +0800 Subject: [PATCH] add group check when verify permission --- e2e/tests/permission_test.go | 115 ++++++++++++++++++++++++---- x/permission/keeper/keeper.go | 21 ++--- x/storage/keeper/keeper.go | 6 ++ x/storage/keeper/permission.go | 25 ++++-- x/storage/types/expected_keepers.go | 2 +- 5 files changed, 136 insertions(+), 33 deletions(-) diff --git a/e2e/tests/permission_test.go b/e2e/tests/permission_test.go index dbc2a4aee..38285a11e 100644 --- a/e2e/tests/permission_test.go +++ b/e2e/tests/permission_test.go @@ -5,11 +5,10 @@ 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" @@ -17,6 +16,10 @@ import ( "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) +} diff --git a/x/permission/keeper/keeper.go b/x/permission/keeper/keeper.go index d94e635ff..21a8b040e 100644 --- a/x/permission/keeper/keeper.go +++ b/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, diff --git a/x/storage/keeper/keeper.go b/x/storage/keeper/keeper.go index b6a350e03..383088411 100644 --- a/x/storage/keeper/keeper.go +++ b/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)) +} diff --git a/x/storage/keeper/permission.go b/x/storage/keeper/permission.go index 2ff0c2271..718216f7a 100644 --- a/x/storage/keeper/permission.go +++ b/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 } diff --git a/x/storage/types/expected_keepers.go b/x/storage/types/expected_keepers.go index 85b43af1f..b225eed5e 100644 --- a/x/storage/types/expected_keepers.go +++ b/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)