fix(server): remove graph path in auth api path by Tsukilc · Pull Request #2899 · apache/hugegraph

Tsukilc · 2025-11-04T08:05:02Z

as tilte

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/GroupAPI.java

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/LoginAPI.java

imbajin · 2025-11-04T10:55:52Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java

+        validType(type);
+        AuthManager authManager = manager.authManager();
+        validUser(authManager, user);
+


⚠️ 重要问题: ManagerAPI 缺少适当的错误处理

在 createManager 方法中，多个验证方法（如 validGraphSpace, validPermission）被调用，但没有看到统一的异常处理策略。

建议:

Suggested change

// 考虑添加更详细的错误消息和异常处理

try {

validGraphSpace(manager, graphSpace);

validPermission(

hasAdminOrSpaceManagerPerm(manager, graphSpace, creator),

creator, "manager.create");

} catch (IllegalArgumentException e) {

LOG.error("Failed to validate graph space: {}", graphSpace, e);

throw new BadRequestException(

String.format("Invalid graph space '%s': %s", graphSpace, e.getMessage()));

}

imbajin · 2025-11-04T10:55:53Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/GroupAPI.java

    @Status(Status.CREATED)
    @Consumes(APPLICATION_JSON)
    @Produces(APPLICATION_JSON_WITH_CHARSET)
+    @RolesAllowed({"admin"})


⚠️ 重要问题: GroupAPI 新增 @RolesAllowed 注解但缺少一致性

在 GroupAPI 中为所有方法添加了 @RolesAllowed({"admin"}) 注解,但其他API（如 AccessAPI, BelongAPI）却没有添加相应的权限控制注解。

问题:

为什么只有 GroupAPI 需要admin权限而其他认证API不需要?

这种不一致性是否符合设计意图?

建议:

统一所有认证相关API的权限控制策略

在代码注释中说明为什么 GroupAPI 需要特殊的权限要求

imbajin · 2025-11-04T10:55:55Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/AccessAPI.java

        HugeAccess access = jsonAccess.build();
        access.id(manager.authManager().createAccess(access));
-        return manager.serializer(g).writeAuthElement(access);
+        return manager.serializer().writeAuthElement(access);


⚠️ 重要问题: manager.serializer() 调用未验证返回值

在所有API中，manager.serializer() 的调用从 manager.serializer(g) 改为 manager.serializer()，但没有看到对应的空值检查或错误处理。

潜在风险:
如果 manager.serializer() 返回 null，会导致 NPE

建议:

Suggested change

return manager.serializer().writeAuthElement(access);

JsonSerializer serializer = manager.serializer();

E.checkNotNull(serializer, "Serializer must not be null");

return serializer.writeAuthElement(access);

imbajin · 2025-11-04T10:55:57Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/TargetAPI.java

@@ -64,15 +63,13 @@ public class TargetAPI extends API {
    @Produces(APPLICATION_JSON_WITH_CHARSET)


⚠️ 重要问题: TargetAPI 缺少权限控制

TargetAPI 移除了 graph 路径参数，但与 GroupAPI 不同，没有添加任何 @RolesAllowed 注解进行权限控制。

安全隐患:
Target 资源的创建、更新、删除操作没有明确的权限限制，可能导致未授权访问。

建议:
根据业务需求添加适当的权限控制注解，例如:

Suggested change

@Produces(APPLICATION_JSON_WITH_CHARSET)

@RolesAllowed({"admin", "space-manager"})

public String create(@Context GraphManager manager,

imbajin · 2025-11-04T10:55:58Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java

+
+        String creator = HugeGraphAuthProxy.getContext().user().username();
+        switch (type) {
+            case SPACE:


⚠️ 重要问题: ManagerAPI 使用 switch-case 但缺少 break

在 createManager 和 delete 方法中，switch-case 语句虽然每个 case 都有明确的逻辑，但代码可读性可以提升。

建议:
虽然当前代码功能正确，但考虑使用策略模式或提取方法来改善代码结构:

Suggested change

case SPACE:

private void handleSpaceManagerCreation(GraphManager manager, String graphSpace,

String user, String creator) {

validGraphSpace(manager, graphSpace);

validPermission(

hasAdminOrSpaceManagerPerm(manager, graphSpace, creator),

creator, "manager.create");

AuthManager authManager = manager.authManager();

if (authManager.isSpaceMember(graphSpace, user)) {

authManager.deleteSpaceMember(graphSpace, user);

}

authManager.createSpaceManager(graphSpace, user);

}

然后在 switch 中调用这些方法，会使代码更易维护。

imbajin · 2025-11-04T10:56:00Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java

+            case ADMIN:
+                adminManagers = authManager.listAdminManager();
+                break;
+            default:


🧹 次要问题: 日志消息不一致

在 ManagerAPI.list 方法中，返回值使用了 writeList("admins", adminManagers)，但列表名称为 "admins" 与实际的 managers 语义不符。

建议:

Suggested change

default:

return manager.serializer().writeList("managers", adminManagers);

保持命名的一致性和准确性。

imbajin · 2025-11-04T10:56:02Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java

+    @Timed
+    @StatusFilter.Status(StatusFilter.Status.CREATED)
+    @Consumes(APPLICATION_JSON)
+    @Produces(APPLICATION_JSON_WITH_CHARSET)


🧹 次要问题: 缺少输入参数验证

在 ManagerAPI.createManager 方法中，应该先验证 graphSpace 和 user 参数的有效性，然后再进行业务逻辑处理。

建议:

Suggested change

@Produces(APPLICATION_JSON_WITH_CHARSET)

public String createManager(@Context GraphManager manager,

@PathParam("graphspace") String graphSpace,

JsonManager jsonManager) {

LOG.debug("Create manager: {}", jsonManager);

E.checkArgumentNotNull(graphSpace, "Graph space cannot be null");

E.checkArgument(!graphSpace.trim().isEmpty(),

"Graph space cannot be empty");

String user = jsonManager.user;

HugePermission type = jsonManager.type;

// ... rest of the method

Copilot

Pull Request Overview

This PR refactors authentication-related APIs to remove graph-level path parameters and dependencies, shifting auth management from graph-scoped to graphspace-scoped. The changes also remove unnecessary permission checks for group operations and update serializer method calls to use a parameterless version.

Key changes:

Auth API endpoints (User, Project, Target, Group, Access, Belong, Manager, Login) now operate at the graphspace level instead of requiring a graph parameter
Permission verification removed from group CRUD operations in HugeGraphAuthProxy
Serializer calls changed from manager.serializer(g) to manager.serializer() across multiple API classes
Test paths updated to match new API structure

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
UserApiTest.java	Updated test path to remove graph segment from auth users endpoint
ProjectApiTest.java	Updated test path to remove graph segment from auth projects endpoint
ManagerApiTest.java	Updated test path to remove graph segment from auth users endpoint
LoginApiTest.java	Updated test paths to use simplified auth endpoints without graph parameters
HugeGraphAuthProxy.java	Removed permission verification calls for group operations
VerticesAPI.java	Changed serializer calls to use parameterless method
PersonalRankAPI.java	Changed serializer calls to use parameterless method
NeighborRankAPI.java	Changed serializer calls to use parameterless method
EdgesAPI.java	Changed serializer calls to use parameterless method
CountAPI.java	Changed serializer calls to use parameterless method
SchemaAPI.java	Changed serializer calls to use parameterless method
VertexLabelAPI.java	Changed serializer calls to use parameterless method
PropertyKeyAPI.java	Changed serializer calls to use parameterless method
IndexLabelAPI.java	Changed serializer calls to use parameterless method
EdgeLabelAPI.java	Changed serializer calls to use parameterless method
VertexAPI.java	Changed serializer calls to use parameterless method
EdgeAPI.java	Changed serializer calls to use parameterless method
UserAPI.java	Removed graph path parameter and HugeGraph dependency from all endpoints
TargetAPI.java	Removed graph path parameter and HugeGraph dependency; added toString method
ProjectAPI.java	Removed graph path parameter and HugeGraph dependency from all endpoints
ManagerAPI.java	New file implementing graphspace-scoped manager API
LoginAPI.java	Simplified to remove graph path parameters; now uses global /auth path
GroupAPI.java	Removed graph path parameter and added @RolesAllowed annotations
BelongAPI.java	Removed graph path parameter and HugeGraph dependency from all endpoints
AccessAPI.java	Removed graph path parameter; fixed log message typo

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-04T11:02:31Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/TargetAPI.java

+
+

[nitpick] Extra blank line should be removed to maintain consistent code formatting.

Suggested change

Copilot · 2025-11-04T11:02:32Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/AccessAPI.java

-                         @PathParam("graph") String graph,
                         JsonAccess jsonAccess) {
-        LOG.debug("Graph [{}] create access: {}", graph, jsonAccess);
+        LOG.debug("GraphSpace [{}] create access: {}", graphSpace, jsonAccess);


Default toString(): JsonAccess inherits toString() from Object, and so is not suitable for printing.

Copilot · 2025-11-04T11:02:32Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/AccessAPI.java

                         @PathParam("id") String id,
                         JsonAccess jsonAccess) {
-        LOG.debug("Graph [{}] update access: {}", graph, jsonAccess);
+        LOG.debug("GraphSpace [{}] update access: {}", graphSpace, jsonAccess);


Default toString(): JsonAccess inherits toString() from Object, and so is not suitable for printing.

Copilot · 2025-11-04T11:02:32Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/BelongAPI.java

-                         @PathParam("graph") String graph,
                         JsonBelong jsonBelong) {
-        LOG.debug("Graph [{}] create belong: {}", graph, jsonBelong);
+        LOG.debug("GraphSpace [{}] create belong: {}", graphSpace, jsonBelong);


Default toString(): JsonBelong inherits toString() from Object, and so is not suitable for printing.

Copilot · 2025-11-04T11:02:32Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/BelongAPI.java

                         @PathParam("id") String id,
                         JsonBelong jsonBelong) {
-        LOG.debug("Graph [{}] update belong: {}", graph, jsonBelong);
+        LOG.debug("GraphSpace [{}] update belong: {}", graphSpace, jsonBelong);


Default toString(): JsonBelong inherits toString() from Object, and so is not suitable for printing.

Copilot · 2025-11-04T11:02:33Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/LoginAPI.java

-                        @PathParam("graph") String graph,
                        JsonLogin jsonLogin) {
-        LOG.debug("Graph [{}] user login: {}", graph, jsonLogin);
+        LOG.debug("user login: {}", jsonLogin);


Default toString(): JsonLogin inherits toString() from Object, and so is not suitable for printing.

Copilot · 2025-11-04T11:02:33Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java

+    public String createManager(@Context GraphManager manager,
+                                @PathParam("graphspace") String graphSpace,
+                                JsonManager jsonManager) {
+        LOG.debug("Create manager: {}", jsonManager);


Default toString(): JsonManager inherits toString() from Object, and so is not suitable for printing.

Copilot · 2025-11-04T11:02:34Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ProjectAPI.java

-                         @PathParam("graph") String graph,
                         JsonProject jsonProject) {
-        LOG.debug("Graph [{}] create project: {}", graph, jsonProject);
+        LOG.debug("GraphSpace [{}] create project: {}", graphSpace, jsonProject);


Default toString(): JsonProject inherits toString() from Object, and so is not suitable for printing.

Copilot · 2025-11-04T11:02:34Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/UserAPI.java

-                         @PathParam("graph") String graph,
                         JsonUser jsonUser) {
-        LOG.debug("Graph [{}] create user: {}", graph, jsonUser);
+        LOG.debug("GraphSpace [{}] create user: {}", graphSpace, jsonUser);


Default toString(): JsonUser inherits toString() from Object, and so is not suitable for printing.

Copilot · 2025-11-04T11:02:34Z

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/UserAPI.java

                         @PathParam("id") String id,
                         JsonUser jsonUser) {
-        LOG.debug("Graph [{}] update user: {}", graph, jsonUser);
+        LOG.debug("GraphSpace [{}] update user: {}", graphSpace, jsonUser);


Default toString(): JsonUser inherits toString() from Object, and so is not suitable for printing.

imbajin

Merge it first, enhance it in another PR (FIXME/TODOs)

Tsukilc added 6 commits November 3, 2025 18:22

feat(server): add managerAPI

e1ff422

fix(server): remove graph in auth api path

44c8ba2

fix(server): remove graphspace in auth api path

80ae910

fix(server): fix login test

1068872

fix(server): fix login api

3bb00e4

fix(server): fix access api

2530d5b

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. api Changes of API labels Nov 4, 2025

fix(server): fix ai review

eec4168

imbajin requested a review from Copilot November 4, 2025 10:52

imbajin reviewed Nov 4, 2025

View reviewed changes

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/GroupAPI.java Show resolved Hide resolved

imbajin reviewed Nov 4, 2025

View reviewed changes

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/LoginAPI.java Show resolved Hide resolved

imbajin reviewed Nov 4, 2025

View reviewed changes

Copilot AI reviewed Nov 4, 2025

View reviewed changes

imbajin approved these changes Nov 4, 2025

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 4, 2025

Pengzna approved these changes Nov 4, 2025

View reviewed changes

VGalaxies approved these changes Nov 4, 2025

View reviewed changes

imbajin merged commit b7998c1 into apache:master Nov 4, 2025
19 checks passed

+// 考虑添加更详细的错误消息和异常处理
+try {
+    validGraphSpace(manager, graphSpace);
+    validPermission(
+            hasAdminOrSpaceManagerPerm(manager, graphSpace, creator),
+            creator, "manager.create");
+} catch (IllegalArgumentException e) {
+    LOG.error("Failed to validate graph space: {}", graphSpace, e);
+    throw new BadRequestException(
+        String.format("Invalid graph space '%s': %s", graphSpace, e.getMessage()));
+}

-        return manager.serializer().writeAuthElement(access);
+JsonSerializer serializer = manager.serializer();
+E.checkNotNull(serializer, "Serializer must not be null");
+return serializer.writeAuthElement(access);

		@@ -64,15 +63,13 @@ public class TargetAPI extends API {
		@Produces(APPLICATION_JSON_WITH_CHARSET)

	@Produces(APPLICATION_JSON_WITH_CHARSET)
	@RolesAllowed({"admin", "space-manager"})
	public String create(@Context GraphManager manager,

	default:
	return manager.serializer().writeList("managers", adminManagers);

-    @Produces(APPLICATION_JSON_WITH_CHARSET)
+public String createManager(@Context GraphManager manager,
+                            @PathParam("graphspace") String graphSpace,
+                            JsonManager jsonManager) {
+    LOG.debug("Create manager: {}", jsonManager);
+    E.checkArgumentNotNull(graphSpace, "Graph space cannot be null");
+    E.checkArgument(!graphSpace.trim().isEmpty(),
+                    "Graph space cannot be empty");
+    String user = jsonManager.user;
+    HugePermission type = jsonManager.type;
+    // ... rest of the method

Conversation

Tsukilc commented Nov 4, 2025

Uh oh!

Uh oh!

Uh oh!

imbajin Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

imbajin Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

imbajin Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

imbajin Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

imbajin Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

imbajin Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

imbajin Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

imbajin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants