imbajin commented on code in PR #3159:
URL: https://github.com/apache/hugegraph/pull/3159#discussion_r3807403565
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -203,20 +205,25 @@ public String checkDefaultRole(@Context GraphManager
manager,
defaultRole.equals(HugeDefaultRole.SPACE)) {
throw new ForbiddenException("Forbidden to check role " + role);
}
- boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER);
- E.checkArgument(!hasGraph || StringUtils.isNotEmpty(graph),
- "Must set a graph for observer");
+ boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER) &&
+ StringUtils.isNotEmpty(graph);
if (hasGraph) {
validGraph(manager, name, graph);
}
boolean result;
if (hasGraph) {
- result = authManager.isDefaultRole(name, graph, user,
- defaultRole);
+ result = authManager.isDefaultRole(name, graph, user, defaultRole);
Review Comment:
‼️ Critical: A space-wide OBSERVER is persisted through
`createSpaceDefaultRole()` under the `ALL_GRAPHS` marker, but this
graph-specific branch checks only `isDefaultRole(name, graph, user,
defaultRole)`. A user with the new space-wide role is therefore reported as
`false` when the same check includes a concrete `graph` parameter; the parallel
`ManagerAPI` path has the same omission. Please treat the `ALL_GRAPHS` role as
covering the requested graph, and add a regression test for both endpoints.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java:
##########
@@ -359,8 +363,15 @@ public static boolean match(Object role, RolePermission
grant,
}
}
- RolePermission rolePerm = RolePermission.fromJson(role);
- return rolePerm.contains(grant);
+ RolePermission grantedRole = RolePermission.fromJson(grant);
+ RolePerm rolePerm = RolePerm.fromJson(role);
+ if (resourceObject != null &&
Review Comment:
⚠️ Important: This new authorization shortcut returns `true` when the
operator is a SPACE manager for the current graph space and the target grant
merely contains an entry for that space. `UserAPI.role()` then serializes the
complete `rolePermission(user)` object, so a `space-a` manager can read the
`space-b` entries of a multi-space grant. Please return a graph-space-scoped
projection (or reject mixed grants) before allowing this path, and add a
cross-space response-isolation test.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java:
##########
@@ -2461,4 +2518,36 @@ public String toString() {
return this.origin.toString();
}
}
+
+ private static Set<HugePermission> traversalPermissions(
+ Traversal.Admin<?, ?> traversal) {
+ Set<HugePermission> permissions = EnumSet.noneOf(HugePermission.class);
+ collectTraversalPermissions(traversal, permissions);
+ return permissions;
+ }
+
+ private static void collectTraversalPermissions(
+ Traversal.Admin<?, ?> traversal,
+ Set<HugePermission> permissions) {
+ for (Step<?, ?> step : traversal.getSteps()) {
+ if (step instanceof AddVertexStartStep ||
Review Comment:
‼️ Critical: The classifier only recognizes concrete `Add*`,
`AddPropertyStep`, and `DropStep` instances. A Groovy lambda such as
`g.V().sideEffect { it.get().property('k', 'v') }` contains none of these
steps, while `vertices()` returns raw graph elements whose `property()` mutates
the backend directly. An EXECUTE-only user can therefore bypass the new WRITE
check. Please reject or sandbox lambda mutations, or route element mutations
through an authorized proxy; add an execute-only strategy-level regression that
calls `apply()` rather than only reflecting over the classifier.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -259,16 +266,19 @@ public void deleteDefaultRole(@Context GraphManager
manager,
E.checkArgument(false, "Invalid role value '%s'", role);
defaultRole = null; // unreachable, satisfies compiler
}
- boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER);
- E.checkArgument(!hasGraph || StringUtils.isNotEmpty(graph),
- "Must set a graph for observer");
+ boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER) &&
StringUtils.isNotEmpty(graph);
if (hasGraph) {
validGraph(manager, name, graph);
}
if (hasGraph) {
authManager.deleteDefaultRole(name, user, defaultRole, graph);
} else {
authManager.deleteDefaultRole(name, user, defaultRole);
Review Comment:
⚠️ Important: Space-wide OBSERVER deletion removes the `ALL_GRAPHS` role
first and then performs independent per-graph deletes. If any later metadata
delete fails, the request returns an error while the global role is already
gone and some legacy graph-level grants remain active, leaving authorization
state partially migrated. Please make this migration transactional or
explicitly idempotent/resumable with compensation, and cover an injected
mid-loop failure.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java:
##########
@@ -2327,7 +2377,9 @@ public TraversalStrategiesProxy(TraversalStrategies
strategies) {
@Override
public List<TraversalStrategy<?>> toList() {
Review Comment:
⚠️ Important: TinkerPop's `TraversalStrategies.toList()` contract requires
an immutable list, and both the interface default and
`DefaultTraversalStrategies` wrap their result with
`Collections.unmodifiableList`. This override returns a mutable `ArrayList`,
allowing callers to add/remove strategy proxies and changing the public API
contract. Please return `Collections.unmodifiableList(proxies)` and add an
immutability regression test.
##########
.github/workflows/docker-build-ci.yml:
##########
@@ -24,10 +24,17 @@ on:
- 'release-*'
pull_request:
paths:
- - '**/Dockerfile*'
+ - '.github/workflows/docker-build-ci.yml'
- '.dockerignore'
- - 'hugegraph-server/hugegraph-dist/docker/**'
- - 'hugegraph-server/hugegraph-dist/src/assembly/static/bin/util.sh'
+ - '.mvn/**'
+ - 'pom.xml'
+ - 'hugegraph-commons/**'
Review Comment:
⚠️ Important: Removing the old `**/Dockerfile*` filter leaves the
repository's `docker/hbase/**` tree outside every `pull_request.paths` entry,
although it still contains a Dockerfile, entrypoint, and HBase configuration.
Changes to that image will no longer trigger Docker Build CI and can merge
without any image validation. Please retain `docker/hbase/**` in the trigger or
add a dedicated HBase build job with checks appropriate to that image.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -2434,19 +2428,14 @@ public static ConsumerWrapper wrap(Consumer consumer) {
@Override
public void accept(T t) {
- boolean grpcThread = false;
try {
- grpcThread = Thread.currentThread().getName().contains("grpc");
- if (grpcThread) {
- HugeGraphAuthProxy.setAdmin();
+ if (Thread.currentThread().getName().contains("grpc")) {
Review Comment:
‼️ Critical: The previous gRPC listener wrapper cleared authentication state
in `finally`; this path now scopes only `AuthContext` through `runAsAdmin()`.
`HugeGraphAuthProxy.CONTEXTS` is an `InheritableThreadLocal`, and
`REQUEST_GRAPH_SPACE` is not cleared here, so a reused or inherited listener
thread can retain a prior request identity/graph space after the callback or an
exception. Please clear or save/restore all auth thread-locals at this listener
boundary in `finally`, and add a thread-reuse/exception regression test.
--
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]
