[NAE-2273] Implement User Impersonation#417
Open
camperko wants to merge 6 commits intorelease/7.0.0-rev10from
Open
[NAE-2273] Implement User Impersonation#417camperko wants to merge 6 commits intorelease/7.0.0-rev10from
camperko wants to merge 6 commits intorelease/7.0.0-rev10from
Conversation
- implement user impersonation - user impersonation is no longer supported in community edition - user impersonation is no longer managed by petrinets - logged user has information about impersonated user
# Conflicts: # application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy # application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy # application-engine/src/main/java/com/netgrif/application/engine/configuration/security/ImpersonationRequestFilter.java # application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationAuthorizationService.java # application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java # application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java # application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java # application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/AbstractAuthorizationService.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/ConfigurableMenuService.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskAuthorizationService.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskSearchService.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java # application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy # nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/workflow/domain/Task.java
- update to engine 7.0.0-rev10
- remove duplicate code
- update controllers to support impersonated user responses - change impersonation object to work just with ids of roles and authorities in place of whole objects - fix some methods which were overwriting loggedUser/impersonated user in context - change start impersonation method to work just with id of impersonation in place of whole object
camperko
added
new feature
camperko
requested review from
Kovy95,
Retoocs,
machacjozef,
renczesstefan and
tuplle
WalkthroughThis PR refactors the impersonation framework by removing legacy service-based implementations, relocating core impersonation entities to the auth layer, introducing direct impersonation support in LoggedUser, and systematically updating public APIs from AbstractUser to LoggedUser with impersonation-aware query logic throughout the system. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/ConfigurableMenuService.java (1)
93-101:⚠️ Potential issue | 🔴 CriticalFix type inconsistency: wrap
role.getName()inI18nStringat line 99.The method declares return type
Map<String, I18nString>, but the GLOBAL_ROLE branch (line 99) returnsrole.getName()directly as aString, while the non-global branch (line 111) correctly wraps it withnew I18nString(v.getName()). SinceProcessRole.getName()returnsString(notI18nString), the GLOBAL_ROLE path has incompatible value types. Change line 99 to:role -> new I18nString(role.getName())🤖 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/ConfigurableMenuService.java` around lines 93 - 101, The GLOBAL_ROLE branch returns Map<String, I18nString> but currently maps values using role.getName() (a String); update the mapping in the block that handles GLOBAL_ROLE (the code using processRoleService.findAllGlobalRoles(...) and filtering with permittedRoles and bannedRoles) to wrap the role name in an I18nString (e.g., use new I18nString(role.getName())) so the values match the declared Map<String, I18nString> return type; keep the same key expression (role.getImportId() + ":" + GLOBAL_ROLE) and the existing filters.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java (1)
306-325:⚠️ Potential issue | 🟡 MinorSilent failure may return incomplete
FileFieldValue.If
getLoggedUserFromContext()returnsnullat line 309, an NPE is caught by the genericcatch (Exception e)block. The method then returnsffvwith uninitializednameandpathfields, potentially causing downstream failures when the caller uses this value.Consider either:
- Failing fast with a meaningful exception if no user is in context, or
- Using a fallback identifier (e.g., timestamp or "anonymous") for the filename.
🛡️ Proposed fix with fallback
protected FileFieldValue createXML(MenuAndFilters menuAndFilters, String parentId, FileField fileField) throws IOException { FileFieldValue ffv = new FileFieldValue(); try { - ffv.setName("menu_" + userService.getLoggedUserFromContext().getName().replaceAll("\\s+", "") + ".xml"); + var loggedUser = userService.getLoggedUserFromContext(); + String userIdentifier = (loggedUser != null && loggedUser.getName() != null) + ? loggedUser.getName().replaceAll("\\s+", "") + : "export_" + System.currentTimeMillis(); + ffv.setName("menu_" + userIdentifier + ".xml"); ffv.setPath(storageResolverService.resolve(fileField.getStorageType()).getPath(parentId, fileField.getImportId(), ffv.getName()));🤖 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/MenuImportExportService.java` around lines 306 - 325, In createXML(MenuAndFilters, String, FileField) guard against a null return from userService.getLoggedUserFromContext() before building ffv.name and path: either throw a clear exception (e.g., IllegalStateException("No logged user in context")) to fail fast, or assign a safe fallback (e.g., "anonymous" or a timestamp) to build the filename, then proceed; locate this logic in createXML and ensure the chosen approach happens before calling storageResolverService.resolve(...) so ffv.name/path are always initialized (and keep the existing catch for IO/errors but do not silently return an uninitialized ffv).nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/ImpersonationConfigurationProperties.java (2)
20-20:⚠️ Potential issue | 🟡 MinorFix Javadoc/source mismatch for Redis namespace expression.
Line 20 says
"redisProperties.namespace", but the actual expression on Line 22 isredisProperties.session.namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/ImpersonationConfigurationProperties.java` at line 20, Update the Javadoc in ImpersonationConfigurationProperties to match the actual SpEL expression used: replace the incorrect "redisProperties.namespace" mention with "redisProperties.session.namespace" (or otherwise mirror the exact expression referenced on Line 22) so the comment and the expression are consistent; ensure the doc string near the field or getter that references the Redis namespace uses the exact symbol redisProperties.session.namespace.
22-23: 🧹 Nitpick | 🔵 TrivialConsolidate property binding to a single mechanism in
ImpersonationConfigurationProperties.The class mixes
@ConfigurationPropertiesand@ValueSpEL binding—it declares@ConfigurationProperties(prefix = "netgrif.impersonation")but injectsredisNamespacevia@Value("#{redisProperties.session.namespace}"). This couples the configuration to another bean by name, reducing testability and making property metadata unclear. Instead, injectDataConfigurationProperties.RedisPropertiesas a dependency and resolve the namespace through constructor or setter injection, or defineredisNamespaceas a configuration property under the "netgrif.impersonation" prefix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/ImpersonationConfigurationProperties.java` around lines 22 - 23, ImpersonationConfigurationProperties mixes `@ConfigurationProperties` with a SpEL `@Value` on the field redisNamespace; remove the `@Value`("#{redisProperties.session.namespace}") and instead either (A) add a constructor or setter to accept DataConfigurationProperties.RedisProperties and set redisNamespace from redisProperties.getSession().getNamespace() (reference: class ImpersonationConfigurationProperties and type DataConfigurationProperties.RedisProperties), or (B) promote redisNamespace to be a regular property under the "netgrif.impersonation" prefix and bind it via the existing `@ConfigurationProperties` mapping (reference: field redisNamespace and class ImpersonationConfigurationProperties); update imports and tests accordingly.nae-user-ce/src/main/java/com/netgrif/application/engine/auth/domain/Impersonator.java (1)
11-28: 🧹 Nitpick | 🔵 TrivialConsider adding
serialVersionUIDforSerializableclass.The class implements
Serializablebut lacks aserialVersionUID. SinceImpersonatorobjects are stored in Redis and may be deserialized across application versions, adding an explicitserialVersionUIDprevents potentialInvalidClassExceptionissues during version upgrades.💡 Proposed fix
import java.io.Serializable; +import java.io.Serial; import java.time.LocalDateTime; `@Data` `@NoArgsConstructor` `@AllArgsConstructor` `@RedisHash`(value = "#{`@impersonatorRedisHash`}") public class Impersonator implements Serializable { + `@Serial` + private static final long serialVersionUID = 1L; + `@Id` protected String impersonatorId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/domain/Impersonator.java` around lines 11 - 28, The Impersonator class implements Serializable but lacks a serialVersionUID; add a private static final long serialVersionUID (e.g. 1L) to the Impersonator class (keep it alongside the other field declarations) to ensure stable serialization across versions; update the class declaration for Impersonator only—no other behavior changes required.nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java (1)
16-40: 🧹 Nitpick | 🔵 TrivialConsider thread-safety for static factory field.
The
factoryfield is a static mutable reference that could be modified from different threads during application startup. While this is typically set once during initialization, consider making itvolatileor using anAtomicReferenceto ensure visibility across threads.💡 Suggested improvement
- private static LoggedUserFactory factory = () -> { + private static volatile LoggedUserFactory factory = () -> { throw new IllegalStateException("No LoggedUserFactory configured"); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java` around lines 16 - 40, The static mutable field `factory` (type LoggedUserFactory) is not thread-safe; make its updates visible across threads by converting it to a volatile variable or wrapping it in an AtomicReference and update `setLoggedUserFactory(LoggedUserFactory f)` to set the atomic/volatile holder, and read from that holder wherever `factory.create()` is used (references: factory, LoggedUserFactory, setLoggedUserFactory, LoggedUser).application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/repositories/CaseRepositoryImpl.java (1)
31-34:⚠️ Potential issue | 🟡 MinorAdd defensive checks before casting
auth.getPrincipal()toLoggedUser.The direct cast
((LoggedUser) auth.getPrincipal())assumes authentication is present and the principal is always aLoggedUser. While the security configuration requires authentication, it does not guarantee the principal type. The codebase shows non-LoggedUserprincipals are possible (seeUserServiceImpl.getLoggedOrSystem()line 463). Additionally, there is no null check on theauthobject itself.Consider adding a defensive check:
Proposed fix
`@Override` public void customize(QuerydslBindings bindings, QCase qCase) { Authentication auth = SecurityContextHolder.getContext().getAuthentication(); + if (auth == null || !(auth.getPrincipal() instanceof LoggedUser loggedUser)) { + return; // or throw an exception if authentication is required + } - List<PetriNetReference> nets = petriNetService.getReferencesByUsersProcessRoles(((LoggedUser) auth.getPrincipal()), null); + List<PetriNetReference> nets = petriNetService.getReferencesByUsersProcessRoles(loggedUser, null);🤖 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/domain/repositories/CaseRepositoryImpl.java` around lines 31 - 34, In customize(QuerydslBindings, QCase) add defensive null/type checks around SecurityContextHolder.getContext().getAuthentication() and its principal before casting to LoggedUser: verify auth is not null and auth.getPrincipal() instanceof LoggedUser, then call petriNetService.getReferencesByUsersProcessRoles with the safely-cast LoggedUser; if those checks fail, use a sensible fallback (e.g. treat as system/no-user or skip user-scoped filtering) so netIds is computed safely and NPE/ClassCastException is avoided (refer to customize, SecurityContextHolder, LoggedUser, and petriNetService.getReferencesByUsersProcessRoles).nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Task.java (1)
330-334: 🧹 Nitpick | 🔵 TrivialStale Javadoc references non-existent fields.
The Javadoc mentions
viewUsersanduserscollections, but the actual field names areviewActorsandactors. This appears to be leftover from a previous refactoring.📝 Proposed fix
/** - * Initializes {`@link` `#viewUsers`} collection. Any user defined in {`@link` `#users`} with permission {`@link` RolePermission#VIEW} - * of true value is added to the {`@link` `#viewUsers`} collection. + * Initializes {`@link` `#viewActors`} collection. Any actor defined in {`@link` `#actors`} with permission {`@link` RolePermission#VIEW} + * of true value is added to the {`@link` `#viewActors`} collection. * - * `@return` true if the {`@link` `#viewUsers`} was modified, false otherwise + * `@return` true if the {`@link` `#viewActors`} was modified, false otherwise */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Task.java` around lines 330 - 334, Update the stale Javadoc in Task.java to reference the actual field names: replace mentions of viewUsers and users with viewActors and actors (keep RolePermission#VIEW as-is) and adjust the `@return` description to state whether viewActors was modified; ensure the Javadoc for the initializer method (the comment above the method that populates viewActors) accurately names viewActors and actors so it matches the Task class fields.docs/javadoc/com/netgrif/application/engine/impersonation/domain/Impersonator.html (1)
122-140:⚠️ Potential issue | 🟡 MinorRegenerate this Javadoc page to resolve package-location inconsistency.
The references now point to
com.netgrif.application.engine.auth.domain.Impersonator, but this page is still under.../impersonation/domainand still presents the old package in the header. This leaves API docs internally inconsistent and can break navigation/search for the moved type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/javadoc/com/netgrif/application/engine/impersonation/domain/Impersonator.html` around lines 122 - 140, The Javadoc page for the Impersonator class shows the old package (impersonation.domain) while the type now lives in com.netgrif.application.engine.auth.domain; regenerate the Javadoc (or re-run the doc generation task) so the Impersonator HTML is rebuilt with the correct package header, links, and serialized-form references pointing to com.netgrif.application.engine.auth.domain, and verify the `@RedisHash/Impersonator` declaration and all internal anchors/see-also links are updated to the new package name.application-engine/src/main/java/com/netgrif/application/engine/configuration/security/AuthenticationService.java (1)
109-119: 🧹 Nitpick | 🔵 TrivialAddress the TODO for event publishing on impersonation end.
The commented-out
ImpersonationEventpublishing could be important for audit trails, logging, or downstream systems that need to track when impersonation sessions end. Consider implementing this before merging.Would you like me to help implement the event publishing logic or open an issue to track this task?
🤖 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/configuration/security/AuthenticationService.java` around lines 109 - 119, In resolveImpersonatorOnLogout(Object principal) implement the commented event publish: cast principal to LoggedUser (e.g., LoggedUser impersonator = (LoggedUser) principal), if impersonator.isImpersonating() build an ImpersonationEvent using impersonator and impersonator.getImpersonated() with RunPhase.STOP and call publisher.publishEvent(...) (do this before or immediately after impersonatorRepository.deleteById(...)); ensure required imports for ImpersonationEvent and RunPhase are present and preserve the existing try/catch logging.application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
970-983: 🧹 Nitpick | 🔵 TrivialConsider extracting repeated LoggedUser conversion to a helper method.
The pattern
author instanceof LoggedUser ? (LoggedUser) author : ActorTransformer.toLoggedUser(author)is repeated multiple times throughout the file (lines 972, 987, 1014, 1025, 1039, 1048, 1067, 1076). Consider extracting this to a private helper method to reduce duplication and ensure consistent behavior.♻️ Proposed helper method
Add a private helper method:
private LoggedUser toLoggedUser(AbstractUser user) { return user instanceof LoggedUser ? (LoggedUser) user : ActorTransformer.toLoggedUser(user) }Then replace occurrences like:
-LoggedUser loggedUser = author instanceof LoggedUser ? (LoggedUser) author : ActorTransformer.toLoggedUser(author) +LoggedUser loggedUser = toLoggedUser(author)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy` around lines 970 - 983, Extract the repeated LoggedUser conversion into a private helper in ActionDelegate: add a private LoggedUser toLoggedUser(AbstractUser user) that returns user instanceof LoggedUser ? (LoggedUser) user : ActorTransformer.toLoggedUser(user), then replace the inline expressions in methods such as createCase(...) (and the other methods in this class that currently use author instanceof LoggedUser ? (LoggedUser) author : ActorTransformer.toLoggedUser(author)) to call toLoggedUser(author) so conversion logic is centralized and consistent.
🤖 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/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`:
- Around line 1242-1244: The new loggedUser() now returns
userService.loggedUserFromContext which can be null when scripts run outside
authenticated contexts; change loggedUser() to fall back to the original
non‑impersonation call (userService.loggedUser) when loggedUserFromContext is
null and/or update calling action scripts to handle a null principal.
Concretely, in ActionDelegate.loggedUser() return
userService.loggedUserFromContext if present, otherwise return
userService.loggedUser, and audit any action scripts that assumed a non-null
result to guard against null or explicitly require an authenticated context.
- Around line 143-144: Remove the unused autowired field ImpersonationService
from the ActionDelegate class: locate the declaration "ImpersonationService
impersonationService" annotated with `@Autowired` in ActionDelegate and delete
that field and its `@Autowired` annotation so the class no longer contains the
dead dependency; ensure no other references to ImpersonationService remain in
ActionDelegate after removal.
- Around line 1114-1154: In ActionDelegate update all overloads of assignRole
and removeRole to use userService.loggedOrSystem as the default parameter
instead of userService.loggedUserFromContext; specifically change the default
argument in the methods assignRole(String roleMongoId...), assignRole(String
roleId, String netId...), assignRole(String roleId, PetriNet...),
assignRole(String roleId, String netId, Version...), and the corresponding
removeRole overloads so they call userService.loggedOrSystem to avoid
ClassCastException when no HTTP-authenticated principal is present.
In
`@application-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java`:
- Around line 102-108: The loop in ElasticController currently checks
user.isProcessAccessDeny() per page causing wasted iterations and misleading
logs; move the access check outside the for-loop: first verify
user.isProcessAccessDeny(), log and return or skip reindexing if denied (so the
"Reindexing cases: X pages" and "Success" responses accurately reflect work
done), then build the Predicate once via searchService.buildQuery(searchBody,
user, locale) and iterate pages calling
reindexingTask.forceReindexPage(predicate, page, numOfPages) only when access is
allowed.
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`:
- Around line 563-566: The pagination total is computed from queryTotal but the
impersonation filter from getImpersonatedProcessesCriteria(user) is only applied
to query, causing PageImpl totals to include inaccessible processes; fix by
applying the same Criteria returned by getImpersonatedProcessesCriteria(user) to
queryTotal when it's non-null (mirror the existing logic that adds the Criteria
to query) so both query and queryTotal use identical filters before executing
counts and fetching results.
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PublicPetriNetController.java`:
- Around line 101-103: The call to PetriNetController::decodeUrl in
getTransitionReferences discards decoded values because forEach doesn't mutate
the list; replace that with transforming the ids list into a decoded list and
pass the decoded list to petriNetService.getTransitionReferences (e.g., map ids
via PetriNetController::decodeUrl and collect to a new List or replace elements
in-place) so getTransitionReferences receives the decoded IDs; update the call
site in PublicPetriNetController.getTransitionReferences accordingly while
keeping userService.getLoggedUserFromContext() and locale parameters unchanged.
In
`@application-engine/src/main/java/com/netgrif/application/engine/security/service/SecurityContextService.java`:
- Around line 74-80: When calling
ActorTransformer.toLoggedUser(userService.findById(...)) guard against a null
return: call userService.findById(...) into a local variable (e.g., foundUser),
if foundUser is null then log a warning/error and skip replacing the current
LoggedUser (do not call ActorTransformer.toLoggedUser) to avoid NPE; otherwise
convert foundUser to reloadedLoggedUser, preserve impersonation state
(reloadedLoggedUser.setImpersonatedUser(...),
setImpersonatedProcessesListAllowing(...), setImpersonatedProcesses(...)) and
assign loggedUser = reloadedLoggedUser as before.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java`:
- Line 320: The code mixes identity sources: some places use
getLoggedUserFromContext() (e.g. the QCase.case$.author.id.eq(...) predicate)
while createFilterImport(AbstractUser author) and
createFilterExport(AbstractUser author) accept an explicit author; unify them by
choosing a single identity source and applying it everywhere—either (A) change
the query and any export/createXML/loadFromXML calls to use the provided author
parameter (preferred) or (B) remove the author parameter and consistently call
getLoggedUserFromContext() before invoking
createFilterImport/createFilterExport; update QCase.case$.author.id.eq(...) and
the code paths that call loadFromXML and createXML so they all use the same
AbstractUser instance (author or the context user) to avoid divergence during
impersonation or cross-user flows.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskAuthorizationService.java`:
- Around line 65-73: In TaskAuthorizationService.isAssignee, make the
comparisons null-safe: replace direct .equals() and unboxed Boolean checks with
safe alternatives—use Objects.equals(task.getUserId(),
user.getSelfOrImpersonatedStringId()) and
Objects.equals(task.getImpersonatorUserId(), user.getStringId()) to avoid NPE
when IDs are null, and use
Boolean.TRUE.equals(user.getAttributeValue("anonymous")) (or equivalent
null-safe check) instead of (Boolean) user.getAttributeValue("anonymous"); add
java.util.Objects import if needed.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java`:
- Around line 705-714: The constructed Mongo query is closed too early:
queryBuilder.append("]}"); is written before appending the impersonation filter,
causing malformed JSON; update the logic around queryBuilder in the
impersonation branch (methods/variables: queryBuilder,
loggedUser.isImpersonating(), loggedUser.getImpersonatedProcesses(),
loggedUser.isImpersonatedProcessesListAllowing()) so that you append the
processIdentifier clause inside the same object before closing the array/object
(i.e., build and append ",\"processIdentifier\":{$in:[...]}" or
",\"processIdentifier\":{$nin:[...]}" while the object is still open) and only
then append the closing "]}" (apply the same fix to the second similar block
referenced around the later lines).
- Line 126: The code converts AbstractUser to LoggedUser unconditionally causing
NPEs when user is null; update the three methods assignTasks(List<Task>,
AbstractUser, Map<String,String>), finishTasks(List<Task>, AbstractUser,
Map<String,String>) and cancelTasks(List<Task>, AbstractUser,
Map<String,String>) so they first check if the passed user is null and if so
obtain the fallback via userService.getLoggedOrSystem(), otherwise
cast/transform as before (i.e. replace the single-line LoggedUser loggedUser =
user instanceof LoggedUser ? (LoggedUser) user :
ActorTransformer.toLoggedUser(user); with a null-safe branch that uses
userService.getLoggedOrSystem() when user==null), ensuring
fillAndValidateAttributes(...) receives the resolved LoggedUser; apply the same
change at the other occurrences noted in the review.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java`:
- Around line 215-217: Guard against a null user before calling
user.isProcessAccessDeny() in WorkflowService (e.g., check user != null) to
avoid NPEs, and when access is denied return an empty Page that preserves the
incoming pageable metadata instead of Page.empty() (use a Page implementation
constructed with the provided Pageable and zero content, or Page.empty(pageable)
if available) so pagination context is retained; update the access-deny checks
around user/isProcessAccessDeny() and the return statements to use the
pageable-aware empty page.
In
`@application-engine/src/test/java/com/netgrif/application/engine/workflow/web/VariableArcsTest.java`:
- Line 176: Tests repeatedly call ActorTransformer.toLoggedUser(testUser);
precompute a LoggedUser once in the test setup (e.g., create a private
LoggedUser testLoggedUser in VariableArcsTest and initialize it in the before()
setup method) and replace all occurrences of
ActorTransformer.toLoggedUser(testUser) in the file with that precomputed
testLoggedUser to reduce duplication and improve readability; update every call
site listed (including the calls at the many lines referenced) to use the new
testLoggedUser variable.
In
`@docs/javadoc/com/netgrif/application/engine/configuration/security/ImpersonationRequestFilter.html`:
- Around line 257-258: The generated Javadoc HTML contains stale package links
(e.g., the method isValid(Impersonator) anchor points to
com.netgrif.application.engine.auth.domain but link text and other signatures
still reference impersonation.domain, and constructor docs reference
IImpersonationService in the old package); regenerate the Javadoc for
ImpersonationRequestFilter so the HTML is rebuilt from current sources and all
references are updated consistently (verify the isValid(Impersonator) entry, the
constructor docs, and any mentions of IImpersonationService reflect the new
package names).
In
`@docs/javadoc/com/netgrif/application/engine/impersonation/domain/class-use/Impersonator.html`:
- Line 6: The Javadoc for the class Uses page still contains old package links:
update the package references for the Impersonator class from
impersonation.domain to the new package
com.netgrif.application.engine.auth.domain across the generated HTML (e.g.,
Impersonator.html and other affected class-use pages), then regenerate the
Javadocs so all internal anchors and hrefs point to
com.netgrif.application.engine.auth.domain.Impersonator; ensure the class title,
link targets, and any cross-reference anchors use the fully qualified name
com.netgrif.application.engine.auth.domain.Impersonator consistently.
In
`@docs/javadoc/com/netgrif/application/engine/impersonation/domain/repository/class-use/ImpersonatorRepository.html`:
- Line 6: The Javadoc file references two package locations for the interface
(com.netgrif.application.engine.auth.repository.ImpersonatorRepository vs
com.netgrif.application.engine.impersonation.domain.repository.ImpersonatorRepository)
causing broken internal links; regenerate the Javadoc (or remove this generated
file from version control) so all references consistently point to the current
package for the ImpersonatorRepository interface, and if regeneration isn’t
possible update the internal anchor hrefs in the generated HTML to match the
declared package name (search for occurrences of ImpersonatorRepository and the
two package strings to fix links).
In
`@docs/javadoc/com/netgrif/application/engine/impersonation/service/ImpersonationSessionService.html`:
- Line 230: The Javadoc links for ImpersonatorRepository point to the old
package (../domain/repository/ImpersonatorRepository.html) causing 404s; update
every href and title that references ImpersonatorRepository in this file (e.g.,
the setImpersonatorRepository method anchor and other occurrences) to the new
package path href="../../auth/repository/ImpersonatorRepository.html" and change
the title attribute to "interface in
com.netgrif.application.engine.auth.repository" so anchors and displayed package
both reflect the relocation.
In `@docs/javadoc/serialized-form.html`:
- Around line 3568-3571: The Javadoc entry for the class Impersonator has
inconsistent package references: the displayed FQCN is
"com.netgrif.application.engine.auth.domain.Impersonator" while the link
target/title/section use "com.netgrif.application.engine.impersonation.domain";
update the Javadoc fragment for the Impersonator class so the displayed FQCN,
the anchor/link href/title, and the surrounding section all use the same package
(choose either auth.domain or impersonation.domain) to restore correct
navigation—adjust the link target (Impersonator.html), the <h3> text, and any
anchor id so they consistently reference the chosen package name.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/ImpersonationServiceImpl.java`:
- Around line 16-20: The current implementation of
ImpersonationServiceImpl.getImpersonation logs an expected community-edition
limitation at error level; change the logging call from log.error to log.warn
(preserving the existing message) so that feature-absence is recorded as a
warning not an error, and apply the same change to all other methods in
ImpersonationServiceImpl that log this "not implemented in community edition"
condition (keep throwing NotImplementedException as before).
- Line 9: Remove the unused import AccessDeniedException from
ImpersonationServiceImpl and clean up any method signatures in that class that
declare "throws AccessDeniedException" since the methods actually throw
NotImplementedException; update the method declarations (in
ImpersonationServiceImpl) to remove the unnecessary throws clause and run a
build to ensure no other references remain.
- Around line 13-14: ImpersonationServiceImpl is not registered as a Spring bean
and still imports AccessDeniedException which isn’t used; either annotate the
class ImpersonationServiceImpl with `@Service` (preferred) or add a `@Bean` method
in AuthBeansConfiguration that returns new ImpersonationServiceImpl() and is
annotated `@ConditionalOnMissingBean` following the pattern of other services so
it becomes injectable, and while editing remove the unused import of
AccessDeniedException and strip it from any method throws clauses where it’s not
actually thrown.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java`:
- Around line 399-400: UserServiceImpl currently calls findById(...) and
immediately passes the result to buildPredicate(...) which will NPE if the
effective user is missing; add a null-check after AbstractUser user =
this.findById(loggedUser.getSelfOrImpersonatedStringId(),
loggedUser.getRealmId()) and if user is null throw a controlled exception (e.g.,
a NotFound/BadRequest or custom AuthenticationException) with a clear message
about the impersonated/stale user, so buildPredicate(user, query) is never
called with null; apply the same guard to the other occurrence around lines
412-413 where findById result is used.
In
`@nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/ImpersonationService.java`:
- Around line 31-37: The interface ImpersonationService methods lack Javadoc
explaining the semantics of the overloaded variants; add Javadoc to the
interface and to each method (startImpersonation(String),
startImpersonation(String, LoggedUser), endImpersonation(),
endImpersonation(LoggedUser)) that states when to use the no-arg vs. LoggedUser
variants (e.g., controller code should call the LoggedUser-taking overload when
the caller is already resolved, service or helper code may call the no-arg which
resolves the current user from SecurityContextHolder), documents the resolution
behavior for the no-arg methods (explicitly mention SecurityContextHolder or
equivalent), and describes the conditions under which IllegalArgumentException
or AccessDeniedException are thrown; include references to the method names in
the Javadoc so callers can easily choose the correct overload.
---
Outside diff comments:
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`:
- Around line 970-983: Extract the repeated LoggedUser conversion into a private
helper in ActionDelegate: add a private LoggedUser toLoggedUser(AbstractUser
user) that returns user instanceof LoggedUser ? (LoggedUser) user :
ActorTransformer.toLoggedUser(user), then replace the inline expressions in
methods such as createCase(...) (and the other methods in this class that
currently use author instanceof LoggedUser ? (LoggedUser) author :
ActorTransformer.toLoggedUser(author)) to call toLoggedUser(author) so
conversion logic is centralized and consistent.
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/security/AuthenticationService.java`:
- Around line 109-119: In resolveImpersonatorOnLogout(Object principal)
implement the commented event publish: cast principal to LoggedUser (e.g.,
LoggedUser impersonator = (LoggedUser) principal), if
impersonator.isImpersonating() build an ImpersonationEvent using impersonator
and impersonator.getImpersonated() with RunPhase.STOP and call
publisher.publishEvent(...) (do this before or immediately after
impersonatorRepository.deleteById(...)); ensure required imports for
ImpersonationEvent and RunPhase are present and preserve the existing try/catch
logging.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/repositories/CaseRepositoryImpl.java`:
- Around line 31-34: In customize(QuerydslBindings, QCase) add defensive
null/type checks around SecurityContextHolder.getContext().getAuthentication()
and its principal before casting to LoggedUser: verify auth is not null and
auth.getPrincipal() instanceof LoggedUser, then call
petriNetService.getReferencesByUsersProcessRoles with the safely-cast
LoggedUser; if those checks fail, use a sensible fallback (e.g. treat as
system/no-user or skip user-scoped filtering) so netIds is computed safely and
NPE/ClassCastException is avoided (refer to customize, SecurityContextHolder,
LoggedUser, and petriNetService.getReferencesByUsersProcessRoles).
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/ConfigurableMenuService.java`:
- Around line 93-101: The GLOBAL_ROLE branch returns Map<String, I18nString> but
currently maps values using role.getName() (a String); update the mapping in the
block that handles GLOBAL_ROLE (the code using
processRoleService.findAllGlobalRoles(...) and filtering with permittedRoles and
bannedRoles) to wrap the role name in an I18nString (e.g., use new
I18nString(role.getName())) so the values match the declared Map<String,
I18nString> return type; keep the same key expression (role.getImportId() + ":"
+ GLOBAL_ROLE) and the existing filters.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java`:
- Around line 306-325: In createXML(MenuAndFilters, String, FileField) guard
against a null return from userService.getLoggedUserFromContext() before
building ffv.name and path: either throw a clear exception (e.g.,
IllegalStateException("No logged user in context")) to fail fast, or assign a
safe fallback (e.g., "anonymous" or a timestamp) to build the filename, then
proceed; locate this logic in createXML and ensure the chosen approach happens
before calling storageResolverService.resolve(...) so ffv.name/path are always
initialized (and keep the existing catch for IO/errors but do not silently
return an uninitialized ffv).
In
`@docs/javadoc/com/netgrif/application/engine/impersonation/domain/Impersonator.html`:
- Around line 122-140: The Javadoc page for the Impersonator class shows the old
package (impersonation.domain) while the type now lives in
com.netgrif.application.engine.auth.domain; regenerate the Javadoc (or re-run
the doc generation task) so the Impersonator HTML is rebuilt with the correct
package header, links, and serialized-form references pointing to
com.netgrif.application.engine.auth.domain, and verify the
`@RedisHash/Impersonator` declaration and all internal anchors/see-also links are
updated to the new package name.
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java`:
- Around line 16-40: The static mutable field `factory` (type LoggedUserFactory)
is not thread-safe; make its updates visible across threads by converting it to
a volatile variable or wrapping it in an AtomicReference and update
`setLoggedUserFactory(LoggedUserFactory f)` to set the atomic/volatile holder,
and read from that holder wherever `factory.create()` is used (references:
factory, LoggedUserFactory, setLoggedUserFactory, LoggedUser).
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Task.java`:
- Around line 330-334: Update the stale Javadoc in Task.java to reference the
actual field names: replace mentions of viewUsers and users with viewActors and
actors (keep RolePermission#VIEW as-is) and adjust the `@return` description to
state whether viewActors was modified; ensure the Javadoc for the initializer
method (the comment above the method that populates viewActors) accurately names
viewActors and actors so it matches the Task class fields.
In
`@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/ImpersonationConfigurationProperties.java`:
- Line 20: Update the Javadoc in ImpersonationConfigurationProperties to match
the actual SpEL expression used: replace the incorrect
"redisProperties.namespace" mention with "redisProperties.session.namespace" (or
otherwise mirror the exact expression referenced on Line 22) so the comment and
the expression are consistent; ensure the doc string near the field or getter
that references the Redis namespace uses the exact symbol
redisProperties.session.namespace.
- Around line 22-23: ImpersonationConfigurationProperties mixes
`@ConfigurationProperties` with a SpEL `@Value` on the field redisNamespace; remove
the `@Value`("#{redisProperties.session.namespace}") and instead either (A) add a
constructor or setter to accept DataConfigurationProperties.RedisProperties and
set redisNamespace from redisProperties.getSession().getNamespace() (reference:
class ImpersonationConfigurationProperties and type
DataConfigurationProperties.RedisProperties), or (B) promote redisNamespace to
be a regular property under the "netgrif.impersonation" prefix and bind it via
the existing `@ConfigurationProperties` mapping (reference: field redisNamespace
and class ImpersonationConfigurationProperties); update imports and tests
accordingly.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/domain/Impersonator.java`:
- Around line 11-28: The Impersonator class implements Serializable but lacks a
serialVersionUID; add a private static final long serialVersionUID (e.g. 1L) to
the Impersonator class (keep it alongside the other field declarations) to
ensure stable serialization across versions; update the class declaration for
Impersonator only—no other behavior changes required.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (93)
application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/service/AuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/service/UserResourceHelperService.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/service/interfaces/IUserResourceHelperService.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/web/AuthenticationController.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/NaeSecurityConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/SessionRegistryConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/security/AuthenticationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/security/ImpersonationRequestFilter.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticViewPermissionService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.javaapplication-engine/src/main/java/com/netgrif/application/engine/export/service/ExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/exceptions/IllegalImpersonationAttemptException.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/exceptions/ImpersonatedUserHasSessionException.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationSessionService.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/service/interfaces/IImpersonationAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/service/interfaces/IImpersonationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/service/interfaces/IImpersonationSessionService.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/web/ImpersonationController.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/web/ImpersonationControllerAdvice.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/web/requestbodies/SearchRequest.javaapplication-engine/src/main/java/com/netgrif/application/engine/impersonation/web/responsebodies/ImpersonationNotAvailableResponse.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/params/ImportPetriNetParams.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PublicPetriNetController.javaapplication-engine/src/main/java/com/netgrif/application/engine/security/service/SecurityContextService.javaapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.javaapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/domain/repositories/CaseRepositoryImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/params/CreateCaseParams.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/params/DelegateTaskParams.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/params/TaskParams.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/AbstractAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/ConfigurableMenuService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskSearchService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/UserFilterSearchService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/ITaskAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IWorkflowAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/web/PublicTaskController.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/web/PublicWorkflowController.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/Task.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/TestHelper.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovyapplication-engine/src/test/java/com/netgrif/application/engine/workflow/WorkflowMvcPerformanceTest.javaapplication-engine/src/test/java/com/netgrif/application/engine/workflow/service/AbstractAuthorizationServiceTest.javaapplication-engine/src/test/java/com/netgrif/application/engine/workflow/service/TaskServiceTest.javaapplication-engine/src/test/java/com/netgrif/application/engine/workflow/web/VariableArcsTest.javadocs/javadoc/com/netgrif/application/engine/configuration/security/ImpersonationRequestFilter.htmldocs/javadoc/com/netgrif/application/engine/impersonation/domain/Impersonator.htmldocs/javadoc/com/netgrif/application/engine/impersonation/domain/class-use/Impersonator.htmldocs/javadoc/com/netgrif/application/engine/impersonation/domain/repository/class-use/ImpersonatorRepository.htmldocs/javadoc/com/netgrif/application/engine/impersonation/service/ImpersonationSessionService.htmldocs/javadoc/serialized-form.htmlnae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Impersonation.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/LoggedUser.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/impersonation/domain/Impersonator.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/impersonation/web/requestbodies/SearchRequest.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/impersonation/web/responsebodies/ImpersonationNotAvailableResponse.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Task.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/ImpersonationConfigurationProperties.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/workflow/domain/Task.javanae-user-ce/src/main/java/com/netgrif/application/engine/auth/domain/Impersonator.javanae-user-ce/src/main/java/com/netgrif/application/engine/auth/repository/ImpersonatorRepository.javanae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/ImpersonationServiceImpl.javanae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.javanae-user-common/src/main/java/com/netgrif/application/engine/auth/service/ImpersonationService.javanae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java
💤 Files with no reviewable changes (20)
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/web/ImpersonationControllerAdvice.java
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/impersonation/web/requestbodies/SearchRequest.java
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/interfaces/IImpersonationAuthorizationService.java
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/exceptions/ImpersonatedUserHasSessionException.java
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/interfaces/IImpersonationSessionService.java
- application-engine/src/main/java/com/netgrif/application/engine/auth/service/AuthorizationService.java
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/web/requestbodies/SearchRequest.java
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/web/responsebodies/ImpersonationNotAvailableResponse.java
- application-engine/src/main/java/com/netgrif/application/engine/auth/service/interfaces/IUserResourceHelperService.java
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationService.java
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/exceptions/IllegalImpersonationAttemptException.java
- application-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.java
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationAuthorizationService.java
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationSessionService.java
- application-engine/src/main/java/com/netgrif/application/engine/auth/service/UserResourceHelperService.java
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/impersonation/web/responsebodies/ImpersonationNotAvailableResponse.java
- application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/web/ImpersonationController.java
- application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/interfaces/IImpersonationService.java
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/impersonation/domain/Impersonator.java
Comment on lines
+143
to
+144
| @Autowired | ||
| ImpersonationService impersonationService |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'impersonationService' --type groovy --type javaRepository: netgrif/application-engine
Length of output: 244
🏁 Script executed:
rg -n '@Autowired' application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy -A 1Repository: netgrif/application-engine
Length of output: 2348
🏁 Script executed:
rg -n 'taskService\.' application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy | head -5Repository: netgrif/application-engine
Length of output: 607
🏁 Script executed:
rg -n 'userService\.' application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy | head -5Repository: netgrif/application-engine
Length of output: 672
Remove the unused ImpersonationService field.
The ImpersonationService is autowired but never used anywhere in the file or the codebase. Other injected services in ActionDelegate (e.g., taskService, userService) are actively used throughout the code, confirming that services are meant to be referenced when injected. This field should be removed as dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`
around lines 143 - 144, Remove the unused autowired field ImpersonationService
from the ActionDelegate class: locate the declaration "ImpersonationService
impersonationService" annotated with `@Autowired` in ActionDelegate and delete
that field and its `@Autowired` annotation so the class no longer contains the
dead dependency; ensure no other references to ImpersonationService remain in
ActionDelegate after removal.
Comment on lines
+1114
to
1154
| AbstractUser assignRole(String roleMongoId, AbstractUser user = userService.loggedUserFromContext) { | ||
| AbstractUser actualUser = userService.addRole(user, roleMongoId) | ||
| return actualUser | ||
| } | ||
|
|
||
| AbstractUser assignRole(String roleId, String netId, AbstractUser user = userService.loggedUser, Pageable pageable = Pageable.unpaged()) { | ||
| AbstractUser assignRole(String roleId, String netId, AbstractUser user = userService.loggedUserFromContext, Pageable pageable = Pageable.unpaged()) { | ||
| List<PetriNet> nets = petriNetService.getByIdentifier(netId, pageable).content | ||
| nets.forEach({ net -> user = assignRole(roleId, net, user) }) | ||
| return user | ||
| } | ||
|
|
||
| AbstractUser assignRole(String roleId, PetriNet net, AbstractUser user = userService.loggedUser) { | ||
| AbstractUser assignRole(String roleId, PetriNet net, AbstractUser user = userService.loggedUserFromContext) { | ||
| AbstractUser actualUser = userService.addRole(user, net.roles.values().find { role -> role.importId == roleId }.stringId) | ||
| return actualUser | ||
| } | ||
|
|
||
| AbstractUser assignRole(String roleId, String netId, Version version, AbstractUser user = userService.loggedUser) { | ||
| AbstractUser assignRole(String roleId, String netId, Version version, AbstractUser user = userService.loggedUserFromContext) { | ||
| PetriNet net = petriNetService.getPetriNet(netId, version) | ||
| return assignRole(roleId, net, user) | ||
| } | ||
|
|
||
| AbstractUser removeRole(String roleMongoId, AbstractUser user = userService.loggedUser) { | ||
| AbstractUser removeRole(String roleMongoId, AbstractUser user = userService.loggedUserFromContext) { | ||
| AbstractUser actualUser = userService.removeRole(user, roleMongoId) | ||
| return actualUser | ||
| } | ||
|
|
||
| AbstractUser removeRole(String roleId, String netId, AbstractUser user = userService.loggedUser, Pageable pageable = Pageable.unpaged()) { | ||
| AbstractUser removeRole(String roleId, String netId, AbstractUser user = userService.loggedUserFromContext, Pageable pageable = Pageable.unpaged()) { | ||
| List<PetriNet> nets = petriNetService.getByIdentifier(netId, pageable).content | ||
| nets.forEach({ net -> user = removeRole(roleId, net, user) }) | ||
| return user | ||
| } | ||
|
|
||
| AbstractUser removeRole(String roleId, PetriNet net, AbstractUser user = userService.loggedUser) { | ||
| AbstractUser removeRole(String roleId, PetriNet net, AbstractUser user = userService.loggedUserFromContext) { | ||
| AbstractUser actualUser = userService.removeRole(user, net.roles.values().find { role -> role.importId == roleId }.stringId) | ||
| return actualUser | ||
| } | ||
|
|
||
| AbstractUser removeRole(String roleId, String netId, Version version, AbstractUser user = userService.loggedUser) { | ||
| AbstractUser removeRole(String roleId, String netId, Version version, AbstractUser user = userService.loggedUserFromContext) { | ||
| PetriNet net = petriNetService.getPetriNet(netId, version) | ||
| return removeRole(roleId, net, user) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find implementations of loggedUserFromContext
rg -n 'def loggedUserFromContext|loggedUserFromContext\s*=' --type groovy --type java -B 2 -A 5
# Find implementations of loggedOrSystem
rg -n 'def loggedOrSystem|getLoggedOrSystem' --type groovy --type java -B 2 -A 5
# Check where these are called from in ActionDelegate context
rg -n 'assignRole|removeRole' application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy | head -20Repository: netgrif/application-engine
Length of output: 50383
🏁 Script executed:
#!/bin/bash
# Find getLoggedUserFromContext implementation
rg -n 'def getLoggedUserFromContext|LoggedUser getLoggedUserFromContext' --type groovy --type java -A 10
# Also search for loggedUserFromContext (method name without get)
rg -n 'loggedUserFromContext\(' --type groovy --type java -B 2 -A 8 | head -60Repository: netgrif/application-engine
Length of output: 2876
Replace loggedUserFromContext with loggedOrSystem for assignRole and removeRole default parameters.
The default parameter changed from userService.loggedOrSystem to userService.loggedUserFromContext in all overloads of assignRole and removeRole (lines 1114-1154). This is a breaking change.
getLoggedUserFromContext() directly casts SecurityContextHolder.getContext().getAuthentication().getPrincipal() to LoggedUser without checking if a user is authenticated. When called outside HTTP request context (e.g., scheduled jobs, event handlers, action scripts), the principal is typically the string "anonymousUser", causing a ClassCastException.
The original getLoggedOrSystem() handles this case by falling back to the system user. Since ActionDelegate processes actions that run in various contexts, not just HTTP requests, use getLoggedOrSystem() as the default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`
around lines 1114 - 1154, In ActionDelegate update all overloads of assignRole
and removeRole to use userService.loggedOrSystem as the default parameter
instead of userService.loggedUserFromContext; specifically change the default
argument in the methods assignRole(String roleMongoId...), assignRole(String
roleId, String netId...), assignRole(String roleId, PetriNet...),
assignRole(String roleId, String netId, Version...), and the corresponding
removeRole overloads so they call userService.loggedOrSystem to avoid
ClassCastException when no HTTP-authenticated principal is present.
Comment on lines
1242
to
1244
| AbstractUser loggedUser() { | ||
| return userService.loggedUser | ||
| return userService.loggedUserFromContext | ||
| } |
There was a problem hiding this comment.
Behavioral change in loggedUser() method.
The return value changed from userService.loggedUser to userService.loggedUserFromContext. This is part of the broader impersonation-aware changes, but action scripts relying on this method may need to be updated if they run outside authenticated contexts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`
around lines 1242 - 1244, The new loggedUser() now returns
userService.loggedUserFromContext which can be null when scripts run outside
authenticated contexts; change loggedUser() to fall back to the original
non‑impersonation call (userService.loggedUser) when loggedUserFromContext is
null and/or update calling action scripts to handle a null principal.
Concretely, in ActionDelegate.loggedUser() return
userService.loggedUserFromContext if present, otherwise return
userService.loggedUser, and audit any action scripts that assumed a non-null
result to guard against null or explicitly require an authenticated context.
Comment on lines
102
to
108
| for (int page = 0; page < numOfPages; page++) { | ||
| log.info("Indexing page {}", page + 1); | ||
| Predicate predicate = searchService.buildQuery(searchBody, user, locale); | ||
| reindexingTask.forceReindexPage(predicate, page, numOfPages); | ||
| if (!user.isProcessAccessDeny()) { | ||
| Predicate predicate = searchService.buildQuery(searchBody, user, locale); | ||
| reindexingTask.forceReindexPage(predicate, page, numOfPages); | ||
| } | ||
| } |
There was a problem hiding this comment.
Move the access check outside the loop to avoid misleading behavior.
The isProcessAccessDeny() check is inside the loop, which causes several issues:
- The loop iterates
numOfPagestimes even when access is denied, doing nothing each iteration - The method logs "Reindexing cases: X pages" and returns "Success" even though no reindexing occurred
- This is both inefficient and potentially misleading to the caller
Suggested fix
if (count == 0) {
log.info("No cases to reindex");
} else {
+ if (user.isProcessAccessDeny()) {
+ log.warn("User [{}] has process access denied, skipping reindex", user.getUsername());
+ return MessageResource.errorMessage("Access denied for reindexing");
+ }
long numOfPages = (count / elasticsearchProperties.getReindexExecutor().getSize()) + 1;
log.info("Reindexing cases: " + numOfPages + " pages");
for (int page = 0; page < numOfPages; page++) {
log.info("Indexing page {}", page + 1);
- if (!user.isProcessAccessDeny()) {
- Predicate predicate = searchService.buildQuery(searchBody, user, locale);
- reindexingTask.forceReindexPage(predicate, page, numOfPages);
- }
+ Predicate predicate = searchService.buildQuery(searchBody, user, locale);
+ reindexingTask.forceReindexPage(predicate, page, numOfPages);
}
}🤖 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/elastic/web/ElasticController.java`
around lines 102 - 108, The loop in ElasticController currently checks
user.isProcessAccessDeny() per page causing wasted iterations and misleading
logs; move the access check outside the for-loop: first verify
user.isProcessAccessDeny(), log and return or skip reindexing if denied (so the
"Reindexing cases: X pages" and "Success" responses accurately reflect work
done), then build the Predicate once via searchService.buildQuery(searchBody,
user, locale) and iterate pages calling
reindexingTask.forceReindexPage(predicate, page, numOfPages) only when access is
allowed.
Comment on lines
+563
to
+566
| Criteria impersonatedProcessesCriteria = getImpersonatedProcessesCriteria(user); | ||
| if (impersonatedProcessesCriteria != null) { | ||
| query.addCriteria(impersonatedProcessesCriteria); | ||
| } |
There was a problem hiding this comment.
Mirror impersonation filters in queryTotal to keep pagination counts correct.
query is filtered, but queryTotal is not, so PageImpl totals can overcount inaccessible processes for impersonated users.
🔧 Suggested fix
Criteria impersonatedProcessesCriteria = getImpersonatedProcessesCriteria(user);
if (impersonatedProcessesCriteria != null) {
query.addCriteria(impersonatedProcessesCriteria);
+ queryTotal.addCriteria(impersonatedProcessesCriteria);
}🤖 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/petrinet/service/PetriNetService.java`
around lines 563 - 566, The pagination total is computed from queryTotal but the
impersonation filter from getImpersonatedProcessesCriteria(user) is only applied
to query, causing PageImpl totals to include inaccessible processes; fix by
applying the same Criteria returned by getImpersonatedProcessesCriteria(user) to
queryTotal when it's non-null (mirror the existing logic that adds the Criteria
to query) so both query and queryTotal use identical filters before executing
counts and fetching results.
| import org.apache.commons.lang3.NotImplementedException; | ||
| import org.springframework.data.domain.Page; | ||
| import org.springframework.data.domain.Pageable; | ||
| import org.springframework.security.access.AccessDeniedException; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused import: AccessDeniedException.
This import is only referenced in method throws declarations but is never actually thrown—all methods throw NotImplementedException instead. While harmless, consider removing it to keep imports clean.
🧹 Proposed fix
-import org.springframework.security.access.AccessDeniedException;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/ImpersonationServiceImpl.java`
at line 9, Remove the unused import AccessDeniedException from
ImpersonationServiceImpl and clean up any method signatures in that class that
declare "throws AccessDeniedException" since the methods actually throw
NotImplementedException; update the method declarations (in
ImpersonationServiceImpl) to remove the unnecessary throws clause and run a
build to ensure no other references remain.
Comment on lines
+13
to
+14
| @Slf4j | ||
| public class ImpersonationServiceImpl implements ImpersonationService { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how ImpersonationServiceImpl is registered as a Spring bean.
# Search for `@Bean` method or configuration that instantiates ImpersonationServiceImpl
echo "=== Searching for bean registration of ImpersonationServiceImpl ==="
rg -n "ImpersonationServiceImpl" --type java -g '!*Test*' -g '!*test*'
echo ""
echo "=== Checking for `@Configuration` classes that might register this bean ==="
ast-grep --pattern $'@Configuration
$$$
class $_ {
$$$
ImpersonationService $$$
$$$
}'
echo ""
echo "=== Also checking for `@Bean` method declarations for ImpersonationService ==="
rg -n "@Bean" --type java -A 3 | grep -A 3 "ImpersonationService"Repository: netgrif/application-engine
Length of output: 465
🏁 Script executed:
#!/bin/bash
# Read the full class file from the beginning to see all annotations
echo "=== Full ImpersonationServiceImpl.java (first 30 lines) ==="
cat -n nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/ImpersonationServiceImpl.java | head -30
echo ""
echo "=== Checking for any conditional registration or auto-configuration ==="
rg -n "@ConditionalOnProperty|@ConditionalOnClass|@Conditional" --type java | grep -i impersonationRepository: netgrif/application-engine
Length of output: 2056
🏁 Script executed:
#!/bin/bash
# Search for EE version (if it exists) to see if it has `@Service`
echo "=== Searching for EE version of ImpersonationServiceImpl ==="
find . -name "ImpersonationServiceImpl.java" -type f | grep -v test
echo ""
echo "=== Checking for configuration class in nae-user-ce that might register beans ==="
find nae-user-ce -name "*Configuration*.java" -o -name "*Config*.java" 2>/dev/null | head -10
echo ""
echo "=== Checking if there's a BeanFactory or similar registration mechanism in CE module ==="
rg -n "ImpersonationService" --type java nae-user-ce/ | head -20Repository: netgrif/application-engine
Length of output: 677
🏁 Script executed:
#!/bin/bash
# Read the AuthBeansConfiguration file to check for bean registration
echo "=== AuthBeansConfiguration.java contents ==="
cat nae-user-ce/src/main/java/com/netgrif/application/engine/auth/config/AuthBeansConfiguration.java
echo ""
echo "=== Search for ImpersonationService registration in the config file ==="
grep -n "ImpersonationService" nae-user-ce/src/main/java/com/netgrif/application/engine/auth/config/AuthBeansConfiguration.javaRepository: netgrif/application-engine
Length of output: 1459
Register this bean in AuthBeansConfiguration.java.
The class lacks a @Service annotation and is not registered in the auth configuration. Add a @Bean method to AuthBeansConfiguration.java following the pattern used for other services (with @ConditionalOnMissingBean), or add @Service directly to the class. Without this, the bean won't be available for dependency injection.
Additionally:
- Unused import:
AccessDeniedException(line 9) is declared in method throws clauses but never actually thrown—remove it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/ImpersonationServiceImpl.java`
around lines 13 - 14, ImpersonationServiceImpl is not registered as a Spring
bean and still imports AccessDeniedException which isn’t used; either annotate
the class ImpersonationServiceImpl with `@Service` (preferred) or add a `@Bean`
method in AuthBeansConfiguration that returns new ImpersonationServiceImpl() and
is annotated `@ConditionalOnMissingBean` following the pattern of other services
so it becomes injectable, and while editing remove the unused import of
AccessDeniedException and strip it from any method throws clauses where it’s not
actually thrown.
Comment on lines
+16
to
+20
| @Override | ||
| public Impersonation getImpersonation(String impersonationId, LoggedUser loggedUser) throws IllegalArgumentException, AccessDeniedException { | ||
| log.error("Trying to call getImpersonation(String impersonationId, LoggedUser loggedUser) in community edition."); | ||
| throw new NotImplementedException("Impersonation not implemented in community edition."); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using log.warn instead of log.error for expected CE behavior.
Since impersonation is intentionally not implemented in the community edition, calls to these methods represent expected limitations rather than application errors. Using log.error may create noise in production logs and trigger unnecessary alerts.
A log.warn level would more accurately reflect the nature of this condition—a feature limitation—while still providing visibility.
♻️ Example change (apply to all methods)
`@Override`
public Impersonation getImpersonation(String impersonationId, LoggedUser loggedUser) throws IllegalArgumentException, AccessDeniedException {
- log.error("Trying to call getImpersonation(String impersonationId, LoggedUser loggedUser) in community edition.");
+ log.warn("Trying to call getImpersonation(String impersonationId, LoggedUser loggedUser) in community edition.");
throw new NotImplementedException("Impersonation not implemented in community edition.");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Override | |
| public Impersonation getImpersonation(String impersonationId, LoggedUser loggedUser) throws IllegalArgumentException, AccessDeniedException { | |
| log.error("Trying to call getImpersonation(String impersonationId, LoggedUser loggedUser) in community edition."); | |
| throw new NotImplementedException("Impersonation not implemented in community edition."); | |
| } | |
| `@Override` | |
| public Impersonation getImpersonation(String impersonationId, LoggedUser loggedUser) throws IllegalArgumentException, AccessDeniedException { | |
| log.warn("Trying to call getImpersonation(String impersonationId, LoggedUser loggedUser) in community edition."); | |
| throw new NotImplementedException("Impersonation not implemented in community edition."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/ImpersonationServiceImpl.java`
around lines 16 - 20, The current implementation of
ImpersonationServiceImpl.getImpersonation logs an expected community-edition
limitation at error level; change the logging call from log.error to log.warn
(preserving the existing message) so that feature-absence is recorded as a
warning not an error, and apply the same change to all other methods in
ImpersonationServiceImpl that log this "not implemented in community edition"
condition (keep throwing NotImplementedException as before).
Comment on lines
+399
to
400
| AbstractUser user = this.findById(loggedUser.getSelfOrImpersonatedStringId(), loggedUser.getRealmId()); | ||
| BooleanExpression predicate = buildPredicate(user, query); |
There was a problem hiding this comment.
Guard against null effective user before predicate construction.
If findById(...) returns null, buildPredicate(user, query) will throw at Line [400] / Line [413]. In impersonation flows, stale/deleted effective users should fail with a controlled error, not a 500.
💡 Proposed fix
@@
- AbstractUser user = this.findById(loggedUser.getSelfOrImpersonatedStringId(), loggedUser.getRealmId());
+ AbstractUser user = requireCoMemberBaseUser(loggedUser);
BooleanExpression predicate = buildPredicate(user, query);
@@
- AbstractUser user = this.findById(loggedUser.getSelfOrImpersonatedStringId(), loggedUser.getRealmId());
+ AbstractUser user = requireCoMemberBaseUser(loggedUser);
BooleanExpression predicate = buildPredicate(user, query);
@@
+ private AbstractUser requireCoMemberBaseUser(LoggedUser loggedUser) {
+ AbstractUser user = this.findById(loggedUser.getSelfOrImpersonatedStringId(), loggedUser.getRealmId());
+ if (user == null) {
+ throw new IllegalStateException(
+ "Unable to resolve effective user [%s] in realm [%s]"
+ .formatted(loggedUser.getSelfOrImpersonatedStringId(), loggedUser.getRealmId())
+ );
+ }
+ return user;
+ }Also applies to: 412-413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java`
around lines 399 - 400, UserServiceImpl currently calls findById(...) and
immediately passes the result to buildPredicate(...) which will NPE if the
effective user is missing; add a null-check after AbstractUser user =
this.findById(loggedUser.getSelfOrImpersonatedStringId(),
loggedUser.getRealmId()) and if user is null throw a controlled exception (e.g.,
a NotFound/BadRequest or custom AuthenticationException) with a clear message
about the impersonated/stale user, so buildPredicate(user, query) is never
called with null; apply the same guard to the other occurrence around lines
412-413 where findById result is used.
Comment on lines
+31
to
+37
| LoggedUser startImpersonation(String impersonationId) throws IllegalArgumentException, AccessDeniedException; | ||
|
|
||
| LoggedUser startImpersonation(String impersonationId, LoggedUser loggedUser) throws IllegalArgumentException, AccessDeniedException; | ||
|
|
||
| LoggedUser endImpersonation() throws IllegalArgumentException; | ||
|
|
||
| LoggedUser endImpersonation(LoggedUser impersonator) throws IllegalArgumentException; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding Javadoc to clarify method semantics.
The overloaded methods for startImpersonation and endImpersonation (with and without LoggedUser parameter) could benefit from documentation explaining:
- When to use each variant (e.g., controller vs. service layer)
- How the no-argument versions resolve the current user (presumably from
SecurityContextHolder)
This will help consumers of the interface choose the correct method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/ImpersonationService.java`
around lines 31 - 37, The interface ImpersonationService methods lack Javadoc
explaining the semantics of the overloaded variants; add Javadoc to the
interface and to each method (startImpersonation(String),
startImpersonation(String, LoggedUser), endImpersonation(),
endImpersonation(LoggedUser)) that states when to use the no-arg vs. LoggedUser
variants (e.g., controller code should call the LoggedUser-taking overload when
the caller is already resolved, service or helper code may call the no-arg which
resolves the current user from SecurityContextHolder), documents the resolution
behavior for the no-arg methods (explicitly mention SecurityContextHolder or
equivalent), and describes the conditions under which IllegalArgumentException
or AccessDeniedException are thrown; include references to the method names in
the Javadoc so callers can easily choose the correct overload.
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
1 participant
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
Implements user Impersonation basics, which are not supported in community edition. Includes Null Impersonation service, which is provided in ActionDelegate. Changes to code, which returns and works with LoggedUser, to support impersonation in enterprise edition. Changed User objects and improve methods for impersonation.
Implements NAE-2273
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?
Tested manually
Checklist:
Summary by CodeRabbit
Release Notes
Refactor
AbstractUsertoLoggedUserthroughout the platform for improved type consistency.Features
Chores