From 52035dad9ee8d6b666329ca0d03950c773d3e1eb Mon Sep 17 00:00:00 2001 From: dark Date: Mon, 17 Aug 2026 03:31:51 +0800 Subject: [PATCH 01/23] chore(server): bump REST API version - identify the default-role REST contract as API 0.72 - preserve 1.7 releases at API 0.71 for client compatibility - document when the manifest version must change --- .../hugegraph-common/src/main/resources/version.properties | 2 +- hugegraph-server/hugegraph-api/pom.xml | 4 ++-- .../main/java/org/apache/hugegraph/version/ApiVersion.java | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/hugegraph-commons/hugegraph-common/src/main/resources/version.properties b/hugegraph-commons/hugegraph-common/src/main/resources/version.properties index 2dffc6f3a6..8d48ef39ca 100644 --- a/hugegraph-commons/hugegraph-common/src/main/resources/version.properties +++ b/hugegraph-commons/hugegraph-common/src/main/resources/version.properties @@ -17,7 +17,7 @@ # hugegraph-common follows the project version defined by ${revision} in the root pom.xml, # and VersionInBash needs to be updated in this file. Version=${revision} -ApiVersion=0.71 +ApiVersion=0.72 ApiCheckBeginVersion=1.0 ApiCheckEndVersion=2.0 VersionInBash=1.7.0 diff --git a/hugegraph-server/hugegraph-api/pom.xml b/hugegraph-server/hugegraph-api/pom.xml index f1a8b918bd..e5a81be208 100644 --- a/hugegraph-server/hugegraph-api/pom.xml +++ b/hugegraph-server/hugegraph-api/pom.xml @@ -201,8 +201,8 @@ - - 0.71.0.0 + + 0.72.0.0 diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/version/ApiVersion.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/version/ApiVersion.java index 7e314f9ed6..faadd1f5a6 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/version/ApiVersion.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/version/ApiVersion.java @@ -121,6 +121,7 @@ public final class ApiVersion { * [0.69] Issue-1748: Support Cypher query RESTful API * [0.70] PR-2242: Add edge-existence RESTful API * [0.71] PR-2286: Support Arthas API & Metric API prometheus format + * [0.72] Support GraphSpace default-role management APIs */ /** From 88a762af422f3ecf986252a2adf879a22f46df9a Mon Sep 17 00:00:00 2001 From: dark Date: Mon, 17 Aug 2026 05:05:22 +0800 Subject: [PATCH 02/23] fix(server): package current reactor artifacts - install reactor outputs before assembling Server images - isolate and lock Maven caches by source revision - verify packaged API versions against source in CI - run Docker CI for every reactor source change --- .github/workflows/docker-build-ci.yml | 47 ++++++++++++++++++++++++--- hugegraph-server/Dockerfile | 5 +-- hugegraph-server/Dockerfile-hstore | 5 +-- 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/.github/workflows/docker-build-ci.yml b/.github/workflows/docker-build-ci.yml index ada012be80..b1be078d91 100644 --- a/.github/workflows/docker-build-ci.yml +++ b/.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/**' + - 'hugegraph-cluster-test/**' + - 'hugegraph-pd/**' + - 'hugegraph-store/**' + - 'hugegraph-struct/**' + - 'hugegraph-server/**' + - 'install-dist/**' jobs: docker-build: @@ -47,7 +54,8 @@ jobs: - name: Build ${{ matrix.dockerfile }} run: | - IMAGE_ID=$(docker build -q -f ${{ matrix.dockerfile }} .) + IMAGE_ID=$(docker build -q --build-arg SOURCE_REVISION="$GITHUB_SHA" \ + -f ${{ matrix.dockerfile }} .) echo "Built: $IMAGE_ID" echo "IMAGE_ID=$IMAGE_ID" >> "$GITHUB_ENV" HC=$(docker inspect --format='{{json .Config.Healthcheck}}' "$IMAGE_ID") @@ -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|.*\([^<]*\).*|\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" ]] || { + echo "ERROR: API manifest is $ACTUAL_MANIFEST; expected $EXPECTED_MANIFEST" + exit 1 + } + [[ "$ACTUAL_PROPERTY" == "$EXPECTED_PROPERTY" ]] || { + echo "ERROR: API property is $ACTUAL_PROPERTY; expected $EXPECTED_PROPERTY" + exit 1 + } diff --git a/hugegraph-server/Dockerfile b/hugegraph-server/Dockerfile index 5caadd23cb..44bc9aa515 100644 --- a/hugegraph-server/Dockerfile +++ b/hugegraph-server/Dockerfile @@ -25,9 +25,10 @@ WORKDIR /pkg COPY . . ARG MAVEN_ARGS +ARG SOURCE_REVISION=local -RUN --mount=type=cache,target=/root/.m2 \ - mvn package $MAVEN_ARGS -e -B -ntp -Dmaven.test.skip=true -Dmaven.javadoc.skip=true \ +RUN --mount=type=cache,id=hugegraph-maven-${SOURCE_REVISION},target=/root/.m2,sharing=locked \ + mvn install $MAVEN_ARGS -e -B -ntp -Dmaven.test.skip=true -Dmaven.javadoc.skip=true \ && rm ./hugegraph-server/*.tar.gz ./hugegraph-pd/*.tar.gz ./hugegraph-store/*.tar.gz # 2nd stage: runtime env diff --git a/hugegraph-server/Dockerfile-hstore b/hugegraph-server/Dockerfile-hstore index 7cd64e8f3b..5d4d96d773 100644 --- a/hugegraph-server/Dockerfile-hstore +++ b/hugegraph-server/Dockerfile-hstore @@ -25,9 +25,10 @@ WORKDIR /pkg COPY . . ARG MAVEN_ARGS +ARG SOURCE_REVISION=local -RUN --mount=type=cache,target=/root/.m2 \ - mvn package $MAVEN_ARGS -e -B -ntp -DskipTests -Dmaven.javadoc.skip=true \ +RUN --mount=type=cache,id=hugegraph-maven-${SOURCE_REVISION},target=/root/.m2,sharing=locked \ + mvn install $MAVEN_ARGS -e -B -ntp -DskipTests -Dmaven.javadoc.skip=true \ && rm ./hugegraph-server/*.tar.gz ./hugegraph-pd/*.tar.gz ./hugegraph-store/*.tar.gz # 2nd stage: runtime env From 3d9d9544e3bcd7f2a29a0562f1a67b47a848d164 Mon Sep 17 00:00:00 2001 From: dark Date: Mon, 17 Aug 2026 10:01:22 +0800 Subject: [PATCH 03/23] fix(pd): validate raft peer addresses - normalize configured and runtime peer addresses - enforce DNS-aware IP authorization for raft traffic - refresh peer allowlists during membership changes - cover service updates and raft authorization integration --- .../apache/hugegraph/pd/raft/PeerUtil.java | 43 +- .../apache/hugegraph/pd/raft/RaftEngine.java | 148 ++++-- .../hugegraph/pd/raft/auth/IpAuthHandler.java | 429 ++++++++++++++++- hugegraph-pd/hg-pd-service/pom.xml | 12 + .../hugegraph/pd/service/PDService.java | 119 ++++- .../pd/service/PDServiceUpdateRaftTest.java | 195 ++++++++ .../hugegraph/pd/core/PDCoreSuiteTest.java | 2 +- .../hugegraph/pd/raft/IpAuthHandlerTest.java | 133 ------ .../raft/RaftEngineIpAuthIntegrationTest.java | 81 +++- .../pd/raft/auth/IpAuthHandlerTest.java | 439 ++++++++++++++++++ 10 files changed, 1375 insertions(+), 226 deletions(-) create mode 100644 hugegraph-pd/hg-pd-service/src/test/java/org/apache/hugegraph/pd/service/PDServiceUpdateRaftTest.java delete mode 100644 hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/IpAuthHandlerTest.java create mode 100644 hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandlerTest.java diff --git a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/PeerUtil.java b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/PeerUtil.java index 265c7d4fc2..bfffdf285c 100644 --- a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/PeerUtil.java +++ b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/PeerUtil.java @@ -17,15 +17,17 @@ package org.apache.hugegraph.pd.raft; -import com.alipay.sofa.jraft.JRaftUtils; -import com.alipay.sofa.jraft.entity.PeerId; -import org.apache.hugegraph.pd.common.KVPair; - import java.util.LinkedList; import java.util.List; import java.util.Objects; +import org.apache.hugegraph.pd.common.KVPair; + +import com.alipay.sofa.jraft.conf.Configuration; +import com.alipay.sofa.jraft.entity.PeerId; + public class PeerUtil { + public static boolean isPeerEquals(PeerId p1, PeerId p2) { if (p1 == null && p2 == null) { return true; @@ -40,19 +42,42 @@ public static List> parseConfig(String conf) { List> result = new LinkedList<>(); if (conf != null && conf.length() > 0) { - for (var s : conf.split(",")) { + for (var s : conf.split(",", -1)) { + String role; + String peer; if (s.endsWith("/leader")) { - result.add(new KVPair<>("leader", JRaftUtils.getPeerId(s.substring(0, s.length() - 7)))); + role = "leader"; + peer = s.substring(0, s.length() - 7); } else if (s.endsWith("/learner")) { - result.add(new KVPair<>("learner", JRaftUtils.getPeerId(s.substring(0, s.length() - 8)))); + role = "learner"; + peer = s.substring(0, s.length() - 8); } else if (s.endsWith("/follower")) { - result.add(new KVPair<>("follower", JRaftUtils.getPeerId(s.substring(0, s.length() - 9)))); + role = "follower"; + peer = s.substring(0, s.length() - 9); } else { - result.add(new KVPair<>("follower", JRaftUtils.getPeerId(s))); + role = "follower"; + peer = s; } + result.add(new KVPair<>(role, parsePeer(peer))); } } return result; } + + public static Configuration parsePeerList(String peerList) { + Configuration configuration = new Configuration(); + for (String peer : peerList.split(",", -1)) { + configuration.addPeer(parsePeer(peer)); + } + return configuration; + } + + private static PeerId parsePeer(String value) { + PeerId peer = new PeerId(); + if (value.isEmpty() || !peer.parse(value)) { + throw new IllegalArgumentException("Invalid Raft peer: " + value); + } + return peer; + } } diff --git a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java index 2b08de7d4e..81543ee1ef 100644 --- a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java +++ b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java @@ -127,14 +127,28 @@ public synchronized boolean init(PDConfig.Raft config) { final PeerId serverId = JRaftUtils.getPeerId(config.getAddress()); - rpcServer = createRaftRpcServer(config.getAddress(), initConf.getPeers()); - // construct raft group and start raft - this.raftGroupService = - new RaftGroupService(groupId, serverId, nodeOptions, rpcServer, true); - this.raftNode = raftGroupService.start(false); - log.info("RaftEngine start successfully: id = {}, peers list = {}", groupId, - nodeOptions.getInitialConf().getPeers()); - return this.raftNode != null; + try { + rpcServer = createRaftRpcServer(config.getAddress(), initConf.getPeers()); + // construct raft group and start raft + this.raftGroupService = + new RaftGroupService(groupId, serverId, nodeOptions, + rpcServer, true); + this.raftNode = raftGroupService.start(false); + if (this.raftNode == null) { + this.shutDown(); + return false; + } + log.info("RaftEngine start successfully: id = {}, peers list = {}", + groupId, nodeOptions.getInitialConf().getPeers()); + return true; + } catch (RuntimeException | Error e) { + try { + this.shutDown(); + } catch (RuntimeException | Error cleanupFailure) { + e.addSuppressed(cleanupFailure); + } + throw e; + } } /** @@ -143,13 +157,32 @@ public synchronized boolean init(PDConfig.Raft config) { private RpcServer createRaftRpcServer(String raftAddr, List peers) { Endpoint endpoint = JRaftUtils.getEndPoint(raftAddr); RpcServer rpcServer = RaftRpcServerFactory.createRaftRpcServer(endpoint); - configureRaftServerIpWhitelist(peers, rpcServer); - RaftRpcProcessor.registerProcessor(rpcServer, this); - rpcServer.init(null); - return rpcServer; + try { + IpAuthHandler ipAuthHandler = IpAuthHandler.getInstance( + peers.stream() + .map(PeerId::getIp) + .collect(Collectors.toSet())); + configureRaftServerIpWhitelist(ipAuthHandler, rpcServer); + RaftRpcProcessor.registerProcessor(rpcServer, this); + if (!rpcServer.init(null)) { + throw new IllegalStateException( + "Failed to initialize Raft RPC server"); + } + return rpcServer; + } catch (RuntimeException | Error e) { + try { + rpcServer.shutdown(); + } catch (RuntimeException | Error cleanupFailure) { + e.addSuppressed(cleanupFailure); + } finally { + IpAuthHandler.shutdownInstance(); + } + throw e; + } } - private static void configureRaftServerIpWhitelist(List peers, RpcServer rpcServer) { + private static void configureRaftServerIpWhitelist( + IpAuthHandler ipAuthHandler, RpcServer rpcServer) { if (rpcServer instanceof BoltRpcServer) { ((BoltRpcServer) rpcServer).getServer().option( BoltServerOption.EXTENDED_NETTY_CHANNEL_HANDLER, @@ -157,11 +190,7 @@ private static void configureRaftServerIpWhitelist(List peers, RpcServer @Override public List frontChannelHandlers() { return Collections.singletonList( - IpAuthHandler.getInstance( - peers.stream() - .map(PeerId::getIp) - .collect(Collectors.toSet()) - ) + ipAuthHandler ); } @@ -175,24 +204,38 @@ public List backChannelHandlers() { } public void shutDown() { - if (this.raftGroupService != null) { - this.raftGroupService.shutdown(); - try { - this.raftGroupService.join(); - } catch (final InterruptedException e) { - this.raftNode = null; - ThrowUtil.throwException(e); + InterruptedException interrupted = null; + try { + if (this.raftGroupService != null) { + this.raftGroupService.shutdown(); + try { + this.raftGroupService.join(); + } catch (InterruptedException e) { + interrupted = e; + } } + } finally { this.raftGroupService = null; + try { + if (this.rpcServer != null) { + this.rpcServer.shutdown(); + } + } finally { + this.rpcServer = null; + try { + if (this.raftNode != null) { + this.raftNode.shutdown(); + } + } finally { + this.raftNode = null; + IpAuthHandler.shutdownInstance(); + } + } } - if (this.rpcServer != null) { - this.rpcServer.shutdown(); - this.rpcServer = null; - } - if (this.raftNode != null) { - this.raftNode.shutdown(); + if (interrupted != null) { + Thread.currentThread().interrupt(); + ThrowUtil.throwException(interrupted); } - this.raftNode = null; } public boolean isLeader() { @@ -352,32 +395,43 @@ public List getMembers() throws ExecutionException, InterruptedEx public Status changePeerList(String peerList) { AtomicReference result = new AtomicReference<>(); - Configuration newPeers = new Configuration(); try { + IpAuthHandler.validatePeerListShape(peerList); String[] peers = peerList.split(",", -1); if ((peers.length & 1) != 1) { throw new PDException(-1, "the number of peer list must be odd."); } - newPeers.parse(peerList); + Configuration newPeers = PeerUtil.parsePeerList(peerList); + Set newIps = newPeers.getPeers() + .stream() + .map(PeerId::getIp) + .collect(Collectors.toSet()); + IpAuthHandler.validateAllowedEntries(newIps); + IpAuthHandler.requireActiveInstance(); CountDownLatch latch = new CountDownLatch(1); this.raftNode.changePeers(newPeers, status -> { - result.compareAndSet(null, status); - if (status != null && status.isOk()) { - IpAuthHandler handler = IpAuthHandler.getInstance(); - if (handler != null) { - Set newIps = newPeers.getPeers() - .stream() - .map(PeerId::getIp) - .collect(Collectors.toSet()); - handler.refresh(newIps); + Status callbackStatus = status; + try { + if (status != null && status.isOk()) { + IpAuthHandler.refreshInstance(newIps); log.info("IpAuthHandler refreshed after peer list change to: {}", peerList); - } else { - log.warn("IpAuthHandler not initialized, skipping refresh for " - + "peer list: {}", peerList); + } else if (status == null) { + callbackStatus = new Status( + RaftError.EINTERNAL, + "changePeers returned no status"); } + } catch (RuntimeException e) { + callbackStatus = new Status( + RaftError.EINTERNAL, + "Raft peers changed but allowlist refresh failed: %s", + e.getMessage()); + log.error("Failed to refresh IpAuthHandler after peer list change to {}", + peerList, e); + } finally { + result.compareAndSet(null, callbackStatus); + latch.countDown(); } - latch.countDown(); }); boolean completed = latch.await(3L * config.getRpcTimeout(), TimeUnit.MILLISECONDS); if (!completed && result.get() == null) { diff --git a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandler.java b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandler.java index bdccb6dd7f..e81c86ecdb 100644 --- a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandler.java +++ b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandler.java @@ -19,28 +19,119 @@ import java.net.InetAddress; import java.net.InetSocketAddress; -import java.net.UnknownHostException; +import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; +import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.concurrent.CancellationException; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import io.netty.channel.ChannelDuplexHandler; import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.nio.NioEventLoopGroup; +import io.netty.channel.socket.nio.NioDatagramChannel; +import io.netty.resolver.dns.DnsNameResolver; +import io.netty.resolver.dns.DnsNameResolverBuilder; import lombok.extern.slf4j.Slf4j; @Slf4j @ChannelHandler.Sharable public class IpAuthHandler extends ChannelDuplexHandler { + private static final long DNS_QUERY_TIMEOUT_MILLIS = 500L; + private static final long DNS_STALE_MILLIS = 30_000L; + private static final long DNS_REFRESH_MILLIS = 1_000L; + private static final int MAX_CONCURRENT_DNS_QUERIES = 8; + private static final int MAX_ALLOWED_ENTRIES = 127; + private static final int MAX_HOST_LENGTH = 253; + private static final int MAX_PEER_LIST_LENGTH = + MAX_ALLOWED_ENTRIES * (MAX_HOST_LENGTH + 16); + + private final HostResolver resolver; + private final long queryTimeoutMillis; + private final long staleMillis; + private final long refreshMillis; + private final Map resolvedByEntry; + private final Map inFlight; + private final Set failedEntries; + private final ScheduledExecutorService refreshExecutor; + private boolean closed; + private int nextResolutionIndex; + private List resolutionOrder; + private volatile Set allowedEntries; private volatile Set resolvedIps; private static volatile IpAuthHandler instance; private IpAuthHandler(Set allowedIps) { - this.resolvedIps = resolveAll(allowedIps); + this(allowedIps, new NettyHostResolver(DNS_QUERY_TIMEOUT_MILLIS), true, + DNS_QUERY_TIMEOUT_MILLIS, DNS_STALE_MILLIS, + DNS_REFRESH_MILLIS); + } + + IpAuthHandler(Set allowedIps, HostResolver resolver, + boolean scheduleRefresh, long queryTimeoutMillis, + long staleMillis, long refreshMillis) { + this.resolver = resolver; + this.queryTimeoutMillis = queryTimeoutMillis; + this.staleMillis = staleMillis; + this.refreshMillis = refreshMillis; + this.resolvedByEntry = new HashMap<>(); + this.inFlight = new HashMap<>(); + this.failedEntries = new HashSet<>(); + this.nextResolutionIndex = 0; + this.resolutionOrder = Collections.emptyList(); + try { + this.replaceAllowedEntries(allowedIps); + } catch (RuntimeException | Error e) { + try { + this.resolver.close(); + } catch (RuntimeException | Error cleanupFailure) { + e.addSuppressed(cleanupFailure); + } + throw e; + } + this.resolvedIps = this.allowedEntries; + this.closed = false; + if (scheduleRefresh) { + this.refreshExecutor = Executors.newSingleThreadScheduledExecutor(task -> { + Thread thread = new Thread(task, "pd-raft-dns-resolver"); + thread.setDaemon(true); + return thread; + }); + } else { + this.refreshExecutor = null; + } + try { + this.refreshResolvedIps(); + if (this.refreshExecutor != null) { + this.refreshExecutor.scheduleWithFixedDelay( + this::refreshSafely, this.refreshMillis, + this.refreshMillis, TimeUnit.MILLISECONDS); + } + } catch (RuntimeException | Error e) { + if (this.refreshExecutor != null) { + this.refreshExecutor.shutdownNow(); + } + try { + this.resolver.close(); + } catch (RuntimeException | Error cleanupFailure) { + e.addSuppressed(cleanupFailure); + } + throw e; + } } public static IpAuthHandler getInstance(Set allowedIps) { + validateAllowedEntries(allowedIps); if (instance == null) { synchronized (IpAuthHandler.class) { if (instance == null) { @@ -59,17 +150,48 @@ public static IpAuthHandler getInstance() { return instance; } + public static IpAuthHandler requireActiveInstance() { + IpAuthHandler handler = instance; + if (handler == null || handler.isClosed()) { + throw new IllegalStateException( + "Raft peer IP allowlist is not active"); + } + return handler; + } + + public static void refreshInstance(Set newAllowedIps) { + requireActiveInstance().refresh(newAllowedIps); + } + /** * Refreshes the resolved IP allowlist from a new set of hostnames or IPs. * Should be called when the Raft peer list changes via RaftEngine#changePeerList(). - * Note: DNS-only changes (e.g. container restart with new IP, same hostname) - * are not automatically detected and still require a process restart. + * DNS is also refreshed in the background so stable peer names can safely + * follow address changes without blocking a Netty event loop. */ - public void refresh(Set newAllowedIps) { - this.resolvedIps = resolveAll(newAllowedIps); + public synchronized void refresh(Set newAllowedIps) { + if (this.closed) { + throw new IllegalStateException( + "Raft peer IP allowlist is closed"); + } + this.replaceAllowedEntries(newAllowedIps); + this.resolvedByEntry.keySet().retainAll(this.allowedEntries); + this.failedEntries.retainAll(this.allowedEntries); + this.inFlight.entrySet().removeIf(entry -> { + if (!this.allowedEntries.contains(entry.getKey())) { + entry.getValue().cancel(); + return true; + } + return false; + }); + this.refreshResolvedIps(); log.info("IpAuthHandler allowlist refreshed, resolved {} entries", resolvedIps.size()); } + private synchronized boolean isClosed() { + return this.closed; + } + @Override public void channelActive(ChannelHandlerContext ctx) throws Exception { String clientIp = getClientIp(ctx); @@ -92,20 +214,301 @@ private boolean isIpAllowed(String ip) { return resolved.isEmpty() || resolved.contains(ip); } - private static Set resolveAll(Set entries) { - Set result = new HashSet<>(entries); + synchronized void refreshResolvedIps() { + this.refreshResolvedIps(true); + } + synchronized void refreshResolvedIps(boolean waitForResults) { + if (this.closed) { + return; + } + Set entries = this.allowedEntries; + this.collectQueries(entries, false); + int attempted = 0; + while (this.inFlight.size() < MAX_CONCURRENT_DNS_QUERIES && + attempted < this.resolutionOrder.size()) { + String entry = this.resolutionOrder.get(this.nextResolutionIndex); + this.nextResolutionIndex = + (this.nextResolutionIndex + 1) % this.resolutionOrder.size(); + attempted++; + if (!this.inFlight.containsKey(entry)) { + this.inFlight.put( + entry, new Query(this.resolver.resolve(entry), + System.nanoTime())); + } + } + this.collectQueries(entries, waitForResults); + + long staleNanos = TimeUnit.MILLISECONDS.toNanos(this.staleMillis); + long now = System.nanoTime(); + this.resolvedByEntry.entrySet().removeIf( + entry -> now - entry.getValue().resolvedAtNanos > staleNanos); + Set resolved = new HashSet<>(entries); + this.resolvedByEntry.values().forEach( + entry -> resolved.addAll(entry.addresses)); + this.resolvedIps = Collections.unmodifiableSet(resolved); + } + + private void collectQueries(Set entries, + boolean waitForResults) { + long deadline = System.nanoTime() + + TimeUnit.MILLISECONDS.toNanos(this.queryTimeoutMillis); for (String entry : entries) { + Query query = this.inFlight.get(entry); + if (query == null) { + continue; + } + CompletableFuture future = query.future; try { - for (InetAddress addr : InetAddress.getAllByName(entry)) { - result.add(addr.getHostAddress()); + ResolvedQuery result; + if (future.isDone()) { + result = future.get(); + } else if (waitForResults) { + long remaining = deadline - System.nanoTime(); + if (remaining <= 0L) { + expireQuery(entry, query); + continue; + } + result = future.get(remaining, TimeUnit.NANOSECONDS); + } else { + long elapsed = System.nanoTime() - query.startedAtNanos; + if (elapsed > TimeUnit.MILLISECONDS.toNanos( + this.queryTimeoutMillis)) { + expireQuery(entry, query); + } + continue; + } + if (result.completedAtNanos - query.startedAtNanos > + TimeUnit.MILLISECONDS.toNanos(this.queryTimeoutMillis)) { + expireQuery(entry, query); + continue; + } + this.resolvedByEntry.put( + entry, new ResolvedEntry(result.addresses, + System.nanoTime())); + this.inFlight.remove(entry); + if (this.failedEntries.remove(entry)) { + log.info("Raft peer address resolution recovered for '{}'", entry); } - } catch (UnknownHostException e) { - log.warn("Could not resolve allowlist entry '{}': {}", entry, e.getMessage()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + markResolutionFailure(entry, e); + throw new IllegalStateException( + "Raft peer address refresh interrupted", e); + } catch (ExecutionException e) { + this.inFlight.remove(entry); + markResolutionFailure(entry, e); + } catch (TimeoutException e) { + expireQuery(entry, query); + } catch (CancellationException e) { + this.inFlight.remove(entry); + markResolutionFailure(entry, e); + } + } + } + + private void expireQuery(String entry, Query query) { + query.cancel(); + this.inFlight.remove(entry); + markResolutionFailure( + entry, new TimeoutException("DNS refresh deadline")); + } + + private void markResolutionFailure(String entry, Exception failure) { + if (this.failedEntries.add(entry)) { + log.warn("Could not resolve Raft peer allowlist entry '{}': {}", + entry, failure.getMessage()); + } + } + + private void refreshSafely() { + try { + this.refreshResolvedIps(false); + } catch (RuntimeException e) { + log.error("Unexpected Raft peer allowlist refresh failure", e); + } + } + + private void replaceAllowedEntries(Set entries) { + validateAllowedEntries(entries); + Set copy = new HashSet<>(entries); + if (copy.equals(this.allowedEntries)) { + return; + } + this.allowedEntries = Collections.unmodifiableSet(copy); + this.resolutionOrder = new ArrayList<>(copy); + Collections.sort(this.resolutionOrder); + this.nextResolutionIndex = 0; + } + + public static void validateAllowedEntries(Set entries) { + if (entries.size() > MAX_ALLOWED_ENTRIES) { + throw new IllegalArgumentException( + "Raft peer allowlist exceeds " + MAX_ALLOWED_ENTRIES + + " entries"); + } + for (String entry : entries) { + if (entry == null || entry.isEmpty() || + entry.length() > MAX_HOST_LENGTH) { + throw new IllegalArgumentException( + "Invalid Raft peer allowlist entry"); } } + } + + public static void validatePeerListShape(String peerList) { + if (peerList == null || peerList.isEmpty() || + peerList.length() > MAX_PEER_LIST_LENGTH) { + throw new IllegalArgumentException( + "Invalid Raft peer list length"); + } + int entries = 1; + for (int i = 0; i < peerList.length(); i++) { + if (peerList.charAt(i) == ',' && + ++entries > MAX_ALLOWED_ENTRIES) { + throw new IllegalArgumentException( + "Raft peer list exceeds " + MAX_ALLOWED_ENTRIES + + " entries"); + } + } + } + + synchronized void shutdown() { + if (this.closed) { + return; + } + this.closed = true; + if (this.refreshExecutor != null) { + this.refreshExecutor.shutdownNow(); + } + this.inFlight.values().forEach(Query::cancel); + this.inFlight.clear(); + this.resolver.close(); + } + + public static synchronized void shutdownInstance() { + if (instance != null) { + instance.shutdown(); + instance = null; + } + } + + @FunctionalInterface + interface HostResolver extends AutoCloseable { + + CompletableFuture> resolve(String host); + + @Override + default void close() { + // Most injected resolvers do not own resources. + } + } + + private static final class ResolvedEntry { + + private final Set addresses; + private final long resolvedAtNanos; + + private ResolvedEntry(Set addresses, + long resolvedAtNanos) { + this.addresses = addresses; + this.resolvedAtNanos = resolvedAtNanos; + } + } + + private static final class Query { + + private final CompletableFuture> source; + private final CompletableFuture future; + private final long startedAtNanos; - return Collections.unmodifiableSet(result); + private Query(CompletableFuture> source, + long startedAtNanos) { + this.source = source; + this.startedAtNanos = startedAtNanos; + this.future = source.thenApply( + addresses -> new ResolvedQuery(addresses, + System.nanoTime())); + } + + private void cancel() { + this.source.cancel(true); + this.future.cancel(true); + } + } + + private static final class ResolvedQuery { + + private final Set addresses; + private final long completedAtNanos; + + private ResolvedQuery(Set addresses, + long completedAtNanos) { + this.addresses = addresses; + this.completedAtNanos = completedAtNanos; + } + } + + private static final class NettyHostResolver implements HostResolver { + + private final NioEventLoopGroup eventLoopGroup; + private final DnsNameResolver resolver; + + private NettyHostResolver(long queryTimeoutMillis) { + this.eventLoopGroup = new NioEventLoopGroup(1, task -> { + Thread thread = new Thread(task, "pd-raft-dns-event-loop"); + thread.setDaemon(true); + return thread; + }); + try { + this.resolver = new DnsNameResolverBuilder( + this.eventLoopGroup.next()) + .channelType(NioDatagramChannel.class) + .ttl(0, 1) + .negativeTtl(0) + .queryTimeoutMillis(queryTimeoutMillis) + .build(); + } catch (RuntimeException | Error e) { + this.eventLoopGroup.shutdownGracefully( + 0L, DNS_QUERY_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS) + .awaitUninterruptibly( + DNS_QUERY_TIMEOUT_MILLIS); + throw e; + } + } + + @Override + public CompletableFuture> resolve(String host) { + io.netty.util.concurrent.Future> query = + this.resolver.resolveAll(host); + CompletableFuture> result = new CompletableFuture<>(); + query.addListener(done -> { + if (!done.isSuccess()) { + result.completeExceptionally(done.cause()); + return; + } + Set addresses = new HashSet<>(); + for (InetAddress address : query.getNow()) { + addresses.add(address.getHostAddress()); + } + result.complete(Collections.unmodifiableSet(addresses)); + }); + result.whenComplete((ignored, failure) -> { + if (result.isCancelled()) { + query.cancel(true); + } + }); + return result; + } + + @Override + public void close() { + this.resolver.close(); + this.eventLoopGroup.shutdownGracefully( + 0L, DNS_QUERY_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS) + .awaitUninterruptibly( + DNS_QUERY_TIMEOUT_MILLIS); + } } @Override diff --git a/hugegraph-pd/hg-pd-service/pom.xml b/hugegraph-pd/hg-pd-service/pom.xml index ee78863f35..7ffb9ccd6d 100644 --- a/hugegraph-pd/hg-pd-service/pom.xml +++ b/hugegraph-pd/hg-pd-service/pom.xml @@ -162,6 +162,18 @@ log4j-jul 2.17.2 + + junit + junit + ${junit.version} + test + + + org.mockito + mockito-core + 3.9.0 + test + diff --git a/hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/PDService.java b/hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/PDService.java index 94d136a844..b31be3bb11 100644 --- a/hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/PDService.java +++ b/hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/PDService.java @@ -27,8 +27,10 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import javax.annotation.PostConstruct; @@ -99,6 +101,7 @@ import com.alipay.sofa.jraft.Status; import com.alipay.sofa.jraft.conf.Configuration; import com.alipay.sofa.jraft.entity.PeerId; +import com.alipay.sofa.jraft.error.RaftError; import io.grpc.ManagedChannel; import io.grpc.stub.StreamObserver; @@ -1683,7 +1686,20 @@ public void updatePdRaft(Pdpb.UpdatePdRaftRequest request, return; } - var list = PeerUtil.parseConfig(request.getConfig()); + List> list; + try { + IpAuthHandler.validatePeerListShape(request.getConfig()); + list = PeerUtil.parseConfig(request.getConfig()); + } catch (IllegalArgumentException e) { + Pdpb.UpdatePdRaftResponse response = + Pdpb.UpdatePdRaftResponse.newBuilder() + .setHeader(newErrorHeader( + 6668, e.getMessage())) + .build(); + observer.onNext(response); + observer.onCompleted(); + return; + } log.info("update raft request: {}, list: {}", request.getConfig(), list); @@ -1732,28 +1748,93 @@ public void updatePdRaft(Pdpb.UpdatePdRaftRequest request, } } + Set newIps = new HashSet<>(); + config.getPeers().forEach(peer -> newIps.add(peer.getIp())); + config.getLearners().forEach(peer -> newIps.add(peer.getIp())); + try { + IpAuthHandler.validateAllowedEntries(newIps); + IpAuthHandler.requireActiveInstance(); + } catch (IllegalArgumentException e) { + response = Pdpb.UpdatePdRaftResponse.newBuilder() + .setHeader(newErrorHeader( + 6668, + e.getMessage())) + .build(); + break; + } catch (IllegalStateException e) { + response = Pdpb.UpdatePdRaftResponse.newBuilder() + .setHeader(newErrorHeader( + 6670, + e.getMessage())) + .build(); + break; + } + log.info("pd raft update with new config: {}", config); - node.changePeers(config, status -> { - if (status.isOk()) { - log.info("updatePdRaft, change peers success"); - // Refresh IpAuthHandler so newly added peers are not blocked - IpAuthHandler handler = IpAuthHandler.getInstance(); - if (handler != null) { - Set newIps = new HashSet<>(); - config.getPeers().forEach(p -> newIps.add(p.getIp())); - config.getLearners().forEach(p -> newIps.add(p.getIp())); - handler.refresh(newIps); - log.info("IpAuthHandler refreshed after updatePdRaft peer change"); - } else { - log.warn("IpAuthHandler not initialized, skipping refresh"); + CountDownLatch changeLatch = new CountDownLatch(1); + AtomicReference changeStatus = new AtomicReference<>(); + try { + node.changePeers(config, status -> { + Status callbackStatus = status; + try { + if (status != null && status.isOk()) { + log.info("updatePdRaft, change peers success"); + IpAuthHandler.refreshInstance(newIps); + log.info("IpAuthHandler refreshed after updatePdRaft peer change"); + } else if (status != null) { + log.error("changePeers status: {}, msg:{}, code: {}, raft error:{}", + status, status.getErrorMsg(), status.getCode(), + status.getRaftError()); + } else { + callbackStatus = new Status( + RaftError.EINTERNAL, + "changePeers returned no status"); + } + } catch (RuntimeException e) { + callbackStatus = new Status( + RaftError.EINTERNAL, + "Raft peers changed but allowlist refresh failed: %s", + e.getMessage()); + log.error("Raft peers changed but IpAuthHandler refresh failed", + e); + } finally { + changeStatus.set(callbackStatus); + changeLatch.countDown(); } - } else { - log.error("changePeers status: {}, msg:{}, code: {}, raft error:{}", - status, status.getErrorMsg(), status.getCode(), - status.getRaftError()); + }); + long timeout = 3L * pdConfig.getRaft().getRpcTimeout(); + if (!changeLatch.await(timeout, TimeUnit.MILLISECONDS)) { + response = Pdpb.UpdatePdRaftResponse.newBuilder() + .setHeader(newErrorHeader( + 6669, + "changePeers timed out")) + .build(); + } else if (changeStatus.get() == null || + !changeStatus.get().isOk()) { + String message = changeStatus.get() == null ? + "changePeers returned no status" : + changeStatus.get().getErrorMsg(); + response = Pdpb.UpdatePdRaftResponse.newBuilder() + .setHeader(newErrorHeader( + 6670, message)) + .build(); } - }); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + response = Pdpb.UpdatePdRaftResponse.newBuilder() + .setHeader(newErrorHeader( + 6670, + "changePeers interrupted")) + .build(); + } catch (RuntimeException e) { + log.error("changePeers failed before callback", e); + response = Pdpb.UpdatePdRaftResponse.newBuilder() + .setHeader(newErrorHeader( + 6670, + e.getMessage())) + .build(); + } } while (false); observer.onNext(response); diff --git a/hugegraph-pd/hg-pd-service/src/test/java/org/apache/hugegraph/pd/service/PDServiceUpdateRaftTest.java b/hugegraph-pd/hg-pd-service/src/test/java/org/apache/hugegraph/pd/service/PDServiceUpdateRaftTest.java new file mode 100644 index 0000000000..d7ee1401c7 --- /dev/null +++ b/hugegraph-pd/hg-pd-service/src/test/java/org/apache/hugegraph/pd/service/PDServiceUpdateRaftTest.java @@ -0,0 +1,195 @@ +/* + * 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.pd.service; + +import java.util.Collections; + +import org.apache.hugegraph.pd.config.PDConfig; +import org.apache.hugegraph.pd.grpc.Pdpb; +import org.apache.hugegraph.pd.raft.RaftEngine; +import org.apache.hugegraph.pd.raft.auth.IpAuthHandler; +import org.apache.hugegraph.testutil.Whitebox; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.ArgumentCaptor; + +import com.alipay.sofa.jraft.Closure; +import com.alipay.sofa.jraft.Node; +import com.alipay.sofa.jraft.Status; +import com.alipay.sofa.jraft.conf.Configuration; +import com.alipay.sofa.jraft.entity.PeerId; +import com.alipay.sofa.jraft.error.RaftError; + +import io.grpc.stub.StreamObserver; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class PDServiceUpdateRaftTest { + + private Node originalRaftNode; + private Node mockNode; + private PDService service; + private PeerId leader; + + @Before + public void setUp() { + this.originalRaftNode = RaftEngine.getInstance().getRaftNode(); + IpAuthHandler.shutdownInstance(); + + this.leader = new PeerId(); + Assert.assertTrue(this.leader.parse("127.0.0.1:8610")); + this.mockNode = mock(Node.class); + when(this.mockNode.isLeader(true)).thenReturn(true); + when(this.mockNode.getLeaderId()).thenReturn(this.leader); + when(this.mockNode.listPeers()).thenReturn( + Collections.singletonList(this.leader)); + Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", + this.mockNode); + IpAuthHandler.getInstance(Collections.singleton("127.0.0.1")); + + PDConfig pdConfig = new PDConfig(); + PDConfig.Raft raft = pdConfig.new Raft(); + raft.setRpcTimeout(1); + pdConfig.setRaft(raft); + this.service = new PDService(); + this.service.setInitConfig(pdConfig); + } + + @After + public void tearDown() { + Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", + this.originalRaftNode); + IpAuthHandler.shutdownInstance(); + } + + @Test + public void testRejectsMalformedConfigBeforeRaft() { + Pdpb.UpdatePdRaftResponse response = update( + "127.0.0.1:8610/leader,bad,127.0.0.2:8610/follower"); + + Assert.assertEquals(6668, response.getHeader().getError().getTypeValue()); + Assert.assertTrue(response.getHeader().getError().getMessage() + .contains("Invalid Raft peer")); + verify(this.mockNode, never()).changePeers( + any(Configuration.class), any(Closure.class)); + } + + @Test + public void testReturnsSuccessAfterRaftCallbackAndAllowlistRefresh() + throws Exception { + IpAuthHandler handler = IpAuthHandler.requireActiveInstance(); + handler.refresh(Collections.singleton("10.0.0.1")); + doAnswer(invocation -> { + Closure closure = invocation.getArgument(1); + closure.run(Status.OK()); + return null; + }).when(this.mockNode).changePeers(any(Configuration.class), + any(Closure.class)); + + Pdpb.UpdatePdRaftResponse response = update( + "127.0.0.1:8610/leader"); + + Assert.assertEquals(Pdpb.ErrorType.OK, + response.getHeader().getError().getType()); + Assert.assertTrue(isIpAllowed(handler, "127.0.0.1")); + Assert.assertFalse(isIpAllowed(handler, "10.0.0.1")); + } + + @Test + public void testReturnsRaftFailureFromCallback() { + doAnswer(invocation -> { + Closure closure = invocation.getArgument(1); + closure.run(new Status(RaftError.EINTERNAL, "simulated failure")); + return null; + }).when(this.mockNode).changePeers(any(Configuration.class), + any(Closure.class)); + + Pdpb.UpdatePdRaftResponse response = update( + "127.0.0.1:8610/leader"); + + Assert.assertEquals(6670, response.getHeader().getError().getTypeValue()); + Assert.assertTrue(response.getHeader().getError().getMessage() + .contains("simulated failure")); + } + + @Test + public void testReturnsTimeoutWhenRaftDoesNotCallback() { + Pdpb.UpdatePdRaftResponse response = update( + "127.0.0.1:8610/leader"); + + Assert.assertEquals(6669, response.getHeader().getError().getTypeValue()); + Assert.assertTrue(response.getHeader().getError().getMessage() + .contains("timed out")); + } + + @Test + public void testRejectsMissingAllowlistBeforeRaft() { + IpAuthHandler.shutdownInstance(); + + Pdpb.UpdatePdRaftResponse response = update( + "127.0.0.1:8610/leader"); + + Assert.assertEquals(6670, response.getHeader().getError().getTypeValue()); + Assert.assertTrue(response.getHeader().getError().getMessage() + .contains("not active")); + verify(this.mockNode, never()).changePeers( + any(Configuration.class), any(Closure.class)); + } + + @Test + public void testMapsSynchronousRaftFailure() { + doThrow(new IllegalStateException("node stopped")) + .when(this.mockNode) + .changePeers(any(Configuration.class), any(Closure.class)); + + Pdpb.UpdatePdRaftResponse response = update( + "127.0.0.1:8610/leader"); + + Assert.assertEquals(6670, response.getHeader().getError().getTypeValue()); + Assert.assertTrue(response.getHeader().getError().getMessage() + .contains("node stopped")); + } + + @SuppressWarnings("unchecked") + private Pdpb.UpdatePdRaftResponse update(String config) { + StreamObserver observer = + mock(StreamObserver.class); + this.service.updatePdRaft( + Pdpb.UpdatePdRaftRequest.newBuilder().setConfig(config).build(), + observer); + ArgumentCaptor response = + ArgumentCaptor.forClass(Pdpb.UpdatePdRaftResponse.class); + verify(observer).onNext(response.capture()); + verify(observer).onCompleted(); + return response.getValue(); + } + + private boolean isIpAllowed(IpAuthHandler handler, String ip) { + return Whitebox.invoke(IpAuthHandler.class, + new Class[]{String.class}, + "isIpAllowed", handler, ip); + } +} diff --git a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/PDCoreSuiteTest.java b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/PDCoreSuiteTest.java index 95b044c76b..613d085594 100644 --- a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/PDCoreSuiteTest.java +++ b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/PDCoreSuiteTest.java @@ -19,7 +19,7 @@ import org.apache.hugegraph.pd.core.meta.MetadataKeyHelperTest; import org.apache.hugegraph.pd.core.store.HgKVStoreImplTest; -import org.apache.hugegraph.pd.raft.IpAuthHandlerTest; +import org.apache.hugegraph.pd.raft.auth.IpAuthHandlerTest; import org.apache.hugegraph.pd.raft.RaftEngineIpAuthIntegrationTest; import org.apache.hugegraph.pd.raft.RaftEngineLeaderAddressTest; import org.junit.runner.RunWith; diff --git a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/IpAuthHandlerTest.java b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/IpAuthHandlerTest.java deleted file mode 100644 index 31647b6d39..0000000000 --- a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/IpAuthHandlerTest.java +++ /dev/null @@ -1,133 +0,0 @@ -/* - * 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.pd.raft; - -import java.net.InetAddress; -import java.util.Collections; -import java.util.HashSet; -import java.util.Set; - -import org.apache.hugegraph.pd.raft.auth.IpAuthHandler; -import org.apache.hugegraph.testutil.Whitebox; -import org.junit.After; -import org.junit.Assert; -import org.junit.Before; -import org.junit.Test; - -public class IpAuthHandlerTest { - - @Before - public void setUp() { - // Must reset BEFORE each test — earlier suite classes (e.g. ConfigServiceTest) - // initialize RaftEngine which creates the IpAuthHandler singleton with their - // own peer IPs. Without this reset, our getInstance() calls return the stale - // singleton and ignore the allowlist passed by the test. - Whitebox.setInternalState(IpAuthHandler.class, "instance", null); - } - - @After - public void tearDown() { - // Must reset AFTER each test — prevents our test singleton from leaking - // into later suite classes that also depend on IpAuthHandler state. - Whitebox.setInternalState(IpAuthHandler.class, "instance", null); - } - - private boolean isIpAllowed(IpAuthHandler handler, String ip) { - return Whitebox.invoke(IpAuthHandler.class, - new Class[]{String.class}, - "isIpAllowed", handler, ip); - } - - @Test - public void testHostnameResolvesToIp() throws Exception { - // "localhost" should resolve to one or more IPs via InetAddress.getAllByName() - // This verifies the core fix: hostname allowlists match numeric remote addresses - // Using dynamic resolution avoids hardcoding "127.0.0.1" which may not be - // returned on IPv6-only or custom resolver environments - IpAuthHandler handler = IpAuthHandler.getInstance( - Collections.singleton("localhost")); - InetAddress[] addresses = InetAddress.getAllByName("localhost"); - // All resolved addresses should be allowed — resolveAll() adds every address - // returned by getAllByName() so none should be blocked - Assert.assertTrue("Expected at least one resolved address", - addresses.length > 0); - for (InetAddress address : addresses) { - Assert.assertTrue( - "Expected " + address.getHostAddress() + " to be allowed", - isIpAllowed(handler, address.getHostAddress())); - } - } - - @Test - public void testUnresolvableHostnameDoesNotCrash() { - // Should log a warning and skip — no exception thrown during construction - // Uses .invalid TLD which is RFC-2606 reserved and guaranteed to never resolve - IpAuthHandler handler = IpAuthHandler.getInstance( - Collections.singleton("nonexistent.invalid")); - // Handler was still created successfully despite bad hostname - Assert.assertNotNull(handler); - // Unresolvable entry is skipped so no IPs should be allowed - Assert.assertFalse(isIpAllowed(handler, "127.0.0.1")); - Assert.assertFalse(isIpAllowed(handler, "192.168.0.1")); - } - - @Test - public void testRefreshUpdatesResolvedIps() { - // Start with 127.0.0.1 - IpAuthHandler handler = IpAuthHandler.getInstance( - Collections.singleton("127.0.0.1")); - Assert.assertTrue(isIpAllowed(handler, "127.0.0.1")); - - // Refresh with a different IP — verifies refresh() swaps the set correctly - Set newIps = new HashSet<>(); - newIps.add("192.168.0.1"); - handler.refresh(newIps); - - // Old IP should no longer be allowed - Assert.assertFalse(isIpAllowed(handler, "127.0.0.1")); - // New IP should now be allowed - Assert.assertTrue(isIpAllowed(handler, "192.168.0.1")); - } - - @Test - public void testEmptyAllowlistAllowsAll() { - // Empty allowlist = no restriction configured = allow all connections - // This is intentional fallback behavior and must be explicitly tested - // because it is a security-relevant boundary - IpAuthHandler handler = IpAuthHandler.getInstance( - Collections.emptySet()); - Assert.assertTrue(isIpAllowed(handler, "1.2.3.4")); - Assert.assertTrue(isIpAllowed(handler, "192.168.99.99")); - } - - @Test - public void testGetInstanceReturnsSingletonIgnoresNewAllowlist() { - // First call creates the singleton with 127.0.0.1 - IpAuthHandler first = IpAuthHandler.getInstance( - Collections.singleton("127.0.0.1")); - // Second call with a different set must return the same instance - // and must NOT reinitialize or override the existing allowlist - IpAuthHandler second = IpAuthHandler.getInstance( - Collections.singleton("192.168.0.1")); - Assert.assertSame(first, second); - // Original allowlist still in effect - Assert.assertTrue(isIpAllowed(second, "127.0.0.1")); - // New set was ignored — 192.168.0.1 should not be allowed - Assert.assertFalse(isIpAllowed(second, "192.168.0.1")); - } -} diff --git a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineIpAuthIntegrationTest.java b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineIpAuthIntegrationTest.java index 1f9857df0f..1aa2921748 100644 --- a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineIpAuthIntegrationTest.java +++ b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineIpAuthIntegrationTest.java @@ -19,6 +19,7 @@ import java.util.Collections; +import org.apache.hugegraph.pd.config.PDConfig; import org.apache.hugegraph.pd.raft.auth.IpAuthHandler; import org.apache.hugegraph.testutil.Whitebox; import org.junit.After; @@ -35,25 +36,35 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; public class RaftEngineIpAuthIntegrationTest { private Node originalRaftNode; + private PDConfig.Raft originalConfig; @Before public void setUp() { // Save original raftNode so we can restore it after the test originalRaftNode = RaftEngine.getInstance().getRaftNode(); + originalConfig = Whitebox.getInternalState(RaftEngine.getInstance(), + "config"); + PDConfig pdConfig = new PDConfig(); + PDConfig.Raft config = pdConfig.new Raft(); + config.setRpcTimeout(100); + Whitebox.setInternalState(RaftEngine.getInstance(), "config", config); // Reset IpAuthHandler singleton for a clean state - Whitebox.setInternalState(IpAuthHandler.class, "instance", null); + IpAuthHandler.shutdownInstance(); } @After public void tearDown() { // Restore original raftNode Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", originalRaftNode); + Whitebox.setInternalState(RaftEngine.getInstance(), "config", originalConfig); // Reset IpAuthHandler singleton - Whitebox.setInternalState(IpAuthHandler.class, "instance", null); + IpAuthHandler.shutdownInstance(); } @Test @@ -80,9 +91,11 @@ public void testChangePeerListRefreshesIpAuthHandler() throws Exception { Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", mockNode); // Call changePeerList with new peer — must be odd count - RaftEngine.getInstance().changePeerList("127.0.0.1:8610"); + Status status = RaftEngine.getInstance().changePeerList( + "127.0.0.1:8610"); // Verify IpAuthHandler was refreshed with the new peer IP + Assert.assertTrue(status.isOk()); Assert.assertTrue(invokeIsIpAllowed(handler, "127.0.0.1")); // Old IP should no longer be allowed Assert.assertFalse(invokeIsIpAllowed(handler, "10.0.0.1")); @@ -109,13 +122,73 @@ public void testChangePeerListDoesNotRefreshOnFailure() throws Exception { Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", mockNode); - RaftEngine.getInstance().changePeerList("127.0.0.1:8610"); + Status status = RaftEngine.getInstance().changePeerList( + "127.0.0.1:8610"); // Handler should NOT be refreshed — old IP still allowed + Assert.assertFalse(status.isOk()); Assert.assertTrue(invokeIsIpAllowed(handler, "10.0.0.1")); Assert.assertFalse(invokeIsIpAllowed(handler, "127.0.0.1")); } + @Test + public void testChangePeerListRejectsNullCallbackStatus() { + IpAuthHandler.getInstance(Collections.singleton("10.0.0.1")); + Node mockNode = mock(Node.class); + doAnswer(invocation -> { + Closure closure = invocation.getArgument(1); + closure.run(null); + return null; + }).when(mockNode).changePeers(any(Configuration.class), + any(Closure.class)); + Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", + mockNode); + + Status status = RaftEngine.getInstance().changePeerList( + "127.0.0.1:8610"); + + Assert.assertNotNull(status); + Assert.assertFalse(status.isOk()); + Assert.assertTrue(status.getErrorMsg() + .contains("returned no status")); + } + + @Test + public void testChangePeerListRejectsOversizedAllowlistBeforeRaft() { + Node mockNode = mock(Node.class); + Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", mockNode); + StringBuilder peers = new StringBuilder(); + for (int i = 0; i < 129; i++) { + if (i > 0) { + peers.append(','); + } + peers.append("pd-").append(i).append(":8610"); + } + + Status status = RaftEngine.getInstance().changePeerList( + peers.toString()); + + Assert.assertNotNull(status); + Assert.assertFalse(status.isOk()); + verify(mockNode, never()).changePeers( + any(Configuration.class), any(Closure.class)); + } + + @Test + public void testChangePeerListRejectsMalformedPeerBeforeRaft() { + Node mockNode = mock(Node.class); + Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", mockNode); + + Status status = RaftEngine.getInstance().changePeerList( + "127.0.0.1:8610,bad,127.0.0.2:8610"); + + Assert.assertNotNull(status); + Assert.assertFalse(status.isOk()); + Assert.assertTrue(status.getErrorMsg().contains("Invalid Raft peer")); + verify(mockNode, never()).changePeers( + any(Configuration.class), any(Closure.class)); + } + private boolean invokeIsIpAllowed(IpAuthHandler handler, String ip) { return Whitebox.invoke(IpAuthHandler.class, new Class[]{String.class}, diff --git a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandlerTest.java b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandlerTest.java new file mode 100644 index 0000000000..833d1eeaa0 --- /dev/null +++ b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandlerTest.java @@ -0,0 +1,439 @@ +/* + * 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.pd.raft.auth; + +import java.net.InetAddress; +import java.net.UnknownHostException; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.hugegraph.testutil.Whitebox; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +public class IpAuthHandlerTest { + + @Before + public void setUp() { + // Must reset BEFORE each test — earlier suite classes (e.g. ConfigServiceTest) + // initialize RaftEngine which creates the IpAuthHandler singleton with their + // own peer IPs. Without this reset, our getInstance() calls return the stale + // singleton and ignore the allowlist passed by the test. + IpAuthHandler.shutdownInstance(); + } + + @After + public void tearDown() { + // Must reset AFTER each test — prevents our test singleton from leaking + // into later suite classes that also depend on IpAuthHandler state. + IpAuthHandler handler = IpAuthHandler.getInstance(); + if (handler != null) { + IpAuthHandler.shutdownInstance(); + } + } + + private boolean isIpAllowed(IpAuthHandler handler, String ip) { + return Whitebox.invoke(IpAuthHandler.class, + new Class[]{String.class}, + "isIpAllowed", handler, ip); + } + + @Test + public void testHostnameResolvesToIp() throws Exception { + // "localhost" should resolve to one or more IPs via InetAddress.getAllByName() + // This verifies the core fix: hostname allowlists match numeric remote addresses + // Using dynamic resolution avoids hardcoding "127.0.0.1" which may not be + // returned on IPv6-only or custom resolver environments + IpAuthHandler handler = IpAuthHandler.getInstance( + Collections.singleton("localhost")); + InetAddress[] addresses = InetAddress.getAllByName("localhost"); + Assert.assertTrue("Expected at least one resolved address", + addresses.length > 0); + boolean matched = false; + for (InetAddress address : addresses) { + matched |= isIpAllowed(handler, address.getHostAddress()); + } + Assert.assertTrue("Expected a resolved address to be allowed", matched); + } + + @Test + public void testTransientDnsFailureRecoversOnRefresh() throws Exception { + AtomicInteger attempts = new AtomicInteger(); + InetAddress expected = InetAddress.getByAddress( + new byte[]{(byte) 192, (byte) 168, 0, 1}); + IpAuthHandler handler = new IpAuthHandler( + Collections.singleton("pd-1"), + host -> { + if (attempts.incrementAndGet() < 3) { + return failed(host); + } + return resolved(expected); + }, + false, 100L, 1_000L, 1_000L); + + Assert.assertFalse(isIpAllowed(handler, expected.getHostAddress())); + handler.refreshResolvedIps(); + handler.refreshResolvedIps(); + Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); + Assert.assertEquals(3, attempts.get()); + handler.shutdown(); + } + + @Test + public void testTransientDnsFailureKeepsLastKnownAddress() throws Exception { + AtomicInteger attempts = new AtomicInteger(); + InetAddress expected = InetAddress.getByAddress( + new byte[]{(byte) 192, (byte) 168, 0, 1}); + IpAuthHandler handler = new IpAuthHandler( + Collections.singleton("pd-1"), + host -> { + if (attempts.incrementAndGet() > 1) { + return failed(host); + } + return resolved(expected); + }, + false, 100L, 1_000L, 1_000L); + + Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); + handler.refreshResolvedIps(); + Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); + handler.shutdown(); + } + + @Test + public void testSlowPeerDoesNotBlockFollowingPeer() throws Exception { + InetAddress expected = InetAddress.getByAddress( + new byte[]{(byte) 192, (byte) 168, 0, 2}); + Set peers = new LinkedHashSet<>(); + peers.add("pd-slow"); + peers.add("pd-ready"); + IpAuthHandler handler = new IpAuthHandler( + peers, + host -> { + if ("pd-slow".equals(host)) { + return new CompletableFuture<>(); + } + return resolved(expected); + }, + false, 10L, 1_000L, 1_000L); + + Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); + handler.shutdown(); + } + + @Test + public void testExpiredAddressFailsClosed() throws Exception { + AtomicInteger attempts = new AtomicInteger(); + InetAddress expected = InetAddress.getByAddress( + new byte[]{(byte) 192, (byte) 168, 0, 3}); + IpAuthHandler handler = new IpAuthHandler( + Collections.singleton("pd-1"), + host -> { + if (attempts.incrementAndGet() > 1) { + return failed(host); + } + return resolved(expected); + }, + false, 100L, 1L, 1_000L); + + Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); + Thread.sleep(5L); + handler.refreshResolvedIps(); + Assert.assertFalse(isIpAllowed(handler, expected.getHostAddress())); + handler.shutdown(); + } + + @Test + public void testScheduledRefreshAddsLatePeerAndRotatesAddress() + throws Exception { + AtomicInteger attempts = new AtomicInteger(); + InetAddress first = InetAddress.getByAddress( + new byte[]{(byte) 192, (byte) 168, 0, 4}); + InetAddress second = InetAddress.getByAddress( + new byte[]{(byte) 192, (byte) 168, 0, 5}); + AtomicReference current = new AtomicReference<>(first); + IpAuthHandler handler = new IpAuthHandler( + Collections.singleton("pd-late"), + host -> { + if (attempts.incrementAndGet() == 1) { + return failed(host); + } + return resolved(current.get()); + }, + true, 20L, 1_000L, 10L); + try { + awaitAllowed(handler, first.getHostAddress()); + current.set(second); + awaitAllowed(handler, second.getHostAddress()); + Assert.assertFalse(isIpAllowed(handler, first.getHostAddress())); + } finally { + handler.shutdown(); + } + } + + @Test + public void testNeverCompletingPeersDoNotStarveReadyPeer() + throws Exception { + InetAddress expected = InetAddress.getByAddress( + new byte[]{(byte) 192, (byte) 168, 0, 6}); + Set peers = new LinkedHashSet<>(); + for (int i = 0; i < 8; i++) { + peers.add("00-pd-slow-" + i); + } + peers.add("99-pd-ready"); + IpAuthHandler handler = new IpAuthHandler( + peers, + host -> { + if (host.startsWith("00-pd-slow-")) { + return new CompletableFuture<>(); + } + return resolved(expected); + }, + false, 10L, 1_000L, 1_000L); + + handler.refresh(peers); + Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); + handler.shutdown(); + } + + @Test + public void testRejectsOversizedAllowlist() { + Set peers = new HashSet<>(); + for (int i = 0; i < 128; i++) { + peers.add("pd-" + i); + } + + try { + new IpAuthHandler(peers, host -> new CompletableFuture<>(), + false, 10L, 1_000L, 1_000L); + Assert.fail("Expected oversized allowlist rejection"); + } catch (IllegalArgumentException e) { + Assert.assertTrue(e.getMessage().contains("127")); + } + } + + @Test + public void testLateSuccessfulResultIsDiscarded() throws Exception { + AtomicInteger attempts = new AtomicInteger(); + InetAddress first = InetAddress.getByAddress( + new byte[]{(byte) 192, (byte) 168, 0, 7}); + InetAddress late = InetAddress.getByAddress( + new byte[]{(byte) 192, (byte) 168, 0, 8}); + CompletableFuture> delayed = new CompletableFuture<>(); + IpAuthHandler handler = new IpAuthHandler( + Collections.singleton("pd-1"), + host -> { + int attempt = attempts.incrementAndGet(); + if (attempt == 1) { + return resolved(first); + } + if (attempt == 2) { + return delayed; + } + return new CompletableFuture<>(); + }, + false, 10L, 1_000L, 1_000L); + + handler.refreshResolvedIps(false); + Thread.sleep(20L); + delayed.complete(resolved(late).get()); + handler.refreshResolvedIps(false); + + Assert.assertTrue(isIpAllowed(handler, first.getHostAddress())); + Assert.assertFalse(isIpAllowed(handler, late.getHostAddress())); + handler.shutdown(); + } + + @Test + public void testRefreshCollectsPreviousBatchBeforeStartingNext() + throws Exception { + Set peers = new HashSet<>(); + Map>> delayed = + new HashMap<>(); + for (int i = 0; i < 17; i++) { + peers.add(String.format("pd-%02d", i)); + if (i >= 8 && i < 16) { + delayed.put(i, new CompletableFuture<>()); + } + } + IpAuthHandler handler = new IpAuthHandler( + peers, + host -> { + int index = Integer.parseInt(host.substring(3)); + CompletableFuture> future = delayed.get(index); + if (future != null) { + return future; + } + return resolved(address(index)); + }, + false, 100L, 1_000L, 1_000L); + + handler.refreshResolvedIps(false); + for (Map.Entry>> entry : + delayed.entrySet()) { + entry.getValue().complete(resolved(address(entry.getKey())).get()); + } + handler.refreshResolvedIps(false); + + Assert.assertTrue(isIpAllowed( + handler, address(16).getHostAddress())); + handler.shutdown(); + } + + @Test + public void testConstructorFailureClosesResolver() { + AtomicBoolean closed = new AtomicBoolean(); + IpAuthHandler.HostResolver resolver = new IpAuthHandler.HostResolver() { + + @Override + public CompletableFuture> resolve(String host) { + throw new IllegalStateException("simulated resolver failure"); + } + + @Override + public void close() { + closed.set(true); + } + }; + + try { + new IpAuthHandler(Collections.singleton("pd-1"), resolver, + false, 10L, 1_000L, 1_000L); + Assert.fail("Expected constructor failure"); + } catch (IllegalStateException e) { + Assert.assertEquals("simulated resolver failure", e.getMessage()); + } + Assert.assertTrue(closed.get()); + } + + @Test + public void testInterruptedRefreshFailsAndPreservesInterrupt() + throws Exception { + InetAddress initial = address(20); + IpAuthHandler handler = new IpAuthHandler( + Collections.singleton("ready"), + host -> { + if ("ready".equals(host)) { + return resolved(initial); + } + return new CompletableFuture<>(); + }, + false, 100L, 1_000L, 1_000L); + try { + Thread.currentThread().interrupt(); + handler.refresh(Collections.singleton("slow")); + Assert.fail("Expected interrupted refresh to fail"); + } catch (IllegalStateException e) { + Assert.assertTrue(e.getMessage().contains("interrupted")); + Assert.assertTrue(Thread.currentThread().isInterrupted()); + } finally { + Thread.interrupted(); + handler.shutdown(); + } + } + + private void awaitAllowed(IpAuthHandler handler, String address) + throws InterruptedException { + long deadline = System.currentTimeMillis() + 1_000L; + while (!isIpAllowed(handler, address) && + System.currentTimeMillis() < deadline) { + Thread.sleep(10L); + } + Assert.assertTrue(isIpAllowed(handler, address)); + } + + @Test + public void testRefreshUpdatesResolvedIps() { + // Start with 127.0.0.1 + IpAuthHandler handler = IpAuthHandler.getInstance( + Collections.singleton("127.0.0.1")); + Assert.assertTrue(isIpAllowed(handler, "127.0.0.1")); + + // Refresh with a different IP — verifies refresh() swaps the set correctly + Set newIps = new HashSet<>(); + newIps.add("192.168.0.1"); + handler.refresh(newIps); + + // Old IP should no longer be allowed + Assert.assertFalse(isIpAllowed(handler, "127.0.0.1")); + // New IP should now be allowed + Assert.assertTrue(isIpAllowed(handler, "192.168.0.1")); + } + + @Test + public void testEmptyAllowlistAllowsAll() { + // Empty allowlist = no restriction configured = allow all connections + // This is intentional fallback behavior and must be explicitly tested + // because it is a security-relevant boundary + IpAuthHandler handler = IpAuthHandler.getInstance( + Collections.emptySet()); + Assert.assertTrue(isIpAllowed(handler, "1.2.3.4")); + Assert.assertTrue(isIpAllowed(handler, "192.168.99.99")); + } + + @Test + public void testGetInstanceReturnsSingletonIgnoresNewAllowlist() { + // First call creates the singleton with 127.0.0.1 + IpAuthHandler first = IpAuthHandler.getInstance( + Collections.singleton("127.0.0.1")); + // Second call with a different set must return the same instance + // and must NOT reinitialize or override the existing allowlist + IpAuthHandler second = IpAuthHandler.getInstance( + Collections.singleton("192.168.0.1")); + Assert.assertSame(first, second); + // Original allowlist still in effect + Assert.assertTrue(isIpAllowed(second, "127.0.0.1")); + // New set was ignored — 192.168.0.1 should not be allowed + Assert.assertFalse(isIpAllowed(second, "192.168.0.1")); + } + + private static CompletableFuture> resolved( + InetAddress... addresses) { + Set result = new HashSet<>(); + for (InetAddress address : addresses) { + result.add(address.getHostAddress()); + } + return CompletableFuture.completedFuture( + Collections.unmodifiableSet(result)); + } + + private static CompletableFuture> failed(String host) { + CompletableFuture> result = new CompletableFuture<>(); + result.completeExceptionally(new UnknownHostException(host)); + return result; + } + + private static InetAddress address(int suffix) { + try { + return InetAddress.getByAddress( + new byte[]{10, 0, 0, (byte) (suffix + 1)}); + } catch (UnknownHostException e) { + throw new AssertionError(e); + } + } +} From ed1310c55fb4d46fe90cdb764863e05dbbf903ff Mon Sep 17 00:00:00 2001 From: dark Date: Mon, 17 Aug 2026 10:01:40 +0800 Subject: [PATCH 04/23] feat(server): support GraphSpace-wide observer - persist observer access against the all-graphs target - apply read-only access to existing and future graphs - migrate and remove legacy graph-scoped observer grants - document the GraphSpace-wide contract in API version 0.72 --- .../apache/hugegraph/api/auth/ManagerAPI.java | 14 +++- .../hugegraph/api/space/GraphSpaceAPI.java | 37 ++++++--- .../apache/hugegraph/version/ApiVersion.java | 2 +- .../hugegraph/auth/StandardAuthManagerV2.java | 10 ++- .../unit/api/space/GraphSpaceAPITest.java | 81 ++++++++++++++++++- 5 files changed, 127 insertions(+), 17 deletions(-) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java index 37aee8c657..5989d48892 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java @@ -287,9 +287,8 @@ public String checkDefaultRole(@Context GraphManager manager, defaultRole = null; // unreachable, satisfies compiler } validGraphSpace(manager, graphSpace); - 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, graphSpace, graph); } @@ -301,6 +300,15 @@ public String checkDefaultRole(@Context GraphManager manager, } else { result = authManager.isDefaultRole(graphSpace, user, defaultRole); + if (!result && defaultRole.equals(HugeDefaultRole.OBSERVER)) { + for (String currentGraph : manager.graphs(graphSpace)) { + if (authManager.isDefaultRole( + graphSpace, currentGraph, user, defaultRole)) { + result = true; + break; + } + } + } } return manager.serializer().writeMap(ImmutableMap.of("check", result)); } diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java index 81f13cf3f0..934508ed3d 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java @@ -146,10 +146,8 @@ public String setDefaultRole(@Context GraphManager manager, 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"); + boolean hasGraph = role.equals(HugeDefaultRole.OBSERVER) && + StringUtils.isNotEmpty(graph); if (hasGraph) { validGraph(manager, name, graph); } @@ -164,6 +162,12 @@ public String setDefaultRole(@Context GraphManager manager, result.put("graph", graph); } else { authManager.createSpaceDefaultRole(name, user, role); + if (role.equals(HugeDefaultRole.OBSERVER)) { + for (String currentGraph : manager.graphs(name)) { + authManager.deleteDefaultRole( + name, user, role, currentGraph); + } + } } return manager.serializer().writeMap(result); @@ -203,9 +207,8 @@ 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); } @@ -217,6 +220,15 @@ public String checkDefaultRole(@Context GraphManager manager, } else { result = authManager.isDefaultRole(name, user, defaultRole); + if (!result && defaultRole.equals(HugeDefaultRole.OBSERVER)) { + for (String currentGraph : manager.graphs(name)) { + if (authManager.isDefaultRole( + name, currentGraph, user, defaultRole)) { + result = true; + break; + } + } + } } return manager.serializer().writeMap(ImmutableMap.of("check", result)); } @@ -259,9 +271,8 @@ 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); } @@ -269,6 +280,12 @@ public void deleteDefaultRole(@Context GraphManager manager, authManager.deleteDefaultRole(name, user, defaultRole, graph); } else { authManager.deleteDefaultRole(name, user, defaultRole); + if (defaultRole.equals(HugeDefaultRole.OBSERVER)) { + for (String currentGraph : manager.graphs(name)) { + authManager.deleteDefaultRole( + name, user, defaultRole, currentGraph); + } + } } } diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/version/ApiVersion.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/version/ApiVersion.java index faadd1f5a6..00e8dad032 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/version/ApiVersion.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/version/ApiVersion.java @@ -121,7 +121,7 @@ public final class ApiVersion { * [0.69] Issue-1748: Support Cypher query RESTful API * [0.70] PR-2242: Add edge-existence RESTful API * [0.71] PR-2286: Support Arthas API & Metric API prometheus format - * [0.72] Support GraphSpace default-role management APIs + * [0.72] Support GraphSpace-wide default-role management APIs */ /** diff --git a/hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManagerV2.java b/hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManagerV2.java index 1f34aa4593..aaf2a9df17 100644 --- a/hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManagerV2.java +++ b/hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManagerV2.java @@ -1815,7 +1815,10 @@ public Id createSpaceDefaultRole(String graphSpace, String owner, @Override public boolean isDefaultRole(String graphSpace, String owner, HugeDefaultRole role) { - return isDefaultRole(graphSpace, owner, role.toString()); + String roleName = role.isGraphRole() ? + getGraphDefaultRole(ALL_GRAPHS, role.toString()) : + role.toString(); + return isDefaultRole(graphSpace, owner, roleName); } @Override @@ -1828,7 +1831,10 @@ public boolean isDefaultRole(String graphSpace, String graph, @Override public void deleteDefaultRole(String graphSpace, String owner, HugeDefaultRole role) { - deleteDefaultRoleByName(graphSpace, owner, role.toString()); + String roleName = role.isGraphRole() ? + getGraphDefaultRole(ALL_GRAPHS, role.toString()) : + role.toString(); + deleteDefaultRoleByName(graphSpace, owner, roleName); } @Override diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/GraphSpaceAPITest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/GraphSpaceAPITest.java index caa659a4d4..6315f3ae7e 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/GraphSpaceAPITest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/GraphSpaceAPITest.java @@ -87,6 +87,81 @@ public void testAdminCanCheckSpaceDefaultRole() { Assert.assertContains("\"check\":true", result); } + @Test + public void testAdminCanCheckSpaceWideObserverRole() { + GraphSpaceAPI api = new GraphSpaceAPI(); + GraphManager manager = managerWithDefaultRoleContext(ADMIN, true); + setContext(ADMIN); + + String result = api.checkDefaultRole(manager, GRAPHSPACE, TARGET, + "OBSERVER", null); + + Assert.assertContains("\"check\":true", result); + } + + @Test + public void testCurrentUserCanCheckSpaceWideObserverRole() { + ManagerAPI api = new ManagerAPI(); + GraphManager manager = managerWithDefaultRoleContext(TARGET, false); + setContext(TARGET); + + String result = api.checkDefaultRole(manager, GRAPHSPACE, + "OBSERVER", null); + + Assert.assertContains("\"check\":true", result); + } + + @Test + public void testCurrentUserObserverCheckFallsBackToLegacyGraphRole() { + ManagerAPI api = new ManagerAPI(); + GraphManager manager = managerWithDefaultRoleContext(TARGET, false); + AuthManager auth = manager.authManager(); + Mockito.when(auth.isDefaultRole( + GRAPHSPACE, TARGET, HugeDefaultRole.OBSERVER)) + .thenReturn(false); + setContext(TARGET); + + String result = api.checkDefaultRole(manager, GRAPHSPACE, + "OBSERVER", null); + + Assert.assertContains("\"check\":true", result); + Mockito.verify(auth).isDefaultRole( + GRAPHSPACE, GRAPH, TARGET, HugeDefaultRole.OBSERVER); + } + + @Test + public void testObserverCheckFallsBackToLegacyGraphRole() { + GraphSpaceAPI api = new GraphSpaceAPI(); + GraphManager manager = managerWithDefaultRoleContext(ADMIN, true); + AuthManager auth = manager.authManager(); + Mockito.when(auth.isDefaultRole( + GRAPHSPACE, TARGET, HugeDefaultRole.OBSERVER)) + .thenReturn(false); + setContext(ADMIN); + + String result = api.checkDefaultRole(manager, GRAPHSPACE, TARGET, + "OBSERVER", null); + + Assert.assertContains("\"check\":true", result); + Mockito.verify(auth).isDefaultRole( + GRAPHSPACE, GRAPH, TARGET, HugeDefaultRole.OBSERVER); + } + + @Test + public void testObserverDeleteCleansSpaceAndLegacyGraphRoles() { + GraphSpaceAPI api = new GraphSpaceAPI(); + GraphManager manager = managerWithDefaultRoleContext(ADMIN, true); + AuthManager auth = manager.authManager(); + setContext(ADMIN); + + api.deleteDefaultRole(manager, GRAPHSPACE, TARGET, "OBSERVER", null); + + Mockito.verify(auth).deleteDefaultRole( + GRAPHSPACE, TARGET, HugeDefaultRole.OBSERVER); + Mockito.verify(auth).deleteDefaultRole( + GRAPHSPACE, TARGET, HugeDefaultRole.OBSERVER, GRAPH); + } + @Test public void testManagerDefaultRoleRejectsMissingGraphSpace() { ManagerAPI api = new ManagerAPI(); @@ -191,6 +266,9 @@ private static GraphManager managerWithDefaultRoleContext(String operator, Mockito.when(authManager.isDefaultRole(GRAPHSPACE, TARGET, HugeDefaultRole.SPACE)) .thenReturn(true); + Mockito.when(authManager.isDefaultRole(GRAPHSPACE, TARGET, + HugeDefaultRole.OBSERVER)) + .thenReturn(true); Mockito.when(authManager.findUser(TARGET)) .thenReturn(new HugeUser(TARGET)); @@ -206,7 +284,8 @@ private static GraphManager managerWithDefaultRoleContext(String operator, MetaManager metaManager = Mockito.mock(MetaManager.class); Mockito.when(metaManager.graphConfigs(GRAPHSPACE)) - .thenReturn(Collections.emptyMap()); + .thenReturn(Collections.singletonMap( + GRAPHSPACE + "-" + GRAPH, Collections.emptyMap())); Whitebox.setInternalState(manager, "metaManager", metaManager); Map graphs = new ConcurrentHashMap<>(); From 0752ec20af2230b884b50a7570b9eec96279654d Mon Sep 17 00:00:00 2001 From: dark Date: Mon, 17 Aug 2026 23:39:13 +0800 Subject: [PATCH 05/23] fix(server): enforce Gremlin mutations - classify mutation steps from Gremlin bytecode - require write access for add and property steps - require delete access for drop steps - cover read write delete and nested traversals --- .../hugegraph/auth/HugeGraphAuthProxy.java | 34 ++++++++++++++++ .../unit/auth/HugeGraphAuthProxyTest.java | 40 +++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java index 4b0aed578f..5b4e704608 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java @@ -22,6 +22,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Date; +import java.util.EnumSet; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -95,6 +96,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal.Symbols; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.apache.tinkerpop.gremlin.process.traversal.translator.GroovyTranslator; import org.apache.tinkerpop.gremlin.structure.Edge; @@ -2414,6 +2416,11 @@ public void apply(Traversal.Admin traversal) { */ String caller = Thread.currentThread().getName(); if (!caller.contains(TraversalStrategiesProxy.REST_WORKER)) { + for (HugePermission permission : + traversalPermissions(traversal.getBytecode())) { + verifyNamePermission(permission, ResourceType.GREMLIN, + script); + } verifyNamePermission(HugePermission.EXECUTE, ResourceType.GREMLIN, script); } @@ -2461,4 +2468,31 @@ public String toString() { return this.origin.toString(); } } + + private static Set traversalPermissions(Bytecode bytecode) { + Set permissions = EnumSet.noneOf(HugePermission.class); + collectTraversalPermissions(bytecode, permissions); + return permissions; + } + + private static void collectTraversalPermissions( + Bytecode bytecode, + Set permissions) { + for (Instruction instruction : bytecode.getStepInstructions()) { + String operator = instruction.getOperator(); + if (Symbols.addV.equals(operator) || + Symbols.addE.equals(operator) || + Symbols.property.equals(operator)) { + permissions.add(HugePermission.WRITE); + } else if (Symbols.drop.equals(operator)) { + permissions.add(HugePermission.DELETE); + } + for (Object argument : instruction.getArguments()) { + if (argument instanceof Bytecode) { + collectTraversalPermissions((Bytecode) argument, + permissions); + } + } + } + } } diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index 1b209c9139..7b1ae32e2f 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -19,13 +19,16 @@ import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.Collections; import java.util.List; +import java.util.Set; import org.apache.hugegraph.HugeGraph; import org.apache.hugegraph.auth.AuthManager; import org.apache.hugegraph.auth.HugeAuthenticator; import org.apache.hugegraph.auth.HugeDefaultRole; import org.apache.hugegraph.auth.HugeGraphAuthProxy; +import org.apache.hugegraph.auth.HugePermission; import org.apache.hugegraph.auth.RolePermission; import org.apache.hugegraph.auth.UserWithRole; import org.apache.hugegraph.backend.id.IdGenerator; @@ -44,6 +47,8 @@ import org.apache.logging.log4j.core.appender.AbstractAppender; import org.apache.logging.log4j.core.config.LoggerConfig; import org.apache.logging.log4j.core.config.Property; +import org.apache.tinkerpop.gremlin.process.traversal.Bytecode; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal.Symbols; import org.junit.After; import org.junit.Test; import org.mockito.Mockito; @@ -366,6 +371,41 @@ public void testValidateUserDoesNotLogBearerToken() { } } + @Test + public void testTraversalPermissions() throws Exception { + Bytecode read = new Bytecode(); + read.addStep(Symbols.V); + Assert.assertTrue(traversalPermissions(read).isEmpty()); + + Bytecode write = new Bytecode(); + write.addStep(Symbols.addV, "person"); + write.addStep(Symbols.property, "name", "marko"); + Assert.assertEquals(Collections.singleton(HugePermission.WRITE), + traversalPermissions(write)); + + Bytecode delete = new Bytecode(); + delete.addStep(Symbols.V); + delete.addStep(Symbols.drop); + Assert.assertEquals(Collections.singleton(HugePermission.DELETE), + traversalPermissions(delete)); + + Bytecode nested = new Bytecode(); + nested.addStep(Symbols.addE, "knows"); + Bytecode parent = new Bytecode(); + parent.addStep(Symbols.sideEffect, nested); + Assert.assertEquals(Collections.singleton(HugePermission.WRITE), + traversalPermissions(parent)); + } + + @SuppressWarnings("unchecked") + private static Set traversalPermissions(Bytecode bytecode) + throws Exception { + Method method = HugeGraphAuthProxy.class.getDeclaredMethod( + "traversalPermissions", Bytecode.class); + method.setAccessible(true); + return (Set) method.invoke(null, bytecode); + } + private static class TestAppender extends AbstractAppender { private final List events; From ddfec942db74bb377dd3a2d39da54cabdfbe63f2 Mon Sep 17 00:00:00 2001 From: dark Date: Mon, 17 Aug 2026 23:50:08 +0800 Subject: [PATCH 06/23] fix(server): inspect traversal mutation steps - inspect realized traversal steps after script evaluation - cover vertex edge property and drop mutations - recurse through nested child traversals - keep read-only traversals executable --- .../hugegraph/auth/HugeGraphAuthProxy.java | 42 ++++++++++++------- .../unit/auth/HugeGraphAuthProxyTest.java | 30 ++++++------- 2 files changed, 39 insertions(+), 33 deletions(-) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java index 5b4e704608..429d0547d6 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java @@ -93,11 +93,18 @@ import org.apache.tinkerpop.gremlin.process.traversal.Bytecode; import org.apache.tinkerpop.gremlin.process.traversal.Bytecode.Instruction; import org.apache.tinkerpop.gremlin.process.traversal.Script; +import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; -import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal.Symbols; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.DropStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.AddEdgeStartStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.AddEdgeStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.AddVertexStartStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.AddVertexStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.AddPropertyStep; import org.apache.tinkerpop.gremlin.process.traversal.translator.GroovyTranslator; import org.apache.tinkerpop.gremlin.structure.Edge; import org.apache.tinkerpop.gremlin.structure.Element; @@ -2417,7 +2424,7 @@ public void apply(Traversal.Admin traversal) { String caller = Thread.currentThread().getName(); if (!caller.contains(TraversalStrategiesProxy.REST_WORKER)) { for (HugePermission permission : - traversalPermissions(traversal.getBytecode())) { + traversalPermissions(traversal)) { verifyNamePermission(permission, ResourceType.GREMLIN, script); } @@ -2469,28 +2476,33 @@ public String toString() { } } - private static Set traversalPermissions(Bytecode bytecode) { + private static Set traversalPermissions( + Traversal.Admin traversal) { Set permissions = EnumSet.noneOf(HugePermission.class); - collectTraversalPermissions(bytecode, permissions); + collectTraversalPermissions(traversal, permissions); return permissions; } private static void collectTraversalPermissions( - Bytecode bytecode, + Traversal.Admin traversal, Set permissions) { - for (Instruction instruction : bytecode.getStepInstructions()) { - String operator = instruction.getOperator(); - if (Symbols.addV.equals(operator) || - Symbols.addE.equals(operator) || - Symbols.property.equals(operator)) { + for (Step step : traversal.getSteps()) { + if (step instanceof AddVertexStartStep || + step instanceof AddVertexStep || + step instanceof AddEdgeStartStep || + step instanceof AddEdgeStep || + step instanceof AddPropertyStep) { permissions.add(HugePermission.WRITE); - } else if (Symbols.drop.equals(operator)) { + } else if (step instanceof DropStep) { permissions.add(HugePermission.DELETE); } - for (Object argument : instruction.getArguments()) { - if (argument instanceof Bytecode) { - collectTraversalPermissions((Bytecode) argument, - permissions); + if (step instanceof TraversalParent) { + TraversalParent parent = (TraversalParent) step; + for (Traversal.Admin child : parent.getLocalChildren()) { + collectTraversalPermissions(child, permissions); + } + for (Traversal.Admin child : parent.getGlobalChildren()) { + collectTraversalPermissions(child, permissions); } } } diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index 7b1ae32e2f..c80f130171 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -47,8 +47,8 @@ import org.apache.logging.log4j.core.appender.AbstractAppender; import org.apache.logging.log4j.core.config.LoggerConfig; import org.apache.logging.log4j.core.config.Property; -import org.apache.tinkerpop.gremlin.process.traversal.Bytecode; -import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal.Symbols; +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.junit.After; import org.junit.Test; import org.mockito.Mockito; @@ -373,37 +373,31 @@ public void testValidateUserDoesNotLogBearerToken() { @Test public void testTraversalPermissions() throws Exception { - Bytecode read = new Bytecode(); - read.addStep(Symbols.V); + Traversal.Admin read = __.V().asAdmin(); Assert.assertTrue(traversalPermissions(read).isEmpty()); - Bytecode write = new Bytecode(); - write.addStep(Symbols.addV, "person"); - write.addStep(Symbols.property, "name", "marko"); + Traversal.Admin write = + __.addV("person").property("name", "marko").asAdmin(); Assert.assertEquals(Collections.singleton(HugePermission.WRITE), traversalPermissions(write)); - Bytecode delete = new Bytecode(); - delete.addStep(Symbols.V); - delete.addStep(Symbols.drop); + Traversal.Admin delete = __.V().drop().asAdmin(); Assert.assertEquals(Collections.singleton(HugePermission.DELETE), traversalPermissions(delete)); - Bytecode nested = new Bytecode(); - nested.addStep(Symbols.addE, "knows"); - Bytecode parent = new Bytecode(); - parent.addStep(Symbols.sideEffect, nested); + Traversal.Admin parent = + __.V().sideEffect(__.addE("knows")).asAdmin(); Assert.assertEquals(Collections.singleton(HugePermission.WRITE), traversalPermissions(parent)); } @SuppressWarnings("unchecked") - private static Set traversalPermissions(Bytecode bytecode) - throws Exception { + private static Set traversalPermissions( + Traversal.Admin traversal) throws Exception { Method method = HugeGraphAuthProxy.class.getDeclaredMethod( - "traversalPermissions", Bytecode.class); + "traversalPermissions", Traversal.Admin.class); method.setAccessible(true); - return (Set) method.invoke(null, bytecode); + return (Set) method.invoke(null, traversal); } private static class TestAppender extends AbstractAppender { From 3d231314927698fedb07b12f81fef08e53b797dc Mon Sep 17 00:00:00 2001 From: dark Date: Mon, 17 Aug 2026 23:54:00 +0800 Subject: [PATCH 07/23] fix(server): proxy copied traversal strategies - keep auth wrappers when strategies are copied to script traversals - align strategy list behavior with its iterator - cover the copied-strategy contract - preserve structured mutation checks after script evaluation --- .../hugegraph/auth/HugeGraphAuthProxy.java | 4 ++- .../unit/auth/HugeGraphAuthProxyTest.java | 28 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java index 429d0547d6..8be1094509 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java @@ -2336,7 +2336,9 @@ public TraversalStrategiesProxy(TraversalStrategies strategies) { @Override public List> toList() { - return this.strategies.toList(); + List> proxies = new ArrayList<>(); + this.iterator().forEachRemaining(proxies::add); + return proxies; } @Override diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index c80f130171..c1594254d0 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -48,6 +48,7 @@ import org.apache.logging.log4j.core.config.LoggerConfig; import org.apache.logging.log4j.core.config.Property; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.junit.After; import org.junit.Test; @@ -391,6 +392,33 @@ public void testTraversalPermissions() throws Exception { traversalPermissions(parent)); } + @Test + public void testTraversalStrategyListKeepsAuthProxy() { + HugeGraph graph = Mockito.mock(HugeGraph.class); + HugeConfig config = Mockito.mock(HugeConfig.class); + AuthManager authManager = Mockito.mock(AuthManager.class); + TaskScheduler scheduler = Mockito.mock(TaskScheduler.class); + + Mockito.when(graph.spaceGraphName()).thenReturn("hugegraph"); + Mockito.when(graph.configuration()).thenReturn(config); + Mockito.when(graph.authManager()).thenReturn(authManager); + Mockito.when(graph.taskScheduler()).thenReturn(scheduler); + Mockito.when(config.get(AuthOptions.AUTH_CACHE_EXPIRE)) + .thenReturn(3600L); + Mockito.when(config.get(AuthOptions.AUTH_CACHE_CAPACITY)) + .thenReturn(100L); + Mockito.when(config.get(AuthOptions.AUTH_AUDIT_LOG_RATE)) + .thenReturn(1000D); + + GraphTraversalSource traversal = + new HugeGraphAuthProxy(graph).traversal(); + Assert.assertFalse(traversal.getStrategies().toList().isEmpty()); + traversal.getStrategies().toList().forEach(strategy -> { + Assert.assertEquals("TraversalStrategyProxy", + strategy.getClass().getSimpleName()); + }); + } + @SuppressWarnings("unchecked") private static Set traversalPermissions( Traversal.Admin traversal) throws Exception { From eae94010cfbae0a2bb2dbdd8c023d34136404695 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 00:02:53 +0800 Subject: [PATCH 08/23] fix(server): isolate GraphSpace membership - keep membership roles out of data action matching - preserve explicit read write and delete permissions - verify members can read without gaining mutations - retain direct GraphSpace administrator handling --- .../hugegraph/auth/HugeAuthenticator.java | 4 +++ .../unit/auth/HugeGraphAuthProxyTest.java | 31 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java index cef1287b14..3ec09c915e 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java @@ -290,6 +290,10 @@ private static Object matchedAction(HugePermission action, } for (Map.Entry e : perms.entrySet()) { HugePermission permission = e.getKey(); + if (permission == HugePermission.SPACE || + permission == HugePermission.SPACE_MEMBER) { + continue; + } // Maybe required = ANY if (action.match(permission) || action.equals(HugePermission.EXECUTE)) { diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index c1594254d0..b25adbc489 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -419,6 +419,37 @@ public void testTraversalStrategyListKeepsAuthProxy() { }); } + @Test + public void testSpaceMemberDoesNotGrantMutationPermissions() { + RolePermission role = RolePermission.fromJson( + "{\"roles\":{\"DEFAULT\":{\"*\":{" + + "\"READ\":{\"ALL\":[{\"type\":\"ALL\"}]}," + + "\"SPACE_MEMBER\":{\"ALL\":[{\"type\":\"ALL\"}]}" + + "}}}}"); + HugeAuthenticator.RequiredPerm read = + new HugeAuthenticator.RequiredPerm() + .graphSpace("DEFAULT") + .owner("hugegraph") + .action("read"); + HugeAuthenticator.RequiredPerm write = + new HugeAuthenticator.RequiredPerm() + .graphSpace("DEFAULT") + .owner("hugegraph") + .action("write"); + HugeAuthenticator.RequiredPerm delete = + new HugeAuthenticator.RequiredPerm() + .graphSpace("DEFAULT") + .owner("hugegraph") + .action("delete"); + + Assert.assertTrue(HugeAuthenticator.RolePerm.matchApiRequiredPerm( + role, read)); + Assert.assertFalse(HugeAuthenticator.RolePerm.matchApiRequiredPerm( + role, write)); + Assert.assertFalse(HugeAuthenticator.RolePerm.matchApiRequiredPerm( + role, delete)); + } + @SuppressWarnings("unchecked") private static Set traversalPermissions( Traversal.Admin traversal) throws Exception { From e2d7fff3eadd990205f2c028924aaf5d829de006 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 02:25:21 +0800 Subject: [PATCH 09/23] fix(server): prepare audit limiter after login - initialize audit limiter only after successful authentication - keep failed password and token attempts out of limiter state - invalidate limiter entries by username when deleting users - remove PD dynamic DNS and IP refresh from this PR - cover password token and cleanup paths with unit tests --- .../apache/hugegraph/pd/raft/PeerUtil.java | 43 +- .../apache/hugegraph/pd/raft/RaftEngine.java | 148 ++---- .../hugegraph/pd/raft/auth/IpAuthHandler.java | 429 +---------------- hugegraph-pd/hg-pd-service/pom.xml | 12 - .../hugegraph/pd/service/PDService.java | 119 +---- .../pd/service/PDServiceUpdateRaftTest.java | 195 -------- .../hugegraph/pd/core/PDCoreSuiteTest.java | 2 +- .../hugegraph/pd/raft/IpAuthHandlerTest.java | 133 ++++++ .../raft/RaftEngineIpAuthIntegrationTest.java | 81 +--- .../pd/raft/auth/IpAuthHandlerTest.java | 439 ------------------ .../hugegraph/auth/HugeGraphAuthProxy.java | 36 +- .../unit/auth/HugeGraphAuthProxyTest.java | 82 +++- 12 files changed, 333 insertions(+), 1386 deletions(-) delete mode 100644 hugegraph-pd/hg-pd-service/src/test/java/org/apache/hugegraph/pd/service/PDServiceUpdateRaftTest.java create mode 100644 hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/IpAuthHandlerTest.java delete mode 100644 hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandlerTest.java diff --git a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/PeerUtil.java b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/PeerUtil.java index bfffdf285c..265c7d4fc2 100644 --- a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/PeerUtil.java +++ b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/PeerUtil.java @@ -17,17 +17,15 @@ package org.apache.hugegraph.pd.raft; +import com.alipay.sofa.jraft.JRaftUtils; +import com.alipay.sofa.jraft.entity.PeerId; +import org.apache.hugegraph.pd.common.KVPair; + import java.util.LinkedList; import java.util.List; import java.util.Objects; -import org.apache.hugegraph.pd.common.KVPair; - -import com.alipay.sofa.jraft.conf.Configuration; -import com.alipay.sofa.jraft.entity.PeerId; - public class PeerUtil { - public static boolean isPeerEquals(PeerId p1, PeerId p2) { if (p1 == null && p2 == null) { return true; @@ -42,42 +40,19 @@ public static List> parseConfig(String conf) { List> result = new LinkedList<>(); if (conf != null && conf.length() > 0) { - for (var s : conf.split(",", -1)) { - String role; - String peer; + for (var s : conf.split(",")) { if (s.endsWith("/leader")) { - role = "leader"; - peer = s.substring(0, s.length() - 7); + result.add(new KVPair<>("leader", JRaftUtils.getPeerId(s.substring(0, s.length() - 7)))); } else if (s.endsWith("/learner")) { - role = "learner"; - peer = s.substring(0, s.length() - 8); + result.add(new KVPair<>("learner", JRaftUtils.getPeerId(s.substring(0, s.length() - 8)))); } else if (s.endsWith("/follower")) { - role = "follower"; - peer = s.substring(0, s.length() - 9); + result.add(new KVPair<>("follower", JRaftUtils.getPeerId(s.substring(0, s.length() - 9)))); } else { - role = "follower"; - peer = s; + result.add(new KVPair<>("follower", JRaftUtils.getPeerId(s))); } - result.add(new KVPair<>(role, parsePeer(peer))); } } return result; } - - public static Configuration parsePeerList(String peerList) { - Configuration configuration = new Configuration(); - for (String peer : peerList.split(",", -1)) { - configuration.addPeer(parsePeer(peer)); - } - return configuration; - } - - private static PeerId parsePeer(String value) { - PeerId peer = new PeerId(); - if (value.isEmpty() || !peer.parse(value)) { - throw new IllegalArgumentException("Invalid Raft peer: " + value); - } - return peer; - } } diff --git a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java index 81543ee1ef..2b08de7d4e 100644 --- a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java +++ b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java @@ -127,28 +127,14 @@ public synchronized boolean init(PDConfig.Raft config) { final PeerId serverId = JRaftUtils.getPeerId(config.getAddress()); - try { - rpcServer = createRaftRpcServer(config.getAddress(), initConf.getPeers()); - // construct raft group and start raft - this.raftGroupService = - new RaftGroupService(groupId, serverId, nodeOptions, - rpcServer, true); - this.raftNode = raftGroupService.start(false); - if (this.raftNode == null) { - this.shutDown(); - return false; - } - log.info("RaftEngine start successfully: id = {}, peers list = {}", - groupId, nodeOptions.getInitialConf().getPeers()); - return true; - } catch (RuntimeException | Error e) { - try { - this.shutDown(); - } catch (RuntimeException | Error cleanupFailure) { - e.addSuppressed(cleanupFailure); - } - throw e; - } + rpcServer = createRaftRpcServer(config.getAddress(), initConf.getPeers()); + // construct raft group and start raft + this.raftGroupService = + new RaftGroupService(groupId, serverId, nodeOptions, rpcServer, true); + this.raftNode = raftGroupService.start(false); + log.info("RaftEngine start successfully: id = {}, peers list = {}", groupId, + nodeOptions.getInitialConf().getPeers()); + return this.raftNode != null; } /** @@ -157,32 +143,13 @@ public synchronized boolean init(PDConfig.Raft config) { private RpcServer createRaftRpcServer(String raftAddr, List peers) { Endpoint endpoint = JRaftUtils.getEndPoint(raftAddr); RpcServer rpcServer = RaftRpcServerFactory.createRaftRpcServer(endpoint); - try { - IpAuthHandler ipAuthHandler = IpAuthHandler.getInstance( - peers.stream() - .map(PeerId::getIp) - .collect(Collectors.toSet())); - configureRaftServerIpWhitelist(ipAuthHandler, rpcServer); - RaftRpcProcessor.registerProcessor(rpcServer, this); - if (!rpcServer.init(null)) { - throw new IllegalStateException( - "Failed to initialize Raft RPC server"); - } - return rpcServer; - } catch (RuntimeException | Error e) { - try { - rpcServer.shutdown(); - } catch (RuntimeException | Error cleanupFailure) { - e.addSuppressed(cleanupFailure); - } finally { - IpAuthHandler.shutdownInstance(); - } - throw e; - } + configureRaftServerIpWhitelist(peers, rpcServer); + RaftRpcProcessor.registerProcessor(rpcServer, this); + rpcServer.init(null); + return rpcServer; } - private static void configureRaftServerIpWhitelist( - IpAuthHandler ipAuthHandler, RpcServer rpcServer) { + private static void configureRaftServerIpWhitelist(List peers, RpcServer rpcServer) { if (rpcServer instanceof BoltRpcServer) { ((BoltRpcServer) rpcServer).getServer().option( BoltServerOption.EXTENDED_NETTY_CHANNEL_HANDLER, @@ -190,7 +157,11 @@ private static void configureRaftServerIpWhitelist( @Override public List frontChannelHandlers() { return Collections.singletonList( - ipAuthHandler + IpAuthHandler.getInstance( + peers.stream() + .map(PeerId::getIp) + .collect(Collectors.toSet()) + ) ); } @@ -204,38 +175,24 @@ public List backChannelHandlers() { } public void shutDown() { - InterruptedException interrupted = null; - try { - if (this.raftGroupService != null) { - this.raftGroupService.shutdown(); - try { - this.raftGroupService.join(); - } catch (InterruptedException e) { - interrupted = e; - } - } - } finally { - this.raftGroupService = null; + if (this.raftGroupService != null) { + this.raftGroupService.shutdown(); try { - if (this.rpcServer != null) { - this.rpcServer.shutdown(); - } - } finally { - this.rpcServer = null; - try { - if (this.raftNode != null) { - this.raftNode.shutdown(); - } - } finally { - this.raftNode = null; - IpAuthHandler.shutdownInstance(); - } + this.raftGroupService.join(); + } catch (final InterruptedException e) { + this.raftNode = null; + ThrowUtil.throwException(e); } + this.raftGroupService = null; } - if (interrupted != null) { - Thread.currentThread().interrupt(); - ThrowUtil.throwException(interrupted); + if (this.rpcServer != null) { + this.rpcServer.shutdown(); + this.rpcServer = null; } + if (this.raftNode != null) { + this.raftNode.shutdown(); + } + this.raftNode = null; } public boolean isLeader() { @@ -395,43 +352,32 @@ public List getMembers() throws ExecutionException, InterruptedEx public Status changePeerList(String peerList) { AtomicReference result = new AtomicReference<>(); + Configuration newPeers = new Configuration(); try { - IpAuthHandler.validatePeerListShape(peerList); String[] peers = peerList.split(",", -1); if ((peers.length & 1) != 1) { throw new PDException(-1, "the number of peer list must be odd."); } - Configuration newPeers = PeerUtil.parsePeerList(peerList); - Set newIps = newPeers.getPeers() - .stream() - .map(PeerId::getIp) - .collect(Collectors.toSet()); - IpAuthHandler.validateAllowedEntries(newIps); - IpAuthHandler.requireActiveInstance(); + newPeers.parse(peerList); CountDownLatch latch = new CountDownLatch(1); this.raftNode.changePeers(newPeers, status -> { - Status callbackStatus = status; - try { - if (status != null && status.isOk()) { - IpAuthHandler.refreshInstance(newIps); + result.compareAndSet(null, status); + if (status != null && status.isOk()) { + IpAuthHandler handler = IpAuthHandler.getInstance(); + if (handler != null) { + Set newIps = newPeers.getPeers() + .stream() + .map(PeerId::getIp) + .collect(Collectors.toSet()); + handler.refresh(newIps); log.info("IpAuthHandler refreshed after peer list change to: {}", peerList); - } else if (status == null) { - callbackStatus = new Status( - RaftError.EINTERNAL, - "changePeers returned no status"); + } else { + log.warn("IpAuthHandler not initialized, skipping refresh for " + + "peer list: {}", peerList); } - } catch (RuntimeException e) { - callbackStatus = new Status( - RaftError.EINTERNAL, - "Raft peers changed but allowlist refresh failed: %s", - e.getMessage()); - log.error("Failed to refresh IpAuthHandler after peer list change to {}", - peerList, e); - } finally { - result.compareAndSet(null, callbackStatus); - latch.countDown(); } + latch.countDown(); }); boolean completed = latch.await(3L * config.getRpcTimeout(), TimeUnit.MILLISECONDS); if (!completed && result.get() == null) { diff --git a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandler.java b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandler.java index e81c86ecdb..bdccb6dd7f 100644 --- a/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandler.java +++ b/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandler.java @@ -19,119 +19,28 @@ import java.net.InetAddress; import java.net.InetSocketAddress; -import java.util.ArrayList; +import java.net.UnknownHostException; import java.util.Collections; -import java.util.HashMap; import java.util.HashSet; -import java.util.List; -import java.util.Map; import java.util.Set; -import java.util.concurrent.CancellationException; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import io.netty.channel.ChannelDuplexHandler; import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelHandlerContext; -import io.netty.channel.nio.NioEventLoopGroup; -import io.netty.channel.socket.nio.NioDatagramChannel; -import io.netty.resolver.dns.DnsNameResolver; -import io.netty.resolver.dns.DnsNameResolverBuilder; import lombok.extern.slf4j.Slf4j; @Slf4j @ChannelHandler.Sharable public class IpAuthHandler extends ChannelDuplexHandler { - private static final long DNS_QUERY_TIMEOUT_MILLIS = 500L; - private static final long DNS_STALE_MILLIS = 30_000L; - private static final long DNS_REFRESH_MILLIS = 1_000L; - private static final int MAX_CONCURRENT_DNS_QUERIES = 8; - private static final int MAX_ALLOWED_ENTRIES = 127; - private static final int MAX_HOST_LENGTH = 253; - private static final int MAX_PEER_LIST_LENGTH = - MAX_ALLOWED_ENTRIES * (MAX_HOST_LENGTH + 16); - - private final HostResolver resolver; - private final long queryTimeoutMillis; - private final long staleMillis; - private final long refreshMillis; - private final Map resolvedByEntry; - private final Map inFlight; - private final Set failedEntries; - private final ScheduledExecutorService refreshExecutor; - private boolean closed; - private int nextResolutionIndex; - private List resolutionOrder; - private volatile Set allowedEntries; private volatile Set resolvedIps; private static volatile IpAuthHandler instance; private IpAuthHandler(Set allowedIps) { - this(allowedIps, new NettyHostResolver(DNS_QUERY_TIMEOUT_MILLIS), true, - DNS_QUERY_TIMEOUT_MILLIS, DNS_STALE_MILLIS, - DNS_REFRESH_MILLIS); - } - - IpAuthHandler(Set allowedIps, HostResolver resolver, - boolean scheduleRefresh, long queryTimeoutMillis, - long staleMillis, long refreshMillis) { - this.resolver = resolver; - this.queryTimeoutMillis = queryTimeoutMillis; - this.staleMillis = staleMillis; - this.refreshMillis = refreshMillis; - this.resolvedByEntry = new HashMap<>(); - this.inFlight = new HashMap<>(); - this.failedEntries = new HashSet<>(); - this.nextResolutionIndex = 0; - this.resolutionOrder = Collections.emptyList(); - try { - this.replaceAllowedEntries(allowedIps); - } catch (RuntimeException | Error e) { - try { - this.resolver.close(); - } catch (RuntimeException | Error cleanupFailure) { - e.addSuppressed(cleanupFailure); - } - throw e; - } - this.resolvedIps = this.allowedEntries; - this.closed = false; - if (scheduleRefresh) { - this.refreshExecutor = Executors.newSingleThreadScheduledExecutor(task -> { - Thread thread = new Thread(task, "pd-raft-dns-resolver"); - thread.setDaemon(true); - return thread; - }); - } else { - this.refreshExecutor = null; - } - try { - this.refreshResolvedIps(); - if (this.refreshExecutor != null) { - this.refreshExecutor.scheduleWithFixedDelay( - this::refreshSafely, this.refreshMillis, - this.refreshMillis, TimeUnit.MILLISECONDS); - } - } catch (RuntimeException | Error e) { - if (this.refreshExecutor != null) { - this.refreshExecutor.shutdownNow(); - } - try { - this.resolver.close(); - } catch (RuntimeException | Error cleanupFailure) { - e.addSuppressed(cleanupFailure); - } - throw e; - } + this.resolvedIps = resolveAll(allowedIps); } public static IpAuthHandler getInstance(Set allowedIps) { - validateAllowedEntries(allowedIps); if (instance == null) { synchronized (IpAuthHandler.class) { if (instance == null) { @@ -150,48 +59,17 @@ public static IpAuthHandler getInstance() { return instance; } - public static IpAuthHandler requireActiveInstance() { - IpAuthHandler handler = instance; - if (handler == null || handler.isClosed()) { - throw new IllegalStateException( - "Raft peer IP allowlist is not active"); - } - return handler; - } - - public static void refreshInstance(Set newAllowedIps) { - requireActiveInstance().refresh(newAllowedIps); - } - /** * Refreshes the resolved IP allowlist from a new set of hostnames or IPs. * Should be called when the Raft peer list changes via RaftEngine#changePeerList(). - * DNS is also refreshed in the background so stable peer names can safely - * follow address changes without blocking a Netty event loop. + * Note: DNS-only changes (e.g. container restart with new IP, same hostname) + * are not automatically detected and still require a process restart. */ - public synchronized void refresh(Set newAllowedIps) { - if (this.closed) { - throw new IllegalStateException( - "Raft peer IP allowlist is closed"); - } - this.replaceAllowedEntries(newAllowedIps); - this.resolvedByEntry.keySet().retainAll(this.allowedEntries); - this.failedEntries.retainAll(this.allowedEntries); - this.inFlight.entrySet().removeIf(entry -> { - if (!this.allowedEntries.contains(entry.getKey())) { - entry.getValue().cancel(); - return true; - } - return false; - }); - this.refreshResolvedIps(); + public void refresh(Set newAllowedIps) { + this.resolvedIps = resolveAll(newAllowedIps); log.info("IpAuthHandler allowlist refreshed, resolved {} entries", resolvedIps.size()); } - private synchronized boolean isClosed() { - return this.closed; - } - @Override public void channelActive(ChannelHandlerContext ctx) throws Exception { String clientIp = getClientIp(ctx); @@ -214,301 +92,20 @@ private boolean isIpAllowed(String ip) { return resolved.isEmpty() || resolved.contains(ip); } - synchronized void refreshResolvedIps() { - this.refreshResolvedIps(true); - } + private static Set resolveAll(Set entries) { + Set result = new HashSet<>(entries); - synchronized void refreshResolvedIps(boolean waitForResults) { - if (this.closed) { - return; - } - Set entries = this.allowedEntries; - this.collectQueries(entries, false); - int attempted = 0; - while (this.inFlight.size() < MAX_CONCURRENT_DNS_QUERIES && - attempted < this.resolutionOrder.size()) { - String entry = this.resolutionOrder.get(this.nextResolutionIndex); - this.nextResolutionIndex = - (this.nextResolutionIndex + 1) % this.resolutionOrder.size(); - attempted++; - if (!this.inFlight.containsKey(entry)) { - this.inFlight.put( - entry, new Query(this.resolver.resolve(entry), - System.nanoTime())); - } - } - this.collectQueries(entries, waitForResults); - - long staleNanos = TimeUnit.MILLISECONDS.toNanos(this.staleMillis); - long now = System.nanoTime(); - this.resolvedByEntry.entrySet().removeIf( - entry -> now - entry.getValue().resolvedAtNanos > staleNanos); - Set resolved = new HashSet<>(entries); - this.resolvedByEntry.values().forEach( - entry -> resolved.addAll(entry.addresses)); - this.resolvedIps = Collections.unmodifiableSet(resolved); - } - - private void collectQueries(Set entries, - boolean waitForResults) { - long deadline = System.nanoTime() + - TimeUnit.MILLISECONDS.toNanos(this.queryTimeoutMillis); for (String entry : entries) { - Query query = this.inFlight.get(entry); - if (query == null) { - continue; - } - CompletableFuture future = query.future; try { - ResolvedQuery result; - if (future.isDone()) { - result = future.get(); - } else if (waitForResults) { - long remaining = deadline - System.nanoTime(); - if (remaining <= 0L) { - expireQuery(entry, query); - continue; - } - result = future.get(remaining, TimeUnit.NANOSECONDS); - } else { - long elapsed = System.nanoTime() - query.startedAtNanos; - if (elapsed > TimeUnit.MILLISECONDS.toNanos( - this.queryTimeoutMillis)) { - expireQuery(entry, query); - } - continue; - } - if (result.completedAtNanos - query.startedAtNanos > - TimeUnit.MILLISECONDS.toNanos(this.queryTimeoutMillis)) { - expireQuery(entry, query); - continue; - } - this.resolvedByEntry.put( - entry, new ResolvedEntry(result.addresses, - System.nanoTime())); - this.inFlight.remove(entry); - if (this.failedEntries.remove(entry)) { - log.info("Raft peer address resolution recovered for '{}'", entry); + for (InetAddress addr : InetAddress.getAllByName(entry)) { + result.add(addr.getHostAddress()); } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - markResolutionFailure(entry, e); - throw new IllegalStateException( - "Raft peer address refresh interrupted", e); - } catch (ExecutionException e) { - this.inFlight.remove(entry); - markResolutionFailure(entry, e); - } catch (TimeoutException e) { - expireQuery(entry, query); - } catch (CancellationException e) { - this.inFlight.remove(entry); - markResolutionFailure(entry, e); - } - } - } - - private void expireQuery(String entry, Query query) { - query.cancel(); - this.inFlight.remove(entry); - markResolutionFailure( - entry, new TimeoutException("DNS refresh deadline")); - } - - private void markResolutionFailure(String entry, Exception failure) { - if (this.failedEntries.add(entry)) { - log.warn("Could not resolve Raft peer allowlist entry '{}': {}", - entry, failure.getMessage()); - } - } - - private void refreshSafely() { - try { - this.refreshResolvedIps(false); - } catch (RuntimeException e) { - log.error("Unexpected Raft peer allowlist refresh failure", e); - } - } - - private void replaceAllowedEntries(Set entries) { - validateAllowedEntries(entries); - Set copy = new HashSet<>(entries); - if (copy.equals(this.allowedEntries)) { - return; - } - this.allowedEntries = Collections.unmodifiableSet(copy); - this.resolutionOrder = new ArrayList<>(copy); - Collections.sort(this.resolutionOrder); - this.nextResolutionIndex = 0; - } - - public static void validateAllowedEntries(Set entries) { - if (entries.size() > MAX_ALLOWED_ENTRIES) { - throw new IllegalArgumentException( - "Raft peer allowlist exceeds " + MAX_ALLOWED_ENTRIES + - " entries"); - } - for (String entry : entries) { - if (entry == null || entry.isEmpty() || - entry.length() > MAX_HOST_LENGTH) { - throw new IllegalArgumentException( - "Invalid Raft peer allowlist entry"); + } catch (UnknownHostException e) { + log.warn("Could not resolve allowlist entry '{}': {}", entry, e.getMessage()); } } - } - - public static void validatePeerListShape(String peerList) { - if (peerList == null || peerList.isEmpty() || - peerList.length() > MAX_PEER_LIST_LENGTH) { - throw new IllegalArgumentException( - "Invalid Raft peer list length"); - } - int entries = 1; - for (int i = 0; i < peerList.length(); i++) { - if (peerList.charAt(i) == ',' && - ++entries > MAX_ALLOWED_ENTRIES) { - throw new IllegalArgumentException( - "Raft peer list exceeds " + MAX_ALLOWED_ENTRIES + - " entries"); - } - } - } - - synchronized void shutdown() { - if (this.closed) { - return; - } - this.closed = true; - if (this.refreshExecutor != null) { - this.refreshExecutor.shutdownNow(); - } - this.inFlight.values().forEach(Query::cancel); - this.inFlight.clear(); - this.resolver.close(); - } - - public static synchronized void shutdownInstance() { - if (instance != null) { - instance.shutdown(); - instance = null; - } - } - - @FunctionalInterface - interface HostResolver extends AutoCloseable { - - CompletableFuture> resolve(String host); - - @Override - default void close() { - // Most injected resolvers do not own resources. - } - } - - private static final class ResolvedEntry { - - private final Set addresses; - private final long resolvedAtNanos; - - private ResolvedEntry(Set addresses, - long resolvedAtNanos) { - this.addresses = addresses; - this.resolvedAtNanos = resolvedAtNanos; - } - } - - private static final class Query { - - private final CompletableFuture> source; - private final CompletableFuture future; - private final long startedAtNanos; - private Query(CompletableFuture> source, - long startedAtNanos) { - this.source = source; - this.startedAtNanos = startedAtNanos; - this.future = source.thenApply( - addresses -> new ResolvedQuery(addresses, - System.nanoTime())); - } - - private void cancel() { - this.source.cancel(true); - this.future.cancel(true); - } - } - - private static final class ResolvedQuery { - - private final Set addresses; - private final long completedAtNanos; - - private ResolvedQuery(Set addresses, - long completedAtNanos) { - this.addresses = addresses; - this.completedAtNanos = completedAtNanos; - } - } - - private static final class NettyHostResolver implements HostResolver { - - private final NioEventLoopGroup eventLoopGroup; - private final DnsNameResolver resolver; - - private NettyHostResolver(long queryTimeoutMillis) { - this.eventLoopGroup = new NioEventLoopGroup(1, task -> { - Thread thread = new Thread(task, "pd-raft-dns-event-loop"); - thread.setDaemon(true); - return thread; - }); - try { - this.resolver = new DnsNameResolverBuilder( - this.eventLoopGroup.next()) - .channelType(NioDatagramChannel.class) - .ttl(0, 1) - .negativeTtl(0) - .queryTimeoutMillis(queryTimeoutMillis) - .build(); - } catch (RuntimeException | Error e) { - this.eventLoopGroup.shutdownGracefully( - 0L, DNS_QUERY_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS) - .awaitUninterruptibly( - DNS_QUERY_TIMEOUT_MILLIS); - throw e; - } - } - - @Override - public CompletableFuture> resolve(String host) { - io.netty.util.concurrent.Future> query = - this.resolver.resolveAll(host); - CompletableFuture> result = new CompletableFuture<>(); - query.addListener(done -> { - if (!done.isSuccess()) { - result.completeExceptionally(done.cause()); - return; - } - Set addresses = new HashSet<>(); - for (InetAddress address : query.getNow()) { - addresses.add(address.getHostAddress()); - } - result.complete(Collections.unmodifiableSet(addresses)); - }); - result.whenComplete((ignored, failure) -> { - if (result.isCancelled()) { - query.cancel(true); - } - }); - return result; - } - - @Override - public void close() { - this.resolver.close(); - this.eventLoopGroup.shutdownGracefully( - 0L, DNS_QUERY_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS) - .awaitUninterruptibly( - DNS_QUERY_TIMEOUT_MILLIS); - } + return Collections.unmodifiableSet(result); } @Override diff --git a/hugegraph-pd/hg-pd-service/pom.xml b/hugegraph-pd/hg-pd-service/pom.xml index 7ffb9ccd6d..ee78863f35 100644 --- a/hugegraph-pd/hg-pd-service/pom.xml +++ b/hugegraph-pd/hg-pd-service/pom.xml @@ -162,18 +162,6 @@ log4j-jul 2.17.2 - - junit - junit - ${junit.version} - test - - - org.mockito - mockito-core - 3.9.0 - test - diff --git a/hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/PDService.java b/hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/PDService.java index b31be3bb11..94d136a844 100644 --- a/hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/PDService.java +++ b/hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/PDService.java @@ -27,10 +27,8 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import javax.annotation.PostConstruct; @@ -101,7 +99,6 @@ import com.alipay.sofa.jraft.Status; import com.alipay.sofa.jraft.conf.Configuration; import com.alipay.sofa.jraft.entity.PeerId; -import com.alipay.sofa.jraft.error.RaftError; import io.grpc.ManagedChannel; import io.grpc.stub.StreamObserver; @@ -1686,20 +1683,7 @@ public void updatePdRaft(Pdpb.UpdatePdRaftRequest request, return; } - List> list; - try { - IpAuthHandler.validatePeerListShape(request.getConfig()); - list = PeerUtil.parseConfig(request.getConfig()); - } catch (IllegalArgumentException e) { - Pdpb.UpdatePdRaftResponse response = - Pdpb.UpdatePdRaftResponse.newBuilder() - .setHeader(newErrorHeader( - 6668, e.getMessage())) - .build(); - observer.onNext(response); - observer.onCompleted(); - return; - } + var list = PeerUtil.parseConfig(request.getConfig()); log.info("update raft request: {}, list: {}", request.getConfig(), list); @@ -1748,93 +1732,28 @@ public void updatePdRaft(Pdpb.UpdatePdRaftRequest request, } } - Set newIps = new HashSet<>(); - config.getPeers().forEach(peer -> newIps.add(peer.getIp())); - config.getLearners().forEach(peer -> newIps.add(peer.getIp())); - try { - IpAuthHandler.validateAllowedEntries(newIps); - IpAuthHandler.requireActiveInstance(); - } catch (IllegalArgumentException e) { - response = Pdpb.UpdatePdRaftResponse.newBuilder() - .setHeader(newErrorHeader( - 6668, - e.getMessage())) - .build(); - break; - } catch (IllegalStateException e) { - response = Pdpb.UpdatePdRaftResponse.newBuilder() - .setHeader(newErrorHeader( - 6670, - e.getMessage())) - .build(); - break; - } - log.info("pd raft update with new config: {}", config); - CountDownLatch changeLatch = new CountDownLatch(1); - AtomicReference changeStatus = new AtomicReference<>(); - try { - node.changePeers(config, status -> { - Status callbackStatus = status; - try { - if (status != null && status.isOk()) { - log.info("updatePdRaft, change peers success"); - IpAuthHandler.refreshInstance(newIps); - log.info("IpAuthHandler refreshed after updatePdRaft peer change"); - } else if (status != null) { - log.error("changePeers status: {}, msg:{}, code: {}, raft error:{}", - status, status.getErrorMsg(), status.getCode(), - status.getRaftError()); - } else { - callbackStatus = new Status( - RaftError.EINTERNAL, - "changePeers returned no status"); - } - } catch (RuntimeException e) { - callbackStatus = new Status( - RaftError.EINTERNAL, - "Raft peers changed but allowlist refresh failed: %s", - e.getMessage()); - log.error("Raft peers changed but IpAuthHandler refresh failed", - e); - } finally { - changeStatus.set(callbackStatus); - changeLatch.countDown(); + node.changePeers(config, status -> { + if (status.isOk()) { + log.info("updatePdRaft, change peers success"); + // Refresh IpAuthHandler so newly added peers are not blocked + IpAuthHandler handler = IpAuthHandler.getInstance(); + if (handler != null) { + Set newIps = new HashSet<>(); + config.getPeers().forEach(p -> newIps.add(p.getIp())); + config.getLearners().forEach(p -> newIps.add(p.getIp())); + handler.refresh(newIps); + log.info("IpAuthHandler refreshed after updatePdRaft peer change"); + } else { + log.warn("IpAuthHandler not initialized, skipping refresh"); } - }); - long timeout = 3L * pdConfig.getRaft().getRpcTimeout(); - if (!changeLatch.await(timeout, TimeUnit.MILLISECONDS)) { - response = Pdpb.UpdatePdRaftResponse.newBuilder() - .setHeader(newErrorHeader( - 6669, - "changePeers timed out")) - .build(); - } else if (changeStatus.get() == null || - !changeStatus.get().isOk()) { - String message = changeStatus.get() == null ? - "changePeers returned no status" : - changeStatus.get().getErrorMsg(); - response = Pdpb.UpdatePdRaftResponse.newBuilder() - .setHeader(newErrorHeader( - 6670, message)) - .build(); + } else { + log.error("changePeers status: {}, msg:{}, code: {}, raft error:{}", + status, status.getErrorMsg(), status.getCode(), + status.getRaftError()); } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - response = Pdpb.UpdatePdRaftResponse.newBuilder() - .setHeader(newErrorHeader( - 6670, - "changePeers interrupted")) - .build(); - } catch (RuntimeException e) { - log.error("changePeers failed before callback", e); - response = Pdpb.UpdatePdRaftResponse.newBuilder() - .setHeader(newErrorHeader( - 6670, - e.getMessage())) - .build(); - } + }); } while (false); observer.onNext(response); diff --git a/hugegraph-pd/hg-pd-service/src/test/java/org/apache/hugegraph/pd/service/PDServiceUpdateRaftTest.java b/hugegraph-pd/hg-pd-service/src/test/java/org/apache/hugegraph/pd/service/PDServiceUpdateRaftTest.java deleted file mode 100644 index d7ee1401c7..0000000000 --- a/hugegraph-pd/hg-pd-service/src/test/java/org/apache/hugegraph/pd/service/PDServiceUpdateRaftTest.java +++ /dev/null @@ -1,195 +0,0 @@ -/* - * 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.pd.service; - -import java.util.Collections; - -import org.apache.hugegraph.pd.config.PDConfig; -import org.apache.hugegraph.pd.grpc.Pdpb; -import org.apache.hugegraph.pd.raft.RaftEngine; -import org.apache.hugegraph.pd.raft.auth.IpAuthHandler; -import org.apache.hugegraph.testutil.Whitebox; -import org.junit.After; -import org.junit.Assert; -import org.junit.Before; -import org.junit.Test; -import org.mockito.ArgumentCaptor; - -import com.alipay.sofa.jraft.Closure; -import com.alipay.sofa.jraft.Node; -import com.alipay.sofa.jraft.Status; -import com.alipay.sofa.jraft.conf.Configuration; -import com.alipay.sofa.jraft.entity.PeerId; -import com.alipay.sofa.jraft.error.RaftError; - -import io.grpc.stub.StreamObserver; - -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -public class PDServiceUpdateRaftTest { - - private Node originalRaftNode; - private Node mockNode; - private PDService service; - private PeerId leader; - - @Before - public void setUp() { - this.originalRaftNode = RaftEngine.getInstance().getRaftNode(); - IpAuthHandler.shutdownInstance(); - - this.leader = new PeerId(); - Assert.assertTrue(this.leader.parse("127.0.0.1:8610")); - this.mockNode = mock(Node.class); - when(this.mockNode.isLeader(true)).thenReturn(true); - when(this.mockNode.getLeaderId()).thenReturn(this.leader); - when(this.mockNode.listPeers()).thenReturn( - Collections.singletonList(this.leader)); - Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", - this.mockNode); - IpAuthHandler.getInstance(Collections.singleton("127.0.0.1")); - - PDConfig pdConfig = new PDConfig(); - PDConfig.Raft raft = pdConfig.new Raft(); - raft.setRpcTimeout(1); - pdConfig.setRaft(raft); - this.service = new PDService(); - this.service.setInitConfig(pdConfig); - } - - @After - public void tearDown() { - Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", - this.originalRaftNode); - IpAuthHandler.shutdownInstance(); - } - - @Test - public void testRejectsMalformedConfigBeforeRaft() { - Pdpb.UpdatePdRaftResponse response = update( - "127.0.0.1:8610/leader,bad,127.0.0.2:8610/follower"); - - Assert.assertEquals(6668, response.getHeader().getError().getTypeValue()); - Assert.assertTrue(response.getHeader().getError().getMessage() - .contains("Invalid Raft peer")); - verify(this.mockNode, never()).changePeers( - any(Configuration.class), any(Closure.class)); - } - - @Test - public void testReturnsSuccessAfterRaftCallbackAndAllowlistRefresh() - throws Exception { - IpAuthHandler handler = IpAuthHandler.requireActiveInstance(); - handler.refresh(Collections.singleton("10.0.0.1")); - doAnswer(invocation -> { - Closure closure = invocation.getArgument(1); - closure.run(Status.OK()); - return null; - }).when(this.mockNode).changePeers(any(Configuration.class), - any(Closure.class)); - - Pdpb.UpdatePdRaftResponse response = update( - "127.0.0.1:8610/leader"); - - Assert.assertEquals(Pdpb.ErrorType.OK, - response.getHeader().getError().getType()); - Assert.assertTrue(isIpAllowed(handler, "127.0.0.1")); - Assert.assertFalse(isIpAllowed(handler, "10.0.0.1")); - } - - @Test - public void testReturnsRaftFailureFromCallback() { - doAnswer(invocation -> { - Closure closure = invocation.getArgument(1); - closure.run(new Status(RaftError.EINTERNAL, "simulated failure")); - return null; - }).when(this.mockNode).changePeers(any(Configuration.class), - any(Closure.class)); - - Pdpb.UpdatePdRaftResponse response = update( - "127.0.0.1:8610/leader"); - - Assert.assertEquals(6670, response.getHeader().getError().getTypeValue()); - Assert.assertTrue(response.getHeader().getError().getMessage() - .contains("simulated failure")); - } - - @Test - public void testReturnsTimeoutWhenRaftDoesNotCallback() { - Pdpb.UpdatePdRaftResponse response = update( - "127.0.0.1:8610/leader"); - - Assert.assertEquals(6669, response.getHeader().getError().getTypeValue()); - Assert.assertTrue(response.getHeader().getError().getMessage() - .contains("timed out")); - } - - @Test - public void testRejectsMissingAllowlistBeforeRaft() { - IpAuthHandler.shutdownInstance(); - - Pdpb.UpdatePdRaftResponse response = update( - "127.0.0.1:8610/leader"); - - Assert.assertEquals(6670, response.getHeader().getError().getTypeValue()); - Assert.assertTrue(response.getHeader().getError().getMessage() - .contains("not active")); - verify(this.mockNode, never()).changePeers( - any(Configuration.class), any(Closure.class)); - } - - @Test - public void testMapsSynchronousRaftFailure() { - doThrow(new IllegalStateException("node stopped")) - .when(this.mockNode) - .changePeers(any(Configuration.class), any(Closure.class)); - - Pdpb.UpdatePdRaftResponse response = update( - "127.0.0.1:8610/leader"); - - Assert.assertEquals(6670, response.getHeader().getError().getTypeValue()); - Assert.assertTrue(response.getHeader().getError().getMessage() - .contains("node stopped")); - } - - @SuppressWarnings("unchecked") - private Pdpb.UpdatePdRaftResponse update(String config) { - StreamObserver observer = - mock(StreamObserver.class); - this.service.updatePdRaft( - Pdpb.UpdatePdRaftRequest.newBuilder().setConfig(config).build(), - observer); - ArgumentCaptor response = - ArgumentCaptor.forClass(Pdpb.UpdatePdRaftResponse.class); - verify(observer).onNext(response.capture()); - verify(observer).onCompleted(); - return response.getValue(); - } - - private boolean isIpAllowed(IpAuthHandler handler, String ip) { - return Whitebox.invoke(IpAuthHandler.class, - new Class[]{String.class}, - "isIpAllowed", handler, ip); - } -} diff --git a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/PDCoreSuiteTest.java b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/PDCoreSuiteTest.java index 613d085594..95b044c76b 100644 --- a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/PDCoreSuiteTest.java +++ b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/PDCoreSuiteTest.java @@ -19,7 +19,7 @@ import org.apache.hugegraph.pd.core.meta.MetadataKeyHelperTest; import org.apache.hugegraph.pd.core.store.HgKVStoreImplTest; -import org.apache.hugegraph.pd.raft.auth.IpAuthHandlerTest; +import org.apache.hugegraph.pd.raft.IpAuthHandlerTest; import org.apache.hugegraph.pd.raft.RaftEngineIpAuthIntegrationTest; import org.apache.hugegraph.pd.raft.RaftEngineLeaderAddressTest; import org.junit.runner.RunWith; diff --git a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/IpAuthHandlerTest.java b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/IpAuthHandlerTest.java new file mode 100644 index 0000000000..31647b6d39 --- /dev/null +++ b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/IpAuthHandlerTest.java @@ -0,0 +1,133 @@ +/* + * 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.pd.raft; + +import java.net.InetAddress; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + +import org.apache.hugegraph.pd.raft.auth.IpAuthHandler; +import org.apache.hugegraph.testutil.Whitebox; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +public class IpAuthHandlerTest { + + @Before + public void setUp() { + // Must reset BEFORE each test — earlier suite classes (e.g. ConfigServiceTest) + // initialize RaftEngine which creates the IpAuthHandler singleton with their + // own peer IPs. Without this reset, our getInstance() calls return the stale + // singleton and ignore the allowlist passed by the test. + Whitebox.setInternalState(IpAuthHandler.class, "instance", null); + } + + @After + public void tearDown() { + // Must reset AFTER each test — prevents our test singleton from leaking + // into later suite classes that also depend on IpAuthHandler state. + Whitebox.setInternalState(IpAuthHandler.class, "instance", null); + } + + private boolean isIpAllowed(IpAuthHandler handler, String ip) { + return Whitebox.invoke(IpAuthHandler.class, + new Class[]{String.class}, + "isIpAllowed", handler, ip); + } + + @Test + public void testHostnameResolvesToIp() throws Exception { + // "localhost" should resolve to one or more IPs via InetAddress.getAllByName() + // This verifies the core fix: hostname allowlists match numeric remote addresses + // Using dynamic resolution avoids hardcoding "127.0.0.1" which may not be + // returned on IPv6-only or custom resolver environments + IpAuthHandler handler = IpAuthHandler.getInstance( + Collections.singleton("localhost")); + InetAddress[] addresses = InetAddress.getAllByName("localhost"); + // All resolved addresses should be allowed — resolveAll() adds every address + // returned by getAllByName() so none should be blocked + Assert.assertTrue("Expected at least one resolved address", + addresses.length > 0); + for (InetAddress address : addresses) { + Assert.assertTrue( + "Expected " + address.getHostAddress() + " to be allowed", + isIpAllowed(handler, address.getHostAddress())); + } + } + + @Test + public void testUnresolvableHostnameDoesNotCrash() { + // Should log a warning and skip — no exception thrown during construction + // Uses .invalid TLD which is RFC-2606 reserved and guaranteed to never resolve + IpAuthHandler handler = IpAuthHandler.getInstance( + Collections.singleton("nonexistent.invalid")); + // Handler was still created successfully despite bad hostname + Assert.assertNotNull(handler); + // Unresolvable entry is skipped so no IPs should be allowed + Assert.assertFalse(isIpAllowed(handler, "127.0.0.1")); + Assert.assertFalse(isIpAllowed(handler, "192.168.0.1")); + } + + @Test + public void testRefreshUpdatesResolvedIps() { + // Start with 127.0.0.1 + IpAuthHandler handler = IpAuthHandler.getInstance( + Collections.singleton("127.0.0.1")); + Assert.assertTrue(isIpAllowed(handler, "127.0.0.1")); + + // Refresh with a different IP — verifies refresh() swaps the set correctly + Set newIps = new HashSet<>(); + newIps.add("192.168.0.1"); + handler.refresh(newIps); + + // Old IP should no longer be allowed + Assert.assertFalse(isIpAllowed(handler, "127.0.0.1")); + // New IP should now be allowed + Assert.assertTrue(isIpAllowed(handler, "192.168.0.1")); + } + + @Test + public void testEmptyAllowlistAllowsAll() { + // Empty allowlist = no restriction configured = allow all connections + // This is intentional fallback behavior and must be explicitly tested + // because it is a security-relevant boundary + IpAuthHandler handler = IpAuthHandler.getInstance( + Collections.emptySet()); + Assert.assertTrue(isIpAllowed(handler, "1.2.3.4")); + Assert.assertTrue(isIpAllowed(handler, "192.168.99.99")); + } + + @Test + public void testGetInstanceReturnsSingletonIgnoresNewAllowlist() { + // First call creates the singleton with 127.0.0.1 + IpAuthHandler first = IpAuthHandler.getInstance( + Collections.singleton("127.0.0.1")); + // Second call with a different set must return the same instance + // and must NOT reinitialize or override the existing allowlist + IpAuthHandler second = IpAuthHandler.getInstance( + Collections.singleton("192.168.0.1")); + Assert.assertSame(first, second); + // Original allowlist still in effect + Assert.assertTrue(isIpAllowed(second, "127.0.0.1")); + // New set was ignored — 192.168.0.1 should not be allowed + Assert.assertFalse(isIpAllowed(second, "192.168.0.1")); + } +} diff --git a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineIpAuthIntegrationTest.java b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineIpAuthIntegrationTest.java index 1aa2921748..1f9857df0f 100644 --- a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineIpAuthIntegrationTest.java +++ b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineIpAuthIntegrationTest.java @@ -19,7 +19,6 @@ import java.util.Collections; -import org.apache.hugegraph.pd.config.PDConfig; import org.apache.hugegraph.pd.raft.auth.IpAuthHandler; import org.apache.hugegraph.testutil.Whitebox; import org.junit.After; @@ -36,35 +35,25 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; public class RaftEngineIpAuthIntegrationTest { private Node originalRaftNode; - private PDConfig.Raft originalConfig; @Before public void setUp() { // Save original raftNode so we can restore it after the test originalRaftNode = RaftEngine.getInstance().getRaftNode(); - originalConfig = Whitebox.getInternalState(RaftEngine.getInstance(), - "config"); - PDConfig pdConfig = new PDConfig(); - PDConfig.Raft config = pdConfig.new Raft(); - config.setRpcTimeout(100); - Whitebox.setInternalState(RaftEngine.getInstance(), "config", config); // Reset IpAuthHandler singleton for a clean state - IpAuthHandler.shutdownInstance(); + Whitebox.setInternalState(IpAuthHandler.class, "instance", null); } @After public void tearDown() { // Restore original raftNode Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", originalRaftNode); - Whitebox.setInternalState(RaftEngine.getInstance(), "config", originalConfig); // Reset IpAuthHandler singleton - IpAuthHandler.shutdownInstance(); + Whitebox.setInternalState(IpAuthHandler.class, "instance", null); } @Test @@ -91,11 +80,9 @@ public void testChangePeerListRefreshesIpAuthHandler() throws Exception { Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", mockNode); // Call changePeerList with new peer — must be odd count - Status status = RaftEngine.getInstance().changePeerList( - "127.0.0.1:8610"); + RaftEngine.getInstance().changePeerList("127.0.0.1:8610"); // Verify IpAuthHandler was refreshed with the new peer IP - Assert.assertTrue(status.isOk()); Assert.assertTrue(invokeIsIpAllowed(handler, "127.0.0.1")); // Old IP should no longer be allowed Assert.assertFalse(invokeIsIpAllowed(handler, "10.0.0.1")); @@ -122,73 +109,13 @@ public void testChangePeerListDoesNotRefreshOnFailure() throws Exception { Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", mockNode); - Status status = RaftEngine.getInstance().changePeerList( - "127.0.0.1:8610"); + RaftEngine.getInstance().changePeerList("127.0.0.1:8610"); // Handler should NOT be refreshed — old IP still allowed - Assert.assertFalse(status.isOk()); Assert.assertTrue(invokeIsIpAllowed(handler, "10.0.0.1")); Assert.assertFalse(invokeIsIpAllowed(handler, "127.0.0.1")); } - @Test - public void testChangePeerListRejectsNullCallbackStatus() { - IpAuthHandler.getInstance(Collections.singleton("10.0.0.1")); - Node mockNode = mock(Node.class); - doAnswer(invocation -> { - Closure closure = invocation.getArgument(1); - closure.run(null); - return null; - }).when(mockNode).changePeers(any(Configuration.class), - any(Closure.class)); - Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", - mockNode); - - Status status = RaftEngine.getInstance().changePeerList( - "127.0.0.1:8610"); - - Assert.assertNotNull(status); - Assert.assertFalse(status.isOk()); - Assert.assertTrue(status.getErrorMsg() - .contains("returned no status")); - } - - @Test - public void testChangePeerListRejectsOversizedAllowlistBeforeRaft() { - Node mockNode = mock(Node.class); - Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", mockNode); - StringBuilder peers = new StringBuilder(); - for (int i = 0; i < 129; i++) { - if (i > 0) { - peers.append(','); - } - peers.append("pd-").append(i).append(":8610"); - } - - Status status = RaftEngine.getInstance().changePeerList( - peers.toString()); - - Assert.assertNotNull(status); - Assert.assertFalse(status.isOk()); - verify(mockNode, never()).changePeers( - any(Configuration.class), any(Closure.class)); - } - - @Test - public void testChangePeerListRejectsMalformedPeerBeforeRaft() { - Node mockNode = mock(Node.class); - Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", mockNode); - - Status status = RaftEngine.getInstance().changePeerList( - "127.0.0.1:8610,bad,127.0.0.2:8610"); - - Assert.assertNotNull(status); - Assert.assertFalse(status.isOk()); - Assert.assertTrue(status.getErrorMsg().contains("Invalid Raft peer")); - verify(mockNode, never()).changePeers( - any(Configuration.class), any(Closure.class)); - } - private boolean invokeIsIpAllowed(IpAuthHandler handler, String ip) { return Whitebox.invoke(IpAuthHandler.class, new Class[]{String.class}, diff --git a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandlerTest.java b/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandlerTest.java deleted file mode 100644 index 833d1eeaa0..0000000000 --- a/hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandlerTest.java +++ /dev/null @@ -1,439 +0,0 @@ -/* - * 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.pd.raft.auth; - -import java.net.InetAddress; -import java.net.UnknownHostException; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.LinkedHashSet; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicReference; - -import org.apache.hugegraph.testutil.Whitebox; -import org.junit.After; -import org.junit.Assert; -import org.junit.Before; -import org.junit.Test; - -public class IpAuthHandlerTest { - - @Before - public void setUp() { - // Must reset BEFORE each test — earlier suite classes (e.g. ConfigServiceTest) - // initialize RaftEngine which creates the IpAuthHandler singleton with their - // own peer IPs. Without this reset, our getInstance() calls return the stale - // singleton and ignore the allowlist passed by the test. - IpAuthHandler.shutdownInstance(); - } - - @After - public void tearDown() { - // Must reset AFTER each test — prevents our test singleton from leaking - // into later suite classes that also depend on IpAuthHandler state. - IpAuthHandler handler = IpAuthHandler.getInstance(); - if (handler != null) { - IpAuthHandler.shutdownInstance(); - } - } - - private boolean isIpAllowed(IpAuthHandler handler, String ip) { - return Whitebox.invoke(IpAuthHandler.class, - new Class[]{String.class}, - "isIpAllowed", handler, ip); - } - - @Test - public void testHostnameResolvesToIp() throws Exception { - // "localhost" should resolve to one or more IPs via InetAddress.getAllByName() - // This verifies the core fix: hostname allowlists match numeric remote addresses - // Using dynamic resolution avoids hardcoding "127.0.0.1" which may not be - // returned on IPv6-only or custom resolver environments - IpAuthHandler handler = IpAuthHandler.getInstance( - Collections.singleton("localhost")); - InetAddress[] addresses = InetAddress.getAllByName("localhost"); - Assert.assertTrue("Expected at least one resolved address", - addresses.length > 0); - boolean matched = false; - for (InetAddress address : addresses) { - matched |= isIpAllowed(handler, address.getHostAddress()); - } - Assert.assertTrue("Expected a resolved address to be allowed", matched); - } - - @Test - public void testTransientDnsFailureRecoversOnRefresh() throws Exception { - AtomicInteger attempts = new AtomicInteger(); - InetAddress expected = InetAddress.getByAddress( - new byte[]{(byte) 192, (byte) 168, 0, 1}); - IpAuthHandler handler = new IpAuthHandler( - Collections.singleton("pd-1"), - host -> { - if (attempts.incrementAndGet() < 3) { - return failed(host); - } - return resolved(expected); - }, - false, 100L, 1_000L, 1_000L); - - Assert.assertFalse(isIpAllowed(handler, expected.getHostAddress())); - handler.refreshResolvedIps(); - handler.refreshResolvedIps(); - Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); - Assert.assertEquals(3, attempts.get()); - handler.shutdown(); - } - - @Test - public void testTransientDnsFailureKeepsLastKnownAddress() throws Exception { - AtomicInteger attempts = new AtomicInteger(); - InetAddress expected = InetAddress.getByAddress( - new byte[]{(byte) 192, (byte) 168, 0, 1}); - IpAuthHandler handler = new IpAuthHandler( - Collections.singleton("pd-1"), - host -> { - if (attempts.incrementAndGet() > 1) { - return failed(host); - } - return resolved(expected); - }, - false, 100L, 1_000L, 1_000L); - - Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); - handler.refreshResolvedIps(); - Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); - handler.shutdown(); - } - - @Test - public void testSlowPeerDoesNotBlockFollowingPeer() throws Exception { - InetAddress expected = InetAddress.getByAddress( - new byte[]{(byte) 192, (byte) 168, 0, 2}); - Set peers = new LinkedHashSet<>(); - peers.add("pd-slow"); - peers.add("pd-ready"); - IpAuthHandler handler = new IpAuthHandler( - peers, - host -> { - if ("pd-slow".equals(host)) { - return new CompletableFuture<>(); - } - return resolved(expected); - }, - false, 10L, 1_000L, 1_000L); - - Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); - handler.shutdown(); - } - - @Test - public void testExpiredAddressFailsClosed() throws Exception { - AtomicInteger attempts = new AtomicInteger(); - InetAddress expected = InetAddress.getByAddress( - new byte[]{(byte) 192, (byte) 168, 0, 3}); - IpAuthHandler handler = new IpAuthHandler( - Collections.singleton("pd-1"), - host -> { - if (attempts.incrementAndGet() > 1) { - return failed(host); - } - return resolved(expected); - }, - false, 100L, 1L, 1_000L); - - Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); - Thread.sleep(5L); - handler.refreshResolvedIps(); - Assert.assertFalse(isIpAllowed(handler, expected.getHostAddress())); - handler.shutdown(); - } - - @Test - public void testScheduledRefreshAddsLatePeerAndRotatesAddress() - throws Exception { - AtomicInteger attempts = new AtomicInteger(); - InetAddress first = InetAddress.getByAddress( - new byte[]{(byte) 192, (byte) 168, 0, 4}); - InetAddress second = InetAddress.getByAddress( - new byte[]{(byte) 192, (byte) 168, 0, 5}); - AtomicReference current = new AtomicReference<>(first); - IpAuthHandler handler = new IpAuthHandler( - Collections.singleton("pd-late"), - host -> { - if (attempts.incrementAndGet() == 1) { - return failed(host); - } - return resolved(current.get()); - }, - true, 20L, 1_000L, 10L); - try { - awaitAllowed(handler, first.getHostAddress()); - current.set(second); - awaitAllowed(handler, second.getHostAddress()); - Assert.assertFalse(isIpAllowed(handler, first.getHostAddress())); - } finally { - handler.shutdown(); - } - } - - @Test - public void testNeverCompletingPeersDoNotStarveReadyPeer() - throws Exception { - InetAddress expected = InetAddress.getByAddress( - new byte[]{(byte) 192, (byte) 168, 0, 6}); - Set peers = new LinkedHashSet<>(); - for (int i = 0; i < 8; i++) { - peers.add("00-pd-slow-" + i); - } - peers.add("99-pd-ready"); - IpAuthHandler handler = new IpAuthHandler( - peers, - host -> { - if (host.startsWith("00-pd-slow-")) { - return new CompletableFuture<>(); - } - return resolved(expected); - }, - false, 10L, 1_000L, 1_000L); - - handler.refresh(peers); - Assert.assertTrue(isIpAllowed(handler, expected.getHostAddress())); - handler.shutdown(); - } - - @Test - public void testRejectsOversizedAllowlist() { - Set peers = new HashSet<>(); - for (int i = 0; i < 128; i++) { - peers.add("pd-" + i); - } - - try { - new IpAuthHandler(peers, host -> new CompletableFuture<>(), - false, 10L, 1_000L, 1_000L); - Assert.fail("Expected oversized allowlist rejection"); - } catch (IllegalArgumentException e) { - Assert.assertTrue(e.getMessage().contains("127")); - } - } - - @Test - public void testLateSuccessfulResultIsDiscarded() throws Exception { - AtomicInteger attempts = new AtomicInteger(); - InetAddress first = InetAddress.getByAddress( - new byte[]{(byte) 192, (byte) 168, 0, 7}); - InetAddress late = InetAddress.getByAddress( - new byte[]{(byte) 192, (byte) 168, 0, 8}); - CompletableFuture> delayed = new CompletableFuture<>(); - IpAuthHandler handler = new IpAuthHandler( - Collections.singleton("pd-1"), - host -> { - int attempt = attempts.incrementAndGet(); - if (attempt == 1) { - return resolved(first); - } - if (attempt == 2) { - return delayed; - } - return new CompletableFuture<>(); - }, - false, 10L, 1_000L, 1_000L); - - handler.refreshResolvedIps(false); - Thread.sleep(20L); - delayed.complete(resolved(late).get()); - handler.refreshResolvedIps(false); - - Assert.assertTrue(isIpAllowed(handler, first.getHostAddress())); - Assert.assertFalse(isIpAllowed(handler, late.getHostAddress())); - handler.shutdown(); - } - - @Test - public void testRefreshCollectsPreviousBatchBeforeStartingNext() - throws Exception { - Set peers = new HashSet<>(); - Map>> delayed = - new HashMap<>(); - for (int i = 0; i < 17; i++) { - peers.add(String.format("pd-%02d", i)); - if (i >= 8 && i < 16) { - delayed.put(i, new CompletableFuture<>()); - } - } - IpAuthHandler handler = new IpAuthHandler( - peers, - host -> { - int index = Integer.parseInt(host.substring(3)); - CompletableFuture> future = delayed.get(index); - if (future != null) { - return future; - } - return resolved(address(index)); - }, - false, 100L, 1_000L, 1_000L); - - handler.refreshResolvedIps(false); - for (Map.Entry>> entry : - delayed.entrySet()) { - entry.getValue().complete(resolved(address(entry.getKey())).get()); - } - handler.refreshResolvedIps(false); - - Assert.assertTrue(isIpAllowed( - handler, address(16).getHostAddress())); - handler.shutdown(); - } - - @Test - public void testConstructorFailureClosesResolver() { - AtomicBoolean closed = new AtomicBoolean(); - IpAuthHandler.HostResolver resolver = new IpAuthHandler.HostResolver() { - - @Override - public CompletableFuture> resolve(String host) { - throw new IllegalStateException("simulated resolver failure"); - } - - @Override - public void close() { - closed.set(true); - } - }; - - try { - new IpAuthHandler(Collections.singleton("pd-1"), resolver, - false, 10L, 1_000L, 1_000L); - Assert.fail("Expected constructor failure"); - } catch (IllegalStateException e) { - Assert.assertEquals("simulated resolver failure", e.getMessage()); - } - Assert.assertTrue(closed.get()); - } - - @Test - public void testInterruptedRefreshFailsAndPreservesInterrupt() - throws Exception { - InetAddress initial = address(20); - IpAuthHandler handler = new IpAuthHandler( - Collections.singleton("ready"), - host -> { - if ("ready".equals(host)) { - return resolved(initial); - } - return new CompletableFuture<>(); - }, - false, 100L, 1_000L, 1_000L); - try { - Thread.currentThread().interrupt(); - handler.refresh(Collections.singleton("slow")); - Assert.fail("Expected interrupted refresh to fail"); - } catch (IllegalStateException e) { - Assert.assertTrue(e.getMessage().contains("interrupted")); - Assert.assertTrue(Thread.currentThread().isInterrupted()); - } finally { - Thread.interrupted(); - handler.shutdown(); - } - } - - private void awaitAllowed(IpAuthHandler handler, String address) - throws InterruptedException { - long deadline = System.currentTimeMillis() + 1_000L; - while (!isIpAllowed(handler, address) && - System.currentTimeMillis() < deadline) { - Thread.sleep(10L); - } - Assert.assertTrue(isIpAllowed(handler, address)); - } - - @Test - public void testRefreshUpdatesResolvedIps() { - // Start with 127.0.0.1 - IpAuthHandler handler = IpAuthHandler.getInstance( - Collections.singleton("127.0.0.1")); - Assert.assertTrue(isIpAllowed(handler, "127.0.0.1")); - - // Refresh with a different IP — verifies refresh() swaps the set correctly - Set newIps = new HashSet<>(); - newIps.add("192.168.0.1"); - handler.refresh(newIps); - - // Old IP should no longer be allowed - Assert.assertFalse(isIpAllowed(handler, "127.0.0.1")); - // New IP should now be allowed - Assert.assertTrue(isIpAllowed(handler, "192.168.0.1")); - } - - @Test - public void testEmptyAllowlistAllowsAll() { - // Empty allowlist = no restriction configured = allow all connections - // This is intentional fallback behavior and must be explicitly tested - // because it is a security-relevant boundary - IpAuthHandler handler = IpAuthHandler.getInstance( - Collections.emptySet()); - Assert.assertTrue(isIpAllowed(handler, "1.2.3.4")); - Assert.assertTrue(isIpAllowed(handler, "192.168.99.99")); - } - - @Test - public void testGetInstanceReturnsSingletonIgnoresNewAllowlist() { - // First call creates the singleton with 127.0.0.1 - IpAuthHandler first = IpAuthHandler.getInstance( - Collections.singleton("127.0.0.1")); - // Second call with a different set must return the same instance - // and must NOT reinitialize or override the existing allowlist - IpAuthHandler second = IpAuthHandler.getInstance( - Collections.singleton("192.168.0.1")); - Assert.assertSame(first, second); - // Original allowlist still in effect - Assert.assertTrue(isIpAllowed(second, "127.0.0.1")); - // New set was ignored — 192.168.0.1 should not be allowed - Assert.assertFalse(isIpAllowed(second, "192.168.0.1")); - } - - private static CompletableFuture> resolved( - InetAddress... addresses) { - Set result = new HashSet<>(); - for (InetAddress address : addresses) { - result.add(address.getHostAddress()); - } - return CompletableFuture.completedFuture( - Collections.unmodifiableSet(result)); - } - - private static CompletableFuture> failed(String host) { - CompletableFuture> result = new CompletableFuture<>(); - result.completeExceptionally(new UnknownHostException(host)); - return result; - } - - private static InetAddress address(int suffix) { - try { - return InetAddress.getByAddress( - new byte[]{10, 0, 0, (byte) (suffix + 1)}); - } catch (UnknownHostException e) { - throw new AssertionError(e); - } - } -} diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java index 8be1094509..660587bf05 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java @@ -172,6 +172,21 @@ public static void resetSpaceContext() { REQUEST_GRAPH_SPACE.remove(); } + private void prepareAuditLimiter(UserWithRole user) { + if (user == null || user.role() == null || + HugeAuthenticator.ROLE_NONE.equals(user.role())) { + return; + } + Id userKey = auditLimiterKey(user.username()); + this.auditLimiters.getOrFetch(userKey, id -> { + return RateLimiter.create(this.auditLogMaxRate); + }); + } + + private static Id auditLimiterKey(String username) { + return IdGenerator.of(username); + } + /** * Get the graph space from current request URL path */ @@ -1571,7 +1586,8 @@ public HugeUser deleteUser(Id id) { "Can't delete user '%s'", user.name()); E.checkArgument(HugeAuthenticator.USER_ADMIN.equals(currentUsername()), "only admin can delete user", user.name()); - HugeGraphAuthProxy.this.auditLimiters.invalidate(user.id()); + HugeGraphAuthProxy.this.auditLimiters.invalidate( + auditLimiterKey(user.name())); this.invalidRoleCache(); return this.authManager.deleteUser(id); } @@ -2015,9 +2031,12 @@ public UserWithRole validateUser(String username, String password) { try { Id userKey = IdGenerator.of(username + password); - return HugeGraphAuthProxy.this.usersRoleCache.getOrFetch(userKey, id -> { - return this.authManager.validateUser(username, password); - }); + UserWithRole user = + HugeGraphAuthProxy.this.usersRoleCache.getOrFetch( + userKey, id -> this.authManager.validateUser( + username, password)); + HugeGraphAuthProxy.this.prepareAuditLimiter(user); + return user; } catch (Exception e) { LOG.error("Failed to validate user {} with error: ", username, e); @@ -2034,9 +2053,12 @@ public UserWithRole validateUser(String token) { try { Id userKey = IdGenerator.of(token); - return HugeGraphAuthProxy.this.usersRoleCache.getOrFetch(userKey, id -> { - return this.authManager.validateUser(token); - }); + UserWithRole user = + HugeGraphAuthProxy.this.usersRoleCache.getOrFetch( + userKey, + id -> this.authManager.validateUser(token)); + HugeGraphAuthProxy.this.prepareAuditLimiter(user); + return user; } catch (Exception e) { LOG.error("Failed to validate token with error: ", e); throw e; diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index b25adbc489..76e0dbe95c 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -29,15 +29,20 @@ import org.apache.hugegraph.auth.HugeDefaultRole; import org.apache.hugegraph.auth.HugeGraphAuthProxy; import org.apache.hugegraph.auth.HugePermission; +import org.apache.hugegraph.auth.HugeUser; import org.apache.hugegraph.auth.RolePermission; import org.apache.hugegraph.auth.UserWithRole; +import org.apache.hugegraph.backend.cache.Cache; +import org.apache.hugegraph.backend.id.Id; import org.apache.hugegraph.backend.id.IdGenerator; import org.apache.hugegraph.config.AuthOptions; import org.apache.hugegraph.config.HugeConfig; import org.apache.hugegraph.task.TaskManager; import org.apache.hugegraph.task.TaskScheduler; import org.apache.hugegraph.testutil.Assert; +import org.apache.hugegraph.testutil.Whitebox; import org.apache.hugegraph.unit.BaseUnitTest; +import org.apache.hugegraph.util.RateLimiter; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.core.Filter; @@ -229,6 +234,7 @@ public void testDefaultRoleMutationInvalidatesUserRoleCache() HugeConfig config = Mockito.mock(HugeConfig.class); AuthManager authManager = Mockito.mock(AuthManager.class); TaskScheduler scheduler = Mockito.mock(TaskScheduler.class); + Id storedUserId = IdGenerator.of("stored-user-id"); Mockito.when(graph.spaceGraphName()).thenReturn("hugegraph"); Mockito.when(graph.configuration()).thenReturn(config); @@ -241,7 +247,11 @@ public void testDefaultRoleMutationInvalidatesUserRoleCache() Mockito.when(config.get(AuthOptions.AUTH_AUDIT_LOG_RATE)) .thenReturn(1000D); Mockito.when(authManager.validateUser("cache_user", "pass")) - .thenReturn(new UserWithRole("cache_user")); + .thenReturn(new UserWithRole( + storedUserId, "cache_user", + RolePermission.all("hugegraph"))); + Mockito.when(authManager.validateUser("invalid", "wrong")) + .thenReturn(new UserWithRole("invalid")); Mockito.when(authManager.createDefaultRole("DEFAULT", "cache_user", HugeDefaultRole.ANALYST, "hugegraph")) @@ -250,7 +260,15 @@ public void testDefaultRoleMutationInvalidatesUserRoleCache() HugeGraphAuthProxy proxy = new HugeGraphAuthProxy(graph); AuthManager proxyAuthManager = proxy.authManager(); + proxyAuthManager.validateUser("invalid", "wrong"); proxyAuthManager.validateUser("cache_user", "pass"); + Cache auditLimiters = + Whitebox.getInternalState(proxy, "auditLimiters"); + Assert.assertFalse(auditLimiters.containsKey( + IdGenerator.of("invalid"))); + Assert.assertTrue(auditLimiters.containsKey( + IdGenerator.of("cache_user"))); + Assert.assertFalse(auditLimiters.containsKey(storedUserId)); proxyAuthManager.validateUser("cache_user", "pass"); Mockito.verify(authManager, Mockito.times(1)) .validateUser("cache_user", "pass"); @@ -269,6 +287,7 @@ public void testLogoutInvalidatesTokenRoleCache() throws Exception { AuthManager authManager = Mockito.mock(AuthManager.class); TaskScheduler scheduler = Mockito.mock(TaskScheduler.class); String token = "cached-token"; + Id storedUserId = IdGenerator.of("stored-user-id"); Mockito.when(graph.spaceGraphName()).thenReturn("hugegraph"); Mockito.when(graph.configuration()).thenReturn(config); @@ -281,11 +300,22 @@ public void testLogoutInvalidatesTokenRoleCache() throws Exception { Mockito.when(config.get(AuthOptions.AUTH_AUDIT_LOG_RATE)) .thenReturn(1000D); Mockito.when(authManager.validateUser(token)) - .thenReturn(new UserWithRole("cache_user")); + .thenReturn(new UserWithRole( + storedUserId, "cache_user", + RolePermission.all("hugegraph"))); + Mockito.when(authManager.validateUser("invalid-token")) + .thenReturn(new UserWithRole("")); - AuthManager proxyAuthManager = - new HugeGraphAuthProxy(graph).authManager(); + HugeGraphAuthProxy proxy = new HugeGraphAuthProxy(graph); + AuthManager proxyAuthManager = proxy.authManager(); + proxyAuthManager.validateUser("invalid-token"); proxyAuthManager.validateUser(token); + Cache auditLimiters = + Whitebox.getInternalState(proxy, "auditLimiters"); + Assert.assertEquals(1L, auditLimiters.size()); + Assert.assertTrue(auditLimiters.containsKey( + IdGenerator.of("cache_user"))); + Assert.assertFalse(auditLimiters.containsKey(storedUserId)); proxyAuthManager.validateUser(token); Mockito.verify(authManager, Mockito.times(1)).validateUser(token); @@ -296,6 +326,50 @@ public void testLogoutInvalidatesTokenRoleCache() throws Exception { Mockito.verify(authManager, Mockito.times(2)).validateUser(token); } + @Test + public void testDeleteUserInvalidatesUsernameAuditLimiter() { + HugeGraph graph = Mockito.mock(HugeGraph.class); + HugeConfig config = Mockito.mock(HugeConfig.class); + AuthManager authManager = Mockito.mock(AuthManager.class); + TaskScheduler scheduler = Mockito.mock(TaskScheduler.class); + Id storedUserId = IdGenerator.of("stored-user-id"); + HugeUser storedUser = new HugeUser(storedUserId, "cache_user"); + + Mockito.when(graph.spaceGraphName()).thenReturn("hugegraph"); + Mockito.when(graph.configuration()).thenReturn(config); + Mockito.when(graph.authManager()).thenReturn(authManager); + Mockito.when(graph.taskScheduler()).thenReturn(scheduler); + Mockito.when(config.get(AuthOptions.AUTH_CACHE_EXPIRE)) + .thenReturn(3600L); + Mockito.when(config.get(AuthOptions.AUTH_CACHE_CAPACITY)) + .thenReturn(100L); + Mockito.when(config.get(AuthOptions.AUTH_AUDIT_LOG_RATE)) + .thenReturn(1000D); + Mockito.when(authManager.validateUser("cache_user", "pass")) + .thenReturn(new UserWithRole( + storedUserId, "cache_user", + RolePermission.all("hugegraph"))); + Mockito.when(authManager.getUser(storedUserId)).thenReturn(storedUser); + + HugeGraphAuthProxy proxy = new HugeGraphAuthProxy(graph); + AuthManager proxyAuthManager = proxy.authManager(); + proxyAuthManager.validateUser("cache_user", "pass"); + Cache auditLimiters = + Whitebox.getInternalState(proxy, "auditLimiters"); + Assert.assertTrue(auditLimiters.containsKey( + IdGenerator.of("cache_user"))); + + setContext(new HugeGraphAuthProxy.Context( + new HugeAuthenticator.User( + HugeAuthenticator.USER_ADMIN, + RolePermission.admin()))); + proxyAuthManager.deleteUser(storedUserId); + + Assert.assertFalse(auditLimiters.containsKey( + IdGenerator.of("cache_user"))); + Mockito.verify(authManager).deleteUser(storedUserId); + } + @Test public void testProxyOverridesEveryScopedDefaultMethod() throws Exception { HugeGraph graph = Mockito.mock(HugeGraph.class); From 7a57568eca9af904bc63bc96134873e0216f6f27 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 03:20:17 +0800 Subject: [PATCH 10/23] fix(server): scope metadata callback admin - run metadata callbacks with an internal admin context - restore the previous context on success or failure - prevent admin propagation into callback child threads - cover task override and context restoration boundaries --- .../hugegraph/auth/HugeGraphAuthProxy.java | 24 +++++-- .../apache/hugegraph/core/GraphManager.java | 24 ++----- .../unit/auth/HugeGraphAuthProxyTest.java | 71 +++++++++++++++++++ 3 files changed, 95 insertions(+), 24 deletions(-) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java index 660587bf05..08e5d6faaf 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java @@ -163,11 +163,13 @@ static Context setContext(Context context) { } public static void resetContext() { + AuthContext.resetContext(); CONTEXTS.remove(); REQUEST_GRAPH_SPACE.remove(); } public static void resetSpaceContext() { + AuthContext.resetContext(); CONTEXTS.remove(); REQUEST_GRAPH_SPACE.remove(); } @@ -202,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) { + String old = AuthContext.getContext(); + try { + AuthContext.setContext(User.ADMIN.toJson()); + runnable.run(); + } finally { + if (old == null) { + AuthContext.resetContext(); + } else { + AuthContext.setContext(old); + } + } } public static Context getContext() { + String internalContext = AuthContext.getContext(); + User internalUser = User.fromJson(internalContext); + if (internalUser != null) { + return new Context(internalUser); + } + // Return task context first String taskContext = TaskManager.getContext(); diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java index 96717e7240..848eeee8cc 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java @@ -1314,9 +1314,6 @@ public HugeGraph createGraph(String graphSpace, String name, String creator, throw new ExistedException("graph", key); } boolean grpcThread = Thread.currentThread().getName().contains("grpc"); - if (grpcThread) { - HugeGraphAuthProxy.setAdmin(); - } E.checkArgumentNotNull(name, "The graph name can't be null"); checkGraphName(name); String nickname; @@ -1426,9 +1423,6 @@ public HugeGraph createGraph(String graphSpace, String name, String creator, String schemas = this.schemaTemplate(graphSpace, schema).schema(); prepareSchema(graph, schemas); } - if (grpcThread) { - HugeGraphAuthProxy.resetContext(); - } return graph; } @@ -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")) { + HugeGraphAuthProxy.runAsAdmin(() -> this.consumer.accept(t)); + } else { + this.consumer.accept(t); } - consumer.accept(t); } catch (Throwable e) { LOG.error("Listener exception occurred.", e); - } finally { - if (grpcThread) { - HugeGraphAuthProxy.resetContext(); - } } } } @@ -2498,11 +2487,6 @@ private void graphAddHandler(T response) { // TODO: add alias graph graph = this.createGraph(parts[0], parts[1], creator, config, false); LOG.info("Add graph space:{} graph:{}", parts[0], parts[1]); - // TODO: use a more secure method to determine administrator privileges - boolean grpcThread = Thread.currentThread().getName().contains("grpc"); - if (grpcThread) { - HugeGraphAuthProxy.setAdmin(); - } graph.started(true); if (graph.tx().isOpen()) { graph.tx().close(); diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index 76e0dbe95c..00f864794a 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -22,6 +22,7 @@ import java.util.Collections; import java.util.List; import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; import org.apache.hugegraph.HugeGraph; import org.apache.hugegraph.auth.AuthManager; @@ -121,6 +122,76 @@ public void testUsernameWithAdminUser() { Assert.assertEquals("admin", username); } + @Test + public void testRunAsAdminRestoresContext() { + HugeAuthenticator.User user = new HugeAuthenticator.User( + "test_user", + RolePermission.admin() + ); + setContext(new HugeGraphAuthProxy.Context(user)); + + HugeGraphAuthProxy.runAsAdmin(() -> { + Assert.assertEquals(HugeAuthenticator.USER_ADMIN, + HugeGraphAuthProxy.username()); + }); + + Assert.assertEquals("test_user", HugeGraphAuthProxy.username()); + } + + @Test + public void testRunAsAdminOverridesTaskContext() { + HugeAuthenticator.User taskUser = new HugeAuthenticator.User( + "task_user", + RolePermission.admin() + ); + TaskManager.setContext(taskUser.toJson()); + + HugeGraphAuthProxy.runAsAdmin(() -> { + Assert.assertEquals(HugeAuthenticator.USER_ADMIN, + HugeGraphAuthProxy.username()); + }); + + Assert.assertEquals("task_user", HugeGraphAuthProxy.username()); + } + + @Test + public void testRunAsAdminRestoresContextAfterException() { + HugeAuthenticator.User taskUser = new HugeAuthenticator.User( + "task_user", + RolePermission.admin() + ); + TaskManager.setContext(taskUser.toJson()); + + Assert.assertThrows(RuntimeException.class, () -> { + HugeGraphAuthProxy.runAsAdmin(() -> { + throw new RuntimeException("expected"); + }); + }); + + Assert.assertEquals("task_user", HugeGraphAuthProxy.username()); + } + + @Test + public void testRunAsAdminDoesNotPropagateToChildThread() + throws InterruptedException { + AtomicReference username = new AtomicReference<>(); + + HugeGraphAuthProxy.runAsAdmin(() -> { + Thread child = new Thread(() -> { + username.set(HugeGraphAuthProxy.username()); + }); + child.start(); + try { + child.join(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } + }); + + Assert.assertEquals("anonymous", username.get()); + } + @Test public void testGetContextReturnsNull() { // Ensure both TaskManager context and CONTEXTS are null From 006a2b33674cdad7377e30df3059cb483592a4a1 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 03:47:03 +0800 Subject: [PATCH 11/23] fix(server): scope space manager user access - allow space managers to inspect users in their own space - reject users without current-space grants and global admins - cover cross-space and multi-space permission boundaries --- .../hugegraph/auth/HugeAuthenticator.java | 11 ++++- .../unit/auth/HugeGraphAuthProxyTest.java | 47 +++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java index 3ec09c915e..4bf0edf086 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java @@ -363,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 && + !RolePermission.isAdmin(grantedRole) && + grantedRole.roles().containsKey(resourceObject.graphSpace()) && + rolePerm.matchSpace(resourceObject.graphSpace(), "space")) { + return true; + } + return RolePermission.fromJson(role).contains(grantedRole); } @SuppressWarnings({"unchecked", "rawtypes"}) diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index 00f864794a..4aafeddb61 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -31,6 +31,7 @@ import org.apache.hugegraph.auth.HugeGraphAuthProxy; import org.apache.hugegraph.auth.HugePermission; import org.apache.hugegraph.auth.HugeUser; +import org.apache.hugegraph.auth.ResourceObject; import org.apache.hugegraph.auth.RolePermission; import org.apache.hugegraph.auth.UserWithRole; import org.apache.hugegraph.backend.cache.Cache; @@ -595,6 +596,52 @@ public void testSpaceMemberDoesNotGrantMutationPermissions() { role, delete)); } + @Test + public void testSpaceManagerCanManageUserGrantInOwnSpace() { + RolePermission managerRole = RolePermission.fromJson( + "{\"roles\":{\"space-a\":{\"*\":{" + + "\"SPACE\":{\"ALL\":[{\"type\":\"ALL\"}]}" + + "}}}}"); + RolePermission memberGrant = RolePermission.fromJson( + "{\"roles\":{\"space-a\":{\"*\":{" + + "\"READ\":{\"ALL\":[{\"type\":\"ALL\"}]}," + + "\"WRITE\":{\"ALL\":[{\"type\":\"ALL\"}]}" + + "}}}}"); + RolePermission otherSpaceGrant = RolePermission.fromJson( + "{\"roles\":{\"space-b\":{\"*\":{" + + "\"READ\":{\"ALL\":[{\"type\":\"ALL\"}]}," + + "\"WRITE\":{\"ALL\":[{\"type\":\"ALL\"}]}" + + "}}}}"); + RolePermission multiSpaceGrant = RolePermission.fromJson( + "{\"roles\":{" + + "\"space-a\":{\"*\":{" + + "\"READ\":{\"ALL\":[{\"type\":\"ALL\"}]}}}," + + "\"space-b\":{\"*\":{" + + "\"READ\":{\"ALL\":[{\"type\":\"ALL\"}]}}}" + + "}}"); + HugeUser member = new HugeUser("member"); + ResourceObject ownSpace = + ResourceObject.of("space-a", "hugegraph", member); + ResourceObject otherSpace = + ResourceObject.of("space-b", "hugegraph", member); + ResourceObject admin = + ResourceObject.of("space-a", "hugegraph", + new HugeUser(HugeAuthenticator.USER_ADMIN)); + + Assert.assertTrue(HugeAuthenticator.RolePerm.match( + managerRole, memberGrant, ownSpace)); + Assert.assertFalse(HugeAuthenticator.RolePerm.match( + managerRole, memberGrant, otherSpace)); + Assert.assertFalse(HugeAuthenticator.RolePerm.match( + managerRole, otherSpaceGrant, ownSpace)); + Assert.assertTrue(HugeAuthenticator.RolePerm.match( + managerRole, multiSpaceGrant, ownSpace)); + Assert.assertFalse(HugeAuthenticator.RolePerm.match( + managerRole, memberGrant, admin)); + Assert.assertFalse(HugeAuthenticator.RolePerm.match( + managerRole, RolePermission.admin(), ownSpace)); + } + @SuppressWarnings("unchecked") private static Set traversalPermissions( Traversal.Admin traversal) throws Exception { From 2f844ed296195d43ebe435771101adc236a01698 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 03:56:50 +0800 Subject: [PATCH 12/23] fix(server): honor custom admin mutations - recognize custom global admins for user updates - allow custom global admins to delete ordinary users - preserve builtin admin behavior and deletion safeguards - cover builtin and custom admin mutation paths --- .../org/apache/hugegraph/auth/HugeGraphAuthProxy.java | 7 +++++-- .../hugegraph/unit/auth/HugeGraphAuthProxyTest.java | 10 ++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java index 08e5d6faaf..f3440d0e57 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java @@ -1586,7 +1586,8 @@ public Id updateUser(HugeUser updatedUser) { String username = currentUsername(); HugeUser user = this.authManager.getUser(updatedUser.id()); if (!user.name().equals(username)) { - E.checkArgument(HugeAuthenticator.USER_ADMIN.equals(username), + E.checkArgument(HugeAuthenticator.USER_ADMIN.equals(username) || + this.authManager.isAdminManager(username), "Only the user themselves or the admin can change this user", user.name()); this.updateCreator(updatedUser); @@ -1600,7 +1601,9 @@ public HugeUser deleteUser(Id id) { HugeUser user = this.authManager.getUser(id); E.checkArgument(!HugeAuthenticator.USER_ADMIN.equals(user.name()), "Can't delete user '%s'", user.name()); - E.checkArgument(HugeAuthenticator.USER_ADMIN.equals(currentUsername()), + String username = currentUsername(); + E.checkArgument(HugeAuthenticator.USER_ADMIN.equals(username) || + this.authManager.isAdminManager(username), "only admin can delete user", user.name()); HugeGraphAuthProxy.this.auditLimiters.invalidate( auditLimiterKey(user.name())); diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index 4aafeddb61..399c685fb2 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -422,6 +422,8 @@ public void testDeleteUserInvalidatesUsernameAuditLimiter() { storedUserId, "cache_user", RolePermission.all("hugegraph"))); Mockito.when(authManager.getUser(storedUserId)).thenReturn(storedUser); + Mockito.when(authManager.isAdminManager("custom_admin")) + .thenReturn(true); HugeGraphAuthProxy proxy = new HugeGraphAuthProxy(graph); AuthManager proxyAuthManager = proxy.authManager(); @@ -435,10 +437,18 @@ public void testDeleteUserInvalidatesUsernameAuditLimiter() { new HugeAuthenticator.User( HugeAuthenticator.USER_ADMIN, RolePermission.admin()))); + proxyAuthManager.updateUser(storedUser); + + setContext(new HugeGraphAuthProxy.Context( + new HugeAuthenticator.User( + "custom_admin", + RolePermission.admin()))); + proxyAuthManager.updateUser(storedUser); proxyAuthManager.deleteUser(storedUserId); Assert.assertFalse(auditLimiters.containsKey( IdGenerator.of("cache_user"))); + Mockito.verify(authManager, Mockito.times(2)).updateUser(storedUser); Mockito.verify(authManager).deleteUser(storedUserId); } From ca0478ae1fa37a08832a7373abb49cce03642f95 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 05:28:55 +0800 Subject: [PATCH 13/23] fix(server): allow admin template management - align template ownership with global admin semantics - preserve creator and GraphSpace manager access - cover all four template management roles --- .../api/space/SchemaTemplateAPI.java | 19 +++-- .../apache/hugegraph/unit/UnitTestSuite.java | 2 + .../unit/api/space/SchemaTemplateAPITest.java | 72 +++++++++++++++++++ 3 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/SchemaTemplateAPITest.java diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java index b2c151687c..7fda423f58 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java @@ -24,6 +24,7 @@ import org.apache.commons.lang3.StringUtils; import org.apache.hugegraph.HugeException; import org.apache.hugegraph.api.API; +import org.apache.hugegraph.auth.AuthManager; import org.apache.hugegraph.api.filter.StatusFilter; import org.apache.hugegraph.auth.HugeGraphAuthProxy; import org.apache.hugegraph.core.GraphManager; @@ -134,9 +135,8 @@ public void delete(@Context GraphManager manager, "Schema template '%s' does not exist", name); String username = HugeGraphAuthProxy.username(); - boolean isSpace = manager.authManager() - .isSpaceManager(graphSpace, username); - if (Objects.equals(st.creator(), username) || isSpace) { + if (canManage(manager.authManager(), graphSpace, st.creator(), + username)) { manager.dropSchemaTemplate(graphSpace, name); } else { throw new ForbiddenException("No permission to delete schema template"); @@ -165,9 +165,8 @@ public String update(@Context GraphManager manager, } String username = HugeGraphAuthProxy.username(); - boolean isSpace = manager.authManager() - .isSpaceManager(graphSpace, username); - if (Objects.equals(old.creator(), username) || isSpace) { + if (canManage(manager.authManager(), graphSpace, old.creator(), + username)) { SchemaTemplate template = jsonSchemaTemplate.build(old); template.creator(old.creator()); template.create(old.create()); @@ -180,6 +179,14 @@ public String update(@Context GraphManager manager, } + private static boolean canManage(AuthManager authManager, + String graphSpace, String creator, + String username) { + return Objects.equals(creator, username) || + authManager.isAdminManager(username) || + authManager.isSpaceManager(graphSpace, username); + } + private static class JsonSchemaTemplate implements Checkable { @JsonProperty("name") diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java index 1733680e3f..da55301deb 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java @@ -31,6 +31,7 @@ import org.apache.hugegraph.unit.api.filter.PathFilterTest; import org.apache.hugegraph.unit.api.gremlin.GremlinQueryAPITest; import org.apache.hugegraph.unit.api.space.GraphSpaceAPITest; +import org.apache.hugegraph.unit.api.space.SchemaTemplateAPITest; import org.apache.hugegraph.unit.auth.HugeGraphAuthProxyTest; import org.apache.hugegraph.unit.cache.CacheManagerTest; import org.apache.hugegraph.unit.cache.CacheTest; @@ -110,6 +111,7 @@ /* api space */ GraphSpaceAPITest.class, + SchemaTemplateAPITest.class, /* cache */ CacheTest.RamCacheTest.class, diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/SchemaTemplateAPITest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/SchemaTemplateAPITest.java new file mode 100644 index 0000000000..c2be357eb4 --- /dev/null +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/SchemaTemplateAPITest.java @@ -0,0 +1,72 @@ +/* + * 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.unit.api.space; + +import org.apache.hugegraph.api.space.SchemaTemplateAPI; +import org.apache.hugegraph.auth.AuthManager; +import org.apache.hugegraph.testutil.Assert; +import org.apache.hugegraph.testutil.Whitebox; +import org.junit.Test; +import org.mockito.Mockito; + +public class SchemaTemplateAPITest { + + private static final String GRAPHSPACE = "space"; + private static final String CREATOR = "creator"; + + @Test + public void testCreatorCanManageTemplate() { + Assert.assertTrue(canManage(authManager(false, false), CREATOR)); + } + + @Test + public void testGlobalAdminCanManageAnotherUsersTemplate() { + Assert.assertTrue(canManage(authManager(true, false), "admin")); + } + + @Test + public void testSpaceManagerCanManageAnotherUsersTemplate() { + Assert.assertTrue(canManage(authManager(false, true), "space-admin")); + } + + @Test + public void testUnrelatedUserCannotManageTemplate() { + Assert.assertFalse(canManage(authManager(false, false), "member")); + } + + private static AuthManager authManager(boolean admin, + boolean spaceManager) { + AuthManager auth = Mockito.mock(AuthManager.class); + Mockito.when(auth.isAdminManager(Mockito.anyString())) + .thenReturn(admin); + Mockito.when(auth.isSpaceManager(GRAPHSPACE, "space-admin")) + .thenReturn(spaceManager); + return auth; + } + + private static boolean canManage(AuthManager auth, String username) { + return Whitebox.invokeStatic( + SchemaTemplateAPI.class, + new Class[]{AuthManager.class, String.class, + String.class, String.class}, + "canManage", + auth, GRAPHSPACE, CREATOR, username); + } +} From d8699ae65adc6d7b0a5b3572067e4f2a04c00084 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 11:25:56 +0800 Subject: [PATCH 14/23] refactor(style): set line width to 120 - align Checkstyle and EditorConfig at 120 columns - update contributor and module style guidance - compact only current PR code without legacy reformatting --- .editorconfig | 4 ++-- .../memories/code_style_and_conventions.md | 2 +- AGENTS.md | 2 +- README.md | 2 +- hugegraph-pd/docs/development.md | 2 +- .../apache/hugegraph/api/auth/ManagerAPI.java | 12 ++++------- .../hugegraph/api/space/GraphSpaceAPI.java | 21 +++++++------------ .../api/space/SchemaTemplateAPI.java | 9 +++----- style/checkstyle.xml | 2 +- 9 files changed, 21 insertions(+), 35 deletions(-) diff --git a/.editorconfig b/.editorconfig index 04a6e64a9f..c64c7bae2b 100644 --- a/.editorconfig +++ b/.editorconfig @@ -21,9 +21,9 @@ root = true charset = utf-8 end_of_line = lf insert_final_newline = true -max_line_length = 100 +max_line_length = 120 ij_wrap_on_typing = true -ij_visual_guides = 100 +ij_visual_guides = 120 [*.{java,xml,py}] diff --git a/.serena/memories/code_style_and_conventions.md b/.serena/memories/code_style_and_conventions.md index 159920cd3b..7a4c310e0b 100644 --- a/.serena/memories/code_style_and_conventions.md +++ b/.serena/memories/code_style_and_conventions.md @@ -6,7 +6,7 @@ - `.licenserc.yaml` + apache-rat-plugin + skywalking-eyes — License header validation ## Core Rules -- **Line length**: 100 chars (120 for XML) +- **Line length**: 120 chars - **Indent**: 4 spaces, continuation 8 spaces - **Charset**: UTF-8, LF line endings, final newline - **Imports**: Sorted `$*` → `java` → `javax` → `org` → `com` → `*`, no star imports (threshold 100) diff --git a/AGENTS.md b/AGENTS.md index 2d6e81b15b..07daf17662 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -85,7 +85,7 @@ Before writing new tests, check existing suites under `hugegraph-server/hugegrap ## Style & Pre-commit -- Line 100, 4-space indent, LF, UTF-8, **no star imports** +- Line 120, 4-space indent, LF, UTF-8, **no star imports** - Commit format: `feat|fix|refactor(module): msg` - Run before pushing: ```bash diff --git a/README.md b/README.md index adf9792776..f4543e073f 100644 --- a/README.md +++ b/README.md @@ -342,7 +342,7 @@ For detailed architecture and development guidance, see [AGENTS.md](AGENTS.md). - Try modifying a test and see what breaks 5. **Code Standards** - - Line length: 100 characters + - Line length: 120 characters - Indentation: 4 spaces - No star imports - Commit format: `feat|fix|refactor(module): description` diff --git a/hugegraph-pd/docs/development.md b/hugegraph-pd/docs/development.md index 3f01b902ea..514bd989a1 100644 --- a/hugegraph-pd/docs/development.md +++ b/hugegraph-pd/docs/development.md @@ -282,7 +282,7 @@ HugeGraph PD follows Apache HugeGraph code style. **Key Style Rules**: - **Indentation**: 4 spaces (no tabs) -- **Line length**: 100 characters (Java), 120 characters (comments) +- **Line length**: 120 characters - **Braces**: K&R style (opening brace on same line) - **Imports**: No wildcard imports (`import java.util.*`) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java index 5989d48892..7f264027ab 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java @@ -287,23 +287,19 @@ public String checkDefaultRole(@Context GraphManager manager, defaultRole = null; // unreachable, satisfies compiler } validGraphSpace(manager, graphSpace); - boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER) && - StringUtils.isNotEmpty(graph); + boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER) && StringUtils.isNotEmpty(graph); if (hasGraph) { validGraph(manager, graphSpace, graph); } boolean result; if (hasGraph) { - result = authManager.isDefaultRole(graphSpace, graph, user, - defaultRole); + result = authManager.isDefaultRole(graphSpace, graph, user, defaultRole); } else { - result = authManager.isDefaultRole(graphSpace, user, - defaultRole); + result = authManager.isDefaultRole(graphSpace, user, defaultRole); if (!result && defaultRole.equals(HugeDefaultRole.OBSERVER)) { for (String currentGraph : manager.graphs(graphSpace)) { - if (authManager.isDefaultRole( - graphSpace, currentGraph, user, defaultRole)) { + if (authManager.isDefaultRole(graphSpace, currentGraph, user, defaultRole)) { result = true; break; } diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java index 934508ed3d..1aafd5f28f 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java @@ -146,8 +146,7 @@ public String setDefaultRole(@Context GraphManager manager, throw new ForbiddenException("Forbidden to set role " + role.toString()); } - boolean hasGraph = role.equals(HugeDefaultRole.OBSERVER) && - StringUtils.isNotEmpty(graph); + boolean hasGraph = role.equals(HugeDefaultRole.OBSERVER) && StringUtils.isNotEmpty(graph); if (hasGraph) { validGraph(manager, name, graph); } @@ -164,8 +163,7 @@ public String setDefaultRole(@Context GraphManager manager, authManager.createSpaceDefaultRole(name, user, role); if (role.equals(HugeDefaultRole.OBSERVER)) { for (String currentGraph : manager.graphs(name)) { - authManager.deleteDefaultRole( - name, user, role, currentGraph); + authManager.deleteDefaultRole(name, user, role, currentGraph); } } } @@ -215,15 +213,12 @@ public String checkDefaultRole(@Context GraphManager manager, boolean result; if (hasGraph) { - result = authManager.isDefaultRole(name, graph, user, - defaultRole); + result = authManager.isDefaultRole(name, graph, user, defaultRole); } else { - result = authManager.isDefaultRole(name, user, - defaultRole); + result = authManager.isDefaultRole(name, user, defaultRole); if (!result && defaultRole.equals(HugeDefaultRole.OBSERVER)) { for (String currentGraph : manager.graphs(name)) { - if (authManager.isDefaultRole( - name, currentGraph, user, defaultRole)) { + if (authManager.isDefaultRole(name, currentGraph, user, defaultRole)) { result = true; break; } @@ -271,8 +266,7 @@ 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) && - StringUtils.isNotEmpty(graph); + boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER) && StringUtils.isNotEmpty(graph); if (hasGraph) { validGraph(manager, name, graph); } @@ -282,8 +276,7 @@ public void deleteDefaultRole(@Context GraphManager manager, authManager.deleteDefaultRole(name, user, defaultRole); if (defaultRole.equals(HugeDefaultRole.OBSERVER)) { for (String currentGraph : manager.graphs(name)) { - authManager.deleteDefaultRole( - name, user, defaultRole, currentGraph); + authManager.deleteDefaultRole(name, user, defaultRole, currentGraph); } } } diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java index 7fda423f58..afdb9505a5 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java @@ -135,8 +135,7 @@ public void delete(@Context GraphManager manager, "Schema template '%s' does not exist", name); String username = HugeGraphAuthProxy.username(); - if (canManage(manager.authManager(), graphSpace, st.creator(), - username)) { + if (canManage(manager.authManager(), graphSpace, st.creator(), username)) { manager.dropSchemaTemplate(graphSpace, name); } else { throw new ForbiddenException("No permission to delete schema template"); @@ -165,8 +164,7 @@ public String update(@Context GraphManager manager, } String username = HugeGraphAuthProxy.username(); - if (canManage(manager.authManager(), graphSpace, old.creator(), - username)) { + if (canManage(manager.authManager(), graphSpace, old.creator(), username)) { SchemaTemplate template = jsonSchemaTemplate.build(old); template.creator(old.creator()); template.create(old.create()); @@ -179,8 +177,7 @@ public String update(@Context GraphManager manager, } - private static boolean canManage(AuthManager authManager, - String graphSpace, String creator, + private static boolean canManage(AuthManager authManager, String graphSpace, String creator, String username) { return Objects.equals(creator, username) || authManager.isAdminManager(username) || diff --git a/style/checkstyle.xml b/style/checkstyle.xml index eec890ec25..d028e10b20 100644 --- a/style/checkstyle.xml +++ b/style/checkstyle.xml @@ -27,7 +27,7 @@ - + From 28641f5431f14dda78001660bf6cf3ff4a61c4d8 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 13:29:26 +0800 Subject: [PATCH 15/23] fix(server): preserve anonymous template ownership - defer authenticator lookup until manager access is needed - keep anonymous creators on the owner mutation path - cover lazy owner and manager authorization paths --- .../api/space/SchemaTemplateAPI.java | 17 +++++++++---- .../unit/api/space/SchemaTemplateAPITest.java | 25 +++++++++++++------ 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java index afdb9505a5..cffca156cd 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java @@ -20,6 +20,7 @@ import java.util.Date; import java.util.Objects; import java.util.Set; +import java.util.function.Supplier; import org.apache.commons.lang3.StringUtils; import org.apache.hugegraph.HugeException; @@ -135,7 +136,8 @@ public void delete(@Context GraphManager manager, "Schema template '%s' does not exist", name); String username = HugeGraphAuthProxy.username(); - if (canManage(manager.authManager(), graphSpace, st.creator(), username)) { + if (canManage(manager::authManager, graphSpace, st.creator(), + username)) { manager.dropSchemaTemplate(graphSpace, name); } else { throw new ForbiddenException("No permission to delete schema template"); @@ -164,7 +166,8 @@ public String update(@Context GraphManager manager, } String username = HugeGraphAuthProxy.username(); - if (canManage(manager.authManager(), graphSpace, old.creator(), username)) { + if (canManage(manager::authManager, graphSpace, old.creator(), + username)) { SchemaTemplate template = jsonSchemaTemplate.build(old); template.creator(old.creator()); template.create(old.create()); @@ -177,10 +180,14 @@ public String update(@Context GraphManager manager, } - private static boolean canManage(AuthManager authManager, String graphSpace, String creator, + private static boolean canManage(Supplier authManagerSupplier, + String graphSpace, String creator, String username) { - return Objects.equals(creator, username) || - authManager.isAdminManager(username) || + if (Objects.equals(creator, username)) { + return true; + } + AuthManager authManager = authManagerSupplier.get(); + return authManager.isAdminManager(username) || authManager.isSpaceManager(graphSpace, username); } diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/SchemaTemplateAPITest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/SchemaTemplateAPITest.java index c2be357eb4..617957fa5f 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/SchemaTemplateAPITest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/space/SchemaTemplateAPITest.java @@ -19,6 +19,8 @@ package org.apache.hugegraph.unit.api.space; +import java.util.function.Supplier; + import org.apache.hugegraph.api.space.SchemaTemplateAPI; import org.apache.hugegraph.auth.AuthManager; import org.apache.hugegraph.testutil.Assert; @@ -33,7 +35,11 @@ public class SchemaTemplateAPITest { @Test public void testCreatorCanManageTemplate() { - Assert.assertTrue(canManage(authManager(false, false), CREATOR)); + Supplier authManager = + Mockito.mock(Supplier.class); + + Assert.assertTrue(canManage(authManager, CREATOR)); + Mockito.verifyZeroInteractions(authManager); } @Test @@ -43,7 +49,8 @@ public void testGlobalAdminCanManageAnotherUsersTemplate() { @Test public void testSpaceManagerCanManageAnotherUsersTemplate() { - Assert.assertTrue(canManage(authManager(false, true), "space-admin")); + Assert.assertTrue(canManage(authManager(false, true), + "space-admin")); } @Test @@ -51,22 +58,24 @@ public void testUnrelatedUserCannotManageTemplate() { Assert.assertFalse(canManage(authManager(false, false), "member")); } - private static AuthManager authManager(boolean admin, - boolean spaceManager) { + private static Supplier authManager(boolean admin, + boolean spaceManager) { AuthManager auth = Mockito.mock(AuthManager.class); Mockito.when(auth.isAdminManager(Mockito.anyString())) .thenReturn(admin); Mockito.when(auth.isSpaceManager(GRAPHSPACE, "space-admin")) .thenReturn(spaceManager); - return auth; + return () -> auth; } - private static boolean canManage(AuthManager auth, String username) { + private static boolean canManage( + Supplier authManager, + String username) { return Whitebox.invokeStatic( SchemaTemplateAPI.class, - new Class[]{AuthManager.class, String.class, + new Class[]{Supplier.class, String.class, String.class, String.class}, "canManage", - auth, GRAPHSPACE, CREATOR, username); + authManager, GRAPHSPACE, CREATOR, username); } } From 8c69441c7c1a0b39c0ef7018d4ad51543ffc421e Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 20:43:49 +0800 Subject: [PATCH 16/23] fix(server): guard merge traversal writes - recognize mergeV and mergeE as write operations - preserve compatibility with the current TinkerPop baseline - reject execute-only create and onMatch traversals - verify recursive child traversal permissions --- .../hugegraph/auth/HugeGraphAuthProxy.java | 31 ++++- .../unit/auth/HugeGraphAuthProxyTest.java | 127 ++++++++++++++++++ 2 files changed, 153 insertions(+), 5 deletions(-) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java index f3440d0e57..1c2b6e2759 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java @@ -2530,11 +2530,7 @@ private static void collectTraversalPermissions( Traversal.Admin traversal, Set permissions) { for (Step step : traversal.getSteps()) { - if (step instanceof AddVertexStartStep || - step instanceof AddVertexStep || - step instanceof AddEdgeStartStep || - step instanceof AddEdgeStep || - step instanceof AddPropertyStep) { + if (isWriteStep(step)) { permissions.add(HugePermission.WRITE); } else if (step instanceof DropStep) { permissions.add(HugePermission.DELETE); @@ -2550,4 +2546,29 @@ private static void collectTraversalPermissions( } } } + + private static boolean isWriteStep(Step step) { + if (step instanceof AddVertexStartStep || + step instanceof AddVertexStep || + step instanceof AddEdgeStartStep || + step instanceof AddEdgeStep || + step instanceof AddPropertyStep) { + return true; + } + + /* + * HugeGraph currently compiles against TinkerPop 3.5, while mergeV/E + * were added later. Avoid a hard dependency so this guard also works + * when an embedding application supplies a newer compatible version. + */ + for (Class type = step.getClass(); type != null; + type = type.getSuperclass()) { + String name = type.getSimpleName(); + if ("MergeVertexStep".equals(name) || + "MergeEdgeStep".equals(name)) { + return true; + } + } + return false; + } } diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index 399c685fb2..a13a563ff0 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -21,6 +21,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.NoSuchElementException; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; @@ -55,12 +56,18 @@ import org.apache.logging.log4j.core.config.LoggerConfig; import org.apache.logging.log4j.core.config.Property; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.junit.After; import org.junit.Test; import org.mockito.Mockito; +import jakarta.ws.rs.ForbiddenException; + public class HugeGraphAuthProxyTest extends BaseUnitTest { private static HugeGraphAuthProxy.Context setContext( @@ -548,6 +555,39 @@ public void testTraversalPermissions() throws Exception { traversalPermissions(parent)); } + @Test + public void testExecuteOnlyCannotCreateVertexWithMerge() throws Exception { + assertExecuteOnlyCannotMerge(false, true); + } + + @Test + public void testExecuteOnlyCannotMatchVertexWithMerge() throws Exception { + assertExecuteOnlyCannotMerge(true, true); + } + + @Test + public void testExecuteOnlyCannotCreateEdgeWithMerge() throws Exception { + assertExecuteOnlyCannotMerge(false, false); + } + + @Test + public void testExecuteOnlyCannotMatchEdgeWithMerge() throws Exception { + assertExecuteOnlyCannotMerge(true, false); + } + + @Test + public void testMergeRecursesChildTraversals() throws Exception { + Traversal.Admin traversal = __.identity().asAdmin(); + MergeVertexStep merge = new MergeVertexStep(traversal, true); + merge.addChild(__.V().drop().asAdmin()); + traversal.addStep(merge); + + Set permissions = traversalPermissions(traversal); + Assert.assertEquals(2, permissions.size()); + Assert.assertTrue(permissions.contains(HugePermission.WRITE)); + Assert.assertTrue(permissions.contains(HugePermission.DELETE)); + } + @Test public void testTraversalStrategyListKeepsAuthProxy() { HugeGraph graph = Mockito.mock(HugeGraph.class); @@ -661,6 +701,93 @@ private static Set traversalPermissions( return (Set) method.invoke(null, traversal); } + @SuppressWarnings({"rawtypes", "unchecked"}) + private static void assertExecuteOnlyCannotMerge(boolean onMatch, + boolean vertex) + throws Exception { + Traversal.Admin traversal = __.identity().asAdmin(); + AbstractStep merge = vertex ? + new MergeVertexStep(traversal, onMatch) : + new MergeEdgeStep(traversal, onMatch); + traversal.addStep(merge); + Assert.assertEquals(Collections.singleton(HugePermission.WRITE), + traversalPermissions(traversal)); + + HugeGraph graph = Mockito.mock(HugeGraph.class); + HugeConfig config = Mockito.mock(HugeConfig.class); + AuthManager authManager = Mockito.mock(AuthManager.class); + TaskScheduler scheduler = Mockito.mock(TaskScheduler.class); + Mockito.when(graph.name()).thenReturn("hugegraph"); + Mockito.when(graph.graphSpace()).thenReturn("DEFAULT"); + Mockito.when(graph.spaceGraphName()).thenReturn("DEFAULT-hugegraph"); + Mockito.when(graph.configuration()).thenReturn(config); + Mockito.when(graph.authManager()).thenReturn(authManager); + Mockito.when(graph.taskScheduler()).thenReturn(scheduler); + Mockito.when(config.get(AuthOptions.AUTH_CACHE_EXPIRE)).thenReturn(3600L); + Mockito.when(config.get(AuthOptions.AUTH_CACHE_CAPACITY)).thenReturn(100L); + Mockito.when(config.get(AuthOptions.AUTH_AUDIT_LOG_RATE)).thenReturn(1000D); + + RolePermission executeOnly = RolePermission.fromJson( + "{\"roles\":{\"DEFAULT\":{\"hugegraph\":{" + + "\"EXECUTE\":{\"GREMLIN\":[{" + + "\"type\":\"GREMLIN\",\"label\":\"*\"," + + "\"properties\":null}]}}}}}"); + setContext(new HugeGraphAuthProxy.Context( + new HugeAuthenticator.User("execute-only", executeOnly))); + + TraversalStrategy strategy = + new HugeGraphAuthProxy(graph).traversal() + .getStrategies().toList().get(0); + Assert.assertThrows(ForbiddenException.class, + () -> strategy.apply(traversal)); + } + + private abstract static class TestMergeStep + extends AbstractStep + implements TraversalParent { + + private final List> children; + + TestMergeStep(Traversal.Admin traversal, boolean onMatch) { + super(traversal); + this.children = new ArrayList<>(); + this.children.add(__.constant(Collections.emptyMap()).asAdmin()); + if (onMatch) { + this.children.add(__.constant(Collections.emptyMap()).asAdmin()); + } + } + + void addChild(Traversal.Admin child) { + this.children.add(child); + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + @Override + public List> getLocalChildren() { + return (List) this.children; + } + + @Override + protected Traverser.Admin processNextStart() + throws NoSuchElementException { + throw new NoSuchElementException(); + } + } + + private static class MergeVertexStep extends TestMergeStep { + + MergeVertexStep(Traversal.Admin traversal, boolean onMatch) { + super(traversal, onMatch); + } + } + + private static class MergeEdgeStep extends TestMergeStep { + + MergeEdgeStep(Traversal.Admin traversal, boolean onMatch) { + super(traversal, onMatch); + } + } + private static class TestAppender extends AbstractAppender { private final List events; From 17615dc0b901bc5d628de2608c487f3f9055a661 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 20:52:48 +0800 Subject: [PATCH 17/23] fix(server): scope merge step detection - match merge steps by exact TinkerPop class names - retain superclass traversal for provider implementations - reject unrelated steps sharing merge simple names - preserve recursive child permission coverage --- .../hugegraph/auth/HugeGraphAuthProxy.java | 9 +- .../unit/auth/HugeGraphAuthProxyTest.java | 98 +++++-------------- 2 files changed, 29 insertions(+), 78 deletions(-) diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java index 1c2b6e2759..16e3a46416 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java @@ -2563,12 +2563,15 @@ private static boolean isWriteStep(Step step) { */ for (Class type = step.getClass(); type != null; type = type.getSuperclass()) { - String name = type.getSimpleName(); - if ("MergeVertexStep".equals(name) || - "MergeEdgeStep".equals(name)) { + if (isMergeStepClassName(type.getName())) { return true; } } return false; } + + private static boolean isMergeStepClassName(String name) { + return "org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep".equals(name) || + "org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeEdgeStep".equals(name); + } } diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index a13a563ff0..64ceb5a80f 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -56,7 +56,6 @@ import org.apache.logging.log4j.core.config.LoggerConfig; import org.apache.logging.log4j.core.config.Property; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; -import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; @@ -66,8 +65,6 @@ import org.junit.Test; import org.mockito.Mockito; -import jakarta.ws.rs.ForbiddenException; - public class HugeGraphAuthProxyTest extends BaseUnitTest { private static HugeGraphAuthProxy.Context setContext( @@ -556,36 +553,34 @@ public void testTraversalPermissions() throws Exception { } @Test - public void testExecuteOnlyCannotCreateVertexWithMerge() throws Exception { - assertExecuteOnlyCannotMerge(false, true); - } - - @Test - public void testExecuteOnlyCannotMatchVertexWithMerge() throws Exception { - assertExecuteOnlyCannotMerge(true, true); + public void testTinkerPopMergeStepsRequireWrite() { + Assert.assertTrue(isMergeStepClassName( + "org.apache.tinkerpop.gremlin.process.traversal.step.map." + + "MergeVertexStep")); + Assert.assertTrue(isMergeStepClassName( + "org.apache.tinkerpop.gremlin.process.traversal.step.map." + + "MergeEdgeStep")); } @Test - public void testExecuteOnlyCannotCreateEdgeWithMerge() throws Exception { - assertExecuteOnlyCannotMerge(false, false); - } + public void testSameSimpleNameOutsideTinkerPopDoesNotRequireWrite() + throws Exception { + Traversal.Admin traversal = __.identity().asAdmin(); + traversal.addStep(new MergeVertexStep(traversal)); - @Test - public void testExecuteOnlyCannotMatchEdgeWithMerge() throws Exception { - assertExecuteOnlyCannotMerge(true, false); + Assert.assertTrue(traversalPermissions(traversal).isEmpty()); } @Test public void testMergeRecursesChildTraversals() throws Exception { Traversal.Admin traversal = __.identity().asAdmin(); - MergeVertexStep merge = new MergeVertexStep(traversal, true); + TestTraversalParent merge = new TestTraversalParent(traversal); merge.addChild(__.V().drop().asAdmin()); traversal.addStep(merge); Set permissions = traversalPermissions(traversal); - Assert.assertEquals(2, permissions.size()); - Assert.assertTrue(permissions.contains(HugePermission.WRITE)); - Assert.assertTrue(permissions.contains(HugePermission.DELETE)); + Assert.assertEquals(Collections.singleton(HugePermission.DELETE), + permissions); } @Test @@ -701,60 +696,20 @@ private static Set traversalPermissions( return (Set) method.invoke(null, traversal); } - @SuppressWarnings({"rawtypes", "unchecked"}) - private static void assertExecuteOnlyCannotMerge(boolean onMatch, - boolean vertex) - throws Exception { - Traversal.Admin traversal = __.identity().asAdmin(); - AbstractStep merge = vertex ? - new MergeVertexStep(traversal, onMatch) : - new MergeEdgeStep(traversal, onMatch); - traversal.addStep(merge); - Assert.assertEquals(Collections.singleton(HugePermission.WRITE), - traversalPermissions(traversal)); - - HugeGraph graph = Mockito.mock(HugeGraph.class); - HugeConfig config = Mockito.mock(HugeConfig.class); - AuthManager authManager = Mockito.mock(AuthManager.class); - TaskScheduler scheduler = Mockito.mock(TaskScheduler.class); - Mockito.when(graph.name()).thenReturn("hugegraph"); - Mockito.when(graph.graphSpace()).thenReturn("DEFAULT"); - Mockito.when(graph.spaceGraphName()).thenReturn("DEFAULT-hugegraph"); - Mockito.when(graph.configuration()).thenReturn(config); - Mockito.when(graph.authManager()).thenReturn(authManager); - Mockito.when(graph.taskScheduler()).thenReturn(scheduler); - Mockito.when(config.get(AuthOptions.AUTH_CACHE_EXPIRE)).thenReturn(3600L); - Mockito.when(config.get(AuthOptions.AUTH_CACHE_CAPACITY)).thenReturn(100L); - Mockito.when(config.get(AuthOptions.AUTH_AUDIT_LOG_RATE)).thenReturn(1000D); - - RolePermission executeOnly = RolePermission.fromJson( - "{\"roles\":{\"DEFAULT\":{\"hugegraph\":{" + - "\"EXECUTE\":{\"GREMLIN\":[{" + - "\"type\":\"GREMLIN\",\"label\":\"*\"," + - "\"properties\":null}]}}}}}"); - setContext(new HugeGraphAuthProxy.Context( - new HugeAuthenticator.User("execute-only", executeOnly))); - - TraversalStrategy strategy = - new HugeGraphAuthProxy(graph).traversal() - .getStrategies().toList().get(0); - Assert.assertThrows(ForbiddenException.class, - () -> strategy.apply(traversal)); + private static boolean isMergeStepClassName(String name) { + return Whitebox.invokeStatic(HugeGraphAuthProxy.class, + "isMergeStepClassName", name); } - private abstract static class TestMergeStep + private static class TestTraversalParent extends AbstractStep implements TraversalParent { private final List> children; - TestMergeStep(Traversal.Admin traversal, boolean onMatch) { + TestTraversalParent(Traversal.Admin traversal) { super(traversal); this.children = new ArrayList<>(); - this.children.add(__.constant(Collections.emptyMap()).asAdmin()); - if (onMatch) { - this.children.add(__.constant(Collections.emptyMap()).asAdmin()); - } } void addChild(Traversal.Admin child) { @@ -774,17 +729,10 @@ protected Traverser.Admin processNextStart() } } - private static class MergeVertexStep extends TestMergeStep { - - MergeVertexStep(Traversal.Admin traversal, boolean onMatch) { - super(traversal, onMatch); - } - } - - private static class MergeEdgeStep extends TestMergeStep { + private static class MergeVertexStep extends TestTraversalParent { - MergeEdgeStep(Traversal.Admin traversal, boolean onMatch) { - super(traversal, onMatch); + MergeVertexStep(Traversal.Admin traversal) { + super(traversal); } } From 8d03d9ca8453705e6e2a9e13ab5f069a6fa5fe1b Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 20:58:06 +0800 Subject: [PATCH 18/23] fix(server): verify merge traversal guards - add pinned-3.5 test fixtures for TinkerPop merge steps - route vertex and edge merge shapes through strategy checks - retain external same-name and child traversal regressions --- .../unit/auth/HugeGraphAuthProxyTest.java | 93 ++++++++++++++++--- .../traversal/step/map/MergeEdgeStep.java | 30 ++++++ .../traversal/step/map/MergeVertexStep.java | 65 +++++++++++++ 3 files changed, 175 insertions(+), 13 deletions(-) create mode 100644 hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java create mode 100644 hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index 64ceb5a80f..1139f0f8d1 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -56,6 +56,7 @@ import org.apache.logging.log4j.core.config.LoggerConfig; import org.apache.logging.log4j.core.config.Property; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; @@ -65,6 +66,8 @@ import org.junit.Test; import org.mockito.Mockito; +import jakarta.ws.rs.ForbiddenException; + public class HugeGraphAuthProxyTest extends BaseUnitTest { private static HugeGraphAuthProxy.Context setContext( @@ -553,13 +556,23 @@ public void testTraversalPermissions() throws Exception { } @Test - public void testTinkerPopMergeStepsRequireWrite() { - Assert.assertTrue(isMergeStepClassName( - "org.apache.tinkerpop.gremlin.process.traversal.step.map." + - "MergeVertexStep")); - Assert.assertTrue(isMergeStepClassName( - "org.apache.tinkerpop.gremlin.process.traversal.step.map." + - "MergeEdgeStep")); + public void testExecuteOnlyCannotCreateVertexWithMerge() throws Exception { + assertExecuteOnlyCannotMerge(false, true); + } + + @Test + public void testExecuteOnlyCannotMatchVertexWithMerge() throws Exception { + assertExecuteOnlyCannotMerge(true, true); + } + + @Test + public void testExecuteOnlyCannotCreateEdgeWithMerge() throws Exception { + assertExecuteOnlyCannotMerge(false, false); + } + + @Test + public void testExecuteOnlyCannotMatchEdgeWithMerge() throws Exception { + assertExecuteOnlyCannotMerge(true, false); } @Test @@ -574,13 +587,16 @@ public void testSameSimpleNameOutsideTinkerPopDoesNotRequireWrite() @Test public void testMergeRecursesChildTraversals() throws Exception { Traversal.Admin traversal = __.identity().asAdmin(); - TestTraversalParent merge = new TestTraversalParent(traversal); + org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep + merge = new org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep( + traversal); merge.addChild(__.V().drop().asAdmin()); traversal.addStep(merge); Set permissions = traversalPermissions(traversal); - Assert.assertEquals(Collections.singleton(HugePermission.DELETE), - permissions); + Assert.assertEquals(2, permissions.size()); + Assert.assertTrue(permissions.contains(HugePermission.WRITE)); + Assert.assertTrue(permissions.contains(HugePermission.DELETE)); } @Test @@ -696,9 +712,60 @@ private static Set traversalPermissions( return (Set) method.invoke(null, traversal); } - private static boolean isMergeStepClassName(String name) { - return Whitebox.invokeStatic(HugeGraphAuthProxy.class, - "isMergeStepClassName", name); + @SuppressWarnings({"rawtypes", "unchecked"}) + private static void assertExecuteOnlyCannotMerge(boolean onMatch, + boolean vertex) + throws Exception { + Traversal.Admin traversal = __.identity().asAdmin(); + AbstractStep merge; + if (vertex) { + org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep + step = new org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep( + traversal); + merge = step; + if (onMatch) { + step.addChild(__.constant(Collections.emptyMap()).asAdmin()); + } + } else { + org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeEdgeStep + step = new org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeEdgeStep( + traversal); + merge = step; + if (onMatch) { + step.addChild(__.constant(Collections.emptyMap()).asAdmin()); + } + } + traversal.addStep(merge); + Assert.assertEquals(Collections.singleton(HugePermission.WRITE), + traversalPermissions(traversal)); + + HugeGraph graph = Mockito.mock(HugeGraph.class); + HugeConfig config = Mockito.mock(HugeConfig.class); + AuthManager authManager = Mockito.mock(AuthManager.class); + TaskScheduler scheduler = Mockito.mock(TaskScheduler.class); + Mockito.when(graph.name()).thenReturn("hugegraph"); + Mockito.when(graph.graphSpace()).thenReturn("DEFAULT"); + Mockito.when(graph.spaceGraphName()).thenReturn("DEFAULT-hugegraph"); + Mockito.when(graph.configuration()).thenReturn(config); + Mockito.when(graph.authManager()).thenReturn(authManager); + Mockito.when(graph.taskScheduler()).thenReturn(scheduler); + Mockito.when(config.get(AuthOptions.AUTH_CACHE_EXPIRE)).thenReturn(3600L); + Mockito.when(config.get(AuthOptions.AUTH_CACHE_CAPACITY)).thenReturn(100L); + Mockito.when(config.get(AuthOptions.AUTH_AUDIT_LOG_RATE)).thenReturn(1000D); + + RolePermission executeOnly = RolePermission.fromJson( + "{\"roles\":{\"DEFAULT\":{\"hugegraph\":{" + + "\"EXECUTE\":{\"GREMLIN\":[{" + + "\"type\":\"GREMLIN\",\"label\":\"*\"," + + "\"properties\":null}]}}}}}"); + setContext(new HugeGraphAuthProxy.Context( + new HugeAuthenticator.User("execute-only", executeOnly))); + + TraversalStrategy strategy = + new HugeGraphAuthProxy(graph).traversal() + .getStrategies().toList().get(0); + Assert.assertThrows(ForbiddenException.class, + () -> strategy.apply(traversal)); } private static class TestTraversalParent diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java new file mode 100644 index 0000000000..cb380ff7a8 --- /dev/null +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java @@ -0,0 +1,30 @@ +/* + * 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.tinkerpop.gremlin.process.traversal.step.map; + +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; + +/* + * Test-only compatibility fixture for TinkerPop 3.7 MergeEdgeStep. + */ +public class MergeEdgeStep extends TestMergeStep { + + public MergeEdgeStep(Traversal.Admin traversal) { + super(traversal); + } +} diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java new file mode 100644 index 0000000000..2da2616675 --- /dev/null +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java @@ -0,0 +1,65 @@ +/* + * 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.tinkerpop.gremlin.process.traversal.step.map; + +import java.util.ArrayList; +import java.util.List; +import java.util.NoSuchElementException; + +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep; + +/* + * Test-only compatibility fixture for TinkerPop 3.7 merge steps. HugeGraph + * currently compiles against 3.5, where these classes do not exist. + */ +public class MergeVertexStep extends TestMergeStep { + + public MergeVertexStep(Traversal.Admin traversal) { + super(traversal); + } +} + +abstract class TestMergeStep extends AbstractStep + implements TraversalParent { + + private final List> children; + + TestMergeStep(Traversal.Admin traversal) { + super(traversal); + this.children = new ArrayList<>(); + } + + public void addChild(Traversal.Admin child) { + this.children.add(child); + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + @Override + public List> getLocalChildren() { + return (List) this.children; + } + + @Override + protected Traverser.Admin processNextStart() + throws NoSuchElementException { + throw new NoSuchElementException(); + } +} From 2ed446b3da8564b0a0fe622ad8b5f74aae1013f7 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 21:08:55 +0800 Subject: [PATCH 19/23] fix(server): isolate merge test fixtures - move compatibility fixtures into test output only - add test output to the unit-test classpath - construct exact-package fixtures reflectively - keep main artifacts free of TinkerPop shadow classes --- hugegraph-server/hugegraph-test/pom.xml | 5 +++ .../unit/auth/HugeGraphAuthProxyTest.java | 45 ++++++++++--------- .../traversal/step/map/MergeEdgeStep.java | 7 ++- .../traversal/step/map/MergeVertexStep.java | 9 +++- 4 files changed, 42 insertions(+), 24 deletions(-) rename hugegraph-server/hugegraph-test/src/{main => test}/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java (85%) rename hugegraph-server/hugegraph-test/src/{main => test}/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java (89%) diff --git a/hugegraph-server/hugegraph-test/pom.xml b/hugegraph-server/hugegraph-test/pom.xml index 259d5a9b9a..f1187eb839 100644 --- a/hugegraph-server/hugegraph-test/pom.xml +++ b/hugegraph-server/hugegraph-test/pom.xml @@ -135,6 +135,11 @@ ${basedir}/target/classes/ + + + ${project.build.testOutputDirectory} + + **/UnitTestSuite.java diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index 1139f0f8d1..8f6aa895a0 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -587,10 +587,8 @@ public void testSameSimpleNameOutsideTinkerPopDoesNotRequireWrite() @Test public void testMergeRecursesChildTraversals() throws Exception { Traversal.Admin traversal = __.identity().asAdmin(); - org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep - merge = new org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep( - traversal); - merge.addChild(__.V().drop().asAdmin()); + AbstractStep merge = mergeStep(traversal, true); + addMergeChild(merge, __.V().drop().asAdmin()); traversal.addStep(merge); Set permissions = traversalPermissions(traversal); @@ -717,23 +715,10 @@ private static void assertExecuteOnlyCannotMerge(boolean onMatch, boolean vertex) throws Exception { Traversal.Admin traversal = __.identity().asAdmin(); - AbstractStep merge; - if (vertex) { - org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep - step = new org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep( - traversal); - merge = step; - if (onMatch) { - step.addChild(__.constant(Collections.emptyMap()).asAdmin()); - } - } else { - org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeEdgeStep - step = new org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeEdgeStep( - traversal); - merge = step; - if (onMatch) { - step.addChild(__.constant(Collections.emptyMap()).asAdmin()); - } + AbstractStep merge = mergeStep(traversal, vertex); + if (onMatch) { + addMergeChild(merge, + __.constant(Collections.emptyMap()).asAdmin()); } traversal.addStep(merge); Assert.assertEquals(Collections.singleton(HugePermission.WRITE), @@ -768,6 +753,24 @@ private static void assertExecuteOnlyCannotMerge(boolean onMatch, () -> strategy.apply(traversal)); } + @SuppressWarnings("unchecked") + private static AbstractStep mergeStep( + Traversal.Admin traversal, boolean vertex) throws Exception { + String type = "org.apache.tinkerpop.gremlin.process.traversal.step.map." + + (vertex ? "MergeVertexStep" : "MergeEdgeStep"); + Class mergeClass = Class.forName(type); + return (AbstractStep) + mergeClass.getConstructor(Traversal.Admin.class) + .newInstance(traversal); + } + + private static void addMergeChild(AbstractStep merge, + Traversal.Admin child) + throws Exception { + merge.getClass().getMethod("addChild", Traversal.Admin.class) + .invoke(merge, child); + } + private static class TestTraversalParent extends AbstractStep implements TraversalParent { diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java b/hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java similarity index 85% rename from hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java rename to hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java index cb380ff7a8..ba457e4243 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java +++ b/hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java @@ -20,11 +20,16 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; /* - * Test-only compatibility fixture for TinkerPop 3.7 MergeEdgeStep. + * Test-output-only compatibility fixture for TinkerPop 3.7 MergeEdgeStep. */ public class MergeEdgeStep extends TestMergeStep { public MergeEdgeStep(Traversal.Admin traversal) { super(traversal); } + + @Override + public void addChild(Traversal.Admin child) { + super.addChild(child); + } } diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java b/hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java similarity index 89% rename from hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java rename to hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java index 2da2616675..e56d936886 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java +++ b/hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java @@ -27,14 +27,19 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep; /* - * Test-only compatibility fixture for TinkerPop 3.7 merge steps. HugeGraph - * currently compiles against 3.5, where these classes do not exist. + * Test-output-only compatibility fixture for TinkerPop 3.7 merge steps. + * HugeGraph currently compiles against 3.5, where these classes do not exist. */ public class MergeVertexStep extends TestMergeStep { public MergeVertexStep(Traversal.Admin traversal) { super(traversal); } + + @Override + public void addChild(Traversal.Admin child) { + super.addChild(child); + } } abstract class TestMergeStep extends AbstractStep From b94347b5ce649c597cbd75e3614943592a876970 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 21:15:11 +0800 Subject: [PATCH 20/23] fix(server): bind merge fixtures to tinkerpop - select fixture sources from the pinned TinkerPop version - keep 3.5.1 compatibility classes in test output only - stop selecting fixtures automatically after a version change - preserve merge permission regression coverage --- hugegraph-server/hugegraph-test/pom.xml | 21 +++++++++++++++++++ .../traversal/step/map/MergeEdgeStep.java | 2 +- .../traversal/step/map/MergeVertexStep.java | 4 ++-- 3 files changed, 24 insertions(+), 3 deletions(-) rename hugegraph-server/hugegraph-test/src/{test => test-tinkerpop-3.5.1}/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java (93%) rename hugegraph-server/hugegraph-test/src/{test => test-tinkerpop-3.5.1}/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java (93%) diff --git a/hugegraph-server/hugegraph-test/pom.xml b/hugegraph-server/hugegraph-test/pom.xml index f1187eb839..e521a8ee3c 100644 --- a/hugegraph-server/hugegraph-test/pom.xml +++ b/hugegraph-server/hugegraph-test/pom.xml @@ -111,6 +111,27 @@ + + org.codehaus.mojo + build-helper-maven-plugin + 3.5.0 + + + add-tinkerpop-test-source + generate-test-sources + + add-test-source + + + + + src/test-tinkerpop-${tinkerpop.version}/java + + + + + + org.apache.maven.plugins maven-surefire-plugin diff --git a/hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java b/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java similarity index 93% rename from hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java rename to hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java index ba457e4243..a8a2fdf7f2 100644 --- a/hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java +++ b/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java @@ -20,7 +20,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; /* - * Test-output-only compatibility fixture for TinkerPop 3.7 MergeEdgeStep. + * Test-output-only compatibility fixture for TinkerPop 3.5.1 MergeEdgeStep. */ public class MergeEdgeStep extends TestMergeStep { diff --git a/hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java b/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java similarity index 93% rename from hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java rename to hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java index e56d936886..ff1d4ba015 100644 --- a/hugegraph-server/hugegraph-test/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java +++ b/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java @@ -27,8 +27,8 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep; /* - * Test-output-only compatibility fixture for TinkerPop 3.7 merge steps. - * HugeGraph currently compiles against 3.5, where these classes do not exist. + * Test-output-only compatibility fixture for TinkerPop 3.5.1, where the + * TinkerPop 3.7 merge classes do not exist. */ public class MergeVertexStep extends TestMergeStep { From f38b4ad6e5e8a6eb321d53f60cae8023889995cf Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 21:19:34 +0800 Subject: [PATCH 21/23] fix(server): align merge fixture API - prefer the official traversal and isStart constructor - use Merge.onMatch through reflection when available - keep an explicit 3.5.1 fixture child fallback - preserve merge authorization coverage --- .../unit/auth/HugeGraphAuthProxyTest.java | 28 +++++++++++++++---- .../traversal/step/map/MergeEdgeStep.java | 2 +- .../traversal/step/map/MergeVertexStep.java | 2 +- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index 8f6aa895a0..f65dc9e666 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -759,16 +759,34 @@ private static AbstractStep mergeStep( String type = "org.apache.tinkerpop.gremlin.process.traversal.step.map." + (vertex ? "MergeVertexStep" : "MergeEdgeStep"); Class mergeClass = Class.forName(type); - return (AbstractStep) - mergeClass.getConstructor(Traversal.Admin.class) - .newInstance(traversal); + try { + return (AbstractStep) + mergeClass.getConstructor(Traversal.Admin.class, + boolean.class) + .newInstance(traversal, true); + } catch (NoSuchMethodException ignored) { + return (AbstractStep) + mergeClass.getConstructor(Traversal.Admin.class) + .newInstance(traversal); + } } + @SuppressWarnings({"rawtypes", "unchecked"}) private static void addMergeChild(AbstractStep merge, Traversal.Admin child) throws Exception { - merge.getClass().getMethod("addChild", Traversal.Admin.class) - .invoke(merge, child); + try { + Class mergeToken = + (Class) Class.forName( + "org.apache.tinkerpop.gremlin.process.traversal.Merge"); + Enum onMatch = Enum.valueOf(mergeToken, "onMatch"); + merge.getClass().getMethod("addChildOption", mergeToken, + Traversal.Admin.class) + .invoke(merge, onMatch, child); + } catch (ClassNotFoundException ignored) { + merge.getClass().getMethod("addChild", Traversal.Admin.class) + .invoke(merge, child); + } } private static class TestTraversalParent diff --git a/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java b/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java index a8a2fdf7f2..402a8a2bc1 100644 --- a/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java +++ b/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java @@ -24,7 +24,7 @@ */ public class MergeEdgeStep extends TestMergeStep { - public MergeEdgeStep(Traversal.Admin traversal) { + public MergeEdgeStep(Traversal.Admin traversal, boolean isStart) { super(traversal); } diff --git a/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java b/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java index ff1d4ba015..81aaa3b7aa 100644 --- a/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java +++ b/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java @@ -32,7 +32,7 @@ */ public class MergeVertexStep extends TestMergeStep { - public MergeVertexStep(Traversal.Admin traversal) { + public MergeVertexStep(Traversal.Admin traversal, boolean isStart) { super(traversal); } From 6f78df53854711c94a13b9a6c3be2fb2455bc6d7 Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 21:24:42 +0800 Subject: [PATCH 22/23] chore(server): remove speculative merge guards - restore the TinkerPop 3.5.1 authorization scope\n- remove future-version merge detection and fixtures\n- keep the Hubble permission closeout focused on reproduced behavior --- .../hugegraph/auth/HugeGraphAuthProxy.java | 34 +--- hugegraph-server/hugegraph-test/pom.xml | 26 --- .../unit/auth/HugeGraphAuthProxyTest.java | 163 ------------------ .../traversal/step/map/MergeEdgeStep.java | 35 ---- .../traversal/step/map/MergeVertexStep.java | 70 -------- 5 files changed, 5 insertions(+), 323 deletions(-) delete mode 100644 hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java delete mode 100644 hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java diff --git a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java index 16e3a46416..f3440d0e57 100644 --- a/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java +++ b/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java @@ -2530,7 +2530,11 @@ private static void collectTraversalPermissions( Traversal.Admin traversal, Set permissions) { for (Step step : traversal.getSteps()) { - if (isWriteStep(step)) { + if (step instanceof AddVertexStartStep || + step instanceof AddVertexStep || + step instanceof AddEdgeStartStep || + step instanceof AddEdgeStep || + step instanceof AddPropertyStep) { permissions.add(HugePermission.WRITE); } else if (step instanceof DropStep) { permissions.add(HugePermission.DELETE); @@ -2546,32 +2550,4 @@ private static void collectTraversalPermissions( } } } - - private static boolean isWriteStep(Step step) { - if (step instanceof AddVertexStartStep || - step instanceof AddVertexStep || - step instanceof AddEdgeStartStep || - step instanceof AddEdgeStep || - step instanceof AddPropertyStep) { - return true; - } - - /* - * HugeGraph currently compiles against TinkerPop 3.5, while mergeV/E - * were added later. Avoid a hard dependency so this guard also works - * when an embedding application supplies a newer compatible version. - */ - for (Class type = step.getClass(); type != null; - type = type.getSuperclass()) { - if (isMergeStepClassName(type.getName())) { - return true; - } - } - return false; - } - - private static boolean isMergeStepClassName(String name) { - return "org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep".equals(name) || - "org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeEdgeStep".equals(name); - } } diff --git a/hugegraph-server/hugegraph-test/pom.xml b/hugegraph-server/hugegraph-test/pom.xml index e521a8ee3c..259d5a9b9a 100644 --- a/hugegraph-server/hugegraph-test/pom.xml +++ b/hugegraph-server/hugegraph-test/pom.xml @@ -111,27 +111,6 @@ - - org.codehaus.mojo - build-helper-maven-plugin - 3.5.0 - - - add-tinkerpop-test-source - generate-test-sources - - add-test-source - - - - - src/test-tinkerpop-${tinkerpop.version}/java - - - - - - org.apache.maven.plugins maven-surefire-plugin @@ -156,11 +135,6 @@ ${basedir}/target/classes/ - - - ${project.build.testOutputDirectory} - - **/UnitTestSuite.java diff --git a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java index f65dc9e666..399c685fb2 100644 --- a/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java +++ b/hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java @@ -21,7 +21,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.NoSuchElementException; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; @@ -56,18 +55,12 @@ import org.apache.logging.log4j.core.config.LoggerConfig; import org.apache.logging.log4j.core.config.Property; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; -import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; -import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; -import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; -import org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.junit.After; import org.junit.Test; import org.mockito.Mockito; -import jakarta.ws.rs.ForbiddenException; - public class HugeGraphAuthProxyTest extends BaseUnitTest { private static HugeGraphAuthProxy.Context setContext( @@ -555,48 +548,6 @@ public void testTraversalPermissions() throws Exception { traversalPermissions(parent)); } - @Test - public void testExecuteOnlyCannotCreateVertexWithMerge() throws Exception { - assertExecuteOnlyCannotMerge(false, true); - } - - @Test - public void testExecuteOnlyCannotMatchVertexWithMerge() throws Exception { - assertExecuteOnlyCannotMerge(true, true); - } - - @Test - public void testExecuteOnlyCannotCreateEdgeWithMerge() throws Exception { - assertExecuteOnlyCannotMerge(false, false); - } - - @Test - public void testExecuteOnlyCannotMatchEdgeWithMerge() throws Exception { - assertExecuteOnlyCannotMerge(true, false); - } - - @Test - public void testSameSimpleNameOutsideTinkerPopDoesNotRequireWrite() - throws Exception { - Traversal.Admin traversal = __.identity().asAdmin(); - traversal.addStep(new MergeVertexStep(traversal)); - - Assert.assertTrue(traversalPermissions(traversal).isEmpty()); - } - - @Test - public void testMergeRecursesChildTraversals() throws Exception { - Traversal.Admin traversal = __.identity().asAdmin(); - AbstractStep merge = mergeStep(traversal, true); - addMergeChild(merge, __.V().drop().asAdmin()); - traversal.addStep(merge); - - Set permissions = traversalPermissions(traversal); - Assert.assertEquals(2, permissions.size()); - Assert.assertTrue(permissions.contains(HugePermission.WRITE)); - Assert.assertTrue(permissions.contains(HugePermission.DELETE)); - } - @Test public void testTraversalStrategyListKeepsAuthProxy() { HugeGraph graph = Mockito.mock(HugeGraph.class); @@ -710,120 +661,6 @@ private static Set traversalPermissions( return (Set) method.invoke(null, traversal); } - @SuppressWarnings({"rawtypes", "unchecked"}) - private static void assertExecuteOnlyCannotMerge(boolean onMatch, - boolean vertex) - throws Exception { - Traversal.Admin traversal = __.identity().asAdmin(); - AbstractStep merge = mergeStep(traversal, vertex); - if (onMatch) { - addMergeChild(merge, - __.constant(Collections.emptyMap()).asAdmin()); - } - traversal.addStep(merge); - Assert.assertEquals(Collections.singleton(HugePermission.WRITE), - traversalPermissions(traversal)); - - HugeGraph graph = Mockito.mock(HugeGraph.class); - HugeConfig config = Mockito.mock(HugeConfig.class); - AuthManager authManager = Mockito.mock(AuthManager.class); - TaskScheduler scheduler = Mockito.mock(TaskScheduler.class); - Mockito.when(graph.name()).thenReturn("hugegraph"); - Mockito.when(graph.graphSpace()).thenReturn("DEFAULT"); - Mockito.when(graph.spaceGraphName()).thenReturn("DEFAULT-hugegraph"); - Mockito.when(graph.configuration()).thenReturn(config); - Mockito.when(graph.authManager()).thenReturn(authManager); - Mockito.when(graph.taskScheduler()).thenReturn(scheduler); - Mockito.when(config.get(AuthOptions.AUTH_CACHE_EXPIRE)).thenReturn(3600L); - Mockito.when(config.get(AuthOptions.AUTH_CACHE_CAPACITY)).thenReturn(100L); - Mockito.when(config.get(AuthOptions.AUTH_AUDIT_LOG_RATE)).thenReturn(1000D); - - RolePermission executeOnly = RolePermission.fromJson( - "{\"roles\":{\"DEFAULT\":{\"hugegraph\":{" + - "\"EXECUTE\":{\"GREMLIN\":[{" + - "\"type\":\"GREMLIN\",\"label\":\"*\"," + - "\"properties\":null}]}}}}}"); - setContext(new HugeGraphAuthProxy.Context( - new HugeAuthenticator.User("execute-only", executeOnly))); - - TraversalStrategy strategy = - new HugeGraphAuthProxy(graph).traversal() - .getStrategies().toList().get(0); - Assert.assertThrows(ForbiddenException.class, - () -> strategy.apply(traversal)); - } - - @SuppressWarnings("unchecked") - private static AbstractStep mergeStep( - Traversal.Admin traversal, boolean vertex) throws Exception { - String type = "org.apache.tinkerpop.gremlin.process.traversal.step.map." + - (vertex ? "MergeVertexStep" : "MergeEdgeStep"); - Class mergeClass = Class.forName(type); - try { - return (AbstractStep) - mergeClass.getConstructor(Traversal.Admin.class, - boolean.class) - .newInstance(traversal, true); - } catch (NoSuchMethodException ignored) { - return (AbstractStep) - mergeClass.getConstructor(Traversal.Admin.class) - .newInstance(traversal); - } - } - - @SuppressWarnings({"rawtypes", "unchecked"}) - private static void addMergeChild(AbstractStep merge, - Traversal.Admin child) - throws Exception { - try { - Class mergeToken = - (Class) Class.forName( - "org.apache.tinkerpop.gremlin.process.traversal.Merge"); - Enum onMatch = Enum.valueOf(mergeToken, "onMatch"); - merge.getClass().getMethod("addChildOption", mergeToken, - Traversal.Admin.class) - .invoke(merge, onMatch, child); - } catch (ClassNotFoundException ignored) { - merge.getClass().getMethod("addChild", Traversal.Admin.class) - .invoke(merge, child); - } - } - - private static class TestTraversalParent - extends AbstractStep - implements TraversalParent { - - private final List> children; - - TestTraversalParent(Traversal.Admin traversal) { - super(traversal); - this.children = new ArrayList<>(); - } - - void addChild(Traversal.Admin child) { - this.children.add(child); - } - - @SuppressWarnings({"rawtypes", "unchecked"}) - @Override - public List> getLocalChildren() { - return (List) this.children; - } - - @Override - protected Traverser.Admin processNextStart() - throws NoSuchElementException { - throw new NoSuchElementException(); - } - } - - private static class MergeVertexStep extends TestTraversalParent { - - MergeVertexStep(Traversal.Admin traversal) { - super(traversal); - } - } - private static class TestAppender extends AbstractAppender { private final List events; diff --git a/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java b/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java deleted file mode 100644 index 402a8a2bc1..0000000000 --- a/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java +++ /dev/null @@ -1,35 +0,0 @@ -/* - * 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.tinkerpop.gremlin.process.traversal.step.map; - -import org.apache.tinkerpop.gremlin.process.traversal.Traversal; - -/* - * Test-output-only compatibility fixture for TinkerPop 3.5.1 MergeEdgeStep. - */ -public class MergeEdgeStep extends TestMergeStep { - - public MergeEdgeStep(Traversal.Admin traversal, boolean isStart) { - super(traversal); - } - - @Override - public void addChild(Traversal.Admin child) { - super.addChild(child); - } -} diff --git a/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java b/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java deleted file mode 100644 index 81aaa3b7aa..0000000000 --- a/hugegraph-server/hugegraph-test/src/test-tinkerpop-3.5.1/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java +++ /dev/null @@ -1,70 +0,0 @@ -/* - * 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.tinkerpop.gremlin.process.traversal.step.map; - -import java.util.ArrayList; -import java.util.List; -import java.util.NoSuchElementException; - -import org.apache.tinkerpop.gremlin.process.traversal.Traversal; -import org.apache.tinkerpop.gremlin.process.traversal.Traverser; -import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; -import org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep; - -/* - * Test-output-only compatibility fixture for TinkerPop 3.5.1, where the - * TinkerPop 3.7 merge classes do not exist. - */ -public class MergeVertexStep extends TestMergeStep { - - public MergeVertexStep(Traversal.Admin traversal, boolean isStart) { - super(traversal); - } - - @Override - public void addChild(Traversal.Admin child) { - super.addChild(child); - } -} - -abstract class TestMergeStep extends AbstractStep - implements TraversalParent { - - private final List> children; - - TestMergeStep(Traversal.Admin traversal) { - super(traversal); - this.children = new ArrayList<>(); - } - - public void addChild(Traversal.Admin child) { - this.children.add(child); - } - - @SuppressWarnings({"rawtypes", "unchecked"}) - @Override - public List> getLocalChildren() { - return (List) this.children; - } - - @Override - protected Traverser.Admin processNextStart() - throws NoSuchElementException { - throw new NoSuchElementException(); - } -} From 7083242676277b2536614e4804c4f93313707bed Mon Sep 17 00:00:00 2001 From: dark Date: Tue, 18 Aug 2026 22:25:19 +0800 Subject: [PATCH 23/23] chore(ci): upgrade dependency review - move dependency review action from v3 to the Node 24 v5 release - use the supported oversized-summary handling - keep existing severity and license policy unchanged --- .github/workflows/check-dependencies.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/check-dependencies.yml b/.github/workflows/check-dependencies.yml index fa804e260c..447162d67f 100644 --- a/.github/workflows/check-dependencies.yml +++ b/.github/workflows/check-dependencies.yml @@ -47,7 +47,7 @@ jobs: - name: 'Checkout Repository' uses: actions/checkout@v4 - name: 'Dependency Review' - uses: actions/dependency-review-action@v3 + uses: actions/dependency-review-action@v5 # Refer: https://github.com/actions/dependency-review-action with: # TODO: reset critical to low before releasing