fix grant/revoke v2 meta and unclear error messages [milvus-io#38110] […

…milvus-io#38130]

Signed-off-by: shaoting-huang <[email protected]>

configs/milvus.yaml

@@ -806,7 +806,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
@@ -818,9 +818,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
     tlsMode: 0
   session:
     ttl: 30 # ttl value when session granting a lease to register service

internal/proxy/impl.go

@@ -5254,21 +5254,27 @@ 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")
+		return merr.WrapErrParameterInvalidMsg("the role in the request is nil")
 	}
 	if err := ValidateRoleName(req.Role.Name); err != nil {
 		return err
 	}
 	if err := ValidatePrivilege(req.Grantor.Privilege.Name); err != nil {
 		return err
 	}
+	// validate built-in privilege group params
+	if err := ValidateBuiltInPrivilegeGroup(req.Grantor.Privilege.Name, req.DbName, req.CollectionName); err != nil {
+		return err
+	}
 	if req.Type != milvuspb.OperatePrivilegeType_Grant && req.Type != milvuspb.OperatePrivilegeType_Revoke {
-		return fmt.Errorf("the type in the request not grant or revoke")
+		return merr.WrapErrParameterInvalidMsg("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
@@ -5287,7 +5293,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)

internal/proxy/util.go

@@ -157,25 +157,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
@@ -925,7 +925,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 {
@@ -939,6 +939,31 @@ func ValidatePrivilege(entity string) error {
 	return validateName(entity, "Privilege")
 }
 
+func ValidateBuiltInPrivilegeGroup(entity string, dbName string, collectionName string) error {
+	if !util.IsBuiltinPrivilegeGroup(entity) {
+		return nil
+	}
+	switch {
+	case strings.HasPrefix(entity, milvuspb.PrivilegeLevel_Cluster.String()):
+		if !util.IsAnyWord(dbName) || !util.IsAnyWord(collectionName) {
+			return merr.WrapErrParameterInvalidMsg("dbName and collectionName should be * for the cluster level privilege: %s", entity)
+		}
+		return nil
+	case strings.HasPrefix(entity, milvuspb.PrivilegeLevel_Database.String()):
+		if collectionName != "" && collectionName != util.AnyWord {
+			return merr.WrapErrParameterInvalidMsg("collectionName should be * for the database level privilege: %s", entity)
+		}
+		return nil
+	case strings.HasPrefix(entity, milvuspb.PrivilegeLevel_Collection.String()):
+		if util.IsAnyWord(dbName) && !util.IsAnyWord(collectionName) && collectionName != "" {
+			return merr.WrapErrParameterInvalidMsg("please specify database name for the collection level privilege: %s", entity)
+		}
+		return nil
+	default:
+		return nil
+	}
+}
+
 func GetCurUserFromContext(ctx context.Context) (string, error) {
 	return contextutil.GetCurUserFromContext(ctx)
 }

internal/rootcoord/meta_table.go

@@ -1524,6 +1524,10 @@ func (mt *MetaTable) OperatePrivilegeGroup(groupName string, privileges []*milvu
 	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(groupName)
 	if err != nil {

internal/rootcoord/meta_table_test.go

@@ -2105,6 +2105,8 @@ func TestMetaTable_PrivilegeGroup(t *testing.T) {
 	assert.NoError(t, err)
 	err = mt.OperatePrivilegeGroup("", []*milvuspb.PrivilegeEntity{}, milvuspb.OperatePrivilegeGroupType_AddPrivilegesToGroup)
 	assert.Error(t, err)
+	err = mt.OperatePrivilegeGroup("ClusterReadOnly", []*milvuspb.PrivilegeEntity{}, milvuspb.OperatePrivilegeGroupType_AddPrivilegesToGroup)
+	assert.Error(t, err)
 	err = mt.OperatePrivilegeGroup("pg3", []*milvuspb.PrivilegeEntity{}, milvuspb.OperatePrivilegeGroupType_AddPrivilegesToGroup)
 	assert.Error(t, err)
 	_, err = mt.GetPrivilegeGroupRoles("")

internal/rootcoord/root_coord.go

@@ -22,7 +22,6 @@ import (
 	"math/rand"
 	"os"
 	"strconv"
-	"strings"
 	"sync"
 	"time"
 
@@ -2572,43 +2571,21 @@ func (c *Core) isValidPrivilege(ctx context.Context, privilegeName string, objec
 	return fmt.Errorf("not found the privilege name[%s] in object[%s]", privilegeName, object)
 }
 
-func (c *Core) isValidPrivilegeV2(ctx context.Context, privilegeName, dbName, collectionName string) error {
+func (c *Core) isValidPrivilegeV2(ctx context.Context, privilegeName string) error {
 	if util.IsAnyWord(privilegeName) {
 		return nil
 	}
-	var privilegeLevel string
-	for group, privileges := range util.BuiltinPrivilegeGroups {
-		if privilegeName == group || lo.Contains(privileges, privilegeName) {
-			privilegeLevel = group
-			break
-		}
-	}
-	if privilegeLevel == "" {
-		customPrivGroup, err := c.meta.IsCustomPrivilegeGroup(privilegeName)
-		if err != nil {
-			return err
-		}
-		if customPrivGroup {
-			return nil
-		}
-		return fmt.Errorf("not found the privilege name[%s] in the custom privilege groups", privilegeName)
+	customPrivGroup, err := c.meta.IsCustomPrivilegeGroup(privilegeName)
+	if err != nil {
+		return err
 	}
-	switch {
-	case strings.HasPrefix(privilegeLevel, milvuspb.PrivilegeLevel_Cluster.String()):
-		if !util.IsAnyWord(dbName) || !util.IsAnyWord(collectionName) {
-			return fmt.Errorf("dbName and collectionName should be * for the cluster level privilege: %s", privilegeName)
-		}
-		return nil
-	case strings.HasPrefix(privilegeLevel, milvuspb.PrivilegeLevel_Database.String()):
-		if collectionName != "" && collectionName != util.AnyWord {
-			return fmt.Errorf("collectionName should be empty or * for the database level privilege: %s", privilegeName)
-		}
-		return nil
-	case strings.HasPrefix(privilegeLevel, milvuspb.PrivilegeLevel_Collection.String()):
+	if customPrivGroup {
 		return nil
-	default:
+	}
+	if util.IsPrivilegeNameDefined(privilegeName) {
 		return nil
 	}
+	return fmt.Errorf("not found the privilege name[%s]", privilegeName)
 }
 
 // OperatePrivilege operate the privilege, including grant and revoke
@@ -2629,26 +2606,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); 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) {
@@ -3087,12 +3065,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(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")
@@ -3110,12 +3088,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(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")
@@ -3302,8 +3280,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")
@@ -3319,13 +3296,17 @@ func (c *Core) expandPrivilegeGroups(grants []*milvuspb.GrantEntity, groups map[
 		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{
@@ -3338,20 +3319,16 @@ func (c *Core) expandPrivilegeGroups(grants []*milvuspb.GrantEntity, groups map[
 
 	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

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()
 }

pkg/util/merr/errors.go

@@ -147,7 +147,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)

pkg/util/merr/utils.go

@@ -455,6 +455,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))