imbajin commented on code in PR #3008:
URL: https://github.com/apache/hugegraph/pull/3008#discussion_r3279774625
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -103,6 +107,161 @@ public Object get(@Context GraphManager manager,
return gsInfo;
}
+ @POST
+ @Timed
+ @Status(Status.CREATED)
+ @Consumes(APPLICATION_JSON)
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @Path("{graphspace}/role")
+ @RolesAllowed({"analyst"})
+ public String setDefaultRole(@Context GraphManager manager,
+ @PathParam("graphspace") String name,
+ JsonDefaultRole jsonRole) {
+ E.checkArgumentNotNull(jsonRole, "Request body cannot be null");
+ E.checkArgument(StringUtils.isNotEmpty(jsonRole.user),
+ "The 'user' field cannot be null or empty");
+ E.checkArgument(StringUtils.isNotEmpty(jsonRole.role),
+ "The 'role' field cannot be null or empty");
+ String user = jsonRole.user;
+ String graph = jsonRole.graph;
+ HugeDefaultRole role;
+ try {
+ role = HugeDefaultRole.valueOf(jsonRole.role.toUpperCase());
+ } catch (IllegalArgumentException e) {
+ E.checkArgument(false, "Invalid role value '%s'", jsonRole.role);
+ role = null; // unreachable, satisfies compiler
+ }
+ LOG.debug("Create default role: {} {} {}", user, role,
+ name);
+ AuthManager authManager;
+ try {
+ authManager = manager.authManager();
+ } catch (IllegalStateException e) {
+ throw new HugeException(STANDALONE_ERROR);
+ }
+ E.checkArgument(authManager.findUser(user) != null ||
+ authManager.findGroup(user) != null,
+ "The user or group is not exist");
+ // only admin can set space admin
+ if (!authManager.isAdminManager(HugeGraphAuthProxy.username()) &&
+ role.equals(HugeDefaultRole.SPACE)) {
+ throw new ForbiddenException("Forbidden to set role " +
role.toString());
+ }
+
+ boolean hasGraph = role.equals(HugeDefaultRole.OBSERVER);
+
+ E.checkArgument(!hasGraph || StringUtils.isNotEmpty(graph),
+ "Must set a graph for observer");
+
+ Map <String, String> result = new HashMap<>();
+ result.put("user", user);
+ result.put("role", jsonRole.role);
+ result.put("graphSpace", name);
+
+ if (hasGraph) {
+ authManager.createDefaultRole(name, user, role, graph);
Review Comment:
⚠️ **Validate graphspace and observer graph existence before default-role
operations**
These role handlers do not verify that the path graphspace exists before
calling the auth manager, and for `OBSERVER` they only check that `graph` is
non-empty before creating a graph-level default role. In PD mode, a typo in the
graphspace or graph name can create role/target/access/belong metadata under an
arbitrary key, and that metadata may become effective later if a graphspace or
graph with that name is created.
Please fail early with the same graphspace validation used by other
graphspace-scoped APIs, and validate the observer graph belongs to this
graphspace before create/check/delete.
##########
hugegraph-server/hugegraph-dist/src/main/java/org/apache/hugegraph/dist/HugeGraphServer.java:
##########
@@ -63,18 +63,33 @@ public HugeGraphServer(String gremlinServerConf, String
restServerConf)
PdMetaDriver.PDAuthConfig.setAuthority(
ServiceConstant.SERVICE_NAME,
ServiceConstant.AUTHORITY);
+
+ // Prepare GremlinServer (registers GRAPH_CREATE listener) BEFORE
+ // RestServer starts loading graphs from PD/meta. This ensures that
+ // graphs loaded during RestServer initialization are captured by
+ // ContextGremlinServer's listener and injected into Gremlin's
bindings.
+ GremlinServer preparedGremlinServer;
+ try {
+ preparedGremlinServer = HugeGremlinServer.prepare(
+ gremlinServerConf, graphsDir, hub);
+ } catch (Throwable e) {
+ LOG.error("HugeGremlinServer prepare error: ", e);
+ throw e;
+ }
+
try {
- // Start HugeRestServer
+ // Start HugeRestServer (loads graphs from PD; events go to
+ // ContextGremlinServer which is already listening)
this.restServer = HugeRestServer.start(restServerConf, hub);
Review Comment:
‼️ **Blocker: startup failures can leak process-wide state and the prepared
Gremlin server**
After `System.setSecurityManager(null)`, the original security manager is
only restored in the final `startPrepared()` `finally` block. If
`HugeGremlinServer.prepare(...)` or `HugeRestServer.start(...)` throws, the
constructor exits before that `finally`, leaving the process-wide
`SecurityManager` as `null`. Also, once `prepare()` succeeds, a later
`HugeRestServer.start(...)` failure rethrows without stopping the prepared
Gremlin server, so the listeners/resources created during preparation are left
behind.
Please wrap the whole startup sequence after disabling the security manager
in an outer `try/finally` that always restores it, and stop the prepared
Gremlin server when REST startup fails before `startPrepared()` is reached.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManagerV2.java:
##########
@@ -1510,6 +1516,211 @@ public HugeGroup findGroup(String name) {
return result;
}
+ @Override
+ public void setDefaultGraph(String graphSpace, String graph, String user) {
+ try {
+ String role = graph + DEFAULT_SETTER_ROLE_KEY;
+ String belongId = this.metaManager.belongId(user, role);
+ Id id = IdGenerator.of(belongId);
+ // Idempotent: if binding already exists, treat as success
+ if (this.metaManager.existBelong(graphSpace, id)) {
+ return;
+ }
+ HugeBelong belong = new HugeBelong(graphSpace,
+ IdGenerator.of(user),
+ IdGenerator.of(graph +
+
DEFAULT_SETTER_ROLE_KEY));
+ this.tryInitDefaultGraph(graphSpace, graph);
+ this.updateCreator(belong);
+ belong.create(belong.update());
+ this.metaManager.createBelong(graphSpace, belong);
+ this.invalidateUserCache();
+ } catch (Exception e) {
+ throw new HugeException("Exception occurs when " +
+ "set default graph", e);
+ }
+ }
+
+ @Override
+ public void unsetDefaultGraph(String graphSpace, String graph, String
user) {
+ String role = graph + DEFAULT_SETTER_ROLE_KEY;
+ String belongId = this.metaManager.belongId(user, role);
+ Id id = IdGenerator.of(belongId);
+ // Check if belong exists before attempting to delete to make this
operation idempotent
+ if (!this.metaManager.existBelong(graphSpace, id)) {
+ // Already unset, treat as success (idempotent behavior)
+ return;
+ }
+ try {
+ this.metaManager.deleteBelong(graphSpace, id);
+ this.invalidateUserCache();
+ } catch (Exception e) {
+ throw new HugeException("Exception occurs when unset default " +
+ "graph", e);
+ }
+ }
+
+ @Override
+ public Map<String, Date> getDefaultGraph(String graphSpace, String user) {
+ List<HugeBelong> belongs = this.listBelongBySource(graphSpace,
+
IdGenerator.of(user),
+ HugeBelong.UR, -1);
+ Map<String, Date> map = new HashMap<>();
+ for (HugeBelong belong : belongs) {
+ String role = belong.target().asString();
+ if (role.endsWith(DEFAULT_SETTER_ROLE_KEY) &&
+ role.length() != DEFAULT_SETTER_ROLE_KEY.length()) {
+ map.put(role.substring(0, role.lastIndexOf(
+ DEFAULT_SETTER_ROLE_KEY)), belong.update());
+ }
+ }
+ return map;
+ }
+
+ @Override
+ public Id createDefaultRole(String graphSpace, String owner,
+ HugeDefaultRole role, String graph) {
+ String roleName = (role.isGraphRole()) ?
+ getGraphDefaultRole(graph, role.toString()) : role.toString();
+ try {
+ HugeBelong belong;
+ String link;
+ if (HugeGroup.isGroup(owner)) {
Review Comment:
‼️ **Blocker: group default-role assignments are stored as user bindings**
`GraphSpaceAPI.setDefaultRole()` accepts either a user or a group
(`findUser(user) != null || findGroup(user) != null`), but this implementation
decides whether to create a group binding by calling
`HugeGroup.isGroup(owner)`. That helper only checks whether the string starts
with `group-`; normal group names such as `ops` are stored and looked up by
name, so they take the user-binding (`UR`) branch instead of the group-binding
(`GR`) branch.
This makes `POST /graphspaces/{gs}/role` appear to succeed for a group name,
but members of that group will not inherit the default role. Please determine
the owner type from the actual auth entity lookup, or pass an explicit
user/group distinction into the auth layer instead of inferring it from the
string prefix.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/GraphsAPI.java:
##########
@@ -155,6 +316,62 @@ public void drop(@Context GraphManager manager,
manager.dropGraph(graphSpace, name, true);
}
+ @PUT
+ @Timed
+ @Path("{name}")
+ @Consumes(APPLICATION_JSON)
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"space"})
+ public Map<String, String> manage(@Context GraphManager manager,
+ @Parameter(description = "The graph
space name")
+ @PathParam("graphspace") String
graphSpace,
+ @Parameter(description = "The graph
name")
+ @PathParam("name") String name,
+ @Parameter(description = "Action map:
{'action':'update','update':{...}}")
+ Map<String, Object> actionMap) {
+ LOG.debug("Manage graph '{}' with action '{}'", name, actionMap);
+ E.checkArgument(actionMap != null &&
actionMap.containsKey(GRAPH_ACTION),
+ "Invalid request body '%s'", actionMap);
+ Object value = actionMap.get(GRAPH_ACTION);
+ E.checkArgument(value instanceof String,
+ "Invalid action type '%s', must be string",
+ value == null ? "null" :
value.getClass().getSimpleName());
+ String action = (String) value;
+ switch (action) {
+ case UPDATE:
+ E.checkArgument(actionMap.containsKey(UPDATE),
+ "Please pass '%s' for graph update",
+ UPDATE);
+ value = actionMap.get(UPDATE);
+ E.checkArgument(value instanceof Map,
+ "The '%s' must be map, but got %s",
+ UPDATE,
+ value == null ? "null" :
value.getClass().getSimpleName());
+ @SuppressWarnings("unchecked")
+ Map<String, Object> graphMap = (Map<String, Object>) value;
+ String graphName = (String) graphMap.get("name");
+ E.checkArgument(graphName != null && graphName.equals(name),
+ "Different name in update body '%s' with path
'%s'",
+ graphName, name);
+ HugeGraph exist = graph(manager, graphSpace, name);
+ String nickname = (String) graphMap.get("nickname");
+ if (!StringUtils.isEmpty(nickname)) {
+ GraphManager.checkNickname(nickname);
+
E.checkArgument(!manager.isExistedGraphNickname(graphSpace, nickname) ||
+ nickname.equals(exist.nickname()),
+ "Nickname '%s' has already existed in
graphspace '%s'",
+ nickname, graphSpace);
+ // Delegate to GraphManager: handles both in-memory update
and
+ // PD-mode persistence (non-PD mode is in-memory only).
+ manager.updateGraphNickname(graphSpace, name, nickname);
+ }
+ return ImmutableMap.of(name, "updated");
+ default:
+ throw new AssertionError(String.format(
Review Comment:
⚠️ **Return 400 for malformed manage requests instead of 500**
This endpoint handles client-provided JSON, but an unknown `action`
currently throws `AssertionError`, which maps to a server error even though the
request is simply invalid. The same method also casts `graphMap.get("name")`
and `graphMap.get("nickname")` directly to `String`; values like `123` will
throw `ClassCastException` and surface as a 500.
Please use `E.checkArgument(false, ...)` for unsupported actions and
validate field types before casting so malformed client requests consistently
return a 4xx response.
--
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]
