github-actions[bot] commented on code in PR #67659:
URL: https://github.com/apache/doris/pull/67659#discussion_r3958238028
##########
fe/fe-core/src/test/java/org/apache/doris/catalog/AIResourceTest.java:
##########
@@ -97,9 +102,20 @@ public void testFromCommand() throws UserException {
.thenReturn(true);
// resource with default settings
- CreateResourceCommand createResourceCommand = new
CreateResourceCommand(
+ CreateResourceCommand initialCreateResourceCommand = new
CreateResourceCommand(
new CreateResourceInfo(true, false, name,
ImmutableMap.copyOf(aiProperties)));
- createResourceCommand.getInfo().validate();
+ initialCreateResourceCommand.getInfo().validate();
+
+ AIResource legacyAIResource = (AIResource)
Resource.fromCommand(initialCreateResourceCommand);
+ Assert.assertFalse(legacyAIResource.isCreatedByRoot());
+
+ AIResource rootCreatedAIResource =
+ (AIResource)
Resource.fromCommand(initialCreateResourceCommand, UserIdentity.ROOT);
+ Assert.assertTrue(rootCreatedAIResource.isCreatedByRoot());
+
+ AIResource aiResource =
Review Comment:
[P1] Repair the new test block so it compiles
This block introduces three Java compilation errors: `Assert` is not
imported (the file already uses JUnit 5 `Assertions`), `aiResource` is declared
here and again four lines below in the same scope, and that second declaration
calls `Resource.fromCommand(createResourceCommand)` before
`createResourceCommand` is declared at line 143. As a result FE test
compilation stops before any of the new coverage can run. Please rename the
admin-created value, use `Assertions`, and build the default-case `aiResource`
from `initialCreateResourceCommand` (or equivalently reorder the declarations).
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterResourceCommand.java:
##########
@@ -64,6 +65,10 @@ private void validate(ConnectContext ctx) throws
AnalysisException {
if (resource == null) {
throw new AnalysisException("Unknown resource: " + resourceName);
}
+ if (resource instanceof AIResource && ((AIResource)
resource).isCreatedByRoot()
Review Comment:
[P2] Authorize the resource instance that is actually mutated
This check is separated from the mutation by a second name lookup in
`ResourceMgr`. Two FE sessions can therefore interleave as follows: ADMIN
validates an existing non-root resource A here; root drops A and creates a
root-owned resource B with the same name; then ADMIN's `alterResource` lookup
returns B and modifies/logs it without rechecking provenance.
`DropResourceCommand` has the analogous issue and can remove B by name. A
`ConcurrentMap` only makes each operation safe, not this check/use sequence.
Please enforce the caller check at the manager mutation boundary and serialize
or conditionally mutate/remove the same map entry that was authorized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
