[NAE 2424] UserRefs negative view permissions aren't resolved#442
Open
SamuelPalaj wants to merge 6 commits into
Open
[NAE 2424] UserRefs negative view permissions aren't resolved#442SamuelPalaj wants to merge 6 commits into
SamuelPalaj wants to merge 6 commits into
Conversation
- negativeViewUsers resolution fix in Case and Task - added test TaskPermissionsTest with test nets view_permission_combinations.xml and view_permission_combinations_no_default.xml
- fix typo in role id
- refactored task view permission query resolution for both elastic search in ElasticViewPermissionService and for mongo in TaskSearchService - added mongo search test to TaskPermissionsTest
# Conflicts: # src/main/java/com/netgrif/application/engine/elastic/service/ElasticViewPermissionService.java # src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java # src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
- after merge fixes
WalkthroughThis PR refactors the view permission system to treat negative view permissions as first-class entities across the domain model, permission resolution, and search backends. Case and Task now populate ChangesView Permission System Refactoring
Sequence Diagram(s)sequenceDiagram
participant Client
participant ElasticTaskService
participant ElasticViewPermissionService
Client->>ElasticTaskService: buildSingleQuery(request, user)
ElasticTaskService->>ElasticTaskService: construct BoolQueryBuilder (no role constraint preprocessing)
ElasticTaskService->>ElasticViewPermissionService: buildViewPermissionQuery
ElasticViewPermissionService->>ElasticViewPermissionService: buildPositiveViewRoleQuery (should clause)
ElasticViewPermissionService->>ElasticViewPermissionService: buildPositiveViewUser (should clause)
ElasticViewPermissionService->>ElasticViewPermissionService: buildNegativeViewUser (mustNot clause)
ElasticViewPermissionService-->>ElasticTaskService: combined permission query
ElasticTaskService-->>Client: final BoolQueryBuilder
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/netgrif/application/engine/elastic/service/ElasticViewPermissionService.java`:
- Around line 18-24: The current buildViewPermissionQuery uses a
BoolQueryBuilder roleOrPositiveUserQuery that requires either roleViewQuery or
buildPositiveViewUser to match, which excludes documents that lack both
viewRoles and viewUsers; change roleOrPositiveUserQuery to also allow documents
where both permission fields are missing by adding an additional should clause
that matches "viewRoles not exists AND viewUsers not exists" (use exists queries
for viewRoles/viewUsers and combine them with mustNot as a should option) so
minimumShouldMatch(1) still applies, and leave the existing
mustNot(buildNegativeViewUser(user)) in place; update
ElasticViewPermissionService.buildViewPermissionQuery (and any helper usage of
roleViewQuery, buildPositiveViewUser, buildNegativeViewUser) to implement this
or alternatively document and implement a migration/backfill that populates
explicit positive defaults for existing documents.
In `@src/main/java/com/netgrif/application/engine/importer/service/Importer.java`:
- Around line 1160-1161: The current guard calls
importTransition.getRoleRef().isEmpty(), getUsersRef().isEmpty(), and
getUserRef().isEmpty() directly and can NPE when any of those refs is null;
change the condition to null-safe checks (e.g., check each ref for null before
calling isEmpty or use a utility like CollectionUtils.isEmpty/Objects.nonNull)
so the early-return only triggers when a non-null collection/string is present
and non-empty; update the condition around getRoleRef(), getUsersRef(), and
getUserRef() in Importer.java to perform null checks first.
In
`@src/test/groovy/com/netgrif/application/engine/workflow/TaskPermissionsTest.groovy`:
- Around line 66-70: The static maps testUsers and correctResults are reused
across tests; modify the init() setup method to reset static state at the
beginning by clearing testUsers and correctResults (call their clear/reset) and
also reset testCase and testCaseNoDefault to null so each test starts with a
clean slate; locate these symbols (testUsers, correctResults, testCase,
testCaseNoDefault, init) and add the clears/null assignments at the top of
init().
- Around line 141-153: The test currently uses header.indexOf(...) (e.g.,
transitionIdIndex and permissionIndex) without checking for -1, which can
silently pick wrong columns; update TaskPermissionsTest.groovy to validate
indices immediately after computing them (for transitionIdIndex and each
permissionIndex derived from header.indexOf(csvColumnName)) and throw a clear
failure (IllegalStateException or assertion) if any index is -1, including the
missing header name in the error message so the test fails fast when required
CSV headers are missing.
🪄 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: 43c90be6-3661-4bd4-9423-0c20d7493b81
⛔ Files ignored due to path filters (2)
src/test/resources/csv/permissions - correct default disabled.csvis excluded by!**/*.csvsrc/test/resources/csv/permissions - correct.csvis excluded by!**/*.csv
📒 Files selected for processing (11)
src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskService.javasrc/main/java/com/netgrif/application/engine/elastic/service/ElasticViewPermissionService.javasrc/main/java/com/netgrif/application/engine/importer/service/Importer.javasrc/main/java/com/netgrif/application/engine/workflow/domain/Case.javasrc/main/java/com/netgrif/application/engine/workflow/domain/Task.javasrc/main/java/com/netgrif/application/engine/workflow/service/TaskSearchService.javasrc/main/java/com/netgrif/application/engine/workflow/service/TaskService.javasrc/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javasrc/test/groovy/com/netgrif/application/engine/workflow/TaskPermissionsTest.groovysrc/test/resources/petriNets/view_permission_combinations.xmlsrc/test/resources/petriNets/view_permission_combinations_no_default.xml
💤 Files with no reviewable changes (1)
- src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskService.java
Comment on lines
+18
to
+24
| BoolQueryBuilder roleOrPositiveUserQuery = boolQuery().should(roleViewQuery) | ||
| .should(buildPositiveViewUser(user)) | ||
| .minimumShouldMatch(1); | ||
|
|
||
| query.filter(permissionQuery); | ||
| // (((Rp!=0 & Rn = 0) or Up!=0) & Un=0) == 1 | ||
| query.filter(roleOrPositiveUserQuery) | ||
| .mustNot(buildNegativeViewUser(user)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/java/com/netgrif/application/engine/elastic/service/ElasticViewPermissionService.java"
# Basic sanity: show file with line numbers (but avoid huge output)
echo "== File: $FILE =="
wc -l "$FILE"
# Show the relevant line ranges mentioned in the review comment
echo
echo "== Lines 1-120 =="
sed -n '1,120p' "$FILE" | nl -ba | sed -n '1,120p'
# Show the specific ranges more precisely
echo
echo "== Lines 1-80 (detailed) =="
sed -n '1,80p' "$FILE" | nl -ba
echo
echo "== Lines 80-200 (possible helpers) =="
sed -n '80,200p' "$FILE" | nl -ba
echo
echo "== Remaining lines (if any) =="
sed -n '200,500p' "$FILE" | nl -ba
# Find definitions of helper methods used in the snippet
echo
echo "== Search for builder methods =="
rg -n "buildPositiveViewUser|buildNegativeViewUser|buildViewPermissionQuery|roleViewQuery|viewRoles|viewUsers|not exists|missing" "$FILE" || true
# Show call sites to see whether any migration/backfill logic exists nearby
echo
echo "== Call sites for buildViewPermissionQuery usage =="
rg -n "buildViewPermissionQuery" src/main/java/com/netgrif/application/engine/elastic/service | head -n 50Repository: netgrif/application-engine
Length of output: 356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/java/com/netgrif/application/engine/elastic/service/ElasticViewPermissionService.java"
echo "== File: $FILE =="
wc -l "$FILE"
echo
echo "== Lines 1-120 =="
awk 'NR>=1 && NR<=120 {printf "%4d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Grep helper/query components =="
rg -n "buildPositiveViewUser|buildNegativeViewUser|buildViewPermissionQuery|roleViewQuery|viewRoles|viewUsers|mustNot|minimumShouldMatch|not exists|missing" "$FILE" || true
echo
echo "== Find usages of buildViewPermissionQuery =="
rg -n "buildViewPermissionQuery" src/main/java/com/netgrif/application/engine/elastic/service
echo
echo "== Locate positive/negative helper method implementations =="
rg -n "buildPositiveViewUser|buildNegativeViewUser" "$FILE"Repository: netgrif/application-engine
Length of output: 5084
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any legacy "no permission fields" / not-exists logic references across Java sources
rg -n --hidden --glob '!**/target/**' \
"viewPermNotExists|view perm(ission)? not exists|permission not exists|not exists|missing.*view|viewRoles.*missing|viewUsers.*missing" \
src/main/java || true
# Search for query parts touching those fields
rg -n --hidden --glob '!**/target/**' \
"viewRoles|viewUsers|negativeViewRoles|negativeViewUsers" \
src/main/java/com/netgrif/application/engine/elastic/service || true
# Specifically check other permission-related services for compatibility paths
rg -n --hidden --glob '!**/target/**' \
"ElasticViewPermissionService|buildViewPermissionQuery|buildPositiveViewRoleQuery|buildPositiveViewUser" \
src/main/java || trueRepository: netgrif/application-engine
Length of output: 3064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TASK_FILE="src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskService.java"
CASE_FILE="src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java"
echo "== ElasticTaskService: show around buildViewPermissionQuery usage (line ~183) =="
awk 'NR>=150 && NR<=230 {printf "%4d\t%s\n", NR, $0}' "$TASK_FILE" | sed -n '1,120p'
echo
echo "== ElasticCaseService: show around buildViewPermissionQuery usage (line ~205) =="
awk 'NR>=170 && NR<=260 {printf "%4d\t%s\n", NR, $0}' "$CASE_FILE" | sed -n '1,120p'
echo
echo "== Check for other view permission logic in those services =="
rg -n "buildViewPermissionQuery|viewRoles|viewUsers|negativeViewRoles|negativeViewUsers|mustNot\\(buildNegativeView|minimumShouldMatch" "$TASK_FILE" "$CASE_FILE" || trueRepository: netgrif/application-engine
Length of output: 7725
Elastic view permission query now excludes documents without explicit viewRoles/viewUsers matches
In ElasticViewPermissionService.buildViewPermissionQuery (lines 18-24, helpers 31-59), roleOrPositiveUserQuery uses minimumShouldMatch(1) across roleViewQuery (positive viewRoles) and buildPositiveViewUser (positive viewUsers), with no “field missing => allowed” branch. As a result, documents that omit viewRoles and viewUsers will not match, impacting both task and case searches since both ElasticTaskService and ElasticCaseService call this shared helper.
Restore the intended legacy “no permission fields” behavior or document and implement a migration/reindex/backfill that populates explicit positive defaults for existing documents.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/netgrif/application/engine/elastic/service/ElasticViewPermissionService.java`
around lines 18 - 24, The current buildViewPermissionQuery uses a
BoolQueryBuilder roleOrPositiveUserQuery that requires either roleViewQuery or
buildPositiveViewUser to match, which excludes documents that lack both
viewRoles and viewUsers; change roleOrPositiveUserQuery to also allow documents
where both permission fields are missing by adding an additional should clause
that matches "viewRoles not exists AND viewUsers not exists" (use exists queries
for viewRoles/viewUsers and combine them with mustNot as a should option) so
minimumShouldMatch(1) still applies, and leave the existing
mustNot(buildNegativeViewUser(user)) in place; update
ElasticViewPermissionService.buildViewPermissionQuery (and any helper usage of
roleViewQuery, buildPositiveViewUser, buildNegativeViewUser) to implement this
or alternatively document and implement a migration/backfill that populates
explicit positive defaults for existing documents.
- null check added to Importer
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
2 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
Refactor of task view permission query resolution for both elastic and mongo search
Fixes NAE-2424
How Has Been This Tested?
Test included in test file TaskPermissionsTest
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests