mirror of
https://github.com/milvus-io/milvus.git
synced 2026-07-21 10:15:43 +00:00
fix: [RBAC] describe_user returns ghost empty role names after repeated grant/revoke (#50058)
- Skip malformed RBAC catalog entries whose parsed etcd key segments are empty or whitespace-only when listing roles, user-role mappings, grants, and policies, so describe_user no longer surfaces ghost empty role names from historical trailing-slash keys. - Keep the fix read-side only and inline with the existing invalid-key handling; write, replay, restore, and cleanup paths remain unchanged. - Add regression coverage for empty-segment RBAC keys across describe-user role lookup, ListRole, ListUserRole, ListGrant, and ListPolicy. related: #49989 Signed-off-by: shaoting-huang <shaoting.huang@zilliz.com>
This commit is contained in:
@@ -1269,7 +1269,7 @@ func (kc *Catalog) ListRole(ctx context.Context, tenant string, entity *milvuspb
|
||||
|
||||
for _, key := range keys {
|
||||
roleMappingInfos := typeutil.AfterN(key, roleMappingKey, "/")
|
||||
if len(roleMappingInfos) != 2 {
|
||||
if len(roleMappingInfos) != 2 || funcutil.IsEmptyString(roleMappingInfos[0]) || funcutil.IsEmptyString(roleMappingInfos[1]) {
|
||||
log.Ctx(ctx).Warn("invalid role mapping key", zap.String("string", key), zap.String("sub_string", roleMappingKey))
|
||||
continue
|
||||
}
|
||||
@@ -1306,7 +1306,7 @@ func (kc *Catalog) ListRole(ctx context.Context, tenant string, entity *milvuspb
|
||||
}
|
||||
for i, key := range keys {
|
||||
infoArr := typeutil.AfterN(key, roleKey, "/")
|
||||
if len(infoArr) != 1 || len(infoArr[0]) == 0 {
|
||||
if len(infoArr) != 1 || funcutil.IsEmptyString(infoArr[0]) {
|
||||
log.Ctx(ctx).Warn("invalid role key", zap.String("string", key), zap.String("sub_string", roleKey))
|
||||
continue
|
||||
}
|
||||
@@ -1342,7 +1342,7 @@ func (kc *Catalog) getRolesByUsername(ctx context.Context, tenant string, userna
|
||||
}
|
||||
for _, key := range keys {
|
||||
roleMappingInfos := typeutil.AfterN(key, k, "/")
|
||||
if len(roleMappingInfos) != 1 {
|
||||
if len(roleMappingInfos) != 1 || funcutil.IsEmptyString(roleMappingInfos[0]) {
|
||||
log.Ctx(ctx).Warn("invalid role mapping key", zap.String("string", key), zap.String("sub_string", k))
|
||||
continue
|
||||
}
|
||||
@@ -1671,7 +1671,7 @@ func (kc *Catalog) ListGrant(ctx context.Context, tenant string, entity *milvusp
|
||||
}
|
||||
for i, key := range keys {
|
||||
granteeIDInfos := typeutil.AfterN(key, granteeIDKey, "/")
|
||||
if len(granteeIDInfos) != 1 {
|
||||
if len(granteeIDInfos) != 1 || funcutil.IsEmptyString(granteeIDInfos[0]) {
|
||||
log.Ctx(ctx).Warn("invalid grantee id", zap.String("string", key), zap.String("sub_string", granteeIDKey))
|
||||
continue
|
||||
}
|
||||
@@ -1735,7 +1735,7 @@ func (kc *Catalog) ListGrant(ctx context.Context, tenant string, entity *milvusp
|
||||
}
|
||||
for i, key := range keys {
|
||||
grantInfos := typeutil.AfterN(key, granteeKey, "/")
|
||||
if len(grantInfos) != 2 {
|
||||
if len(grantInfos) != 2 || funcutil.IsEmptyString(grantInfos[0]) || funcutil.IsEmptyString(grantInfos[1]) {
|
||||
log.Ctx(ctx).Warn("invalid grantee key", zap.String("string", key), zap.String("sub_string", granteeKey))
|
||||
continue
|
||||
}
|
||||
@@ -1974,7 +1974,10 @@ func (kc *Catalog) ListPolicy(ctx context.Context, tenant string) ([]*milvuspb.G
|
||||
|
||||
for i, key := range keys {
|
||||
grantInfos := typeutil.AfterN(key, granteeKey, "/")
|
||||
if len(grantInfos) != 3 {
|
||||
if len(grantInfos) != 3 ||
|
||||
funcutil.IsEmptyString(grantInfos[0]) ||
|
||||
funcutil.IsEmptyString(grantInfos[1]) ||
|
||||
funcutil.IsEmptyString(grantInfos[2]) {
|
||||
log.Ctx(ctx).Warn("invalid grantee key", zap.String("string", key), zap.String("sub_string", granteeKey))
|
||||
continue
|
||||
}
|
||||
@@ -1986,7 +1989,7 @@ func (kc *Catalog) ListPolicy(ctx context.Context, tenant string) ([]*milvuspb.G
|
||||
}
|
||||
for _, idKey := range idKeys {
|
||||
granteeIDInfos := typeutil.AfterN(idKey, granteeIDKey, "/")
|
||||
if len(granteeIDInfos) != 1 {
|
||||
if len(granteeIDInfos) != 1 || funcutil.IsEmptyString(granteeIDInfos[0]) {
|
||||
log.Ctx(ctx).Warn("invalid grantee id", zap.String("string", idKey), zap.String("sub_string", granteeIDKey))
|
||||
continue
|
||||
}
|
||||
@@ -2023,7 +2026,7 @@ func (kc *Catalog) ListUserRole(ctx context.Context, tenant string) ([]string, e
|
||||
|
||||
for _, key := range keys {
|
||||
userRolesInfos := typeutil.AfterN(key, k, "/")
|
||||
if len(userRolesInfos) != 2 {
|
||||
if len(userRolesInfos) != 2 || funcutil.IsEmptyString(userRolesInfos[0]) || funcutil.IsEmptyString(userRolesInfos[1]) {
|
||||
log.Ctx(ctx).Warn("invalid user-role key", zap.String("string", key), zap.String("sub_string", k))
|
||||
continue
|
||||
}
|
||||
|
||||
@@ -4046,6 +4046,162 @@ func TestRBACPrefixMatch(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
func TestRBACReadSkipsEmptyKeySegments(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
tenant := util.DefaultTenant
|
||||
|
||||
t.Run("getRolesByUsername", func(t *testing.T) {
|
||||
username := "ai_voice"
|
||||
prefix := funcutil.HandleTenantForEtcdPrefix(RoleMappingPrefix, tenant, username)
|
||||
kvmock := mocks.NewTxnKV(t)
|
||||
c := NewCatalog(kvmock).(*Catalog)
|
||||
|
||||
kvmock.EXPECT().LoadWithPrefix(mock.Anything, prefix).Return(
|
||||
[]string{
|
||||
prefix + "default_db_rw",
|
||||
prefix,
|
||||
prefix + "kb_db_rw",
|
||||
},
|
||||
nil,
|
||||
nil,
|
||||
)
|
||||
|
||||
roles, err := c.getRolesByUsername(ctx, tenant, username)
|
||||
require.NoError(t, err)
|
||||
require.ElementsMatch(t, []string{"default_db_rw", "kb_db_rw"}, roles)
|
||||
require.NotContains(t, roles, "")
|
||||
})
|
||||
|
||||
t.Run("ListUserRole", func(t *testing.T) {
|
||||
prefix := funcutil.HandleTenantForEtcdPrefix(RoleMappingPrefix, tenant)
|
||||
kvmock := mocks.NewTxnKV(t)
|
||||
c := NewCatalog(kvmock)
|
||||
|
||||
kvmock.EXPECT().LoadWithPrefix(mock.Anything, prefix).Return(
|
||||
[]string{
|
||||
prefix + "user1/role1",
|
||||
prefix + "user1/",
|
||||
prefix + "/role2",
|
||||
},
|
||||
nil,
|
||||
nil,
|
||||
)
|
||||
|
||||
userRoles, err := c.ListUserRole(ctx, tenant)
|
||||
require.NoError(t, err)
|
||||
require.ElementsMatch(t, []string{"user1/role1"}, userRoles)
|
||||
})
|
||||
|
||||
t.Run("ListRole", func(t *testing.T) {
|
||||
roleName := "role1"
|
||||
prefix := funcutil.HandleTenantForEtcdPrefix(RoleMappingPrefix, tenant)
|
||||
kvmock := mocks.NewTxnKV(t)
|
||||
c := NewCatalog(kvmock)
|
||||
|
||||
kvmock.EXPECT().LoadWithPrefix(mock.Anything, prefix).Return(
|
||||
[]string{
|
||||
prefix + "user1/" + roleName,
|
||||
prefix + "/" + roleName,
|
||||
prefix + "user2/",
|
||||
},
|
||||
nil,
|
||||
nil,
|
||||
)
|
||||
kvmock.EXPECT().Load(mock.Anything, RolePrefix+"/"+roleName).Return("", nil)
|
||||
|
||||
roles, err := c.ListRole(ctx, tenant, &milvuspb.RoleEntity{Name: roleName}, true)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, roles, 1)
|
||||
require.Equal(t, roleName, roles[0].GetRole().GetName())
|
||||
require.Len(t, roles[0].GetUsers(), 1)
|
||||
require.Equal(t, "user1", roles[0].GetUsers()[0].GetName())
|
||||
})
|
||||
|
||||
t.Run("ListRoleAllRoles", func(t *testing.T) {
|
||||
prefix := funcutil.HandleTenantForEtcdPrefix(RolePrefix, tenant)
|
||||
kvmock := mocks.NewTxnKV(t)
|
||||
c := NewCatalog(kvmock)
|
||||
|
||||
kvmock.EXPECT().LoadWithPrefix(mock.Anything, prefix).Return(
|
||||
[]string{
|
||||
prefix + "role1",
|
||||
prefix + " ",
|
||||
},
|
||||
nil,
|
||||
nil,
|
||||
)
|
||||
|
||||
roles, err := c.ListRole(ctx, tenant, nil, false)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, roles, 1)
|
||||
require.Equal(t, "role1", roles[0].GetRole().GetName())
|
||||
})
|
||||
|
||||
t.Run("ListGrant", func(t *testing.T) {
|
||||
roleName := "role1"
|
||||
granteePrefix := funcutil.HandleTenantForEtcdPrefix(GranteePrefix, tenant, roleName)
|
||||
validIDPrefix := funcutil.HandleTenantForEtcdPrefix(GranteeIDPrefix, tenant, "grant-id")
|
||||
kvmock := mocks.NewTxnKV(t)
|
||||
c := NewCatalog(kvmock)
|
||||
|
||||
kvmock.EXPECT().LoadWithPrefix(mock.Anything, granteePrefix).Return(
|
||||
[]string{
|
||||
granteePrefix + "Collection/default.coll",
|
||||
granteePrefix + "Collection/",
|
||||
},
|
||||
[]string{"grant-id", "bad-id"},
|
||||
nil,
|
||||
)
|
||||
kvmock.EXPECT().LoadWithPrefix(mock.Anything, validIDPrefix).Return(
|
||||
[]string{validIDPrefix + "PrivilegeLoad", validIDPrefix},
|
||||
[]string{"root", "root"},
|
||||
nil,
|
||||
)
|
||||
|
||||
grants, err := c.ListGrant(ctx, tenant, &milvuspb.GrantEntity{
|
||||
Role: &milvuspb.RoleEntity{Name: roleName},
|
||||
DbName: util.AnyWord,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, grants, 1)
|
||||
require.Equal(t, roleName, grants[0].GetRole().GetName())
|
||||
require.Equal(t, "Collection", grants[0].GetObject().GetName())
|
||||
require.Equal(t, "coll", grants[0].GetObjectName())
|
||||
require.Equal(t, util.DefaultDBName, grants[0].GetDbName())
|
||||
require.Equal(t, util.PrivilegeNameForAPI("PrivilegeLoad"), grants[0].GetGrantor().GetPrivilege().GetName())
|
||||
})
|
||||
|
||||
t.Run("ListPolicy", func(t *testing.T) {
|
||||
granteePrefix := funcutil.HandleTenantForEtcdPrefix(GranteePrefix, tenant)
|
||||
validIDPrefix := funcutil.HandleTenantForEtcdPrefix(GranteeIDPrefix, tenant, "grant-id")
|
||||
kvmock := mocks.NewTxnKV(t)
|
||||
c := NewCatalog(kvmock)
|
||||
|
||||
kvmock.EXPECT().LoadWithPrefix(mock.Anything, granteePrefix).Return(
|
||||
[]string{
|
||||
granteePrefix + "role1/Collection/default.coll",
|
||||
granteePrefix + "role2/Collection/",
|
||||
},
|
||||
[]string{"grant-id", "bad-id"},
|
||||
nil,
|
||||
)
|
||||
kvmock.EXPECT().LoadWithPrefix(mock.Anything, validIDPrefix).Return(
|
||||
[]string{validIDPrefix + "PrivilegeLoad", validIDPrefix},
|
||||
nil,
|
||||
nil,
|
||||
)
|
||||
|
||||
policy, err := c.ListPolicy(ctx, tenant)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, policy, 1)
|
||||
require.Equal(t, "role1", policy[0].GetRole().GetName())
|
||||
require.Equal(t, "Collection", policy[0].GetObject().GetName())
|
||||
require.Equal(t, "coll", policy[0].GetObjectName())
|
||||
require.Equal(t, util.DefaultDBName, policy[0].GetDbName())
|
||||
require.Equal(t, util.PrivilegeNameForAPI("PrivilegeLoad"), policy[0].GetGrantor().GetPrivilege().GetName())
|
||||
})
|
||||
}
|
||||
|
||||
func TestCatalog_FileResource(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
mockErr := errors.New("mock error")
|
||||
|
||||
Reference in New Issue
Block a user