fix: fix grant/revoke v2 meta and unclear error messages (#38110)

related issue: #37031 fixed issues: #37974: better error messages for grant v2 interface #37903: fix meta built-in privilege group object name #37843: better error messages for custom privilege group interface #38002: fix built-in privilege group meta to pass proxy interceptor check #38008: fix revoke v2 to support revoking v1 granted privileges Signed-off-by: shaoting-huang <[email protected]>
milvus-io · Dec 2, 2024 · 23dc313 · 23dc313
1 parent cfe5613
commit 23dc313
Show file tree

Hide file tree

Showing 10 changed files with 161 additions and 86 deletions.
diff --git a/configs/milvus.yaml b/configs/milvus.yaml
@@ -830,7 +830,7 @@ common:
         readwrite:
           privileges: ListDatabases,SelectOwnership,SelectUser,DescribeResourceGroup,ListResourceGroups,FlushAll,TransferNode,TransferReplica,UpdateResourceGroups # Cluster level readwrite privileges
         admin:
-          privileges: ListDatabases,SelectOwnership,SelectUser,DescribeResourceGroup,ListResourceGroups,FlushAll,TransferNode,TransferReplica,UpdateResourceGroups,BackupRBAC,RestoreRBAC,CreateDatabase,DropDatabase,CreateOwnership,DropOwnership,ManageOwnership,CreateResourceGroup,DropResourceGroup,UpdateUser # Cluster level admin privileges
+          privileges: ListDatabases,SelectOwnership,SelectUser,DescribeResourceGroup,ListResourceGroups,FlushAll,TransferNode,TransferReplica,UpdateResourceGroups,BackupRBAC,RestoreRBAC,CreateDatabase,DropDatabase,CreateOwnership,DropOwnership,ManageOwnership,CreateResourceGroup,DropResourceGroup,UpdateUser,RenameCollection # Cluster level admin privileges
       database:
         readonly:
           privileges: ShowCollections,DescribeDatabase # Database level readonly privileges
@@ -842,9 +842,9 @@ common:
         readonly:
           privileges: Query,Search,IndexDetail,GetFlushState,GetLoadState,GetLoadingProgress,HasPartition,ShowPartitions,DescribeCollection,DescribeAlias,GetStatistics,ListAliases # Collection level readonly privileges
         readwrite:
-          privileges: Query,Search,IndexDetail,GetFlushState,GetLoadState,GetLoadingProgress,HasPartition,ShowPartitions,DescribeCollection,DescribeAlias,GetStatistics,ListAliases,Load,Release,Insert,Delete,Upsert,Import,Flush,Compaction,LoadBalance,RenameCollection,CreateIndex,DropIndex,CreatePartition,DropPartition # Collection level readwrite privileges
+          privileges: Query,Search,IndexDetail,GetFlushState,GetLoadState,GetLoadingProgress,HasPartition,ShowPartitions,DescribeCollection,DescribeAlias,GetStatistics,ListAliases,Load,Release,Insert,Delete,Upsert,Import,Flush,Compaction,LoadBalance,CreateIndex,DropIndex,CreatePartition,DropPartition # Collection level readwrite privileges
         admin:
-          privileges: Query,Search,IndexDetail,GetFlushState,GetLoadState,GetLoadingProgress,HasPartition,ShowPartitions,DescribeCollection,DescribeAlias,GetStatistics,ListAliases,Load,Release,Insert,Delete,Upsert,Import,Flush,Compaction,LoadBalance,RenameCollection,CreateIndex,DropIndex,CreatePartition,DropPartition,CreateAlias,DropAlias # Collection level admin privileges
+          privileges: Query,Search,IndexDetail,GetFlushState,GetLoadState,GetLoadingProgress,HasPartition,ShowPartitions,DescribeCollection,DescribeAlias,GetStatistics,ListAliases,Load,Release,Insert,Delete,Upsert,Import,Flush,Compaction,LoadBalance,CreateIndex,DropIndex,CreatePartition,DropPartition,CreateAlias,DropAlias # Collection level admin privileges
     internaltlsEnabled: false
     tlsMode: 0
   session:

diff --git a/internal/proxy/impl.go b/internal/proxy/impl.go
@@ -5332,7 +5332,7 @@ func (node *Proxy) validPrivilegeParams(req *milvuspb.OperatePrivilegeRequest) e
 	return nil
 }
 
-func (node *Proxy) validOperatePrivilegeV2Params(req *milvuspb.OperatePrivilegeV2Request) error {
+func (node *Proxy) validateOperatePrivilegeV2Params(req *milvuspb.OperatePrivilegeV2Request) error {
 	if req.Role == nil {
 		return fmt.Errorf("the role in the request is nil")
 	}
@@ -5345,8 +5345,10 @@ func (node *Proxy) validOperatePrivilegeV2Params(req *milvuspb.OperatePrivilegeV
 	if req.Type != milvuspb.OperatePrivilegeType_Grant && req.Type != milvuspb.OperatePrivilegeType_Revoke {
 		return fmt.Errorf("the type in the request not grant or revoke")
 	}
-	if err := ValidateObjectName(req.DbName); err != nil {
-		return err
+	if req.DbName != "" && !util.IsAnyWord(req.DbName) {
+		if err := ValidateDatabaseName(req.DbName); err != nil {
+			return err
+		}
 	}
 	if err := ValidateObjectName(req.CollectionName); err != nil {
 		return err
@@ -5365,7 +5367,7 @@ func (node *Proxy) OperatePrivilegeV2(ctx context.Context, req *milvuspb.Operate
 	if err := merr.CheckHealthy(node.GetStateCode()); err != nil {
 		return merr.Status(err), nil
 	}
-	if err := node.validOperatePrivilegeV2Params(req); err != nil {
+	if err := node.validateOperatePrivilegeV2Params(req); err != nil {
 		return merr.Status(err), nil
 	}
 	curUser, err := GetCurUserFromContext(ctx)

diff --git a/internal/proxy/util.go b/internal/proxy/util.go
@@ -158,25 +158,25 @@ func validateCollectionNameOrAlias(entity, entityType string) error {
 
 func ValidatePrivilegeGroupName(groupName string) error {
 	if groupName == "" {
-		return merr.WrapErrDatabaseNameInvalid(groupName, "privilege group name couldn't be empty")
+		return merr.WrapErrPrivilegeGroupNameInvalid("privilege group name should not be empty")
 	}
 
 	if len(groupName) > Params.ProxyCfg.MaxNameLength.GetAsInt() {
-		return merr.WrapErrDatabaseNameInvalid(groupName,
-			fmt.Sprintf("the length of a privilege group name must be less than %d characters", Params.ProxyCfg.MaxNameLength.GetAsInt()))
+		return merr.WrapErrPrivilegeGroupNameInvalid(
+			"the length of a privilege group name %s must be less than %s characters", groupName, Params.ProxyCfg.MaxNameLength.GetValue())
 	}
 
 	firstChar := groupName[0]
 	if firstChar != '_' && !isAlpha(firstChar) {
-		return merr.WrapErrDatabaseNameInvalid(groupName,
-			"the first character of a privilege group name must be an underscore or letter")
+		return merr.WrapErrPrivilegeGroupNameInvalid(
+			"the first character of a privilege group name %s must be an underscore or letter", groupName)
 	}
 
 	for i := 1; i < len(groupName); i++ {
 		c := groupName[i]
 		if c != '_' && !isAlpha(c) && !isNumber(c) {
-			return merr.WrapErrDatabaseNameInvalid(groupName,
-				"privilege group name can only contain numbers, letters and underscores")
+			return merr.WrapErrParameterInvalidMsg(
+				"privilege group name %s can only contain numbers, letters and underscores", groupName)
 		}
 	}
 	return nil
@@ -1099,7 +1099,7 @@ func ValidateObjectName(entity string) error {
 	if util.IsAnyWord(entity) {
 		return nil
 	}
-	return validateName(entity, "role name")
+	return validateName(entity, "object name")
 }
 
 func ValidateObjectType(entity string) error {

diff --git a/internal/rootcoord/meta_table.go b/internal/rootcoord/meta_table.go
@@ -1577,6 +1577,10 @@ func (mt *MetaTable) OperatePrivilegeGroup(ctx context.Context, groupName string
 	mt.permissionLock.Lock()
 	defer mt.permissionLock.Unlock()
 
+	if util.IsBuiltinPrivilegeGroup(groupName) {
+		return merr.WrapErrParameterInvalidMsg("the privilege group name [%s] is defined by built in privilege groups in system", groupName)
+	}
+
 	// validate input params
 	definedByUsers, err := mt.IsCustomPrivilegeGroup(ctx, groupName)
 	if err != nil {

diff --git a/internal/rootcoord/meta_table_test.go b/internal/rootcoord/meta_table_test.go
@@ -2106,6 +2106,8 @@ func TestMetaTable_PrivilegeGroup(t *testing.T) {
 	assert.NoError(t, err)
 	err = mt.OperatePrivilegeGroup(context.TODO(), "", []*milvuspb.PrivilegeEntity{}, milvuspb.OperatePrivilegeGroupType_AddPrivilegesToGroup)
 	assert.Error(t, err)
+	err = mt.OperatePrivilegeGroup(context.TODO(), "ClusterReadOnly", []*milvuspb.PrivilegeEntity{}, milvuspb.OperatePrivilegeGroupType_AddPrivilegesToGroup)
+	assert.Error(t, err)
 	err = mt.OperatePrivilegeGroup(context.TODO(), "pg3", []*milvuspb.PrivilegeEntity{}, milvuspb.OperatePrivilegeGroupType_AddPrivilegesToGroup)
 	assert.Error(t, err)
 	_, err = mt.GetPrivilegeGroupRoles(context.TODO(), "")

diff --git a/internal/rootcoord/root_coord.go b/internal/rootcoord/root_coord.go
@@ -2645,26 +2645,27 @@ func (c *Core) OperatePrivilege(ctx context.Context, in *milvuspb.OperatePrivile
 		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_OperatePrivilegeFailure), nil
 	}
 
+	privName := in.Entity.Grantor.Privilege.Name
 	switch in.Version {
 	case "v2":
-		if err := c.isValidPrivilegeV2(ctx, in.Entity.Grantor.Privilege.Name,
-			in.Entity.DbName, in.Entity.ObjectName); err != nil {
+		if err := c.isValidPrivilegeV2(ctx, privName, in.Entity.DbName, in.Entity.ObjectName); err != nil {
 			ctxLog.Error("", zap.Error(err))
 			return merr.StatusWithErrorCode(err, commonpb.ErrorCode_OperatePrivilegeFailure), nil
 		}
+		// set up object type for metastore, to be compatible with v1 version
+		in.Entity.Object.Name = util.GetObjectType(privName)
 	default:
-		if err := c.isValidPrivilege(ctx, in.Entity.Grantor.Privilege.Name, in.Entity.Object.Name); err != nil {
+		if err := c.isValidPrivilege(ctx, privName, in.Entity.Object.Name); err != nil {
 			ctxLog.Error("", zap.Error(err))
 			return merr.StatusWithErrorCode(err, commonpb.ErrorCode_OperatePrivilegeFailure), nil
 		}
 		// set up object name if it is global object type and not built in privilege group
-		if in.Entity.Object.Name == commonpb.ObjectType_Global.String() && !lo.Contains(lo.Keys(util.BuiltinPrivilegeGroups), in.Entity.Grantor.Privilege.Name) {
+		if in.Entity.Object.Name == commonpb.ObjectType_Global.String() && !util.IsBuiltinPrivilegeGroup(in.Entity.Grantor.Privilege.Name) {
 			in.Entity.ObjectName = util.AnyWord
 		}
 	}
 
-	// set up privilege name for metastore
-	privName := in.Entity.Grantor.Privilege.Name
+	privName = in.Entity.Grantor.Privilege.Name
 
 	redoTask := newBaseRedoTask(c.stepExecutor)
 	redoTask.AddSyncStep(NewSimpleStep("operate privilege meta data", func(ctx context.Context) ([]nestedStep, error) {
@@ -3103,12 +3104,12 @@ func (c *Core) CreatePrivilegeGroup(ctx context.Context, in *milvuspb.CreatePriv
 	ctxLog.Debug(method)
 
 	if err := merr.CheckHealthy(c.GetStateCode()); err != nil {
-		return merr.Status(err), nil
+		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_CreatePrivilegeGroupFailure), nil
 	}
 
 	if err := c.meta.CreatePrivilegeGroup(ctx, in.GroupName); err != nil {
 		ctxLog.Warn("fail to create privilege group", zap.Error(err))
-		return merr.Status(err), nil
+		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_CreatePrivilegeGroupFailure), nil
 	}
 
 	ctxLog.Debug(method + " success")
@@ -3126,12 +3127,12 @@ func (c *Core) DropPrivilegeGroup(ctx context.Context, in *milvuspb.DropPrivileg
 	ctxLog.Debug(method)
 
 	if err := merr.CheckHealthy(c.GetStateCode()); err != nil {
-		return merr.Status(err), nil
+		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_DropPrivilegeGroupFailure), nil
 	}
 
 	if err := c.meta.DropPrivilegeGroup(ctx, in.GroupName); err != nil {
 		ctxLog.Warn("fail to drop privilege group", zap.Error(err))
-		return merr.Status(err), nil
+		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_DropPrivilegeGroupFailure), nil
 	}
 
 	ctxLog.Debug(method + " success")
@@ -3318,8 +3319,7 @@ func (c *Core) OperatePrivilegeGroup(ctx context.Context, in *milvuspb.OperatePr
 	if err != nil {
 		errMsg := "fail to execute task when operate privilege group"
 		ctxLog.Warn(errMsg, zap.Error(err))
-		status := merr.StatusWithErrorCode(errors.New(errMsg), commonpb.ErrorCode_OperatePrivilegeGroupFailure)
-		return status, nil
+		return merr.StatusWithErrorCode(err, commonpb.ErrorCode_OperatePrivilegeGroupFailure), nil
 	}
 
 	ctxLog.Debug(method + " success")
@@ -3335,13 +3335,17 @@ func (c *Core) expandPrivilegeGroups(ctx context.Context, grants []*milvuspb.Gra
 		if err != nil {
 			return nil, err
 		}
-		if objectType := util.GetObjectType(privilegeName); objectType != "" {
-			grant.Object.Name = objectType
+		objectType := &milvuspb.ObjectEntity{
+			Name: util.GetObjectType(privilegeName),
+		}
+		objectName := grant.ObjectName
+		if objectType.Name == commonpb.ObjectType_Global.String() {
+			objectName = util.AnyWord
 		}
 		return &milvuspb.GrantEntity{
 			Role:       grant.Role,
-			Object:     grant.Object,
-			ObjectName: grant.ObjectName,
+			Object:     objectType,
+			ObjectName: objectName,
 			Grantor: &milvuspb.GrantorEntity{
 				User: grant.Grantor.User,
 				Privilege: &milvuspb.PrivilegeEntity{
@@ -3354,20 +3358,16 @@ func (c *Core) expandPrivilegeGroups(ctx context.Context, grants []*milvuspb.Gra
 
 	for _, grant := range grants {
 		privName := grant.Grantor.Privilege.Name
-		if privGroup, exists := groups[privName]; !exists {
-			newGrant, err := createGrantEntity(grant, privName)
+		privGroup, exists := groups[privName]
+		if !exists {
+			privGroup = []*milvuspb.PrivilegeEntity{{Name: privName}}
+		}
+		for _, priv := range privGroup {
+			newGrant, err := createGrantEntity(grant, priv.Name)
 			if err != nil {
 				return nil, err
 			}
 			newGrants = append(newGrants, newGrant)
-		} else {
-			for _, priv := range privGroup {
-				newGrant, err := createGrantEntity(grant, priv.Name)
-				if err != nil {
-					return nil, err
-				}
-				newGrants = append(newGrants, newGrant)
-			}
 		}
 	}
 	// uniq by role + object + object name + grantor user + privilege name + db name

diff --git a/pkg/util/constant.go b/pkg/util/constant.go
@@ -332,7 +332,6 @@ var (
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeFlush.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeCompaction.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeLoadBalance.String()),
-		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeRenameCollection.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeCreateIndex.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeDropIndex.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeCreatePartition.String()),
@@ -384,6 +383,7 @@ var (
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeCreateResourceGroup.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeDropResourceGroup.String()),
 		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeUpdateUser.String()),
+		MetaStore2API(commonpb.ObjectPrivilege_PrivilegeRenameCollection.String()),
 	)
 )
 
@@ -475,5 +475,5 @@ func GetObjectType(privName string) string {
 			return objectType
 		}
 	}
-	return ""
+	return commonpb.ObjectType_Global.String()
 }
diff --git a/pkg/util/merr/errors.go b/pkg/util/merr/errors.go
@@ -148,7 +148,8 @@ var (
 	// this operation is denied because the user not authorized, user need to login in first
 	ErrPrivilegeNotAuthenticated = newMilvusError("not authenticated", 1400, false)
 	// this operation is denied because the user has no permission to do this, user need higher privilege
-	ErrPrivilegeNotPermitted = newMilvusError("privilege not permitted", 1401, false)
+	ErrPrivilegeNotPermitted     = newMilvusError("privilege not permitted", 1401, false)
+	ErrPrivilegeGroupInvalidName = newMilvusError("invalid privilege group name", 1402, false)
 
 	// Alias related
 	ErrAliasNotFound               = newMilvusError("alias not found", 1600, false)

diff --git a/pkg/util/merr/utils.go b/pkg/util/merr/utils.go
@@ -460,6 +460,14 @@ func WrapErrDatabaseNameInvalid(database any, msg ...string) error {
 	return err
 }
 
+func WrapErrPrivilegeGroupNameInvalid(privilegeGroup any, msg ...string) error {
+	err := wrapFields(ErrPrivilegeGroupInvalidName, value("privielgeGroup", privilegeGroup))
+	if len(msg) > 0 {
+		err = errors.Wrap(err, strings.Join(msg, "->"))
+	}
+	return err
+}
+
 // Collection related
 func WrapErrCollectionNotFound(collection any, msg ...string) error {
 	err := wrapFields(ErrCollectionNotFound, value("collection", collection))