imbajin commented on code in PR #2899:
URL:
https://github.com/apache/incubator-hugegraph/pull/2899#discussion_r2489973445
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/GroupAPI.java:
##########
@@ -49,7 +49,7 @@
import jakarta.ws.rs.QueryParam;
import jakarta.ws.rs.core.Context;
-@Path("graphspaces/{graphspace}/graphs/{graph}/auth/groups")
+@Path("/auth/groups")
Review Comment:
**‼️ 关键问题: GroupAPI 路径变更导致破坏性变化**
`GroupAPI` 的路径从 `graphspaces/{graphspace}/graphs/{graph}/auth/groups` 改为
`/auth/groups`，这是一个不向后兼容的重大变更。这会破坏现有客户端的调用。
**建议:**
1. 如果这是有意为之的架构调整,需要在PR描述中明确说明这是破坏性变更
2. 考虑在API版本化策略中处理这种变更
3. 确保文档更新以反映这个变更
4. 检查是否需要保留旧的API端点以实现平滑迁移
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/LoginAPI.java:
##########
@@ -46,12 +45,11 @@
import jakarta.ws.rs.NotAuthorizedException;
import jakarta.ws.rs.POST;
import jakarta.ws.rs.Path;
-import jakarta.ws.rs.PathParam;
import jakarta.ws.rs.Produces;
import jakarta.ws.rs.core.Context;
import jakarta.ws.rs.core.HttpHeaders;
Review Comment:
**‼️ 关键问题: LoginAPI 路径变更可能破坏现有客户端**
路径从 `graphspaces/{graphspace}/graphs/{graph}/auth` 变更为
`/auth`，这会影响所有现有的登录和认证流程。
**影响:**
- 所有客户端的登录端点需要更新
- Token验证端点也发生变化
- 登出端点路径改变
**建议:**
确保这个变更已经过充分的影响评估，并有相应的迁移计划。
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java:
##########
@@ -0,0 +1,279 @@
+/*
+ * Copyright 2017 HugeGraph Authors
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hugegraph.api.auth;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.hugegraph.api.API;
+import org.apache.hugegraph.api.filter.StatusFilter;
+import org.apache.hugegraph.auth.AuthManager;
+import org.apache.hugegraph.auth.HugeGraphAuthProxy;
+import org.apache.hugegraph.auth.HugePermission;
+import org.apache.hugegraph.core.GraphManager;
+import org.apache.hugegraph.define.Checkable;
+import org.apache.hugegraph.util.E;
+import org.apache.hugegraph.util.Log;
+import org.slf4j.Logger;
+
+import com.codahale.metrics.annotation.Timed;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import io.swagger.v3.oas.annotations.tags.Tag;
+import jakarta.inject.Singleton;
+import jakarta.ws.rs.Consumes;
+import jakarta.ws.rs.DELETE;
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.POST;
+import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
+import jakarta.ws.rs.Produces;
+import jakarta.ws.rs.QueryParam;
+import jakarta.ws.rs.core.Context;
+
+@Path("graphspaces/{graphspace}/auth/managers")
+@Singleton
+@Tag(name = "ManagerAPI")
+public class ManagerAPI extends API {
+
+ private static final Logger LOG = Log.logger(ManagerAPI.class);
+
+ @POST
+ @Timed
+ @StatusFilter.Status(StatusFilter.Status.CREATED)
+ @Consumes(APPLICATION_JSON)
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ public String createManager(@Context GraphManager manager,
+ @PathParam("graphspace") String graphSpace,
+ JsonManager jsonManager) {
+ LOG.debug("Create manager: {}", jsonManager);
+ String user = jsonManager.user;
+ HugePermission type = jsonManager.type;
+ // graphSpace now comes from @PathParam instead of JsonManager
+
+ validType(type);
+ AuthManager authManager = manager.authManager();
+ validUser(authManager, user);
+
Review Comment:
**⚠️ 重要问题: ManagerAPI 缺少适当的错误处理**
在 `createManager` 方法中，多个验证方法（如 `validGraphSpace`,
`validPermission`）被调用，但没有看到统一的异常处理策略。
**建议:**
```suggestion
// 考虑添加更详细的错误消息和异常处理
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()));
}
```
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/GroupAPI.java:
##########
@@ -61,33 +61,29 @@ public class GroupAPI extends API {
@Status(Status.CREATED)
@Consumes(APPLICATION_JSON)
@Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"admin"})
Review Comment:
**⚠️ 重要问题: GroupAPI 新增 @RolesAllowed 注解但缺少一致性**
在 `GroupAPI` 中为所有方法添加了 `@RolesAllowed({"admin"})` 注解,但其他API（如 `AccessAPI`,
`BelongAPI`）却没有添加相应的权限控制注解。
**问题:**
1. 为什么只有 `GroupAPI` 需要admin权限而其他认证API不需要?
2. 这种不一致性是否符合设计意图?
**建议:**
- 统一所有认证相关API的权限控制策略
- 在代码注释中说明为什么 `GroupAPI` 需要特殊的权限要求
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/AccessAPI.java:
##########
@@ -64,15 +63,13 @@ public class AccessAPI extends API {
@Produces(APPLICATION_JSON_WITH_CHARSET)
public String create(@Context GraphManager manager,
@PathParam("graphspace") String graphSpace,
- @PathParam("graph") String graph,
JsonAccess jsonAccess) {
- LOG.debug("Graph [{}] create access: {}", graph, jsonAccess);
+ LOG.debug("GraphSpace [{}] create access: {}", graphSpace, jsonAccess);
checkCreatingBody(jsonAccess);
- HugeGraph g = graph(manager, graphSpace, graph);
HugeAccess access = jsonAccess.build();
access.id(manager.authManager().createAccess(access));
- return manager.serializer(g).writeAuthElement(access);
+ return manager.serializer().writeAuthElement(access);
Review Comment:
**⚠️ 重要问题: manager.serializer() 调用未验证返回值**
在所有API中，`manager.serializer()` 的调用从 `manager.serializer(g)` 改为
`manager.serializer()`，但没有看到对应的空值检查或错误处理。
**潜在风险:**
如果 `manager.serializer()` 返回 null，会导致 NPE
**建议:**
```suggestion
JsonSerializer serializer = manager.serializer();
E.checkNotNull(serializer, "Serializer must not be null");
return serializer.writeAuthElement(access);
```
##########
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)
Review Comment:
**⚠️ 重要问题: TargetAPI 缺少权限控制**
`TargetAPI` 移除了 `graph` 路径参数，但与 `GroupAPI` 不同，没有添加任何 `@RolesAllowed`
注解进行权限控制。
**安全隐患:**
Target 资源的创建、更新、删除操作没有明确的权限限制，可能导致未授权访问。
**建议:**
根据业务需求添加适当的权限控制注解，例如:
```suggestion
@RolesAllowed({"admin", "space-manager"})
public String create(@Context GraphManager manager,
```
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java:
##########
@@ -0,0 +1,279 @@
+/*
+ * Copyright 2017 HugeGraph Authors
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hugegraph.api.auth;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.hugegraph.api.API;
+import org.apache.hugegraph.api.filter.StatusFilter;
+import org.apache.hugegraph.auth.AuthManager;
+import org.apache.hugegraph.auth.HugeGraphAuthProxy;
+import org.apache.hugegraph.auth.HugePermission;
+import org.apache.hugegraph.core.GraphManager;
+import org.apache.hugegraph.define.Checkable;
+import org.apache.hugegraph.util.E;
+import org.apache.hugegraph.util.Log;
+import org.slf4j.Logger;
+
+import com.codahale.metrics.annotation.Timed;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import io.swagger.v3.oas.annotations.tags.Tag;
+import jakarta.inject.Singleton;
+import jakarta.ws.rs.Consumes;
+import jakarta.ws.rs.DELETE;
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.POST;
+import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
+import jakarta.ws.rs.Produces;
+import jakarta.ws.rs.QueryParam;
+import jakarta.ws.rs.core.Context;
+
+@Path("graphspaces/{graphspace}/auth/managers")
+@Singleton
+@Tag(name = "ManagerAPI")
+public class ManagerAPI extends API {
+
+ private static final Logger LOG = Log.logger(ManagerAPI.class);
+
+ @POST
+ @Timed
+ @StatusFilter.Status(StatusFilter.Status.CREATED)
+ @Consumes(APPLICATION_JSON)
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ public String createManager(@Context GraphManager manager,
+ @PathParam("graphspace") String graphSpace,
+ JsonManager jsonManager) {
+ LOG.debug("Create manager: {}", jsonManager);
+ String user = jsonManager.user;
+ HugePermission type = jsonManager.type;
+ // graphSpace now comes from @PathParam instead of JsonManager
+
+ validType(type);
+ AuthManager authManager = manager.authManager();
+ validUser(authManager, user);
+
+ String creator = HugeGraphAuthProxy.getContext().user().username();
+ switch (type) {
+ case SPACE:
Review Comment:
**⚠️ 重要问题: ManagerAPI 使用 switch-case 但缺少 break**
在 `createManager` 和 `delete` 方法中，switch-case 语句虽然每个 case 都有明确的逻辑，但代码可读性可以提升。
**建议:**
虽然当前代码功能正确，但考虑使用策略模式或提取方法来改善代码结构:
```suggestion
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 中调用这些方法，会使代码更易维护。
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java:
##########
@@ -0,0 +1,279 @@
+/*
+ * Copyright 2017 HugeGraph Authors
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hugegraph.api.auth;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.hugegraph.api.API;
+import org.apache.hugegraph.api.filter.StatusFilter;
+import org.apache.hugegraph.auth.AuthManager;
+import org.apache.hugegraph.auth.HugeGraphAuthProxy;
+import org.apache.hugegraph.auth.HugePermission;
+import org.apache.hugegraph.core.GraphManager;
+import org.apache.hugegraph.define.Checkable;
+import org.apache.hugegraph.util.E;
+import org.apache.hugegraph.util.Log;
+import org.slf4j.Logger;
+
+import com.codahale.metrics.annotation.Timed;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import io.swagger.v3.oas.annotations.tags.Tag;
+import jakarta.inject.Singleton;
+import jakarta.ws.rs.Consumes;
+import jakarta.ws.rs.DELETE;
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.POST;
+import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
+import jakarta.ws.rs.Produces;
+import jakarta.ws.rs.QueryParam;
+import jakarta.ws.rs.core.Context;
+
+@Path("graphspaces/{graphspace}/auth/managers")
+@Singleton
+@Tag(name = "ManagerAPI")
+public class ManagerAPI extends API {
+
+ private static final Logger LOG = Log.logger(ManagerAPI.class);
+
+ @POST
+ @Timed
+ @StatusFilter.Status(StatusFilter.Status.CREATED)
+ @Consumes(APPLICATION_JSON)
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ public String createManager(@Context GraphManager manager,
+ @PathParam("graphspace") String graphSpace,
+ JsonManager jsonManager) {
+ LOG.debug("Create manager: {}", jsonManager);
+ String user = jsonManager.user;
+ HugePermission type = jsonManager.type;
+ // graphSpace now comes from @PathParam instead of JsonManager
+
+ validType(type);
+ AuthManager authManager = manager.authManager();
+ validUser(authManager, user);
+
+ String creator = HugeGraphAuthProxy.getContext().user().username();
+ switch (type) {
+ case SPACE:
+ validGraphSpace(manager, graphSpace);
+ validPermission(
+ hasAdminOrSpaceManagerPerm(manager, graphSpace,
creator),
+ creator, "manager.create");
+ if (authManager.isSpaceMember(graphSpace, user)) {
+ authManager.deleteSpaceMember(graphSpace, user);
+ }
+ authManager.createSpaceManager(graphSpace, user);
+ break;
+ case SPACE_MEMBER:
+ validGraphSpace(manager, graphSpace);
+ validPermission(
+ hasAdminOrSpaceManagerPerm(manager, graphSpace,
creator),
+ creator, "manager.create");
+ if (authManager.isSpaceManager(graphSpace, user)) {
+ authManager.deleteSpaceManager(graphSpace, user);
+ }
+ authManager.createSpaceMember(graphSpace, user);
+ break;
+ case ADMIN:
+ validPermission(hasAdminPerm(manager, creator),
+ creator, "manager.create");
+ authManager.createAdminManager(user);
+ break;
+ default:
+ throw new IllegalArgumentException("Invalid type");
+ }
+
+ return manager.serializer()
+ .writeMap(ImmutableMap.of("user", user, "type", type,
+ "graphspace", graphSpace));
+ }
+
+ @DELETE
+ @Timed
+ @Consumes(APPLICATION_JSON)
+ public void delete(@Context GraphManager manager,
+ @PathParam("graphspace") String graphSpace,
+ @QueryParam("user") String user,
+ @QueryParam("type") HugePermission type) {
+ LOG.debug("Delete graph manager: {} {} {}", user, type, graphSpace);
+ E.checkArgument(!"admin".equals(user) ||
+ type != HugePermission.ADMIN,
+ "User 'admin' can't be removed from ADMIN");
+
+ AuthManager authManager = manager.authManager();
+ validType(type);
+ validUser(authManager, user);
+ String actionUser = HugeGraphAuthProxy.getContext().user().username();
+
+ switch (type) {
+ case SPACE:
+ // only space manager and admin can delete user permission
+ validGraphSpace(manager, graphSpace);
+ validPermission(
+ hasAdminOrSpaceManagerPerm(manager, graphSpace,
actionUser),
+ actionUser, "manager.delete");
+ authManager.deleteSpaceManager(graphSpace, user);
+ break;
+ case SPACE_MEMBER:
+ validGraphSpace(manager, graphSpace);
+ validPermission(
+ hasAdminOrSpaceManagerPerm(manager, graphSpace,
actionUser),
+ actionUser, "manager.delete");
+ authManager.deleteSpaceMember(graphSpace, user);
+ break;
+ case ADMIN:
+ validPermission(
+ hasAdminPerm(manager, actionUser),
+ actionUser, "manager.delete");
+ authManager.deleteAdminManager(user);
+ break;
+ default:
+ throw new IllegalArgumentException("Invalid type");
+ }
+ }
+
+ @GET
+ @Timed
+ @Consumes(APPLICATION_JSON)
+ public String list(@Context GraphManager manager,
+ @PathParam("graphspace") String graphSpace,
+ @QueryParam("type") HugePermission type) {
+ LOG.debug("list graph manager: {} {}", type, graphSpace);
+
+ AuthManager authManager = manager.authManager();
+ validType(type);
+ List<String> adminManagers;
+ switch (type) {
+ case SPACE:
+ validGraphSpace(manager, graphSpace);
+ adminManagers = authManager.listSpaceManager(graphSpace);
+ break;
+ case SPACE_MEMBER:
+ validGraphSpace(manager, graphSpace);
+ adminManagers = authManager.listSpaceMember(graphSpace);
+ break;
+ case ADMIN:
+ adminManagers = authManager.listAdminManager();
+ break;
+ default:
Review Comment:
**🧹 次要问题: 日志消息不一致**
在 `ManagerAPI.list` 方法中，返回值使用了 `writeList("admins", adminManagers)`，但列表名称为
"admins" 与实际的 managers 语义不符。
**建议:**
```suggestion
return manager.serializer().writeList("managers", adminManagers);
```
保持命名的一致性和准确性。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
