Skip to content

Commit 0c1ce36

Browse files
authored
fix(core): requirement checks for a permitted node following a not-permitted node (#691)
* Fix issue with permitted node after not-permitted node * Add test
1 parent a611eb5 commit 0c1ce36

5 files changed

Lines changed: 166 additions & 31 deletions

File tree

cloud-core/src/main/java/org/incendo/cloud/CommandManager.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,7 @@
9898
@API(status = API.Status.STABLE)
9999
public abstract class CommandManager<C> implements Stateful<RegistrationState>, CommandBuilderSource<C> {
100100

101-
private final Configurable<ManagerSetting> settings = Configurable.enumConfigurable(ManagerSetting.class)
102-
.set(ManagerSetting.ENFORCE_INTERMEDIARY_PERMISSIONS, true);
101+
private final Configurable<ManagerSetting> settings = Configurable.enumConfigurable(ManagerSetting.class);
103102
private final ServicePipeline servicePipeline = ServicePipeline.builder().build();
104103
private final ParserRegistry<C> parserRegistry = new StandardParserRegistry<>();
105104
private final Collection<Command<C>> commands = new LinkedList<>();

cloud-core/src/main/java/org/incendo/cloud/CommandTree.java

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1045,7 +1045,7 @@ public void verifyAndRegister() {
10451045
}
10461046
});
10471047

1048-
this.getLeavesRaw(this.internalTree).forEach(this::propagateRequirements);
1048+
this.getExecutorNodes(this.internalTree).forEach(this::propagateRequirements);
10491049
}
10501050

10511051
/**
@@ -1071,34 +1071,19 @@ private void propagateRequirements(final @NonNull CommandNode<C> leafNode) {
10711071
if (senderType == null) {
10721072
senderType = Object.class;
10731073
}
1074-
/* All leaves must necessarily have an owning command */
1075-
leafNode.nodeMeta().put(CommandNode.META_KEY_PERMISSION, commandPermission);
1076-
leafNode.nodeMeta().put(CommandNode.META_KEY_SENDER_TYPES, new HashSet<>(Collections.singletonList(senderType)));
1077-
// Get chain and order it tail->head then skip the tail (leaf node)
1074+
// Get chain and order it tail->head
10781075
List<CommandNode<C>> chain = this.getChain(leafNode);
10791076
Collections.reverse(chain);
1080-
chain = chain.subList(1, chain.size());
1081-
// Go through all nodes from the tail upwards until a collision occurs
10821077
for (final CommandNode<C> commandArgumentNode : chain) {
10831078
final Permission existingPermission = (Permission) commandArgumentNode.nodeMeta().get(CommandNode.META_KEY_PERMISSION);
10841079

1085-
Permission permission;
1080+
final Permission permission;
10861081
if (existingPermission != null) {
10871082
permission = Permission.anyOf(commandPermission, existingPermission);
10881083
} else {
10891084
permission = commandPermission;
10901085
}
10911086

1092-
/* Now also check if there's a command handler attached to an upper level node */
1093-
if (commandArgumentNode.component() != null && commandArgumentNode.command() != null) {
1094-
final Command<C> command = commandArgumentNode.command();
1095-
if (this.commandManager().settings().get(ManagerSetting.ENFORCE_INTERMEDIARY_PERMISSIONS)) {
1096-
permission = command.commandPermission();
1097-
} else {
1098-
permission = Permission.anyOf(permission, command.commandPermission());
1099-
}
1100-
}
1101-
11021087
commandArgumentNode.nodeMeta().put(CommandNode.META_KEY_PERMISSION, permission);
11031088

11041089
final Set<Type> senderTypes = (Set<Type>) commandArgumentNode.nodeMeta()
@@ -1190,6 +1175,19 @@ private void checkAmbiguity(final @NonNull CommandNode<C> node) throws Ambiguous
11901175
return leaves;
11911176
}
11921177

1178+
private @NonNull List<@NonNull CommandNode<C>> getExecutorNodes(
1179+
final @NonNull CommandNode<C> node
1180+
) {
1181+
final List<CommandNode<C>> leaves = new LinkedList<>();
1182+
if (node.command() != null) {
1183+
leaves.add(node);
1184+
}
1185+
for (final CommandNode<C> child : node.children()) {
1186+
leaves.addAll(this.getExecutorNodes(child));
1187+
}
1188+
return leaves;
1189+
}
1190+
11931191
/**
11941192
* Returns all leaf nodes attached to the given {@code node} or its children.
11951193
*

cloud-core/src/main/java/org/incendo/cloud/setting/ManagerSetting.java

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,6 @@
3333
*/
3434
@API(status = API.Status.STABLE)
3535
public enum ManagerSetting implements Setting {
36-
/**
37-
* Do not create a compound permission and do not look greedily
38-
* for child permission values, if a preceding command in the tree path
39-
* has a command handler attached
40-
*/
41-
ENFORCE_INTERMEDIARY_PERMISSIONS,
42-
4336
/**
4437
* Force sending of an empty suggestion (i.e. a singleton list containing an empty string)
4538
* when no suggestions are present
@@ -52,14 +45,12 @@ public enum ManagerSetting implements Setting {
5245
* For example, if a platform serializes the command tree and sends it to clients,
5346
* this will allow modifying the command tree after it has been sent, as long as these modifications are not blocked by
5447
* the underlying platform
55-
*
5648
*/
5749
@API(status = API.Status.STABLE)
5850
ALLOW_UNSAFE_REGISTRATION,
5951

6052
/**
6153
* Enables overriding of existing commands on supported platforms.
62-
*
6354
*/
6455
@API(status = API.Status.STABLE)
6556
OVERRIDE_EXISTING_COMMANDS,
@@ -68,7 +59,6 @@ public enum ManagerSetting implements Setting {
6859
* Allows parsing flags at any position after the last literal by appending flag argument nodes between each command node.
6960
* It can have some conflicts when integrating with other command systems like Brigadier,
7061
* and code inspecting the command tree may need to be adjusted.
71-
*
7262
*/
7363
@API(status = API.Status.EXPERIMENTAL)
7464
LIBERAL_FLAG_PARSING

cloud-core/src/test/java/org/incendo/cloud/PermissionTest.java

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,13 @@
2525

2626
import java.util.Arrays;
2727
import java.util.List;
28+
import java.util.concurrent.CompletableFuture;
2829
import java.util.concurrent.CompletionException;
2930
import java.util.concurrent.atomic.AtomicBoolean;
3031
import java.util.function.Function;
3132
import org.checkerframework.checker.nullness.qual.NonNull;
3233
import org.incendo.cloud.exception.NoPermissionException;
34+
import org.incendo.cloud.execution.CommandResult;
3335
import org.incendo.cloud.execution.ExecutionCoordinator;
3436
import org.incendo.cloud.key.CloudKey;
3537
import org.incendo.cloud.permission.Permission;
@@ -44,6 +46,7 @@
4446

4547
import static com.google.common.truth.Truth.assertThat;
4648
import static org.incendo.cloud.parser.standard.IntegerParser.integerParser;
49+
import static org.incendo.cloud.truth.CompletableFutureSubject.assertThat;
4750
import static org.junit.jupiter.api.Assertions.assertThrows;
4851
import static org.mockito.ArgumentMatchers.anyString;
4952
import static org.mockito.Mockito.when;
@@ -114,6 +117,29 @@ void testSubCommandPermission() {
114117
assertThat(exception).hasCauseThat().isInstanceOf(NoPermissionException.class);
115118
}
116119

120+
@Test
121+
void testPermittedNodeFollowingNotPermittedNode() {
122+
// Arrange
123+
this.manager.command(this.manager.commandBuilder("root")
124+
.literal("no")
125+
.permission("0"));
126+
this.manager.command(this.manager.commandBuilder("root")
127+
.literal("no")
128+
.literal("yes")
129+
.permission("1"));
130+
final TestCommandSender sender = new TestCommandSender("1");
131+
132+
// Act
133+
final CompletableFuture<CommandResult<TestCommandSender>> noPermission = this.manager.commandExecutor()
134+
.executeCommand(sender, "root no");
135+
final CompletableFuture<CommandResult<TestCommandSender>> permitted = this.manager.commandExecutor()
136+
.executeCommand(sender, "root no yes");
137+
138+
// Assert
139+
assertThat(noPermission).hasFailureThat().isInstanceOf(NoPermissionException.class);
140+
assertThat(permitted).hasResult();
141+
}
142+
117143
@Test
118144
void testAndPermissionsMissingOne() {
119145
// Arrange
@@ -260,7 +286,7 @@ public boolean hasPermission(
260286
final @NonNull TestCommandSender sender,
261287
final @NonNull String permission
262288
) {
263-
return this.permissionFunction.apply(permission);
289+
return this.permissionFunction.apply(permission) || sender.hasPermisison(permission);
264290
}
265291
}
266292
}
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
//
2+
// MIT License
3+
//
4+
// Copyright (c) 2024 Incendo
5+
//
6+
// Permission is hereby granted, free of charge, to any person obtaining a copy
7+
// of this software and associated documentation files (the "Software"), to deal
8+
// in the Software without restriction, including without limitation the rights
9+
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
10+
// copies of the Software, and to permit persons to whom the Software is
11+
// furnished to do so, subject to the following conditions:
12+
//
13+
// The above copyright notice and this permission notice shall be included in all
14+
// copies or substantial portions of the Software.
15+
//
16+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
17+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
18+
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
19+
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
20+
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
21+
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
22+
// SOFTWARE.
23+
//
24+
package org.incendo.cloud.truth;
25+
26+
import com.google.common.truth.Fact;
27+
import com.google.common.truth.FailureMetadata;
28+
import com.google.common.truth.OptionalSubject;
29+
import com.google.common.truth.Subject;
30+
import com.google.common.truth.ThrowableSubject;
31+
import java.util.Optional;
32+
import java.util.concurrent.CompletableFuture;
33+
import java.util.concurrent.CompletionException;
34+
import org.checkerframework.checker.nullness.qual.NonNull;
35+
import org.checkerframework.checker.nullness.qual.Nullable;
36+
37+
import static com.google.common.truth.Truth.assertAbout;
38+
39+
/**
40+
* Subject for assertions about completable futures. Futures are join()ed to get
41+
* results or exceptions, and {@link CompletionException}'s cause is unwrapped to
42+
* {@link #failure()}. Any other exceptions thrown by join() are unexpected failures.
43+
* This class also does not properly handle futures where null is a successful result.
44+
*
45+
* @param <T> future result type
46+
*/
47+
public final class CompletableFutureSubject<T> extends Subject {
48+
49+
public static <T> @NonNull Factory<CompletableFutureSubject<T>, CompletableFuture<? extends @NonNull T>> completableFuture() {
50+
return CompletableFutureSubject::new;
51+
}
52+
53+
public static <T> @NonNull CompletableFutureSubject<T> assertThat(
54+
final @Nullable CompletableFuture<? extends @NonNull T> actual
55+
) {
56+
return assertAbout(CompletableFutureSubject.<T>completableFuture()).that(actual);
57+
}
58+
59+
private final CompletableFuture<? extends @NonNull T> actual;
60+
61+
private CompletableFutureSubject(
62+
final @NonNull FailureMetadata metadata,
63+
final @Nullable CompletableFuture<? extends @NonNull T> actual
64+
) {
65+
super(metadata, actual);
66+
this.actual = actual;
67+
}
68+
69+
public @NonNull OptionalSubject result() {
70+
if (this.actual == null) {
71+
this.failWithActual(Fact.simpleFact("expected future to not be null"));
72+
}
73+
try {
74+
final T join = this.actual.join();
75+
if (join == null) {
76+
this.failWithActual(Fact.simpleFact("expected result to not be null"));
77+
}
78+
return this.check("join() result or null on failure").about(OptionalSubject.optionals()).that(Optional.of(join));
79+
} catch (final CompletionException e) {
80+
return this.check("join() result or null on failure").about(OptionalSubject.optionals()).that(Optional.empty());
81+
}
82+
}
83+
84+
public @NonNull OptionalSubject failure() {
85+
if (this.actual == null) {
86+
this.failWithActual(Fact.simpleFact("expected future to not be null"));
87+
}
88+
try {
89+
this.actual.join();
90+
return this.check("cause of CompletionException thrown by join()").about(OptionalSubject.optionals()).that(Optional.empty());
91+
} catch (final CompletionException e) {
92+
return this.check("cause of CompletionException thrown by join()").about(OptionalSubject.optionals()).that(Optional.of(e.getCause()));
93+
}
94+
}
95+
96+
public void hasResult() {
97+
this.result().isPresent();
98+
this.failure().isEmpty();
99+
}
100+
101+
public void hasResult(final @NonNull T value) {
102+
this.result().hasValue(value);
103+
this.failure().isEmpty();
104+
}
105+
106+
public void hasFailure(final @NonNull Throwable throwable) {
107+
this.failure().hasValue(throwable);
108+
this.result().isEmpty();
109+
}
110+
111+
public @NonNull ThrowableSubject hasFailureThat() {
112+
this.result().isEmpty();
113+
this.failure().isPresent();
114+
try {
115+
this.actual.join();
116+
this.failWithActual(Fact.simpleFact("expected future to throw on join()"));
117+
throw new IllegalStateException();
118+
} catch (final CompletionException e) {
119+
return this.check("cause of CompletionException thrown by join()").that(e.getCause());
120+
}
121+
}
122+
}

0 commit comments

Comments
 (0)