chore: refine permission module for code quality (#122)

* add comments for permission verify

* format protobuf

* fix NewDefaultPolicyForGroupMember

* fix update group member error

* remove action_group_member

* remove some useless code

* fix group member duplication check

---------

Co-authored-by: Owen <owen.h@nodereal.io>

e2e/core/basesuite.go

@@ -85,6 +85,7 @@ func (s *BaseSuite) SendTxBlockWithExpectErrorString(msg sdk.Msg, from keys.KeyM
 	}
 	s.Client.SetKeyManager(from)
 	_, err := s.Client.BroadcastTx([]sdk.Msg{msg}, txOpt)
+	s.T().Logf("tx failed, err: %s, expect error string: %s", err, expectErrorString)
 	s.Require().Error(err)
 	s.Require().True(strings.Contains(err.Error(), expectErrorString))
 }

e2e/tests/permission_test.go

@@ -7,7 +7,6 @@ import (
 	"math"
 	"time"
 
-	sdkmath "cosmossdk.io/math"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/bnb-chain/greenfield/e2e/core"
@@ -632,9 +631,7 @@ func (s *StorageTestSuite) TestGrantsPermissionToGroup() {
 	headGroupMemberRequest := storagetypes.QueryHeadGroupMemberRequest{Member: user[1].GetAddr().String(), GroupOwner: user[0].GetAddr().String(), GroupName: testGroupName}
 	headGroupMemberResponse, err := s.Client.HeadGroupMember(ctx, &headGroupMemberRequest)
 	s.Require().NoError(err)
-	resGroupId, err := sdkmath.ParseUint(headGroupMemberResponse.GroupId)
-	s.Require().NoError(err)
-	s.Require().Equal(resGroupId, headGroupResponse.GetGroupInfo().Id)
+	s.Require().Equal(headGroupMemberResponse.GroupMember.GroupId, headGroupResponse.GetGroupInfo().Id)
 
 	// Put bucket policy
 	statement := &types.Statement{

e2e/tests/storage_test.go

@@ -215,13 +215,13 @@ func (s *StorageTestSuite) TestCreateGroup() {
 	}
 	queryHeadGroupMemberResp, err := s.Client.HeadGroupMember(ctx, &queryHeadGroupMemberReq)
 	s.Require().NoError(err)
-	s.Require().Equal(queryHeadGroupMemberResp.GroupId, queryHeadGroupResp.GroupInfo.Id.String())
+	s.Require().Equal(queryHeadGroupMemberResp.GroupMember.GroupId, queryHeadGroupResp.GroupInfo.Id)
 
 	// 4. UpdateGroupMember
 	member2 := s.GenAndChargeAccounts(1, 1000000)[0]
 	membersToAdd := []sdk.AccAddress{member2.GetAddr()}
 	membersToDelete := []sdk.AccAddress{member.GetAddr()}
-	msgUpdateGroupMember := storagetypes.NewMsgUpdateGroupMember(owner.GetAddr(), groupName, membersToAdd, membersToDelete)
+	msgUpdateGroupMember := storagetypes.NewMsgUpdateGroupMember(owner.GetAddr(), owner.GetAddr(), groupName, membersToAdd, membersToDelete)
 	s.SendTxBlock(msgUpdateGroupMember, owner)
 
 	// 5. HeadGroupMember (delete)
@@ -240,7 +240,11 @@ func (s *StorageTestSuite) TestCreateGroup() {
 	}
 	queryHeadGroupMemberRespAdd, err := s.Client.HeadGroupMember(ctx, &queryHeadGroupMemberReqAdd)
 	s.Require().NoError(err)
-	s.Require().Equal(queryHeadGroupMemberRespAdd.GroupId, queryHeadGroupResp.GroupInfo.Id.String())
+	s.Require().Equal(queryHeadGroupMemberRespAdd.GroupMember.GroupId, queryHeadGroupResp.GroupInfo.Id)
+
+	// 6. Create a group with the same name
+	msgCreateGroup = storagetypes.NewMsgCreateGroup(owner.GetAddr(), groupName, []sdk.AccAddress{member.GetAddr()})
+	s.SendTxBlockWithExpectErrorString(msgCreateGroup, owner, "exists")
 }
 
 func (s *StorageTestSuite) TestDeleteBucket() {

proto/greenfield/permission/common.proto

@@ -12,19 +12,19 @@ option go_package = "github.com/bnb-chain/greenfield/x/permission/types";
 enum ActionType {
   option (gogoproto.goproto_enum_prefix) = false;
 
-  ACTION_UPDATE_BUCKET_INFO = 0;
-  ACTION_DELETE_BUCKET = 1;
+  ACTION_UNSPECIFIED = 0;
+  ACTION_UPDATE_BUCKET_INFO = 1;
+  ACTION_DELETE_BUCKET = 2;
 
-  ACTION_CREATE_OBJECT = 2;
-  ACTION_DELETE_OBJECT = 3;
-  ACTION_COPY_OBJECT = 4;
-  ACTION_GET_OBJECT = 5;
-  ACTION_EXECUTE_OBJECT = 6;
-  ACTION_LIST_OBJECT = 7;
+  ACTION_CREATE_OBJECT = 3;
+  ACTION_DELETE_OBJECT = 4;
+  ACTION_COPY_OBJECT = 5;
+  ACTION_GET_OBJECT = 6;
+  ACTION_EXECUTE_OBJECT = 7;
+  ACTION_LIST_OBJECT = 8;
 
-  ACTION_UPDATE_GROUP_MEMBER = 8;
-  ACTION_DELETE_GROUP = 9;
-  ACTION_GROUP_MEMBER = 10;
+  ACTION_UPDATE_GROUP_MEMBER = 9;
+  ACTION_DELETE_GROUP = 10;
 
   ACTION_TYPE_ALL = 99;
 }
@@ -33,10 +33,9 @@ enum ActionType {
 enum Effect {
   option (gogoproto.goproto_enum_prefix) = false;
 
-  EFFECT_ALLOW = 0;
-  EFFECT_DENY = 1;
-  // Use internally, means skipped. there is no explicit ALLOW or DENY, and further permission checks are required.
-  EFFECT_PASS = 2;
+  EFFECT_UNSPECIFIED = 0;
+  EFFECT_ALLOW = 1;
+  EFFECT_DENY = 2;
 }
 
 message Statement {
@@ -50,24 +49,28 @@ message Statement {
   // However, if the sub-resource is defined as 'bucket/test_*,' in the statement, then only objects with a 'test_'
   // prefix can be accessed by the principal.
   repeated string resources = 3;
-  // expiration_time defines how long the permission is valid
+  // expiration_time defines how long the permission is valid. If not explicitly specified, it means it will not expire.
   google.protobuf.Timestamp expiration_time = 4 [
     (gogoproto.stdtime) = true,
     (gogoproto.nullable) = true
   ];
-  // limit_size defines the total data size that is allowed to operate
+  // limit_size defines the total data size that is allowed to operate. If not explicitly specified, it means it will not limit.
   common.UInt64Value limit_size = 5 [(gogoproto.nullable) = true];
 }
 
+// PrincipalType refers to the identity type of system users or entities.
+// In Greenfield, it usually refers to accounts or groups.
 enum PrincipalType {
   option (gogoproto.goproto_enum_prefix) = false;
-  // Reserved for extended use
-  TYPE_GNFD_ACCOUNT = 0;
-  TYPE_GNFD_GROUP = 1;
+  PRINCIPAL_TYPE_UNSPECIFIED = 0;
+  PRINCIPAL_TYPE_GNFD_ACCOUNT = 1;
+  PRINCIPAL_TYPE_GNFD_GROUP = 2;
 }
 
 // Principal define the roles that can grant permissions. Currently, it can be account or group.
 message Principal {
   PrincipalType type = 1;
+  // When the type is an account, its value is sdk.AccAddress().String();
+  // when the type is a group, its value is math.Uint().String()
   string value = 2;
 }

proto/greenfield/permission/params.proto

@@ -9,6 +9,10 @@ option go_package = "github.com/bnb-chain/greenfield/x/permission/types";
 message Params {
   option (gogoproto.goproto_stringer) = false;
 
+  // maximum_statements_num defines the maximum number of statements allowed in a policy
   uint64 maximum_statements_num = 1;
+  // maximum_group_num used to set the upper limit on the number of groups to which a resource can grant access permissions.
+  // By placing a cap on the number of group permissions, permission control policies can be made more robust and better
+  // enforced, thereby reducing the chances of DDos and other security incidents.
   uint64 maximum_group_num = 2;
 }

proto/greenfield/permission/types.proto

@@ -34,8 +34,6 @@ message Policy {
     (gogoproto.stdtime) = true,
     (gogoproto.nullable) = true
   ];
-  // member_statement defines a special policy which indicates that the principal is a member of the group
-  permission.Statement member_statement = 7;
 }
 
 // PolicyGroup refers to a group of policies which grant permission to Group, which is limited to MaxGroupNum (default 10).
@@ -57,3 +55,20 @@ message PolicyGroup {
   // items define a pair of policy_id and group_id. Each resource can only grant its own permissions to a limited number of groups
   repeated Item items = 1;
 }
+
+message GroupMember {
+  // id is an unique u256 sequence for each group member. It also be used as NFT tokenID
+  string id = 1 [
+    (cosmos_proto.scalar) = "cosmos.Uint",
+    (gogoproto.customtype) = "Uint",
+    (gogoproto.nullable) = false
+  ];
+  // group_id is the unique id of the group
+  string group_id = 2 [
+    (cosmos_proto.scalar) = "cosmos.Uint",
+    (gogoproto.customtype) = "Uint",
+    (gogoproto.nullable) = false
+  ];
+  // member is the account address of the member
+  string member = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"];
+}

proto/greenfield/resource/types.proto

@@ -9,7 +9,8 @@ option go_package = "github.com/bnb-chain/greenfield/types/resource";
 enum ResourceType {
   option (gogoproto.goproto_enum_prefix) = false;
 
-  RESOURCE_TYPE_BUCKET = 0;
-  RESOURCE_TYPE_OBJECT = 1;
-  RESOURCE_TYPE_GROUP = 2;
+  RESOURCE_TYPE_UNSPECIFIED = 0;
+  RESOURCE_TYPE_BUCKET = 1;
+  RESOURCE_TYPE_OBJECT = 2;
+  RESOURCE_TYPE_GROUP = 3;
 }

proto/greenfield/sp/types.proto

@@ -35,8 +35,6 @@ enum Status {
 }
 
 // StorageProvider defines the meta info of storage provider
-// TODO: add endpoint for RPC/HTTP/Websocket and p2p identity
-// TODO: add more account address for different role.
 message StorageProvider {
   option (gogoproto.equal) = false;
   option (gogoproto.goproto_stringer) = false;

proto/greenfield/storage/query.proto

@@ -91,7 +91,7 @@ service Query {
 
   // Queries a list of ListGroup items.
   rpc ListGroup(QueryListGroupRequest) returns (QueryListGroupResponse) {
-    option (google.api.http).get = "/bnb-chain/greenfield/storage/list_group";
+    option (google.api.http).get = "/greenfield/storage/list_group/{group_owner}";
   }
 
   // Queries a list of HeadGroupMember items.
@@ -226,7 +226,7 @@ message QueryHeadGroupMemberRequest {
 }
 
 message QueryHeadGroupMemberResponse {
-  string group_id = 1;
+  permission.GroupMember group_member = 1;
 }
 
 message QueryPolicyForGroupRequest {

proto/greenfield/storage/tx.proto

@@ -217,12 +217,14 @@ message MsgUpdateGroupMember {
 
   // operator is the account address of the operator who has the UpdateGroupMember permission of the group.
   string operator = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
+  // group_owner is the account address of the group owner
+  string group_owner = 2 [(cosmos_proto.scalar) = "cosmos.AddressString"];
   // group_name is the name of the group which to be updated
-  string group_name = 2;
+  string group_name = 3;
   // members_to_add is a list of members account address which will be add to the group
-  repeated string members_to_add = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"];
+  repeated string members_to_add = 4 [(cosmos_proto.scalar) = "cosmos.AddressString"];
   // members_to_delete is a list of members account address which will be remove from the group
-  repeated string members_to_delete = 4 [(cosmos_proto.scalar) = "cosmos.AddressString"];
+  repeated string members_to_delete = 5 [(cosmos_proto.scalar) = "cosmos.AddressString"];
 }
 
 message MsgUpdateGroupMemberResponse {}