[NAE-2389] Actor ref resolution does not includes realmId#421
Open
renczesstefan wants to merge 2 commits intorelease/7.0.0-rev10from
Open
[NAE-2389] Actor ref resolution does not includes realmId#421renczesstefan wants to merge 2 commits intorelease/7.0.0-rev10from
renczesstefan wants to merge 2 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.
WalkthroughExtracts inline actor existence checks into a new private helper method 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.
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.
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
3 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