[NAE-2389] Actor ref resolution does not includes realmId#421
Merged
machacjozef merged 5 commits intorelease/7.0.0-rev10from Apr 9, 2026
Merged
[NAE-2389] Actor ref resolution does not includes realmId#421machacjozef merged 5 commits intorelease/7.0.0-rev10from
machacjozef merged 5 commits intorelease/7.0.0-rev10from
Conversation
Moved the logic for validating actor existence into a dedicated `actorExists` method to improve code readability and reduce duplication. This change was applied to both `WorkflowService` and `TaskService` for consistency.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughExtracted inline actor existence checks into a private helper Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Retoocs
previously approved these changes
Mar 6, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java`:
- Around line 285-296: The actorExists method should defensively check that
actorFieldValue.getId() is not null or empty before calling userService.findById
or groupService.findById; update the actorExists(ActorFieldValue) implementation
to return false immediately when actorFieldValue.getId() is null/blank, then
proceed to call userService.findById(actorFieldValue.getId(),
actorFieldValue.getRealmId()) and fallback to
groupService.findById(actorFieldValue.getId()) as before (catching
IllegalArgumentException).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: adeab910-3f18-4072-ab2a-2f36a4c2a426
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
...on-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
Show resolved
Hide resolved
tuplle
previously approved these changes
Mar 6, 2026
Added null checks for actorFieldValue and its ID in actorExists methods of WorkflowService and TaskService. This prevents potential NullPointerExceptions and ensures robustness in cases where invalid or null input is provided.
renczesstefan
commented
Mar 6, 2026
...on-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java (1)
285-299:⚠️ Potential issue | 🟠 MajorGuard malformed actor IDs before the user lookup.
Line 289 still assumes every non-null id is a valid user
ObjectId. Innae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java:339-344,findById(id, realmId)constructsnew ObjectId(id), so blank or malformed ids will throw before the group fallback runs. This should stay a softfalseduring actor filtering, not break case resolution.🤖 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/WorkflowService.java` around lines 285 - 299, actorExists currently calls userService.findById with any non-null id which can throw on malformed ObjectId; fix by validating the id before the user lookup: in actorExists(ActorFieldValue) first check actorFieldValue.getId() is not blank and ObjectId.isValid(id) (or equivalent try-new-ObjectId wrapper) and only then call userService.findById; if the id is not a valid ObjectId, skip the userService call and fall back to the existing groupService.findById try/catch so malformed or blank ids return false without throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java`:
- Around line 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.
---
Duplicate comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java`:
- Around line 285-299: actorExists currently calls userService.findById with any
non-null id which can throw on malformed ObjectId; fix by validating the id
before the user lookup: in actorExists(ActorFieldValue) first check
actorFieldValue.getId() is not blank and ObjectId.isValid(id) (or equivalent
try-new-ObjectId wrapper) and only then call userService.findById; if the id is
not a valid ObjectId, skip the userService call and fall back to the existing
groupService.findById try/catch so malformed or blank ids return false without
throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6209912c-9833-4a44-b843-37b9896e3aa3
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
Comment on lines
+867
to
+881
| private boolean actorExists(ActorFieldValue actorFieldValue) { | ||
| if (actorFieldValue == null || actorFieldValue.getId() == null) { | ||
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🧩 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.
Member
Author
There was a problem hiding this comment.
Please check it once more.
There was a problem hiding this comment.
🧩 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!
machacjozef
previously approved these changes
Apr 8, 2026
Enhanced the `actorExists` method to check if the actor ID is blank or invalid using `ObjectId.isValid`. This ensures more robust validation and prevents potential errors when processing malformed IDs.
renczesstefan
dismissed stale reviews from machacjozef and coderabbitai[bot]
via
f8f3649
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java`:
- Around line 285-299: TaskService.actorExists currently only checks for null
IDs causing inconsistent behavior vs WorkflowService.actorExists; update
TaskService.actorExists to mirror WorkflowService by returning false if
actorFieldValue is null or actorFieldValue.getId() is null or isBlank() or
ObjectId.isValid(...) is false, then proceed to call
userService.findById(actorFieldValue.getId(), actorFieldValue.getRealmId()) and
return true if non-null, otherwise attempt
groupService.findById(actorFieldValue.getId()) and return true unless it throws
IllegalArgumentException (return false in that case). Ensure you reference the
same validation logic (isBlank and ObjectId.isValid) used in WorkflowService to
keep behavior consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9bf8deb-e250-415e-a54a-41701ab10ed2
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
...on-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
Show resolved
Hide resolved
Updated the actorExists method to validate that the actor ID is not blank and adheres to ObjectId format. This prevents invalid or malformed IDs from causing issues during runtime.
Kovy95
approved these changes
Apr 9, 2026
machacjozef
approved these changes
Apr 9, 2026
Retoocs
approved these changes
Apr 9, 2026
Member
Author
|
@coderabbitai please review the PR and approve if its ok |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5046 🏁 Script executed: Length of output: 3626
✅ Review SummaryCore fix is correct. The bug was that
Validation improvements are solid:
Stream order is correct: Group fallback is unchanged (
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Added realmId inclusion when searching for user at resolving actor permission on cases and tasks.
Fixes NAE-2389
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit