[NAE-2169] User flag for internal or external #360
Conversation
- added UserType enum - added type parameter to AbstractUser - added list of domains to Realm
- UserService type refactor, now works with User instead of AbstractUser - slight refactor related to above mentioned change - added UpdateUserRequest to nae-user-common - added anonymous user type enum value to UserType
# Conflicts: # application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java # nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
- removed duplicate class UpdateUserRequest.java - moved user type resolution from createUser method to saveUser method in UserServiceImpl.java
SamuelPalaj
requested review from
Retoocs,
machacjozef,
renczesstefan and
tuplle
WalkthroughMigrates public APIs and internal usages from AbstractUser to concrete User across services, controllers, and adapters. Introduces UserType enum and propagates it through domain, logged user, DTOs, and constructors. Adds UpdateUserRequest-based update methods, realm domains handling, and adjusts related authorization/impersonation and role/group services accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as UserController/API
participant USvc as UserService
participant Repo as UserRepository
participant Realm as RealmService
Client->>API: PATCH /users/{id} (UpdateUserRequest)
API->>USvc: update(userId, realmId, UpdateUserRequest)
USvc->>Repo: findById(userId, realmId)
Repo-->>USvc: User
USvc->>USvc: resolveUserUpdateValue(...) per field
USvc->>Realm: resolve realm (if needed)
Realm-->>USvc: Realm
USvc->>Repo: save(User with updated fields/type)
Repo-->>USvc: User
USvc-->>API: User
API-->>Client: 200 OK (User DTO with type)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-09-04T11:09:31.264ZApplied to files:
📚 Learning: 2025-09-04T11:09:31.264ZApplied to files:
📚 Learning: 2025-09-04T11:09:31.264ZApplied to files:
🧬 Code graph analysis (1)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
added
improvement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (34)
application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerExecutor.java (1)
53-56: Avoid resolving class twice; hoistrunnerClass.Minor cleanup to compute once and reuse in catch.
- protected void callRunner(T runner, ApplicationRunnerOrderResolver.SortedRunners<T> runners, ApplicationArguments args) throws Exception { - try { - Class<?> runnerClass = resolveClass(runner); + protected void callRunner(T runner, ApplicationRunnerOrderResolver.SortedRunners<T> runners, ApplicationArguments args) throws Exception { + Class<?> runnerClass = resolveClass(runner); + try { if (runners.getReplaced().containsKey(runnerClass)) { log.warn("Runner {} is starting that replaced runner {}", runnerClass.getSimpleName(), runners.getReplaced().get(runnerClass).getSimpleName()); } else { log.info("Runner {} is starting", runnerClass.getSimpleName()); } executeRunner(runner, args); log.info("Runner {} has ended", runnerClass.getSimpleName()); } catch (Exception e) { - Class<?> runnerClass = resolveClass(runner); if (runnerClass.isAnnotationPresent(OptionalRunner.class)) { log.error("Runner {} has failed. Runner is flagged as optional, execution will continue", runnerClass.getSimpleName(), e); } else { log.error("Runner {} has failed", runnerClass.getSimpleName(), e); throw new RuntimeException(e); } } }Also applies to: 63-66
application-engine/src/main/java/com/netgrif/application/engine/auth/web/AuthenticationController.java (1)
196-216: Missing subject check allows cross-account password changeAny authenticated user can change another user’s password if they know the current password. Enforce “self or admin” and guard null auth.
Apply:
- Optional<User> user = userService.findUserByUsername(request.login, null); + Optional<User> user = userService.findUserByUsername(request.login, null); if (user.isEmpty() || request.password == null || request.newPassword == null) { return MessageResource.errorMessage("Incorrect login!"); } + if (auth == null || !(auth.getPrincipal() instanceof LoggedUser principal)) { + return MessageResource.errorMessage("Not authenticated!"); + } + String newPassword = new String(Base64.getDecoder().decode(request.newPassword)); if (!registrationService.isPasswordSufficient(newPassword)) { return MessageResource.errorMessage("Insufficient password!"); } String password = new String(Base64.getDecoder().decode(request.password)); - if (registrationService.stringMatchesUserPassword(user.get(), password)) { - registrationService.changePassword((user.get()), newPassword); - securityContextService.saveToken(((LoggedUser) auth.getPrincipal()).getStringId()); - securityContextService.reloadSecurityContext((LoggedUser) auth.getPrincipal()); + User target = user.get(); + if (!principal.isAdmin() && !principal.getStringId().equals(target.getStringId())) { + return MessageResource.errorMessage("Forbidden!"); + } + if (registrationService.stringMatchesUserPassword(target, password)) { + registrationService.changePassword(target, newPassword); + if (principal.getStringId().equals(target.getStringId())) { + securityContextService.saveToken(principal.getStringId()); + securityContextService.reloadSecurityContext(principal); + } } else { return MessageResource.errorMessage("Incorrect password!"); }application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (3)
630-642: Fix immutable List mutation: List.of(...) causes runtime UnsupportedOperationExceptionBoth appendChildCaseIdInDataSet and appendChildCaseIdInMemory create immutable lists via List.of(...) and later mutate them. This will blow up on the next append.
Apply this diff:
@@ - protected void appendChildCaseIdInDataSet(Case folderCase, String childItemCaseId, Map<String, Map<String, Object>> dataSet) { - List<String> childIds = MenuItemUtils.getCaseIdsFromCaseRef(folderCase, MenuItemConstants.FIELD_CHILD_ITEM_IDS); - if (childIds == null || childIds.isEmpty()) { - childIds = List.of(childItemCaseId); - } else { - childIds.add(childItemCaseId); - } + protected void appendChildCaseIdInDataSet(Case folderCase, String childItemCaseId, Map<String, Map<String, Object>> dataSet) { + List<String> existing = MenuItemUtils.getCaseIdsFromCaseRef(folderCase, MenuItemConstants.FIELD_CHILD_ITEM_IDS); + List<String> childIds = new ArrayList<>(existing == null ? Collections.emptyList() : existing); + childIds.add(childItemCaseId); @@ - protected void appendChildCaseIdInMemory(Case folderCase, String childItemCaseId) { - List<String> childIds = MenuItemUtils.getCaseIdsFromCaseRef(folderCase, MenuItemConstants.FIELD_CHILD_ITEM_IDS); - if (childIds == null || childIds.isEmpty()) { - folderCase.getDataField(MenuItemConstants.FIELD_CHILD_ITEM_IDS).setValue(List.of(childItemCaseId)); - } else { - childIds.add(childItemCaseId); - folderCase.getDataField(MenuItemConstants.FIELD_CHILD_ITEM_IDS).setValue(childIds); - } + protected void appendChildCaseIdInMemory(Case folderCase, String childItemCaseId) { + List<String> existing = MenuItemUtils.getCaseIdsFromCaseRef(folderCase, MenuItemConstants.FIELD_CHILD_ITEM_IDS); + List<String> childIds = new ArrayList<>(existing == null ? Collections.emptyList() : existing); + childIds.add(childItemCaseId); + folderCase.getDataField(MenuItemConstants.FIELD_CHILD_ITEM_IDS).setValue(childIds); folderCase.getDataField(MenuItemConstants.FIELD_HAS_CHILDREN).setValue(MenuItemUtils.hasFolderChildren(folderCase)); }Also applies to: 645-653
239-244: Use the constant for process identifier; remove leftover TODOHardcoding "menu_item" is brittle and inconsistent with the rest of the class.
@Override public boolean existsMenuItem(String identifier) { - //TODO mongo - Case menuItem = this.workflowService.searchOne(QCase.case$.processIdentifier.eq("menu_item") + Case menuItem = this.workflowService.searchOne(QCase.case$.processIdentifier + .eq(MenuProcessRunner.MENU_NET_IDENTIFIER) .and(QCase.case$.dataSet.get(MenuItemConstants.FIELD_IDENTIFIER).value.eq(identifier))); return menuItem != null;
693-701: nameFromPath returns a leading slash for nested pathsFor "/a/b" it returns "/b". Likely the intent is "b".
- protected String nameFromPath(String path) { + protected String nameFromPath(String path) { if (path == null || path.equals(MenuItemConstants.PATH_SEPARATOR) || path.isEmpty()) { return ""; } - if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) { - return path.replace(MenuItemConstants.PATH_SEPARATOR, ""); - } - return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR)); + int idx = path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR); + if (idx == 0) return path.substring(1); + return path.substring(idx + 1); }application-engine/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/UserResourceAssembler.java (1)
32-39: Exception message and unusedsmallflag
- Message still says “toResource”; this class exposes toModel.
- The initialized ‘small’ flag is never used.
- throw new IllegalStateException("You must initialize the UserResourceAssembler before calling the toResource method! To initialize the assembler call the initialize method."); + throw new IllegalStateException("You must initialize the UserResourceAssembler before calling the toModel method! Call initialize(locale, small, selfRel) first.");If
smallshould affect projection, consider threading it intouserFactory.getUser(...)or theUserResourceconstructor.application-engine/src/main/java/com/netgrif/application/engine/auth/web/PublicUserController.java (1)
56-71: Align getLoggedUser to domain UserStay consistent with the migration away from AbstractUser.
- AbstractUser user; + com.netgrif.application.engine.objects.auth.domain.User user; try { user = userService.findById(loggedUser.getStringId(), loggedUser.getRealmId());Update imports accordingly.
application-engine/src/main/java/com/netgrif/application/engine/impersonation/web/ImpersonationController.java (1)
69-77: Use User.class in ResourceLinkAssembler.addLinksLinks are being added under AbstractUser.class while the page now holds User. This can break link assembly or produce inconsistent rels.
- ResourceLinkAssembler.addLinks(resources, AbstractUser.class, selfLink.getRel().toString()); + ResourceLinkAssembler.addLinks(resources, User.class, selfLink.getRel().toString());nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/LoggedUserImpl.java (1)
24-25: Remove Lombok @AllArgsConstructor to avoid generating an invalid no‑arg ctorThis class declares no fields; @AllArgsConstructor will generate a no‑arg constructor calling super(), which may not exist on LoggedUser and can break compilation or allow unintended construction paths. Rely on the explicit ctors only.
Apply:
-import lombok.AllArgsConstructor; @@ -@AllArgsConstructor public class LoggedUserImpl extends LoggedUser implements UserDetails {nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java (1)
19-21: Remove Lombok @AllArgsConstructor to prevent an incomplete ctor without id/realmId@AllArgsConstructor here generates a constructor for this class’s fields only (not id/realmId in the superclass), competing with explicit ctors and enabling partially‑initialized instances in subclasses.
Apply:
-import lombok.AllArgsConstructor; @@ -@AllArgsConstructor @EqualsAndHashCode(callSuper = true) public abstract class AbstractUser extends AbstractActor {application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.java (3)
219-230: Missing authorization guard on role assignment endpointEndpoint allows assigning roles without @PreAuthorize; spec says ADMIN required. Add explicit guard.
Apply:
- public ResponseEntity<ResponseMessage> assignRolesToUser(@PathVariable("realmId") String realmId, @PathVariable("id") String userId, @RequestBody Set<String> roleIds, Authentication auth) { + @PreAuthorize("@authorizationService.hasAuthority('ADMIN')") + public ResponseEntity<ResponseMessage> assignRolesToUser(@PathVariable("realmId") String realmId, @PathVariable("id") String userId, @RequestBody Set<String> roleIds, Authentication auth) {
278-287: Success/error messages are swappedOn failure you return a “success” message; on success you return an “error” message. Swap factories.
Apply:
- } catch (IllegalArgumentException e) { - log.error("Assigning authority to user [{}] has failed!", userId, e); - return ResponseEntity.badRequest().body(ResponseMessage.createSuccessMessage("Assigning authority to user " + userId + " has failed!")); - } - return ResponseEntity.ok(ResponseMessage.createErrorMessage("Authority was assigned to user successfully")); + } catch (IllegalArgumentException e) { + log.error("Assigning authority to user [{}] has failed!", userId, e); + return ResponseEntity.badRequest().body(ResponseMessage.createErrorMessage("Assigning authority to user " + userId + " has failed!")); + } + return ResponseEntity.ok(ResponseMessage.createSuccessMessage("Authority was assigned to user successfully"));
161-164: Use 403 Forbidden instead of 401 Unauthorized for authorization failuresThe caller is authenticated but not allowed to access another user; 403 is the correct status.
Apply:
- return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); + return ResponseEntity.status(HttpStatus.FORBIDDEN).build();application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (2)
314-319: Bug: persisting with null realm.Calling saveUser(user, null) will overwrite user.realmId with null in UserServiceImpl, risking collection routing issues.
Apply:
- userService.saveUser(user, null); + userService.saveUser(user);
210-241: Remove unused private helpers in ProcessRoleServiceupdateRequestedRoles(...) and getPetriNetIdRoleBelongsTo/getPetriNetIdFromFirstRole are unused — ripgrep shows only their definitions in application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (lines 210–241). Delete them to remove dead/duplicated code.
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java (4)
190-206: Lookup loop works; consider direct query.Current paging + filter is O(n). If feasible, add repository method findByOwnerIdAndIdentifier(ownerId, username) to avoid scanning.
247-253: Mirror the realm guard for removal; consider atomicity.- public Group removeUser(User user, Group group) { + // @Transactional + public Group removeUser(User user, Group group) { + if (!Objects.equals(user.getRealmId(), group.getRealmId())) { + throw new IllegalArgumentException( + "User [%s] and Group [%s] belong to different realms".formatted(user.getStringId(), group.getStringId())); + } log.info("Removing user [{}] from group [{}]", user.getStringId(), group.getStringId()); user.removeGroupId(group.getStringId()); group.removeMemberId(user.getStringId()); userService.saveUser(user, user.getRealmId()); return save(group); }
385-392: Use exact match for realmId to avoid slow/incorrect matches.realmId is an identifier; regex "i" is unnecessary and can mis-match.
- if (searchDto.getRealmId() != null && !searchDto.getRealmId().isBlank()) { - filters.add(Criteria.where("realmId").regex(searchDto.getRealmId(), "i")); - } + if (searchDto.getRealmId() != null && !searchDto.getRealmId().isBlank()) { + filters.add(Criteria.where("realmId").is(searchDto.getRealmId())); + }
232-238: Enforce same‑realm and make addUser atomic
- Add a realm check at the start of addUser(User, Group) and make the user+group saves atomic. Location: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java — addUser(User, Group).
- Repo scan: @transactional is used elsewhere but no MongoTransactionManager or explicit Mongo transaction bean was found — verify Mongo multi‑document transactions (MongoTransactionManager + replica set) before relying on @transactional; if not available, use MongoTemplate.executeInTransaction/TransactionTemplate or a compensating update.
Suggested change (small diff):
- public Group addUser(User user, Group group) { + // Optionally annotate if Mongo transactions are enabled + // @Transactional + public Group addUser(User user, Group group) { + if (!Objects.equals(user.getRealmId(), group.getRealmId())) { + throw new IllegalArgumentException( + "User [%s] and Group [%s] belong to different realms".formatted(user.getStringId(), group.getStringId())); + } log.info("Adding user [{}] to group [{}]", user.getStringId(), group.getStringId()); user.addGroupId(group.getStringId()); group.addMemberId(user.getStringId()); userService.saveUser(user, user.getRealmId()); return save(group); }If using @transactional, also add: import org.springframework.transaction.annotation.Transactional;
application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationAuthorizationService.java (1)
53-66: Pagination total is wrong — totalElements must count unique impersonated users, not casesYou dedupe user IDs from cases.getContent() but still pass cases.getTotalElements() (total cases) to PageImpl; that makes paging/total incorrect. Compute the total number of DISTINCT impersonated user IDs for the same filters and use that as the PageImpl total (e.g., DB DISTINCT/count or an ES cardinality aggregation). Temporary fallback: users.size() will only reflect the current page and is not a correct global total.
File: application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationAuthorizationService.java (lines 53–66)
application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java (9)
54-59: Use the all-realms removal API; current call likely only affects the default realmAvoid passing
nullrealmId into a single‑realm method. Use the multi‑realm variant to actually sweep all realms.Apply this diff:
- public void removeExpiredUsers() { + public void removeExpiredUsers() { log.info("Removing expired unactivated invited users"); - userService.removeAllByStateAndExpirationDateBefore(UserState.INACTIVE, LocalDateTime.now(),null); + userService.removeAllByStateAndExpirationDateBeforeForRealms(UserState.INACTIVE, LocalDateTime.now(), null); }
89-94: Enforce password policy in changePasswordValidate strength before encoding.
Apply:
public void changePassword(User user, String newPassword) { - user.setPassword(newPassword); + if (!isPasswordSufficient(newPassword)) { + throw new IllegalArgumentException("Password does not meet minimal requirements"); + } + user.setPassword(newPassword); encodeUserPassword(user); userService.saveUser(user, null); log.info("Changed password for user " + user.getEmail() + "."); }
97-106: Don’t log raw tokens; guard against null expiration to avoid NPELogging secrets is unsafe. Also,
getExpirationDate()can be null.Apply:
public boolean verifyToken(String token) { try { - log.info("Verifying token:" + token); - String[] tokenParts = decodeToken(token); - User user = userService.findByEmail(tokenParts[0], null); - return user != null && Objects.equals(user.getToken(), tokenParts[1]) && user.getExpirationDate().isAfter(LocalDateTime.now()); + String[] tokenParts = decodeToken(token); + log.debug("Verifying token for {}", tokenParts[0]); + User user = userService.findByEmail(tokenParts[0], null); + return user != null + && Objects.equals(user.getToken(), tokenParts[1]) + && user.getExpirationDate() != null + && user.getExpirationDate().isAfter(LocalDateTime.now()); } catch (InvalidUserTokenException e) { log.error(e.getMessage()); return false; } }
110-116: Consider enforcing minimal password length here as wellConsistent enforcement protects other call sites.
public void encodeUserPassword(User user) { String pass = user.getPassword(); if (pass == null) { throw new IllegalArgumentException("User has no password"); } + if (!isPasswordSufficient(pass)) { + throw new IllegalArgumentException("Password does not meet minimal requirements"); + } user.setPassword(passwordEncoder.encode(pass)); }
144-149: Avoid redundant cast and premature save inside addRole path
(User)cast onsaveUserreturn is redundant.- Calling
addRolebefore the first save forces a save withrealmId == null.Prefer saving first, then assigning default role; or at least drop the cast.
- userService.addRole(user, processRoleService.getDefaultRole().getStringId()); - user = (User) userService.saveUser(user, null); + user = userService.saveUser(user, null); + user = userService.addRole(user, processRoleService.getDefaultRole().getStringId());If changing the order is risky for this release, at minimum:
- user = (User) userService.saveUser(user, null); + user = userService.saveUser(user, null);
160-177: Critical: registerUser persists raw passwordMust hash before saving; also enforce policy.
public User registerUser(RegistrationRequest registrationRequest) throws InvalidUserTokenException { String email = decodeToken(registrationRequest.token)[0]; log.info("Registering user " + email); User user = userService.findByEmail(email, null); if (user == null) { return null; } user.setFirstName(registrationRequest.name); user.setLastName(registrationRequest.surname); - user.setPassword(registrationRequest.password); + if (!isPasswordSufficient(registrationRequest.password)) { + throw new IllegalArgumentException("Password does not meet minimal requirements"); + } + user.setPassword(registrationRequest.password); + encodeUserPassword(user); user.setToken(StringUtils.EMPTY); user.setExpirationDate(null); user.setState(UserState.ACTIVE); return userService.saveUser(user, null); }
180-194: Reset flow: avoid(User)cast and consider disabling vs nulling password
findByEmailalready returnsUser; the cast is unnecessary.- Setting password to
nullcan break password matches in unrelated code paths. Consider a sentinel like"N/A"or a dedicated credential flag.- User user = (User) userService.findByEmail(email, null); + User user = userService.findByEmail(email, null); @@ - user.setPassword(null); + user.setPassword(null); // consider sentinel or credential flag instead of null
197-210: Recover flow: enforce password policy before encode/saveSame policy as register/change.
public User recover(String email, String newPassword) { log.info("Recovering user " + email); User user = userService.findByEmail(email, null); if (user == null) { return null; } user.setState(UserState.ACTIVE); - user.setPassword(newPassword); + if (!isPasswordSufficient(newPassword)) { + throw new IllegalArgumentException("Password does not meet minimal requirements"); + } + user.setPassword(newPassword); encodeUserPassword(user); user.setToken(null); user.setExpirationDate(null); return userService.saveUser(user, null); }
229-235: Decode with explicit charsetBe explicit to avoid platform default differences.
- String decodedString = new String(decodedBytes); + String decodedString = new String(decodedBytes, java.nio.charset.StandardCharsets.UTF_8);nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java (2)
415-418: Typo: singular type, plural name
realmIdsis aStringhere. Rename torealmId.- Page<User> findAllByStateAndExpirationDateBefore(UserState state, LocalDateTime expirationDate, String realmIds, Pageable pageable); + Page<User> findAllByStateAndExpirationDateBefore(UserState state, LocalDateTime expirationDate, String realmId, Pageable pageable);
24-44: Add an all-realms finder to support scheduled maintenance
RegistrationService.resetExpiredToken()needs a cross‑realm cursor. Propose adding:+ /** + * Finds users across all realms by state and expiration threshold. + */ + Page<User> findAllByStateAndExpirationDateBeforeForAllRealms(UserState state, LocalDateTime expirationDate, Pageable pageable);And implement in the CE module by iterating
collectionNameProvider.getCollectionNamesForAllRealm().nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (3)
193-205: Double semicolon and redundant double-save
- Remove the stray
;;.- You save the user twice in
createUser(User, String). If the second save is intentional (post group ops), add a brief comment; otherwise drop the first save.- user = this.saveUser(user, realmId);; + user = this.saveUser(user, realmId); @@ - user = this.saveUser(user, realmId); + // Re-save only if group operations mutated the user; otherwise this is redundant. + user = this.saveUser(user, realmId);If redundant, remove the first save and move
setPasswordbefore the single save as needed.
151-159: Remove redundant Optional.mapNo-op
.map(user -> user).- Optional<User> userOpt = userRepository.findByUsername(username, mongoTemplate, collectionName).map(user -> user); + Optional<User> userOpt = userRepository.findByUsername(username, mongoTemplate, collectionName);
631-637: Unnecessary cast and missing null check in updateAdminWithRoles
findByEmailalready returnsUser; also guard for missing admin.- User admin = (User) findByEmail(UserConstants.ADMIN_USER_EMAIL, null); + User admin = findByEmail(UserConstants.ADMIN_USER_EMAIL, null); + if (admin == null) { + throw new IllegalStateException("Admin user not found"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (46)
application-engine/pom.xml(2 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/integration/modules/ModuleServiceInjector.groovy(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java(9 hunks)application-engine/src/main/java/com/netgrif/application/engine/auth/service/interfaces/IRegistrationService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/auth/web/AuthenticationController.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/auth/web/PublicUserController.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/UserResourceAssembler.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationAuthorizationService.java(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationService.java(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/interfaces/IImpersonationAuthorizationService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/interfaces/IImpersonationService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/impersonation/web/ImpersonationController.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerExecutor.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseEventHandler.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MongoSearchService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IWorkflowService.java(1 hunks)application-engine/src/test/java/com/netgrif/application/engine/MockService.java(2 hunks)nae-object-library/pom.xml(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java(4 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/LoggedUser.java(3 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Realm.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/enums/UserType.java(1 hunks)nae-spring-core-adapter/pom.xml(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/AnonymousUser.java(2 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/LoggedUserImpl.java(3 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/service/ProcessRoleService.java(2 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/utils/NaeReflectionUtils.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/workflow/domain/Case.java(1 hunks)nae-user-ce/pom.xml(1 hunks)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java(9 hunks)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java(2 hunks)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(24 hunks)nae-user-common/pom.xml(1 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java(7 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/RealmService.java(0 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java(27 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/requestbodies/UpdateUserRequest.java(3 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java(3 hunks)pom.xml(1 hunks)
💤 Files with no reviewable changes (1)
- nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/RealmService.java
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-31T08:05:21.587Z
Learnt from: renczesstefan
PR: netgrif/application-engine#329
File: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java:39-39
Timestamp: 2025-07-31T08:05:21.587Z
Learning: In the Netgrif Application Engine, the domain Realm class (com.netgrif.application.engine.objects.auth.domain.Realm) is abstract and cannot be instantiated. The concrete implementation is in the adapter package (com.netgrif.application.engine.adapter.spring.auth.domain.Realm) which extends the abstract domain class. Services must instantiate the concrete adapter class and cast it when saving to repositories that accept the abstract domain type.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Realm.javanae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In the SuperCreatorRunner.java file, setPassword() sets the same credential as setCredential("password", passwordCredential), so there's no need to call both methods when creating the super user.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser(), and both setPassword() and setCredential() ultimately reference the same credential system.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java
🧬 Code graph analysis (7)
application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerExecutor.java (1)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/utils/NaeReflectionUtils.java (1)
NaeReflectionUtils(8-57)
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/constants/UserConstants.java (1)
UserConstants(3-11)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseEventHandler.java (2)
application-engine/src/main/java/com/netgrif/application/engine/importer/service/FieldFactory.java (1)
org(31-862)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/workflow/domain/Case.java (1)
Document(19-109)
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java (1)
MenuItemConstants(7-39)
application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java (1)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/utils/NaeReflectionUtils.java (1)
NaeReflectionUtils(8-57)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java (3)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java (1)
Getter(21-311)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.java (1)
Getter(17-172)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
Slf4j(39-719)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (4)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/constants/UserConstants.java (1)
UserConstants(3-11)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java (1)
Getter(21-311)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.java (1)
Getter(17-172)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Authority.java (1)
Getter(17-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (53)
pom.xml (1)
9-9: Root version bump acknowledged.
Downstream POMs appear updated; keep jackson.version consistent with module usages (see comments in module POMs).nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java (1)
46-54: Constructor visibility tightened — good encapsulation.
Protected ctors prevent external instantiation while keeping subclass access. No concerns.nae-user-ce/pom.xml (1)
9-9: Parent bump verified — all POMs use 7.0.0-RC7.1
Search output shows a single parent version (7.0.0-RC7.1) across the repo; no action required.nae-object-library/pom.xml (1)
10-10: Align jackson-datatype-jsr310 to parent-managed ${jackson.version}Two modules explicitly pin jsr310 to 2.18.2 — switch them to ${jackson.version} for consistency.
Locations: nae-user-common/pom.xml (lines 98–99), nae-object-library/pom.xml (lines 78–79)
Apply:
- <dependency> - <groupId>com.fasterxml.jackson.datatype</groupId> - <artifactId>jackson-datatype-jsr310</artifactId> - <version>2.18.2</version> - <scope>provided</scope> - </dependency> + <dependency> + <groupId>com.fasterxml.jackson.datatype</groupId> + <artifactId>jackson-datatype-jsr310</artifactId> + <version>${jackson.version}</version> + <scope>provided</scope> + </dependency>Likely an incorrect or invalid review comment.
nae-spring-core-adapter/pom.xml (1)
10-10: Version alignment confirmed for adapter; application-engine not verifiablenae-spring-core-adapter/pom.xml (parent) contains 7.0.0-RC7.1. No application-engine parent POM was found in the repo to confirm the downstream bump — verify that application-engine picked up 7.0.0-RC7.1.
application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerExecutor.java (1)
15-16: Centralizing class resolution via NaeReflectionUtils is good.Static import of
resolveClassaligns with other modules and handles proxies. LGTM.nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/enums/UserType.java (1)
1-9: Enum introduction looks good.Clear, minimal enum. Consider documenting semantics (e.g., who can assign SYSTEM) in javadocs elsewhere.
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IWorkflowService.java (1)
86-87: setPetriNet added — implemented internally; downstream check neededIWorkflowService#setPetriNet(Case) declared at application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IWorkflowService.java:86 and implemented in application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java:501; callers at application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseEventHandler.java:37 and WorkflowService.java:126,142. No other implementers found in this repo — verify downstream consumers if this is a published API.
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/AnonymousUser.java (1)
6-6: Correctly setting anonymous user type; verify persistence/migration.Setting
this.type = UserType.ANONYMOUSis appropriate. Please verify that:
AbstractUserpersiststypeand serializers/deserializers handle missing/null type (older records) with a safe default.Also applies to: 45-46
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MongoSearchService.java (1)
5-6: Method name vs. lookup mismatch; confirm correct identifier and query shape.
resolveAuthorByEmailcallsuserService.findUserByUsername(email, null). If usernames aren’t emails, this returns wrong users. Also confirm whether downstream expects a quoted string or an{$oid:"..."}fragment.If emails are the intended identifier, switch to
findUserByEmail(...)(or add such method). If username is indeed the email, consider renaming toresolveAuthorByUsernameto avoid confusion.- protected String resolveAuthorByEmail(String email) { - Optional<User> user = userService.findUserByUsername(email, null); - return user.map(User::getStringId).orElse(null); - } + protected String resolveAuthorByEmail(String email) { + // Prefer a dedicated email lookup if available + Optional<User> user = userService.findUserByEmail(email); // adjust to actual API + return user.map(User::getStringId).orElse(null); + }Also applies to: 248-250
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java (1)
500-509: Guard against null PetriNetId and document idempotency
- new ObjectId(useCase.getPetriNetId()) will throw if getPetriNetId() is null — add an explicit null-check and clear exception (include useCase.getStringId()). I could not find the Case class/getPetriNetId implementation in this module to confirm if it can be null; verify and fix accordingly.
- Decide & document whether setPetriNet should be idempotent (avoid re‑initializing tokens/arcs when a model is already attached) or add a guard.
@Override public void setPetriNet(Case useCase) { PetriNet model = useCase.getPetriNet(); - if (model == null) { - model = new com.netgrif.application.engine.adapter.spring.petrinet.domain.PetriNet((com.netgrif.application.engine.adapter.spring.petrinet.domain.PetriNet) petriNetService.get(new ObjectId(useCase.getPetriNetId()))); - useCase.setPetriNet(model); - } - model.initializeTokens(useCase.getActivePlaces()); - model.initializeArcs(useCase.getDataSet()); + if (model == null) { + String netId = useCase.getPetriNetId(); + if (netId == null) { + throw new IllegalStateException("Case " + useCase.getStringId() + " has no PetriNetId"); + } + model = new com.netgrif.application.engine.adapter.spring.petrinet.domain.PetriNet( + (com.netgrif.application.engine.adapter.spring.petrinet.domain.PetriNet) + petriNetService.get(new ObjectId(netId))); + useCase.setPetriNet(model); + } + // If re-initialization on existing model is not required, guard with a flag/condition. + model.initializeTokens(useCase.getActivePlaces()); + model.initializeArcs(useCase.getDataSet()); }nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Realm.java (1)
116-116: Constructor visibility tightened — OKProtected ctor on an abstract class is appropriate and non-breaking for subclasses.
Please confirm the adapter subclass (com.netgrif.application.engine.adapter.spring.auth.domain.Realm) still calls super(name) successfully in all constructors.
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java (1)
39-39: Expose User.type in responses — OKMapping
typefrom domain to response is correct and backwards compatible.Confirm all clients tolerate the new field (no strict JSON schema enforcement). If needed, document in release notes.
Also applies to: 67-67
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java (1)
41-41: Switched to domain User — OKType aligns with refactor across services.
application-engine/pom.xml (2)
9-9: Parent bump to 7.0.0-RC7.1 — verifiedAll module POMs reference application-engine-parent version 7.0.0-RC7.1; no mismatches found.
138-142: Approve — nae-spring-core-adapter module present and dependency added
Root pom.xml includes nae-spring-core-adapter and application-engine/pom.xml declares the dependency (lines 138–142); required for NaeReflectionUtils — OK.application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (4)
212-216: Predicate refactor LGTMQueryDSL-based lookup by name and path is clear and type-safe.
227-229: Predicate refactor LGTMFolder lookup via QCase predicate reads well.
388-390: Good extractionCentralizing predicate-based lookups through findCase(Predicate) reduces duplication.
158-163: Incorrect — identifiers are sanitized before persistence; no change required.MenuItemBody.getIdentifier() calls MenuItemUtils.sanitize(...) and toDataSet() writes that sanitized value, and MenuItemService already uses sanitized identifiers for creation/update/lookups (so lookups against FIELD_IDENTIFIER are consistent). See: nae-object-library/.../MenuItemBody.java:90 (getIdentifier) and :163 (putDataSetEntry FIELD_IDENTIFIER); application-engine/.../MenuItemService.java:99-101 (createMenuItem), 155-161 (createOrUpdateMenuItem), 197-201 (findMenuItem query).
Likely an incorrect or invalid review comment.
application-engine/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/UserResourceAssembler.java (1)
11-11: Switch to domain User type: OKInterface generic updated to User; consistent with the PR-wide migration.
application-engine/src/main/java/com/netgrif/application/engine/auth/web/PublicUserController.java (2)
83-87: Search now returns Page: OKType migration looks correct, including roles mapping and principal usage.
130-136: DTO conversion helpers: OKSignatures align with the domain User type and response DTO mapping.
application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java (1)
17-19: Static imports of NaeReflectionUtils: OKDelegating resolve/index helpers to the shared utility reduces duplication. SortedRunners logic remains intact.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/LoggedUser.java (1)
70-76: Constructor changes verified — LoggedUserImpl updated
- nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/LoggedUserImpl.java calls super(..., sessionTimeout, type); no other subclasses extending LoggedUser found.
application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/interfaces/IImpersonationService.java (1)
31-34: Signature migration to User — implemented and resolved.ImpersonationService implements IImpersonationService and defines reloadImpersonatedUserRoles (application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationService.java:143) and applyRolesAndAuthorities (application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationService.java:155) with User signatures; no other callers found (only a commented reference at application-engine/src/main/java/com/netgrif/application/engine/auth/service/UserResourceHelperService.java:55).
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/service/ProcessRoleService.java (1)
25-27: API change to User — implementers & call sites updatedNew User-based signatures are implemented and in use. Locations: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/service/ProcessRoleService.java (interface); application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (implementation + internal calls); application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.java (controller call site).
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/utils/NaeReflectionUtils.java (1)
24-28: LGTM: proxy‑aware class resolutionresolveClass correctly unwraps Spring AOP proxies and handles Class inputs.
application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.java (1)
103-105: Type migration to domain User: controller layer changes look consistentFQCN usage for domain User avoids collision with response DTO and the mapping to UI User via userFactory remains intact.
Please ensure Swagger/OpenAPI schemas remain unchanged for responses; run your API docs generation and spot-check /api/users/{realmId}/all and /api/users/search.
Also applies to: 141-146, 328-334
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java (1)
75-84: Enforce non‑null UserType and revisit equality semantics
- Require non-null for type in both constructors:
@@ - this.type = type; + this.type = java.util.Objects.requireNonNull(type, "user type must not be null");
- If identity should remain based on id/realmId, exclude type from equals/hashCode:
-@EqualsAndHashCode(callSuper = true) +@EqualsAndHashCode(callSuper = true, exclude = "type")Confirm intended equality semantics for users across caches/collections. Applies to nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java lines 75-84 and 99-108.
application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/interfaces/IImpersonationAuthorizationService.java (1)
5-5: API signature change verified — implementor and callers updatedIImpersonationAuthorizationService now uses User/LoggedUser; the sole implementor (ImpersonationAuthorizationService) and callers (ImpersonationService, ImpersonationController) already use User/LoggedUser — no AbstractUser-based signatures remain for these interface methods.
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (3)
140-150: Context reload flow is sound.Persist-then-reload for current principal is correct.
104-112: Signature migration to User looks consistent — verify no remaining AbstractUser call sitesOverload delegates correctly; automated rg in the sandbox produced no matches, so cannot confirm absence of callers passing AbstractUser. Run locally:
rg -nP --type=java -C2 '\bassignRolesToUser\s*(\s*AbstractUser\b'
410-427: Incorrect — passing realm.getName() is correct. The UserService API is findAllByProcessRoles(..., String realmId, ...); realm.getName() supplies the expected realm identifier.Likely an incorrect or invalid review comment.
application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationService.java (3)
58-64: User switch is OK.findById now returning User integrates cleanly here.
73-77: OK: config-based impersonated lookup uses User.
143-152: Return type change to User is consistent.nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java (4)
56-64: OK: default system group helper now accepts User.
169-179: OK: addUser(User, String) and addUser(User, Group).
187-197: OK: removeUser(User, …) overloads.
38-49: API migration to User — breaking changeSignatures now accept User (breaking). Ensure all implementors/consumers are updated.
- Implementor found: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java
- Interface: nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java
- Notable references to check/update: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/config/AuthBeansConfiguration.java, application-engine/src/main/java/com/netgrif/application/engine/orgstructure/web/GroupController.java, application-engine/src/main/java/com/netgrif/application/engine/startup/runner/GroupRunner.java, application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java, application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java, application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java, application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java, nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
Action: search for callers passing AbstractUser/LoggedUser to GroupService methods and either update them to pass User, add adapter/overloads, or restore compatibility.
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java (3)
92-102: Member paging switch to Page looks good.
167-175: create(User) change is fine.Guards duplicate default groups (except for system) and delegates to main creator.
177-187: create(String, String, User) is fine.Owner/group membership are synchronized.
application-engine/src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationAuthorizationService.java (3)
76-78: OK: canImpersonateUser now works with User.
92-100: Authority filtering logic is fine.Only authorities both present on user and allowed by config are kept.
103-114: Roles filtering logic is fine.Includes default role and intersects config roles with user’s roles.
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (4)
288-306: Authority/role defaults look fineDefault authorities/role assignment is correctly guarded and persisted.
621-628: transformToUser overloads LGTMBoth correctly delegate to
findById.
667-673: Casting helper assumes content type — safe for nowGiven repository returns
User, this is fine.
701-719: Confirm whether User extends AbstractUser — if not, update method parameter types.
verifyPasswords(...) and canUpdatePassword(...) accept AbstractUser; this is only correct if your concrete User type extends AbstractUser — otherwise change signatures to the concrete User type or a shared interface.
Location: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java:701-719application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java (1)
67-86: resetExpiredToken — verify null‑realm handling; fix counting
- findAllByStateAndExpirationDateBefore delegates to collectionNameProvider.getCollectionNameForRealm(realmId); passing null is handled by that provider — confirm whether null maps to a single/default realm. If it doesn't iterate all realms, either loop realms here or add a UserService method that does (like the remove variant).
- Final log prints only the last page size; accumulate a total and remove the unnecessary stream when saving.
Suggested minimal fix:
- do { + int total = 0; + do { users = userService.findAllByStateAndExpirationDateBefore(UserState.BLOCKED, LocalDateTime.now(), null, pageable); if (users == null || users.isEmpty()) { log.info("There are none expired tokens. Everything is awesome."); return; } users.forEach(user -> { user.setToken(null); user.setExpirationDate(null); }); - userService.saveUsers(users.getContent().stream().toList()); + userService.saveUsers(users.getContent()); + total += users.getNumberOfElements(); pageable = pageable.next(); } while (users.hasNext()); - log.info("Reset " + users.getContent().size() + " expired user tokens"); + log.info("Reset " + total + " expired user tokens");application-engine/src/main/java/com/netgrif/application/engine/auth/service/interfaces/IRegistrationService.java (1)
16-31: API migration to User looks goodSignatures consistently use domain
User. Ensure downstream controllers/services updated accordingly (looks aligned in this PR).Run to check for lingering
AbstractUserreferences in registration layer:
| Class serviceClass = NaeReflectionUtils.resolveClass(entry.value) | ||
| ModuleService[] annotations = serviceClass.getAnnotationsByType(ModuleService.class) | ||
| if (annotations.length == 0) throw new IllegalStateException("Module Service bean must have @ModuleService annotations") |
There was a problem hiding this comment.
🧹 Nitpick
Honor meta-annotations and repeatables when resolving @ModuleService
Use Spring’s AnnotatedElementUtils to merge repeatable and composed annotations on proxies.
- Class serviceClass = NaeReflectionUtils.resolveClass(entry.value)
- ModuleService[] annotations = serviceClass.getAnnotationsByType(ModuleService.class)
+ Class serviceClass = NaeReflectionUtils.resolveClass(entry.value)
+ def annotations = AnnotatedElementUtils.findMergedRepeatableAnnotations(serviceClass, ModuleService)Then treat annotations as a collection:
- if (annotations.length == 0) throw new IllegalStateException("Module Service bean must have @ModuleService annotations")
- ModuleService annotation = annotations[0]
+ if (annotations.isEmpty()) throw new IllegalStateException("Module Service bean must have @ModuleService annotations")
+ ModuleService annotation = annotations.iterator().next()📝 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.
| Class serviceClass = NaeReflectionUtils.resolveClass(entry.value) | |
| ModuleService[] annotations = serviceClass.getAnnotationsByType(ModuleService.class) | |
| if (annotations.length == 0) throw new IllegalStateException("Module Service bean must have @ModuleService annotations") | |
| Class serviceClass = NaeReflectionUtils.resolveClass(entry.value) | |
| def annotations = AnnotatedElementUtils.findMergedRepeatableAnnotations(serviceClass, ModuleService) | |
| if (annotations.isEmpty()) throw new IllegalStateException("Module Service bean must have @ModuleService annotations") | |
| ModuleService annotation = annotations.iterator().next() |
🤖 Prompt for AI Agents
In
application-engine/src/main/groovy/com/netgrif/application/engine/integration/modules/ModuleServiceInjector.groovy
around lines 80 to 82, replace the direct getAnnotationsByType usage with
Spring's AnnotatedElementUtils merged/ repeatable lookup (e.g.
AnnotatedElementUtils.findMergedRepeatableAnnotations or an equivalent
AnnotatedElementUtils call) so meta-annotations and repeatable/composed
@ModuleService annotations on proxies are honored; then treat the result as a
collection (check for empty and iterate over elements) instead of relying on a
raw array length check.
...on-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java
Show resolved
Hide resolved
...src/main/java/com/netgrif/application/engine/impersonation/service/ImpersonationService.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public Case findMenuItem(String identifier, boolean retry) { | ||
| String query = String.format("processIdentifier:%s AND dataSet.%s.textValue.keyword:\"%s\"", | ||
| MenuProcessRunner.MENU_NET_IDENTIFIER, MenuItemConstants.FIELD_IDENTIFIER, identifier); | ||
| return findCase(MenuProcessRunner.MENU_NET_IDENTIFIER, query); | ||
| return findCase(QCase.case$.processIdentifier | ||
| .eq(MenuProcessRunner.MENU_NET_IDENTIFIER) | ||
| .and(QCase.case$.dataSet.get(MenuItemConstants.FIELD_IDENTIFIER).value.eq(identifier))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Unused retry parameter
The retry flag isn’t used anymore. Either remove the overload or implement a small retry/backoff if eventual consistency is still a concern.
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java
around lines 196 to 201, the method signature includes an unused boolean
parameter `retry`; remove the parameter from the method signature and all
callers (and any interface/override declarations) if retry behavior is no longer
needed, or alternatively implement a simple retry loop with backoff (e.g.,
attempt findCase, catch transient exceptions or check null, sleep with
exponential backoff and retry a few times) and document the behavior; ensure
method overrides and unit tests are updated to match the chosen fix.
There was a problem hiding this comment.
Will be removed in other PR.
There was a problem hiding this comment.
@renczesstefan Got it! Thanks for confirming that the unused retry parameter will be cleaned up in a future PR. That approach makes sense for keeping this PR focused on the main changes.
...n-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java
Show resolved
Hide resolved
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java
Outdated
Show resolved
Hide resolved
| Optional<User> findUserByUsername(String username, String realmName); | ||
|
|
||
| Page<AbstractUser> findAllUsersByQuery(Query query, String realmName, Pageable pageable); | ||
| Page<User> findAllUsersByQuery(Query query, String realmName, Pageable pageable); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Parameter naming: use realmId consistently
realmName is used here but most methods use realmId. Align to avoid confusion.
- Page<User> findAllUsersByQuery(Query query, String realmName, Pageable pageable);
+ Page<User> findAllUsersByQuery(Query query, String realmId, Pageable pageable);📝 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.
| Optional<User> findUserByUsername(String username, String realmName); | |
| Page<AbstractUser> findAllUsersByQuery(Query query, String realmName, Pageable pageable); | |
| Page<User> findAllUsersByQuery(Query query, String realmName, Pageable pageable); | |
| Optional<User> findUserByUsername(String username, String realmName); | |
| Page<User> findAllUsersByQuery(Query query, String realmId, Pageable pageable); |
🤖 Prompt for AI Agents
In
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java
around lines 72 to 75, the method parameters use realmName but the rest of the
codebase expects realmId; rename the parameter from realmName to realmId for
both findUserByUsername and findAllUsersByQuery, update their Javadoc/parameter
references if present, and ensure all implementing classes and callers are
updated to use the new parameter name to keep the API consistent.
| Page<User> findAllUsers(String realmName, Pageable pageable); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Parameter naming: realmName → realmId for consistency
Same rationale as above.
- Page<User> findAllUsers(String realmName, Pageable pageable);
+ Page<User> findAllUsers(String realmId, Pageable pageable);📝 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.
| Page<User> findAllUsers(String realmName, Pageable pageable); | |
| Page<User> findAllUsers(String realmId, Pageable pageable); |
🤖 Prompt for AI Agents
In
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java
around lines 83 to 84, the method parameter name realmName should be renamed to
realmId for consistency; update the method signature to Page<User>
findAllUsers(String realmId, Pageable pageable) and then propagate that rename
to all implementing classes, overriding methods, callers, unit tests, and any
Javadoc or comments to use realmId instead of realmName to keep the API
consistent across the codebase.
...n/src/main/java/com/netgrif/application/engine/auth/web/requestbodies/UpdateUserRequest.java
Show resolved
Hide resolved
| ", type='" + type + '\'' + | ||
| '}'; |
There was a problem hiding this comment.
🧹 Nitpick
Avoid logging PII and sensitive changes in toString().
toString() prints email and now type. Prefer redaction or avoid using toString() in logs.
- ", email='" + email + '\'' +
- ", type='" + type + '\'' +
+ ", email='***'" +
+ ", type='" + type + '\'' +📝 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.
| ", type='" + type + '\'' + | |
| '}'; | |
| ", email='***'" + | |
| ", type='" + type + '\'' + | |
| '}'; |
🤖 Prompt for AI Agents
In
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/requestbodies/UpdateUserRequest.java
around lines 35-36, the toString() currently includes sensitive fields (email
and type); update it to avoid exposing PII by removing those fields from the
string representation or replacing them with redacted/masked placeholders (e.g.,
"[REDACTED]" or show only hashed/partial values). Ensure toString() returns
non-sensitive context (IDs or non-PII fields) only, and document why sensitive
fields are omitted.
renczesstefan
changed the base branch from
release/7.0.0-rev8
to
release/7.0.0-rev7
renczesstefan
changed the base branch from
release/7.0.0-rev7
to
release/7.0.0-rev8
| @Override | ||
| public Case findMenuItem(String identifier, boolean retry) { | ||
| String query = String.format("processIdentifier:%s AND dataSet.%s.textValue.keyword:\"%s\"", | ||
| MenuProcessRunner.MENU_NET_IDENTIFIER, MenuItemConstants.FIELD_IDENTIFIER, identifier); | ||
| return findCase(MenuProcessRunner.MENU_NET_IDENTIFIER, query); | ||
| return findCase(QCase.case$.processIdentifier | ||
| .eq(MenuProcessRunner.MENU_NET_IDENTIFIER) | ||
| .and(QCase.case$.dataSet.get(MenuItemConstants.FIELD_IDENTIFIER).value.eq(identifier))); | ||
| } |
There was a problem hiding this comment.
Will be removed in other PR.
...c/main/java/com/netgrif/application/engine/auth/service/interfaces/IRegistrationService.java
Show resolved
Hide resolved
machacjozef
changed the base branch from
release/7.0.0-rev8
to
release/7.0.0-rev9
machacjozef
dismissed
renczesstefan’s stale review
The base branch was changed.
Description
Implements NAE-2169
How Has Been This Tested?
manualy
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor