[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9263: IGNITE-15101 Fixes security context propagation for compute tasks.

[GitBox <gi...@apache.org>]

alex-plekhanov commented on a change in pull request #9263:
URL: https://github.com/apache/ignite/pull/9263#discussion_r678323529



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/IgniteComputeImpl.java
##########
@@ -481,9 +481,10 @@ private IgniteComputeImpl(GridKernalContext ctx, ClusterGroupAdapter prj, UUID s
 
         try {
             ctx.task().setThreadContextIfNotNull(TC_SUBGRID_PREDICATE, prj.predicate());
-            ctx.task().setThreadContextIfNotNull(TC_SUBJ_ID, subjId);
 
-            return ctx.task().execute(taskName, arg, execName);
+            try (OperationSecurityContext ignored = ctx.security().withContext(subjId)) {

Review comment:
       Why do we need context wrapping here? Looks like context of "execute" method caller should be used instead of context of "compute" creator, and in this case we don't need `subjId` field at all. Do I miss something?

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/security/IgniteSecurityProcessor.java
##########
@@ -367,13 +378,38 @@ else if (packAccess.contains(',' + IGNITE_INTERNAL_PACKAGE))
         secPrc.dropUser(login);
     }
 
+    /** {@inheritDoc} */
+    @Override public void onLocalJoin() {
+        try {
+            SecurityContext secCtx = nodeSecurityContext(
+                marsh,
+                U.resolveClassLoader(ctx.config()),
+                ctx.discovery().localNode());
+
+            nodeSecCtxReadyFut.onDone(secCtx);
+        } catch (Throwable e) {

Review comment:
       NL after `}`

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/client/compute/ClientComputeTask.java
##########
@@ -100,15 +100,14 @@ void execute(long taskId, String taskName, Object arg, Set<UUID> nodeIds, byte f
         IgnitePredicate<ClusterNode> nodePredicate = F.isEmpty(nodeIds) ? node -> !node.isClient() :
             F.nodeForNodeIds(nodeIds);
 
-        UUID subjId = ctx.securityContext() == null ? null : ctx.securityContext().subject().id();
-
         task.setThreadContext(TC_SUBGRID_PREDICATE, nodePredicate);
-        task.setThreadContextIfNotNull(TC_SUBJ_ID, subjId);
         task.setThreadContext(TC_TIMEOUT, timeout);
         task.setThreadContext(TC_NO_FAILOVER, (flags & NO_FAILOVER_FLAG_MASK) != 0);
         task.setThreadContext(TC_NO_RESULT_CACHE, (flags & NO_RESULT_CACHE_FLAG_MASK) != 0);
 
-        taskFut = task.execute(taskName, arg);
+        try (OperationSecurityContext ignored = ctx.kernalContext().security().withContext(ctx.securityContext())) {

Review comment:
       Why do we need additional wrapping here? As far as I understand, context is already wrapped in `ClientListenerNioListener.onMessage()` method.

##########
File path: modules/clients/src/test/java/org/apache/ignite/internal/TaskEventSubjectIdSelfTest.java
##########
@@ -139,7 +139,7 @@ public void testSimpleTask() throws Exception {
         assert evt != null;
 
         assertEquals(EVT_TASK_STARTED, evt.type());
-        assertEquals(nodeId, evt.subjectId());

Review comment:
       I don't get what this test is currently testing? Description says that it should test subject Id, but currently it checked that all subject ids is null. Perhaps we should enable security for this test and revert `evt.subjectId()` checking. 




-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org