imbajin commented on code in PR #3159:
URL: https://github.com/apache/hugegraph/pull/3159#discussion_r3838617170
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java:
##########
@@ -2414,6 +2466,11 @@ public void apply(Traversal.Admin<?, ?> traversal) {
*/
String caller = Thread.currentThread().getName();
if (!caller.contains(TraversalStrategiesProxy.REST_WORKER)) {
+ for (HugePermission permission :
Review Comment:
‼️ Critical
Blocking: yes. Summary: The new traversal pre-check maps data mutations to
`ResourceType.GREMLIN` WRITE/DELETE, so existing roles with data-resource
WRITE/DELETE plus GREMLIN EXECUTE are rejected even though the public Gremlin
contract only grants `gremlin_execute`. Evidence: lines 2469-2475 call
`verifyNamePermission` with `ResourceType.GREMLIN`, while the existing role
fixtures grant WRITE on VERTEX and EXECUTE on GREMLIN; resource matching is
type-specific. Please check the actual graph data resource or introduce and
migrate explicit GREMLIN WRITE/DELETE permissions with compatibility tests.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedSchemaTransactionV2.java:
##########
@@ -467,6 +467,7 @@ public void clear() {
// Clear schema info firstly
super.clear();
this.clearCache(false);
+ this.notifySchemaCacheClear();
Review Comment:
⚠️ Important
Blocking: no. Summary: HStore clear now broadcasts `schema-cache-clear`
directly, and `storeProvider.clear()` immediately emits `STORE_CLEAR`, whose
listener calls `clearCache(true)` and broadcasts the same event again under the
default `task.sync_deletion=false`. Evidence: the new
`notifySchemaCacheClear()` at line 470 follows `clearCache(false)`, while the
existing STORE_CLEAR listener uses `notify = !STORE_INIT` and
`clearCache(true)`. Please make one layer own the broadcast or add a
clear-operation guard, and verify the event is emitted once.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java:
##########
@@ -178,13 +204,27 @@ public static void setRequestGraphSpace(String
graphSpace) {
REQUEST_GRAPH_SPACE.set(graphSpace);
}
- public static Context setAdmin() {
- Context old = getContext();
- AuthContext.useAdmin();
- return old;
+ public static void runAsAdmin(Runnable runnable) {
Review Comment:
‼️ Critical
Blocking: yes. Summary: This new public admin closure can be invoked by
untrusted Gremlin reflection unless it is explicitly hidden, and it changes the
caller seen by every operation in the closure to `User.ADMIN`. Evidence:
`runAsAdmin()` sets `AuthContext` to `User.ADMIN` at lines 207-211,
`getContext()` gives that context priority at lines 221-225, while
`HugeFactoryAuthProxy.registerPrivateActions()` filters
`getContext`/`resetContext` but does not filter `runAsAdmin`. Please make this
trusted bridge non-public or add it to the script method denylist, and add an
unauthorized Gremlin regression test.
##########
.github/workflows/docker-build-ci.yml:
##########
@@ -78,3 +86,34 @@ jobs:
echo "ERROR: no usable socket-table tool (ss/netstat) in ${{
matrix.dockerfile }}"
exit 1
}
+
+ - name: Server image API versions match source
+ if: ${{ startsWith(matrix.dockerfile, 'hugegraph-server/') }}
+ run: |
+ CHECK_DIR=$(mktemp -d)
+ trap 'rm -rf "$CHECK_DIR"' EXIT
+ docker run --rm --entrypoint bash \
+ -v "$CHECK_DIR:/check" "$IMAGE_ID" -c \
+ 'cp /hugegraph-server/lib/hugegraph-api-*.jar \
+ /hugegraph-server/lib/hugegraph-common-*.jar /check/'
+
+ API_JAR=$(find "$CHECK_DIR" -name 'hugegraph-api-*.jar' -print -quit)
+ COMMON_JAR=$(find "$CHECK_DIR" -name 'hugegraph-common-*.jar' -print
-quit)
+ EXPECTED_MANIFEST=$(sed -n \
+
's|.*<Implementation-Version>\([^<]*\)</Implementation-Version>.*|\1|p' \
+ hugegraph-server/hugegraph-api/pom.xml)
+ ACTUAL_MANIFEST=$(unzip -p "$API_JAR" META-INF/MANIFEST.MF |
+ sed -n 's/^Implementation-Version: *//p' | tr -d '\r')
+ EXPECTED_PROPERTY=$(sed -n 's/^ApiVersion=//p' \
+
hugegraph-commons/hugegraph-common/src/main/resources/version.properties)
+ ACTUAL_PROPERTY=$(unzip -p "$COMMON_JAR" version.properties |
+ sed -n 's/^ApiVersion=//p' | tr -d '\r')
+
+ [[ "$ACTUAL_MANIFEST" == "$EXPECTED_MANIFEST" ]] || {
Review Comment:
⚠️ Important
Blocking: no. Summary: This check validates the manifest and
`version.properties` against separate source literals, but it does not verify
the version returned by the runtime endpoint or that the two API version
representations are equivalent. Evidence: `ApiVersion.VERSION` prefers the API
JAR manifest and `VersionAPI` returns it, so this head can pass with manifest
`0.72.0.0` and property `0.72`; lines 112-118 compare each artifact
independently and never exercise `/versions`. Please compare normalized
canonical values and assert the packaged runtime API response as well.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -519,6 +519,9 @@ public void clearBackend() {
LockUtil.lock(this.spaceGraphName(), LockUtil.GRAPH_LOCK);
try {
+ if (this.isHstore()) {
+ ((CachedSchemaTransactionV2) this.schemaTransaction()).clear();
Review Comment:
‼️ Critical
Blocking: yes. Summary: The new HStore schema clear is protected only by
`GRAPH_LOCK`, but schema writes use independent per-type/per-id locks, so a
concurrent schema write can recreate metadata after `super.clear()` and leave
it inconsistent with the subsequently cleared store. Evidence:
`StandardHugeGraph.clearBackend()` calls `CachedSchemaTransactionV2.clear()` at
line 523, while `SchemaTransactionV2.saveSchema()` and `removeSchema()` acquire
only `hugeType2Group(schema.type())` locks. Please coordinate clear with the
same graph-wide/distributed schema-write lock and add a concurrent clear/write
regression.
##########
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);
+ if (defaultRole.equals(HugeDefaultRole.OBSERVER)) {
+ for (String currentGraph : manager.graphs(name)) {
Review Comment:
⚠️ Important
Blocking: yes. Summary: Space-wide OBSERVER cleanup only visits graphs
currently returned by `manager.graphs(name)`, so a legacy `<graph>_observer`
role for a graph deleted earlier is never removed and can become active again
if that graph name is recreated. Evidence: this loop is the only legacy-role
cleanup at lines 276-280, while `GraphManager.dropGraph()` removes graph config
before clearing the backend and does not remove the graph-scoped auth metadata.
Please enumerate legacy observer roles from auth metadata or clean them during
graph deletion, and cover delete/recreate with a user and group.
--
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]
