Skip to content

[NAE-2389] Actor ref resolution does not includes realmId #421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
machacjozef merged 5 commits into from
Apr 9, 2026
Merged

[NAE-2389] Actor ref resolution does not includes realmId #421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
267ff2e
[NAE-2389] Actor ref resolution does not includes realmId
renczesstefan Mar 6, 2026
c68380b
Handle null checks for actorFieldValue in actorExists method
renczesstefan Mar 6, 2026
f8f3649
Validate actor ID format and handle blank IDs.
renczesstefan Apr 8, 2026
f8f510e
Ensure valid actor ID in actorExists method
renczesstefan Apr 8, 2026
d923e4f
Merge remote-tracking branch 'origin/release/7.0.0-rev10' into NAE-2389
machacjozef Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions ...ion-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,22 +859,27 @@ private List<String> getExistingActors(ActorListFieldValue actorListFieldValue)
return null;
}
return actorListFieldValue.getActorValues().stream()
.filter(this::actorExists)
.map(ActorFieldValue::getId)
.filter(actorId -> {
AbstractUser user = userService.findById(actorId, null);
if (user != null) {
return true;
}
try {
groupService.findById(actorId);
return true;
} catch (IllegalArgumentException ignored) {
return false;
}
})
.collect(Collectors.toList());
}

private boolean actorExists(ActorFieldValue actorFieldValue) {
if (actorFieldValue == null || actorFieldValue.getId() == null || actorFieldValue.getId().isBlank() || !ObjectId.isValid(actorFieldValue.getId())) {
return false;
}
AbstractUser user = userService.findById(actorFieldValue.getId(), actorFieldValue.getRealmId());
if (user != null) {
return true;
}
try {
groupService.findById(actorFieldValue.getId());
return true;
} catch (IllegalArgumentException ignored) {
return false;
}
}
Comment on lines +867 to +881
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Catch invalid user IDs here instead of aborting task resolution.

Line 871 can still throw on blank or malformed ids because userService.findById(...) builds an ObjectId internally. That means one bad actor entry can fail the whole resolution path instead of being filtered out. Treat blank ids as missing and swallow IllegalArgumentException from the user lookup before falling back to groups.

Proposed fix 
 private boolean actorExists(ActorFieldValue actorFieldValue) {
-    if (actorFieldValue == null || actorFieldValue.getId() == null) {
+    if (actorFieldValue == null || actorFieldValue.getId() == null || actorFieldValue.getId().isBlank()) {
         return false;
     }
-    AbstractUser user = userService.findById(actorFieldValue.getId(), actorFieldValue.getRealmId());
-    if (user != null) {
-        return true;
+    try {
+        AbstractUser user = userService.findById(actorFieldValue.getId(), actorFieldValue.getRealmId());
+        if (user != null) {
+            return true;
+        }
+    } catch (IllegalArgumentException ignored) {
+        // Not a valid user id, continue with group lookup.
     }
     try {
         groupService.findById(actorFieldValue.getId());
         return true;
     } catch (IllegalArgumentException ignored) {
         return false;
     }
 }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java`
around lines 867 - 881, actorExists currently calls userService.findById(...)
which can throw IllegalArgumentException for blank/malformed ids and abort
resolution; update actorExists to treat null/blank actorFieldValue.getId() as
missing, wrap the userService.findById(...) call in a try/catch that catches
IllegalArgumentException and treats it like a non-existent user, then only if
the user lookup fails proceed to try groupService.findById(...) (also swallowing
IllegalArgumentException similarly) so a bad actor id is filtered out rather
than causing the whole resolution to fail; refer to the actorExists method and
the userService.findById and groupService.findById calls when making the change.

Copy link
Copy Markdown
Member Author

@renczesstefan renczesstefan Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added check to the first IF statement to check whether the ID is valid ObjectID, because If it its not valid, both the userService and groupService will throw IllegalArgumentException, so in both cases we should return false.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

Let me check the current state of the actorExists method on the PR branch.

🏁 Script executed:

#!/bin/bash
# Check the current state of the actorExists method in TaskService.java
rg -n "actorExists|isValid|ObjectId" application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java -A 3 -B 1

Length of output: 893

Let me see the full current method.

🏁 Script executed:

#!/bin/bash
# Get the full actorExists method
sed -n '867,890p' application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java

Length of output: 1015

@renczesstefan, that's a clean approach — using ObjectId.isValid(id) as an upfront guard is more concise than wrapping both service calls in separate try-catch blocks. Since ObjectId is already imported at Line 45, it fits naturally here.

The guard would look something like:

if (actorFieldValue == null || actorFieldValue.getId() == null || !ObjectId.isValid(actorFieldValue.getId())) {
    return false;
}

One small note: ObjectId.isValid(null) returns false in the Mongo driver, so the explicit actorFieldValue.getId() == null check is technically redundant when placed before it — but keeping it is fine for readability.

The fix hasn't appeared on the branch yet though — could you push the commit? Once it's up I'll mark this resolved.

🧠 Learnings used 
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java:98-104
Timestamp: 2025-12-12T13:40:00.786Z
Learning: In application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java, the permission constraint logic in buildQuery() intentionally applies negative actor constraints globally: ((roleConstraint AND NOT negRoleConstraint) OR actorConstraint) AND NOT negActorConstraint. This means users in negativeViewActors are excluded even if they have positive role permissions, which is the intended authorization behavior.

Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java:239-247
Timestamp: 2025-12-19T07:55:13.427Z
Learning: In the netgrif/application-engine project, the `taskService.findOne()` method (from ITaskService) never returns null. When a task is not found, it handles the error through other means (such as throwing an exception) rather than returning null, so null checks after `findOne()` calls are unnecessary.

Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:18-20
Timestamp: 2025-12-12T12:40:35.354Z
Learning: In the actor refactoring, UserFieldValue is a subclass of ActorFieldValue, and ActorListFieldValue accepts ActorFieldValue instances. Therefore, passing UserFieldValue instances to ActorListFieldValue is valid through polymorphism.

Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/resources/petriNets/engine-processes/org_group.xml:36-39
Timestamp: 2025-12-12T12:36:06.651Z
Learning: In the org_group.xml file (application-engine/src/main/resources/petriNets/engine-processes/org_group.xml), variable names in action blocks like userField and user_selection should be kept as-is even when the field type changes from "user" to "actor".

Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nField.java:15-16
Timestamp: 2026-01-30T10:32:47.754Z
Learning: In the NAE-2285 refactoring (PR `#397`), breaking API changes to field visibility (public to protected) and type changes (String[] to Set<String>) in I18nField.java (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nField.java) are acceptable and intentional as part of the user-to-actor migration.

Copy link
Copy Markdown
Member Author

@renczesstefan renczesstefan Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check it once more.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the current state of actorExists method
rg -n "actorExists|isValid|isBlank" application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java -A 2 -B 1

Length of output: 738

@renczesstefan, the fix is in place. Line 868 now guards with all four conditions:

if (actorFieldValue == null || actorFieldValue.getId() == null || actorFieldValue.getId().isBlank() || !ObjectId.isValid(actorFieldValue.getId())) {
    return false;
}

This correctly short-circuits before either userService.findById(...) or groupService.findById(...) are called for null, blank, or malformed IDs — no IllegalArgumentException can propagate from those lookups due to a bad ID.

🧠 Learnings used 
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java:98-104
Timestamp: 2025-12-12T13:40:00.786Z
Learning: In application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java, the permission constraint logic in buildQuery() intentionally applies negative actor constraints globally: ((roleConstraint AND NOT negRoleConstraint) OR actorConstraint) AND NOT negActorConstraint. This means users in negativeViewActors are excluded even if they have positive role permissions, which is the intended authorization behavior.

Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java:239-247
Timestamp: 2025-12-19T07:55:13.427Z
Learning: In the netgrif/application-engine project, the `taskService.findOne()` method (from ITaskService) never returns null. When a task is not found, it handles the error through other means (such as throwing an exception) rather than returning null, so null checks after `findOne()` calls are unnecessary.

Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:18-20
Timestamp: 2025-12-12T12:40:35.354Z
Learning: In the actor refactoring, UserFieldValue is a subclass of ActorFieldValue, and ActorListFieldValue accepts ActorFieldValue instances. Therefore, passing UserFieldValue instances to ActorListFieldValue is valid through polymorphism.

Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/resources/petriNets/engine-processes/org_group.xml:36-39
Timestamp: 2025-12-12T12:36:06.651Z
Learning: In the org_group.xml file (application-engine/src/main/resources/petriNets/engine-processes/org_group.xml), variable names in action blocks like userField and user_selection should be kept as-is even when the field type changes from "user" to "actor".

Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nField.java:15-16
Timestamp: 2026-01-30T10:32:47.754Z
Learning: In the NAE-2285 refactoring (PR `#397`), breaking API changes to field visibility (public to protected) and type changes (String[] to Set<String>) in I18nField.java (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nField.java) are acceptable and intentional as part of the user-to-actor migration.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!


private Task createFromTransition(Transition transition, Case useCase) {
final Task task = com.netgrif.application.engine.adapter.spring.workflow.domain.Task.with()
.title(transition.getTitle())
Expand Down
34 changes: 19 additions & 15 deletions ...engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,9 @@ public Case resolveActorRef(Case useCase, boolean canSaveUseCase) {
/**
* Resolves actor permissions for the useCase based on the actor list data field.
*
* @param useCase useCase where to resolve actor permissions
* @param useCase useCase where to resolve actor permissions
* @param actorFieldId field id of the actor list
* @param permission permission associated with the useCase and actor list
*
* @param permission permission associated with the useCase and actor list
* @return true if the useCase was modified, false otherwise
*/
private boolean resolveActorRefPermissions(Case useCase, String actorFieldId, Map<String, Boolean> permission) {
Expand All @@ -278,22 +277,27 @@ private List<String> getExistingActors(ActorListFieldValue actorListFieldValue)
return null;
}
return actorListFieldValue.getActorValues().stream()
.filter(this::actorExists)
.map(ActorFieldValue::getId)
.filter(actorId -> {
AbstractUser user = userService.findById(actorId, null);
if (user != null) {
return true;
}
try {
groupService.findById(actorId);
return true;
} catch (IllegalArgumentException ignored) {
return false;
}
})
.collect(Collectors.toList());
}

private boolean actorExists(ActorFieldValue actorFieldValue) {
if (actorFieldValue == null || actorFieldValue.getId() == null || actorFieldValue.getId().isBlank() || !ObjectId.isValid(actorFieldValue.getId())) {
return false;
}
AbstractUser user = userService.findById(actorFieldValue.getId(), actorFieldValue.getRealmId());
if (user != null) {
return true;
}
try {
groupService.findById(actorFieldValue.getId());
return true;
} catch (IllegalArgumentException ignored) {
return false;
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

public CreateCaseEventOutcome createCase(CreateCaseParams createCaseParams) {
fillAndValidateAttributes(createCaseParams);
PetriNet petriNet = createCaseParams.getProcess();
Expand Down
Loading