Open
Conversation
- Introduced methods in `ActionDelegate` for CRUD operations on groups: - Added methods for creating, deleting, saving, and finding groups (`createGroup`, `deleteGroup`, `saveGroup`, `findGroupByIdentifier`, etc.). - Supported user assignments to groups with methods like `addUserToGroup`, `removeUserFromGroup`. - Added operations for managing group roles and authorities. - Enhanced `GroupService` with detailed methods for: - Adding/removing users, roles, and authorities to/from groups. - Managing subgroup relationships, including creation and deletion of parent-child links. - Extended MongoDB query support for group searches. - Annotated `Group` entity with custom annotations: - Ensured collection using `@EnsureCollection`. - Enhanced query capabilities with `@Indexed` on key fields. - Improved documentation for all newly added methods. These changes introduce robust support for group-related operations, enhancing scalability and clarity for permissions and role management.
- Added `NewGroupDto` to support new group creation with validation constraints. - Renamed `GroupSearchDto` -> `SearchGroupDto` for consistent naming. - Renamed `assignAuthority` methods to `addAuthority` in `GroupService` for clarity. - Added `GroupController` to handle API operations for group management, including: - Retrieving a group by ID. - Searching for groups with search criteria. - Creating, updating, and deleting groups with proper authorization checks.
- added GroupDto class
- Replaced `SearchGroupDto` with `GroupSearchRequestDto` in `GroupService` and `GroupController` for improved DTO consistency. - Introduced `CreateGroupRequestDto` and `UpdateGroupRequestDto` to support group creation and updates, respectively. - Added role management endpoints in `GroupController`: - Assign roles to a group (`/roles/assign`). - Add roles to a group (`/roles/add`). - Refactored the `createGroup` and `updateGroup` methods for better validation and error handling. - Removed the redundant `NewGroupRequest` class and its usage. - Added `SecurityRequirement` annotations to secure endpoints in `GroupController`. These changes improve the flexibility and security of group management services while maintaining a cleaner and more consistent codebase.
- resolved unused import
- updated return types
- updated validation dependencies
Replaced references to AuthorizationService with AuthorizationServiceImpl across the application for consistency. Updated method and class names, adjusted related documentation, and ensured all `@PreAuthorize` annotations now reference the renamed implementation.
- Added `assignSubgroups`, `addSubgroups`, and `removeSubgroups` API endpoints in `GroupController` to support dynamic subgroup management. - Extended `GroupService` interface and implemented methods in `GroupServiceImpl` for assigning and handling subgroups. - Updated `GroupSearchRequestDto` DTO to include support for filtering by `ids`. - Modified `GroupDto` to include `groupIds` and `subGroupIds` for enriched group representation. - Introduced `Pair` for returning parent-child group relationships in `ActionDelegate`. - Enhanced search functionality in `GroupServiceImpl` to filter groups by IDs in `search` method.
- Introduced new methods `addSubGroup` and `removeSubGroup` in `ActionDelegate` to manage subgroup relationships using `Pair`. - Extended `GroupSearchRequestDto` to support filtering by `ids`. - Updated `GroupDto` to include `groupIds` and `subGroupIds` for better representation of group relationships. - Added API endpoints in `GroupController` for assigning, adding, and removing subgroups: - `assignSubgroupsToGroup` - `addSubgroupsToGroup` - `removeSubgroupsFromGroup` - Enhanced `GroupService` with methods for handling subgroup operations such as `assignSubgroups`. - Improved MongoDB search capabilities in `GroupServiceImpl` to filter by group IDs in addition to full-text searches.
Enhanced the group creation flow with validation for null or blank identifiers and a check for existing group identifiers to prevent duplicates. Added corresponding error handling in `GroupController` to return appropriate HTTP responses for invalid input or unexpected errors. Updated `GroupRepository` with a method to check identifier existence.
The `@Validated` import was removed from `GroupController` as it was unused. Additionally, a redundant empty check on the group search IDs in `GroupServiceImpl` was simplified for clarity and efficiency.
Updated `assignRolesToGroup` to return void and adjusted related calls. Renamed `groupIdentifier` parameter to `groupId` in `removeUser` methods for better alignment with naming conventions. These changes improve code readability and align method behaviors across the codebase.
Refactored GroupServiceTest by modularizing existing tests into discrete methods and added new test cases for role and subgroup operations. Introduced `@BeforeEach` setup to streamline initial data preparation. Added a supporting XML file for testing role-related functionality.
Updated test cases to include the `username` field in user creation calls for consistency. Enhanced the system user creation logic to prevent duplication and improve maintainability. These changes ensure better alignment with application requirements and improve test reliability.
Replaced `email` with `username` where appropriate to better align with existing conventions. Simplified and streamlined system group creation by removing redundant update logic. Refactored `createSystemUser` to ensure proper handling of existing system users and improved clarity.
Added null assertions to ensure consistency and prevent issues caused by null parameters in critical methods. Refactored authority management by introducing overloads for adding and removing authorities with Group and Authority parameters, improving modularity and clarity.
Replaced `User` with `UserDto` across the application for better DTO consistency. Added validation for `CreateGroupRequestDto` and implemented enhanced group management operations, including retrieving group members, assigning users to groups, and realm-wide group management.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds full group management: new GroupController and DTOs/requests, extensive GroupService/GroupServiceImpl API and implementation changes, repository/index annotations, ActionDelegate group methods, User→UserDto migration (username added), auth interface/impl rename, tests updated and new test resources. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GroupController
participant GroupService
participant GroupRepo
participant UserService
participant ProcessRoleService
Client->>GroupController: POST /api/groups (CreateGroupRequestDto)
GroupController->>GroupService: createGroup(request)
GroupService->>GroupRepo: save(group)
GroupRepo-->>GroupService: saved Group
GroupService-->>GroupController: GroupDto
GroupController-->>Client: 201 Created
Client->>GroupController: PUT /api/groups/{id}/users (Set<userIds>)
GroupController->>GroupService: assignUsersToGroup(groupId, userIds)
GroupService->>UserService: findAllByIds(userIds)
UserService-->>GroupService: List<AbstractUser>
GroupService->>GroupRepo: save(updatedGroup)
GroupRepo-->>GroupService: saved Group
GroupService-->>GroupController: GroupDto
GroupController-->>Client: 200 OK
Client->>GroupController: PUT /api/groups/{id}/roles (Set<roleIds>)
GroupController->>GroupService: addRole(groupId, roleId)
GroupService->>ProcessRoleService: findById(roleId)
ProcessRoleService-->>GroupService: ProcessRole
GroupService->>GroupRepo: save(updatedGroup)
GroupRepo-->>GroupService: saved Group
GroupService-->>GroupController: GroupDto
GroupController-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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. 📝 Coding Plan
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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java (2)
255-297:⚠️ Potential issue | 🟠 MajorPreserve realm isolation in the new group-id membership path.
These overloads now resolve the target group globally by
_id, but neither path verifies thatuser.getRealmId()matchesgroup.getRealmId(). If a caller knows a foreign realm's group ID, they can create or remove cross-realm memberships.🔒 Suggested guard
public Group addUser(AbstractUser user, Group group) { Assert.notNull(user, "User cannot be null"); Assert.notNull(group, "Group cannot be null"); + Assert.isTrue(Objects.equals(user.getRealmId(), group.getRealmId()), + "User and group must belong to the same realm"); 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); } public Group removeUser(AbstractUser user, Group group) { Assert.notNull(user, "User cannot be null"); Assert.notNull(group, "Group cannot be null"); + Assert.isTrue(Objects.equals(user.getRealmId(), group.getRealmId()), + "User and group must belong to the same realm"); 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); }🤖 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/GroupServiceImpl.java` around lines 255 - 297, The addUser/removeUser overloads that accept a Group resolved by ID (methods addUser(AbstractUser user, String groupId) and removeUser(AbstractUser user, String groupId) and the internal addUser(AbstractUser user, Group group)/removeUser(AbstractUser user, Group group)) must enforce realm isolation: after resolving the Group (via findById) check that user.getRealmId() equals group.getRealmId() and fail fast if not (use Assert.isTrue or throw an AccessDenied/IllegalArgumentException with a clear message). Apply this guard before mutating user/group (before user.addGroupId()/group.addMemberId() and before saving) so cross-realm membership cannot be created or removed.
411-424:⚠️ Potential issue | 🟠 MajorReject subgroup cycles, not just self-links.
The current check only blocks
A -> A. It still allowsA -> B -> C -> A, which corrupts the hierarchy and can break any recursive traversal or inherited-permission logic built on top of it.assignSubgroups()routes through this path as well.🤖 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/GroupServiceImpl.java` around lines 411 - 424, The code currently only prevents self-links in addSubgroup(Group parentGroup, Group childGroup) but must also reject any cycle where parent would become reachable from child (e.g., A->B->C->A). Before mutating parentGroup/childGroup, perform a graph reachability check: starting from childGroup, traverse its subGroupIds recursively (DFS/BFS) using the same group lookup used elsewhere in GroupServiceImpl (e.g., the repository/service methods you already use to load groups in assignSubgroups or other methods) and if you encounter parentGroup.getStringId() throw an IllegalArgumentException indicating a cycle; only if the parent id is not found proceed to addSubGroupId/addGroupId, save both groups and return as before. Ensure the traversal guards against infinite loops by tracking visited ids.application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.java (2)
156-163:⚠️ Potential issue | 🟠 MajorUse
getStringId()in the self-access check.
userIdis the path parameter string, but this guard compares it tologgedUser.getId(). IfgetId()is the persisted/ObjectId form, non-admin users will never be able to fetch their own record.💡 Proposed fix
- if (!loggedUser.isAdmin() && !Objects.equals(loggedUser.getId(), userId)) { + if (!loggedUser.isAdmin() && !Objects.equals(loggedUser.getStringId(), userId)) { log.info("User [{}] trying to get another user with ID [{}]", actualUser.getUsername(), userId); return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); }🤖 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/auth/web/UserController.java` around lines 156 - 163, In getUser replace the self-access check that compares the path param userId to loggedUser.getId() with a comparison to loggedUser.getStringId() so non-admin users can match their string ID; update the Objects.equals call to use loggedUser.getStringId() (method on LoggedUser) instead of getId() in the authorization guard inside the getUser method.
268-286:⚠️ Potential issue | 🟠 MajorThe
ResponseMessagehelpers are inverted here.The
400branch currently builds a success message, while the200branch builds an error message. Clients that look at the response body will read the result backwards.💡 Proposed fix
} 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.badRequest().body(ResponseMessage.createErrorMessage("Assigning authority to user " + userId + " has failed!")); } - return ResponseEntity.ok(ResponseMessage.createErrorMessage("Authority was assigned to user successfully")); + return ResponseEntity.ok(ResponseMessage.createSuccessMessage("Authority was assigned to user successfully")); }🤖 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/auth/web/UserController.java` around lines 268 - 286, In assignAuthorityToUser, the ResponseMessage helpers are inverted: when catching IllegalArgumentException you should return a bad request with ResponseMessage.createErrorMessage containing the failure text (e.g. "Assigning authority to user {userId} has failed!"), and on success return ResponseEntity.ok with ResponseMessage.createSuccessMessage containing "Authority was assigned to user successfully"; update the two ResponseMessage calls (ResponseMessage.createSuccessMessage and ResponseMessage.createErrorMessage) accordingly while keeping the existing status codes and messages and leaving userService.assignAuthority as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/auth/web/GroupController.java`:
- Around line 273-281: The addRolesToGroup controller currently iterates roleIds
and calls groupService.addRole per ID, which can leave partial changes if one ID
fails; validate all roleIds up front and replace the per-ID loop with a single
transactional bulk operation on the service (e.g., addRolesToGroup -> call a new
or existing groupService.addRoles(groupId, Set<String>) that performs validation
and a single DB transaction) or wrap the loop in a service-level transaction
that rolls back on error; apply the same pattern to the other endpoints
mentioned (the similar loops at lines referenced: 294-302, 315-323, 336-344,
417-425, 438-446) so all batch mutations are atomic and validated before
mutating.
- Around line 67-69: The mapping currently rebuilds the Page using
groups.size(), losing the original total count and breaking pagination; update
calls that map Page<Group> to Page<GroupDto> (e.g., getAllGroupsOfRealm and the
other methods that call transformPageContent with a Page<Group> named "groups")
so the reconstructed PageImpl uses groups.getTotalElements() (and the original
pageable/sort) instead of groups.size(); ensure the PageImpl constructor is
passed mappedContent, the incoming Pageable, and groups.getTotalElements() so
totalElements/totalPages remain correct.
- Around line 87-98: The code validates request.realmId() but then resolves the
owner with userService.findById(request.ownerId(), null) and might create the
group with a user from a different realm; change the lookup to resolve the owner
inside the requested realm (e.g. call userService.findById(request.ownerId(),
request.realmId()) or otherwise verify the returned AbstractUser's realm equals
request.realmId()), handle a null return by returning NOT_FOUND with the
existing error message, and only call groupService.create(request.identifier(),
request.displayName(), user) once the owner has been validated to belong to the
requested realm.
In
`@application-engine/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/UserResource.java`:
- Around line 10-11: The constructor for UserResource currently ignores the
selfRel parameter; update the UserResource(UserDto content, String selfRel)
constructor to include the provided selfRel as the entity's self link instead of
passing new ArrayList<>() to the superclass (or remove the selfRel parameter if
you prefer API cleanup). Specifically, construct and pass a Link (or Collection
of Links) containing a self link built from selfRel into the call to
super(content, ...), so callers in UserResourceHelperService and
UserResourceAssembler (which supply "profile" or a variable selfRel) have their
link preserved in the resulting EntityModel; alternatively drop the unused
selfRel parameter from the constructor and adjust callers accordingly.
In
`@application-engine/src/main/java/com/netgrif/application/engine/orgstructure/web/GroupController.java`:
- Around line 24-55: The legacy GET /api/group/all route was removed and the
commented GroupController/getAllGroups is left in-source; either reintroduce
compatible routing in the new GroupController (add an endpoint that maps GET
/api/group/all and delegates to the existing service/getAllGroups logic or
routes to the new handler) or remove the commented legacy controller and add
explicit migration docs explaining the breaking change and the new path format
(api/groups/{realmId}/all) with example client update steps; ensure references
to `@RequestMapping`, GroupController, and getAllGroups are updated/removed
consistently and do not leave commented legacy endpoints in the codebase.
In
`@application-engine/src/main/java/com/netgrif/application/engine/startup/runner/GroupRunner.java`:
- Around line 35-39: The createDefaultGroup method in GroupRunner duplicates
lazy-creation logic; replace its manual lookup/create flow by calling
GroupService.getDefaultSystemGroup() (via the groupService instance) to obtain
the default system group instead of using groupService.findByIdentifier(...) and
groupService.create(...); update any logging to reflect that
getDefaultSystemGroup() was invoked and remove the duplicated creation logic so
startup uses the canonical service behavior.
In
`@application-engine/src/test/groovy/com/netgrif/application/engine/orgstructure/groups/GroupServiceTest.groovy`:
- Around line 104-108: The FileInputStream passed into
petriNetService.importPetriNet via ImportPetriNetParams.with().xmlFile(new
FileInputStream("src/test/resources/simple_role.xml")) is never closed; change
the test to open the XML stream in a try-with-resources (or Groovy's
withCloseable/using) so the InputStream is closed after calling
petriNetService.importPetriNet (referencing ImportPetriNetEventOutcome and
petriNetService.importPetriNet); ensure the stream variable is used for
xmlFile(...) and automatically closed to prevent file handle leaks.
In
`@docs/javadoc/com/netgrif/application/engine/auth/service/AuthorizationService.html`:
- Line 128: The Javadoc file is inconsistent: the inheritance tree references
AuthorizationServiceImpl while the title, declaration block, constructors, and
anchors still describe AuthorizationService; regenerate the entire Javadoc page
for the AuthorizationService/AuthorizationServiceImpl classes instead of
hand-editing a single line. Run the Javadoc generation for the package
com.netgrif.application.engine.auth.service (so the AuthorizationService and
AuthorizationServiceImpl pages are rebuilt), ensure the generated HTML now
consistently shows AuthorizationServiceImpl where appropriate and that anchors,
declaration blocks, and constructor sections match the actual class names.
In
`@docs/javadoc/com/netgrif/application/engine/auth/service/class-use/AuthorizationService.html`:
- Line 6: The Javadoc page class-use/AuthorizationService.html was manually
edited to reference AuthorizationServiceImpl causing inconsistent title/body;
revert the manual changes to this file and regenerate the Javadoc so the page
correctly documents AuthorizationService (not AuthorizationServiceImpl).
Specifically, restore/remove any references where AuthorizationServiceImpl was
inserted into class-use/AuthorizationService.html (and the similar mismatches
you saw for other occurrences) and run the project's Javadoc generation task so
the generated title/header/body consistently reflect the AuthorizationService
class and its actual usages.
In
`@docs/javadoc/com/netgrif/application/engine/auth/service/interfaces/class-use/IAuthorizationService.html`:
- Line 6: The Javadoc page class-use/IAuthorizationService.html is a stale
artifact that was only partially renamed (title changed to AuthorizationService)
while links and nav still reference the interface IAuthorizationService;
regenerate the Javadoc for the type
com.netgrif.application.engine.auth.service.AuthorizationService (so
class-use/IAuthorizationService.html and related class-use pages are rebuilt
consistently) or delete the stale class-use/IAuthorizationService.html file (and
any other mismatched artifacts) so that references to IAuthorizationService.html
and AuthorizationService are not contradictory in the docs.
In
`@docs/javadoc/com/netgrif/application/engine/auth/web/responsebodies/UserResource.html`:
- Line 162: The generated Javadoc for the UserResource constructor is
inconsistent: some constructor anchors reference UserDto while other parts
(hierarchy and constructor body) still reference User/User.html, causing mixed
APIs; regenerate the docs from sources and fix the Javadoc generation so all
constructor links and type references are consistent by updating the
UserResource constructor signature to use UserDto everywhere (ensure the
constructor anchor for UserResource(<init>(UserDto,String)) and all related type
links and the hierarchy refer to
com.netgrif.application.engine.auth.web.responsebodies.UserDto instead of User),
then rebuild the Javadoc to produce a single coherent page.
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/dto/request/group/UpdateGroupRequestDto.java`:
- Line 3: The UpdateGroupRequestDto record currently allows null/blank values;
annotate the record components with validation constraints (e.g., add `@NotNull`
on id and `@NotBlank` on identifier and displayName) and import the corresponding
Jakarta Validation annotations so the DTO fails validation instead of causing an
NPE in updateGroup; also update the controller method that receives this DTO
(the updateGroup endpoint parameter) to be annotated with `@Valid` (and keep
`@RequestBody` if present) so validation is triggered before processing.
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/dto/response/authority/AuthorityDto.java`:
- Around line 11-15: AuthorityDto currently uses ObjectId for its id field;
change the record signature to use String id to match the DTO pattern and
GroupDto, and update the factory to call authority.getStringId() instead of
get_id(); specifically modify the record declaration AuthorityDto(ObjectId id,
String name) to AuthorityDto(String id, String name) and update the
fromAuthority(Authority authority) method to construct the record with
authority.getStringId() and authority.getName().
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/dto/response/group/GroupDto.java`:
- Around line 21-27: The factory method GroupDto.fromGroup currently passes
group.getGroupIds() and group.getSubgroupIds() directly into the GroupDto, which
leaks mutable backing collections; make defensive copies before storing them in
the DTO by replacing those arguments with new collection instances created from
group.getGroupIds() and group.getSubgroupIds() (e.g., copy into new
HashSet/ArrayList or Collections.unmodifiableSet) so the DTO holds its own
immutable/mutably-isolated copies.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/AuthorizationServiceImpl.java`:
- Around line 7-8: The service bean is currently unnamed causing Spring to
generate authorizationServiceImpl and coupling security expressions; update the
class declaration for AuthorizationServiceImpl (which implements
AuthorizationService) to declare an explicit bean name (e.g.
`@Service`("authorizationService")) so the bean id is stable, then update all
`@PreAuthorize` references that use `@authorizationServiceImpl` to use
`@authorizationService` instead in the controllers that reference it
(WorkflowController, ProcessRoleController, PetriNetController,
ElasticController, UserController, GroupController); ensure the interface name
AuthorizationService remains the type and only the bean name changes.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java`:
- Around line 239-247: The bulk-diff logic in assignUsersToGroup (and similarly
in assignSubgroups) assumes group.getMemberIds()/parentGroup.getSubgroupIds()
and the incoming userIds/childGroupIds are non-null; create defensive
normalization before constructing new HashSet instances by replacing any null
with Collections.emptySet() or new HashSet<>() (e.g., Set<String>
currentGroupMemberIds =
Optional.ofNullable(group.getMemberIds()).orElse(Collections.emptySet()); and
Set<String> userIdsSafe =
Optional.ofNullable(userIds).orElse(Collections.emptySet());), then compute
removableMemberIds and newMemberIds from these safe sets; apply the same pattern
in assignSubgroups using parentGroup.getSubgroupIds() and childGroupIds to avoid
NPEs.
In
`@nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java`:
- Around line 163-171: The overloads for group membership operations are
inconsistent (mixing groupIdentifier and groupId) which can cause callers to
pass the wrong value; update the public API so all symmetric methods use the
same parameter name/type (prefer persisted IDs) — change method signatures like
Group addUser(String userId, String groupIdentifier, String realmId) and its
counterparts (removeUser, addUsers, removeUsers, etc.) to accept a single
consistent parameter name (e.g., groupId) and update all implementations and
callers (controllers, services) to pass persisted group IDs accordingly so the
API surface is uniform.
In
`@nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/UserDto.java`:
- Around line 87-89: The createUser(AbstractUser user, List<GroupDto> groups)
factory in UserDto currently does new HashSet<>(groups) which throws when groups
is null; update the method (UserDto.createUser) to treat a null groups argument
as an empty collection before constructing the HashSet (e.g., use an explicit
null check or default to Collections.emptySet()/Collections.emptyList()) so
result.setGroups(...) always receives a non-null, possibly-empty Set.
---
Outside diff comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.java`:
- Around line 156-163: In getUser replace the self-access check that compares
the path param userId to loggedUser.getId() with a comparison to
loggedUser.getStringId() so non-admin users can match their string ID; update
the Objects.equals call to use loggedUser.getStringId() (method on LoggedUser)
instead of getId() in the authorization guard inside the getUser method.
- Around line 268-286: In assignAuthorityToUser, the ResponseMessage helpers are
inverted: when catching IllegalArgumentException you should return a bad request
with ResponseMessage.createErrorMessage containing the failure text (e.g.
"Assigning authority to user {userId} has failed!"), and on success return
ResponseEntity.ok with ResponseMessage.createSuccessMessage containing
"Authority was assigned to user successfully"; update the two ResponseMessage
calls (ResponseMessage.createSuccessMessage and
ResponseMessage.createErrorMessage) accordingly while keeping the existing
status codes and messages and leaving userService.assignAuthority as-is.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java`:
- Around line 255-297: The addUser/removeUser overloads that accept a Group
resolved by ID (methods addUser(AbstractUser user, String groupId) and
removeUser(AbstractUser user, String groupId) and the internal
addUser(AbstractUser user, Group group)/removeUser(AbstractUser user, Group
group)) must enforce realm isolation: after resolving the Group (via findById)
check that user.getRealmId() equals group.getRealmId() and fail fast if not (use
Assert.isTrue or throw an AccessDenied/IllegalArgumentException with a clear
message). Apply this guard before mutating user/group (before
user.addGroupId()/group.addMemberId() and before saving) so cross-realm
membership cannot be created or removed.
- Around line 411-424: The code currently only prevents self-links in
addSubgroup(Group parentGroup, Group childGroup) but must also reject any cycle
where parent would become reachable from child (e.g., A->B->C->A). Before
mutating parentGroup/childGroup, perform a graph reachability check: starting
from childGroup, traverse its subGroupIds recursively (DFS/BFS) using the same
group lookup used elsewhere in GroupServiceImpl (e.g., the repository/service
methods you already use to load groups in assignSubgroups or other methods) and
if you encounter parentGroup.getStringId() throw an IllegalArgumentException
indicating a cycle; only if the parent id is not found proceed to
addSubGroupId/addGroupId, save both groups and return as before. Ensure the
traversal guards against infinite loops by tracking visited ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1e2fbde-f276-43dc-8cba-7cd503e85058
📒 Files selected for processing (52)
application-engine/pom.xmlapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/auth/service/UserResourceHelperService.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/service/interfaces/IAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/service/interfaces/IUserResourceHelperService.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/web/GroupController.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/web/PublicUserController.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/UserResource.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.javaapplication-engine/src/main/java/com/netgrif/application/engine/orgstructure/web/GroupController.javaapplication-engine/src/main/java/com/netgrif/application/engine/orgstructure/web/responsebodies/GroupsResource.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.javaapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/GroupRunner.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/action/AssignActionTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/LoginAttemptsTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/TaskAuthorizationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/orgstructure/groups/GroupServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/permissions/ElasticSearchViewPermissionTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/permissions/QueryDSLViewPermissionTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileListFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/web/PetriNetControllerTest.groovyapplication-engine/src/test/java/com/netgrif/application/engine/workflow/web/VariableArcsTest.javaapplication-engine/src/test/resources/simple_role.xmldocs/javadoc/com/netgrif/application/engine/auth/service/AuthorizationService.htmldocs/javadoc/com/netgrif/application/engine/auth/service/class-use/AuthorizationService.htmldocs/javadoc/com/netgrif/application/engine/auth/service/interfaces/class-use/IAuthorizationService.htmldocs/javadoc/com/netgrif/application/engine/auth/web/responsebodies/UserResource.htmlnae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/dto/GroupSearchDto.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/dto/request/group/CreateGroupRequestDto.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/dto/request/group/GroupSearchRequestDto.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/dto/request/group/UpdateGroupRequestDto.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/dto/response/authority/AuthorityDto.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/dto/response/group/GroupDto.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/dto/response/petrinet/ProcessRoleDto.javanae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/AuthorizationServiceImpl.javanae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.javanae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserFactoryImpl.javanae-user-common/src/main/java/com/netgrif/application/engine/auth/repository/GroupRepository.javanae-user-common/src/main/java/com/netgrif/application/engine/auth/service/AuthorizationService.javanae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.javanae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserFactory.javanae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/UserDto.java
💤 Files with no reviewable changes (2)
- application-engine/src/main/java/com/netgrif/application/engine/auth/service/interfaces/IAuthorizationService.java
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/dto/GroupSearchDto.java
Comment on lines
+67
to
+69
| public ResponseEntity<Page<GroupDto>> getAllGroupsOfRealm(@PathVariable("realmId") String realmId, Pageable pageable, Locale locale) { | ||
| Page<Group> groups = groupService.findAllFromRealm(realmId, pageable); | ||
| return ResponseEntity.ok(transformPageContent(groups.getContent(), pageable, locale)); |
There was a problem hiding this comment.
Preserve the original page metadata when mapping to GroupDto.
transformPageContent(...) rebuilds the page with groups.size() as the total, so /all and /search report only the current page size instead of the real match count. Pagination becomes wrong as soon as there is more than one page.
💡 Proposed fix
public ResponseEntity<Page<GroupDto>> getAllGroupsOfRealm(`@PathVariable`("realmId") String realmId, Pageable pageable, Locale locale) {
Page<Group> groups = groupService.findAllFromRealm(realmId, pageable);
- return ResponseEntity.ok(transformPageContent(groups.getContent(), pageable, locale));
+ return ResponseEntity.ok(transformPageContent(groups, locale));
}
@@
`@GetMapping`(produces = MediaType.APPLICATION_JSON_VALUE)
public ResponseEntity<Page<GroupDto>> search(GroupSearchRequestDto query, Pageable pageable, Locale locale) {
- List<Group> groups = groupService.search(query, pageable).getContent();
- return ResponseEntity.ok(transformPageContent(groups, pageable, locale));
+ Page<Group> groups = groupService.search(query, pageable);
+ return ResponseEntity.ok(transformPageContent(groups, locale));
}
@@
- private Page<GroupDto> transformPageContent(List<Group> groups, Pageable pageable, Locale locale) {
- return new PageImpl<>(groups.stream().map(group -> GroupDto.fromGroup(group, locale)).toList(), pageable, groups.size());
+ private Page<GroupDto> transformPageContent(Page<Group> groups, Locale locale) {
+ return new PageImpl<>(
+ groups.getContent().stream().map(group -> GroupDto.fromGroup(group, locale)).toList(),
+ groups.getPageable(),
+ groups.getTotalElements()
+ );
}Also applies to: 355-357, 450-451
🤖 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/auth/web/GroupController.java`
around lines 67 - 69, The mapping currently rebuilds the Page using
groups.size(), losing the original total count and breaking pagination; update
calls that map Page<Group> to Page<GroupDto> (e.g., getAllGroupsOfRealm and the
other methods that call transformPageContent with a Page<Group> named "groups")
so the reconstructed PageImpl uses groups.getTotalElements() (and the original
pageable/sort) instead of groups.size(); ensure the PageImpl constructor is
passed mappedContent, the incoming Pageable, and groups.getTotalElements() so
totalElements/totalPages remain correct.
Comment on lines
+87
to
+98
| if (!realmExists(request.realmId())) { | ||
| String message = "Cannot create group, realm with id [" + request.realmId() + "] does not exist"; | ||
| log.error(message); | ||
| return ResponseEntity.badRequest().build(); | ||
| } | ||
| AbstractUser user = userService.findById(request.ownerId(), null); | ||
| if (user == null) { | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND).body(ResponseMessage.createErrorMessage("User with id [%s] not found".formatted(request.ownerId()))); | ||
| } | ||
| try { | ||
| groupService.create(request.identifier(), request.displayName(), user); | ||
| return ResponseEntity.ok(ResponseMessage.createSuccessMessage("Group created successfully")); |
There was a problem hiding this comment.
Resolve the owner inside the requested realm before creating the group.
This endpoint validates request.realmId(), but then loads the owner with findById(request.ownerId(), null) and creates the group from that user. A mismatched owner can therefore create the group in a different realm than the one the request just validated.
💡 Proposed fix
- AbstractUser user = userService.findById(request.ownerId(), null);
- if (user == null) {
- return ResponseEntity.status(HttpStatus.NOT_FOUND).body(ResponseMessage.createErrorMessage("User with id [%s] not found".formatted(request.ownerId())));
- }
try {
+ AbstractUser user = userService.findById(request.ownerId(), request.realmId());
+ if (user == null) {
+ return ResponseEntity.status(HttpStatus.NOT_FOUND)
+ .body(ResponseMessage.createErrorMessage("User with id [%s] not found in realm [%s]".formatted(request.ownerId(), request.realmId())));
+ }
groupService.create(request.identifier(), request.displayName(), user);
- return ResponseEntity.ok(ResponseMessage.createSuccessMessage("Group created successfully"));
+ return ResponseEntity.status(HttpStatus.CREATED)
+ .body(ResponseMessage.createSuccessMessage("Group created successfully"));
} catch (IllegalArgumentException e) {
return ResponseEntity.badRequest().body(ResponseMessage.createErrorMessage(e.getMessage()));
} catch (Exception e) {📝 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
| if (!realmExists(request.realmId())) { | |
| String message = "Cannot create group, realm with id [" + request.realmId() + "] does not exist"; | |
| log.error(message); | |
| return ResponseEntity.badRequest().build(); | |
| } | |
| AbstractUser user = userService.findById(request.ownerId(), null); | |
| if (user == null) { | |
| return ResponseEntity.status(HttpStatus.NOT_FOUND).body(ResponseMessage.createErrorMessage("User with id [%s] not found".formatted(request.ownerId()))); | |
| } | |
| try { | |
| groupService.create(request.identifier(), request.displayName(), user); | |
| return ResponseEntity.ok(ResponseMessage.createSuccessMessage("Group created successfully")); | |
| if (!realmExists(request.realmId())) { | |
| String message = "Cannot create group, realm with id [" + request.realmId() + "] does not exist"; | |
| log.error(message); | |
| return ResponseEntity.badRequest().build(); | |
| } | |
| try { | |
| AbstractUser user = userService.findById(request.ownerId(), request.realmId()); | |
| if (user == null) { | |
| return ResponseEntity.status(HttpStatus.NOT_FOUND) | |
| .body(ResponseMessage.createErrorMessage("User with id [%s] not found in realm [%s]".formatted(request.ownerId(), request.realmId()))); | |
| } | |
| groupService.create(request.identifier(), request.displayName(), user); | |
| return ResponseEntity.status(HttpStatus.CREATED) | |
| .body(ResponseMessage.createSuccessMessage("Group created successfully")); | |
| } catch (IllegalArgumentException e) { | |
| return ResponseEntity.badRequest().body(ResponseMessage.createErrorMessage(e.getMessage())); | |
| } catch (Exception e) { |
🤖 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/auth/web/GroupController.java`
around lines 87 - 98, The code validates request.realmId() but then resolves the
owner with userService.findById(request.ownerId(), null) and might create the
group with a user from a different realm; change the lookup to resolve the owner
inside the requested realm (e.g. call userService.findById(request.ownerId(),
request.realmId()) or otherwise verify the returned AbstractUser's realm equals
request.realmId()), handle a null return by returning NOT_FOUND with the
existing error message, and only call groupService.create(request.identifier(),
request.displayName(), user) once the owner has been validated to belong to the
requested realm.
Comment on lines
+273
to
+281
| public ResponseEntity<ResponseMessage> addRolesToGroup(@PathVariable("id") String groupId, @RequestBody Set<String> roleIds) { | ||
| try { | ||
| roleIds.forEach(roleId -> groupService.addRole(groupId, roleId)); | ||
| log.info("Process roles {} added to group with id [{}]", roleIds, groupId); | ||
| return ResponseEntity.ok(ResponseMessage.createSuccessMessage("Selected roles added to group " + groupId)); | ||
| } catch (IllegalArgumentException e) { | ||
| String message = "Adding roles to group [" + groupId + "] has failed!"; | ||
| log.error(message, e); | ||
| return ResponseEntity.badRequest().body(ResponseMessage.createErrorMessage("Adding roles to group " + groupId + " has failed!")); |
There was a problem hiding this comment.
These batch mutations can leave the group half-updated.
Each endpoint applies changes one ID at a time and only then catches IllegalArgumentException. If a later ID is invalid, earlier additions/removals are already committed even though the API returns 400. Please validate the whole request up front and execute the mutation through a single service-level bulk operation or transaction.
Also applies to: 294-302, 315-323, 336-344, 417-425, 438-446
🤖 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/auth/web/GroupController.java`
around lines 273 - 281, The addRolesToGroup controller currently iterates
roleIds and calls groupService.addRole per ID, which can leave partial changes
if one ID fails; validate all roleIds up front and replace the per-ID loop with
a single transactional bulk operation on the service (e.g., addRolesToGroup ->
call a new or existing groupService.addRoles(groupId, Set<String>) that performs
validation and a single DB transaction) or wrap the loop in a service-level
transaction that rolls back on error; apply the same pattern to the other
endpoints mentioned (the similar loops at lines referenced: 294-302, 315-323,
336-344, 417-425, 438-446) so all batch mutations are atomic and validated
before mutating.
Comment on lines
+10
to
11
| public UserResource(UserDto content, String selfRel) { | ||
| super(content, new ArrayList<>()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "UserResource.java" | head -20Repository: netgrif/application-engine
Length of output: 177
🏁 Script executed:
cat -n ./application-engine/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/UserResource.javaRepository: netgrif/application-engine
Length of output: 488
🏁 Script executed:
# Check EntityModel usage patterns in the codebase
rg "EntityModel" --type java -A 3 -B 1 | head -100Repository: netgrif/application-engine
Length of output: 13963
🏁 Script executed:
# Search for UserResource instantiation
rg "new UserResource" --type java -B 2 -A 2Repository: netgrif/application-engine
Length of output: 2152
🏁 Script executed:
# Let's look at UserResourceAssembler to understand the full context
cat -n ./application-engine/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/UserResourceAssembler.javaRepository: netgrif/application-engine
Length of output: 1621
🏁 Script executed:
# Let's check UserResourceHelperService
cat -n ./application-engine/src/main/java/com/netgrif/application/engine/auth/service/UserResourceHelperService.javaRepository: netgrif/application-engine
Length of output: 2789
🏁 Script executed:
# Check LocalisedTaskResource to see how it handles links
cat -n ./application-engine/src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedTaskResource.javaRepository: netgrif/application-engine
Length of output: 2715
🏁 Script executed:
# Check if there's a method in UserResource that adds links or uses selfRel
rg "UserResource" --type java -A 10 -B 2 | grep -A 10 "class UserResource"Repository: netgrif/application-engine
Length of output: 4088
Use the selfRel parameter or remove it from the constructor.
The constructor accepts a selfRel argument but ignores it, always passing an empty link list to the parent EntityModel. Both UserResourceHelperService and UserResourceAssembler pass meaningful values ("profile" and the selfRel variable respectively), but these are silently discarded. The serialized HAL response will be missing the _links.self reference.
Either apply the self link in the constructor or remove the unused parameter from the public API.
Suggested fix
import org.springframework.hateoas.EntityModel;
+import org.springframework.hateoas.Link;
-import java.util.ArrayList;
+import java.util.Collections;
public class UserResource extends EntityModel<UserDto> {
public UserResource(UserDto content, String selfRel) {
- super(content, new ArrayList<>());
+ super(content, selfRel == null
+ ? Collections.emptyList()
+ : Collections.singletonList(Link.of(selfRel).withSelfRel()));
}🤖 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/auth/web/responsebodies/UserResource.java`
around lines 10 - 11, The constructor for UserResource currently ignores the
selfRel parameter; update the UserResource(UserDto content, String selfRel)
constructor to include the provided selfRel as the entity's self link instead of
passing new ArrayList<>() to the superclass (or remove the selfRel parameter if
you prefer API cleanup). Specifically, construct and pass a Link (or Collection
of Links) containing a self link built from selfRel into the call to
super(content, ...), so callers in UserResourceHelperService and
UserResourceAssembler (which supply "profile" or a variable selfRel) have their
link preserved in the resulting EntityModel; alternatively drop the unused
selfRel parameter from the constructor and adjust callers accordingly.
Comment on lines
+24
to
+55
| //@RestController | ||
| //@RequestMapping("/api/group") | ||
| //@ConditionalOnProperty( | ||
| // value = "netgrif.engine.security.web.group-enabled", | ||
| // havingValue = "true", | ||
| // matchIfMissing = true | ||
| //) | ||
| //@Tag(name = "Group") | ||
| //public class GroupController { | ||
| // | ||
| // private final GroupService service; | ||
| // | ||
| // public GroupController(GroupService service) { | ||
| // this.service = service; | ||
| // } | ||
| // | ||
| // @PreAuthorize("@authorizationService.hasAuthority('ADMIN')") | ||
| // @Operation(summary = "Get all groups in the system", | ||
| // description = "Caller must have the ADMIN role", | ||
| // security = {@SecurityRequirement(name = "BasicAuth")}) | ||
| // @GetMapping(value = "/all", produces = MediaTypes.HAL_JSON_VALUE) | ||
| // @ApiResponses(value = { | ||
| // @ApiResponse(responseCode = "200", description = "OK"), | ||
| // @ApiResponse(responseCode = "403", description = "Caller doesn't fulfill the authorisation requirements"), | ||
| // }) | ||
| // public GroupsResource getAllGroups() { | ||
| // List<com.netgrif.application.engine.objects.auth.domain.Group> groups = service.findAll(Pageable.unpaged()).getContent(); | ||
| // Set<Group> groupResponse = groups.stream() | ||
| // .map(g -> new Group(g.getStringId(), g.getDisplayName())) | ||
| // .collect(Collectors.toCollection(HashSet::new)); | ||
| // return new GroupsResource(groupResponse); | ||
| // } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
application-engine/src/main/java/com/netgrif/application/engine/orgstructure/web/GroupController.java \
application-engine/src/main/java/com/netgrif/application/engine/auth/web/GroupController.java
do
if [ -f "$f" ]; then
echo "== $f =="
rg -n -C2 '@RequestMapping|@GetMapping|/api/group|/all|class GroupController' "$f" || true
fi
done
if [ -d docs ]; then
echo "== docs references =="
rg -n -C2 '/api/group/all|group/all' docs || true
fiRepository: netgrif/application-engine
Length of output: 2538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for migration, changelog, or breaking changes documentation
rg -l 'CHANGELOG|MIGRATION|BREAKING|MIGRATION_GUIDE|BREAKING_CHANGES|UPGRADE' . --type md --type txt --type asciidoc 2>/dev/null | head -20
# Also search the README for migration info
if [ -f README.md ]; then
echo "=== README.md references to group or migration ==="
rg -n -i 'group.*migration|breaking.*change|v[0-9]+\.[0-9]+' README.md || true
fi
# Look for any release notes or version-specific docs
find . -maxdepth 3 -type f \( -name "*CHANGELOG*" -o -name "*MIGRATION*" -o -name "*RELEASE*" \) 2>/dev/null | head -10Repository: netgrif/application-engine
Length of output: 52
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List files at repo root that might be docs
ls -la | grep -i -E 'readme|changelog|migration|doc|guide' || echo "No matching files at root"
# Check if docs directory exists and list contents
if [ -d docs ]; then
echo "=== Docs directory exists ==="
find docs -maxdepth 2 -type f | head -20
else
echo "No docs directory found"
fiRepository: netgrif/application-engine
Length of output: 815
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search within docs for group-related or migration-related information
echo "=== Searching docs for Group API changes ==="
rg -n 'group|migration|breaking|deprecat' docs --type md --type txt -i | grep -v '.nojekyll' | head -30
echo -e "\n=== Files mentioning Group or API ==="
find docs -name "*.md" -type f -exec grep -l -i 'group\|api' {} \; | head -10Repository: netgrif/application-engine
Length of output: 3518
The legacy GET /api/group/all route has changed incompatibly and needs explicit migration documentation.
The new controller at auth/web/GroupController.java uses @RequestMapping("api/groups") and serves GET /api/groups/{realmId}/all instead. Existing clients calling the old GET /api/group/all endpoint will receive 404s. Either restore the legacy route in the new controller, or document this as a breaking change in your migration guide with upgrade instructions for affected clients. The commented-out source code should be deleted unless there is a documented migration path.
🤖 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/orgstructure/web/GroupController.java`
around lines 24 - 55, The legacy GET /api/group/all route was removed and the
commented GroupController/getAllGroups is left in-source; either reintroduce
compatible routing in the new GroupController (add an endpoint that maps GET
/api/group/all and delegates to the existing service/getAllGroups logic or
routes to the new handler) or remove the commented legacy controller and add
explicit migration docs explaining the breaking change and the new path format
(api/groups/{realmId}/all) with example client update steps; ensure references
to `@RequestMapping`, GroupController, and getAllGroups are updated/removed
consistently and do not leave commented legacy endpoints in the codebase.
Comment on lines
+21
to
+27
| public static GroupDto fromGroup(Group group, Locale locale) { | ||
| return new GroupDto(group.getStringId(), group.getDisplayName(), group.getIdentifier(), group.getOwnerUsername(), | ||
| group.getAuthoritySet().stream().map(AuthorityDto::fromAuthority).collect(Collectors.toSet()), | ||
| group.getProcessRoles().stream().map(processRole -> new ProcessRoleDto(processRole, locale)).collect(Collectors.toSet()), | ||
| group.getGroupIds(), | ||
| group.getSubgroupIds() | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'getGroupIds|getSubgroupIds|groupIds|subgroupIds|subGroupIds' \
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.java || trueRepository: netgrif/application-engine
Length of output: 1454
🏁 Script executed:
rg -n 'getGroupIds|getSubGroupIds|getSubgroupIds' nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.java -A 3Repository: netgrif/application-engine
Length of output: 52
🏁 Script executed:
rg -n 'private.*groupIds|public.*get.*[Gg]roup[Ii]ds' nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.java -A 3Repository: netgrif/application-engine
Length of output: 226
🏁 Script executed:
wc -l nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.javaRepository: netgrif/application-engine
Length of output: 168
🏁 Script executed:
cat -n nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.java | head -100Repository: netgrif/application-engine
Length of output: 3221
🏁 Script executed:
cat -n nae-object-library/src/main/java/com/netgrif/application/engine/objects/dto/response/group/GroupDto.javaRepository: netgrif/application-engine
Length of output: 1623
🏁 Script executed:
cat -n nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.java | tail -100Repository: netgrif/application-engine
Length of output: 3709
🏁 Script executed:
find nae-object-library -name "AbstractActor.java" -exec cat -n {} \;Repository: netgrif/application-engine
Length of output: 11777
Copy ID collections before storing them in the record.
The factory materializes new sets for authorities and roles (lines 23-24), but lines 25-26 pass groupIds and subGroupIds directly. Since Lombok's @Getter returns direct field references and these setters only defend writes, the DTO receives mutable backing collections. Subsequent mutations to the Group's ID sets will leak into the response.
🧱 Proposed fix
return new GroupDto(group.getStringId(), group.getDisplayName(), group.getIdentifier(), group.getOwnerUsername(),
group.getAuthoritySet().stream().map(AuthorityDto::fromAuthority).collect(Collectors.toSet()),
group.getProcessRoles().stream().map(processRole -> new ProcessRoleDto(processRole, locale)).collect(Collectors.toSet()),
- group.getGroupIds(),
- group.getSubgroupIds()
+ group.getGroupIds() == null ? Set.of() : Set.copyOf(group.getGroupIds()),
+ group.getSubgroupIds() == null ? Set.of() : Set.copyOf(group.getSubgroupIds())
);🤖 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/dto/response/group/GroupDto.java`
around lines 21 - 27, The factory method GroupDto.fromGroup currently passes
group.getGroupIds() and group.getSubgroupIds() directly into the GroupDto, which
leaks mutable backing collections; make defensive copies before storing them in
the DTO by replacing those arguments with new collection instances created from
group.getGroupIds() and group.getSubgroupIds() (e.g., copy into new
HashSet/ArrayList or Collections.unmodifiableSet) so the DTO holds its own
immutable/mutably-isolated copies.
Comment on lines
7
to
+8
| @Service | ||
| public class AuthorizationService implements IAuthorizationService { | ||
| public class AuthorizationServiceImpl implements AuthorizationService { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd 'AuthorizationServiceImpl\.java$' | xargs -r rg -n -C2 '@Service|class AuthorizationServiceImpl'
rg -n '@PreAuthorize\(".*authorizationServiceImpl' --glob '*.java'
rg -n '@PreAuthorize\(".*authorizationService\.' --glob '*.java'Repository: netgrif/application-engine
Length of output: 4944
Pin an explicit Spring bean name to decouple security expressions from implementation details.
The class currently uses @Service without an explicit bean name, which causes Spring to auto-generate the bean ID as authorizationServiceImpl (implementation name). This couples 26 @PreAuthorize annotations across multiple controllers to the implementation class name instead of the interface. Add an explicit bean name to stabilize this and enable future refactoring without touching security logic.
♻️ Suggested change
-@Service
+@Service("authorizationService")
public class AuthorizationServiceImpl implements AuthorizationService {After applying this change, update the 26 @PreAuthorize annotations currently referencing @authorizationServiceImpl to use @authorizationService instead (references found in WorkflowController, ProcessRoleController, PetriNetController, ElasticController, UserController, and GroupController).
🤖 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/AuthorizationServiceImpl.java`
around lines 7 - 8, The service bean is currently unnamed causing Spring to
generate authorizationServiceImpl and coupling security expressions; update the
class declaration for AuthorizationServiceImpl (which implements
AuthorizationService) to declare an explicit bean name (e.g.
`@Service`("authorizationService")) so the bean id is stable, then update all
`@PreAuthorize` references that use `@authorizationServiceImpl` to use
`@authorizationService` instead in the controllers that reference it
(WorkflowController, ProcessRoleController, PetriNetController,
ElasticController, UserController, GroupController); ensure the interface name
AuthorizationService remains the type and only the bean name changes.
Comment on lines
+239
to
+247
| public Group assignUsersToGroup(String groupId, Set<String> userIds) { | ||
| Group group = this.findById(groupId); | ||
| Set<String> currentGroupMemberIds = group.getMemberIds(); | ||
|
|
||
| Set<String> removableMemberIds = new HashSet<>(currentGroupMemberIds); | ||
| removableMemberIds.removeAll(userIds); | ||
|
|
||
| Set<String> newMemberIds = new HashSet<>(userIds); | ||
| newMemberIds.removeAll(currentGroupMemberIds); |
There was a problem hiding this comment.
Null-safe the bulk diff logic before creating HashSets.
group.getMemberIds() / parentGroup.getSubgroupIds() can be null in this class, and the incoming replacement sets can also be null. Both new HashSet<>(currentGroupMemberIds) and new HashSet<>(childGroupIds) will then fail with NullPointerException before any validation runs.
💡 Suggested fix
public Group assignUsersToGroup(String groupId, Set<String> userIds) {
Group group = this.findById(groupId);
- Set<String> currentGroupMemberIds = group.getMemberIds();
+ Set<String> currentGroupMemberIds = Optional.ofNullable(group.getMemberIds()).orElseGet(Collections::emptySet);
+ Set<String> requestedUserIds = Optional.ofNullable(userIds).orElseGet(Collections::emptySet);
Set<String> removableMemberIds = new HashSet<>(currentGroupMemberIds);
- removableMemberIds.removeAll(userIds);
+ removableMemberIds.removeAll(requestedUserIds);
- Set<String> newMemberIds = new HashSet<>(userIds);
+ Set<String> newMemberIds = new HashSet<>(requestedUserIds);
newMemberIds.removeAll(currentGroupMemberIds);Apply the same normalization in assignSubgroups(...).
Also applies to: 367-375
🤖 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/GroupServiceImpl.java`
around lines 239 - 247, The bulk-diff logic in assignUsersToGroup (and similarly
in assignSubgroups) assumes group.getMemberIds()/parentGroup.getSubgroupIds()
and the incoming userIds/childGroupIds are non-null; create defensive
normalization before constructing new HashSet instances by replacing any null
with Collections.emptySet() or new HashSet<>() (e.g., Set<String>
currentGroupMemberIds =
Optional.ofNullable(group.getMemberIds()).orElse(Collections.emptySet()); and
Set<String> userIdsSafe =
Optional.ofNullable(userIds).orElse(Collections.emptySet());), then compute
removableMemberIds and newMemberIds from these safe sets; apply the same pattern
in assignSubgroups using parentGroup.getSubgroupIds() and childGroupIds to avoid
NPEs.
There was a problem hiding this comment.
@renczesstefan, thanks for the update! The incoming userIds null-check in assignUsersToGroup looks good, but the fix is still incomplete in two places:
assignUsersToGroup—group.getMemberIds()is still not null-safe. If it returnsnull,new HashSet<>(currentGroupMemberIds)will throw aNullPointerException:
// still needed
Set<String> currentGroupMemberIds = Optional.ofNullable(group.getMemberIds()).orElseGet(Collections::emptySet);assignSubgroups— neitherparentGroup.getSubgroupIds()nor the incomingchildGroupIdsare guarded:
childGroupIds = childGroupIds == null ? new HashSet<>() : childGroupIds;
Set<String> currentSubgroupIds = Optional.ofNullable(parentGroup.getSubgroupIds()).orElseGet(Collections::emptySet);Could you apply these remaining guards as well?
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java
Outdated
Show resolved
Hide resolved
Comment on lines
+87
to
+89
| public static UserDto createUser(AbstractUser user, List<GroupDto> groups) { | ||
| UserDto result = createUser(user); | ||
| result.setGroups(new HashSet<>(groups)); |
There was a problem hiding this comment.
Handle null in the new groups overload.
new HashSet<>(groups) will throw immediately when the caller omits expanded group data. This factory has no non-null contract, so it should degrade to an empty set instead of failing at runtime.
💡 Proposed fix
public static UserDto createUser(AbstractUser user, List<GroupDto> groups) {
UserDto result = createUser(user);
- result.setGroups(new HashSet<>(groups));
+ result.setGroups(groups == null ? Set.of() : new HashSet<>(groups));
return result;
}🤖 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/web/responsebodies/UserDto.java`
around lines 87 - 89, The createUser(AbstractUser user, List<GroupDto> groups)
factory in UserDto currently does new HashSet<>(groups) which throws when groups
is null; update the method (UserDto.createUser) to treat a null groups argument
as an empty collection before constructing the HashSet (e.g., use an explicit
null check or default to Collections.emptySet()/Collections.emptyList()) so
result.setGroups(...) always receives a non-null, possibly-empty Set.
Added group fetching and mapping in `UserFactoryImpl` to include group details in `UserDto`. Updated `createUser` method in `UserDto` to handle optional group data gracefully. Removed obsolete `getUser` method from `UserFactoryImpl`.
coderabbitai
bot
added
the
new feature
coderabbitai
bot
added
the
breaking change
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserFactoryImpl.java`:
- Around line 26-27: In UserFactoryImpl.getUser, guard against a null
AbstractUser.getGroupIds() before calling groupService.findAllByIds: compute a
local Collection<Long> groupIds = (user.getGroupIds() == null ?
Collections.emptyList() : user.getGroupIds()), call
groupService.findAllByIds(groupIds, Pageable.unpaged()), map results with
GroupDto.fromGroup(locale) into a List<GroupDto> groups variable, then pass that
groups list into UserDto.createUser; this both prevents NPEs and makes the line
more readable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 485ad185-6934-41be-a39a-c735874bddeb
📒 Files selected for processing (2)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserFactoryImpl.javanae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/UserDto.java
Comment on lines
+26
to
+27
| public UserDto getUser(AbstractUser user, Locale locale) { | ||
| UserDto result = UserDto.createUser(user, groupService.findAllByIds(user.getGroupIds(), Pageable.unpaged()).stream().map(group -> GroupDto.fromGroup(group, locale)).collect(Collectors.toList())); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the GroupService.findAllByIds method signature and implementation for null handling
# Find the GroupService interface definition
ast-grep --pattern $'interface GroupService {
$$$
findAllByIds($_, $_) {
$$$
}
$$$
}'
# Search for findAllByIds method signatures
rg -n -A5 'findAllByIds.*Set.*String'Repository: netgrif/application-engine
Length of output: 1811
🌐 Web query:
netgrif application engine GroupService findAllByIds null handling
💡 Result:
In NETGRIF Application Engine, the group service method is INextGroupService#findByIds(Collection<String> groupIds) (there is no findAllByIds on the group service in the public API). [1]
For null handling, you should not pass null as the IDs collection. Under the hood, these lookups typically delegate to Spring Data repository methods like CrudRepository#findAllById(Iterable<ID> ids), whose parameters are required to be non-null (“must not be null”), so null will result in a runtime exception (commonly IllegalArgumentException / NullPointerException, depending on the call path). [2]
Recommended pattern:
Collection<String> idsSafe = (ids == null) ? List.of() : ids;
List<Case> groups = groupService.findByIds(idsSafe);Sources: INextGroupService Javadoc [1], Spring Data CrudRepository Javadoc (findAllById parameter must not be null) [2].
Potential NullPointerException if user.getGroupIds() returns null.
The group service method requires a non-null collection parameter. If AbstractUser.getGroupIds() returns null, it will cause a runtime exception. Add a null-safety check before passing group IDs to the service.
The line is also quite long; extracting the group mapping logic improves readability.
Proposed null-safe refactoring
`@Override`
public UserDto getUser(AbstractUser user, Locale locale) {
- UserDto result = UserDto.createUser(user, groupService.findAllByIds(user.getGroupIds(), Pageable.unpaged()).stream().map(group -> GroupDto.fromGroup(group, locale)).collect(Collectors.toList()));
+ Collection<String> groupIds = user.getGroupIds();
+ List<GroupDto> groupDtos = (groupIds != null && !groupIds.isEmpty())
+ ? groupService.findAllByIds(groupIds, Pageable.unpaged())
+ .stream()
+ .map(group -> GroupDto.fromGroup(group, locale))
+ .collect(Collectors.toList())
+ : List.of();
+ UserDto result = UserDto.createUser(user, groupDtos);📝 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
| public UserDto getUser(AbstractUser user, Locale locale) { | |
| UserDto result = UserDto.createUser(user, groupService.findAllByIds(user.getGroupIds(), Pageable.unpaged()).stream().map(group -> GroupDto.fromGroup(group, locale)).collect(Collectors.toList())); | |
| public UserDto getUser(AbstractUser user, Locale locale) { | |
| Collection<String> groupIds = user.getGroupIds(); | |
| List<GroupDto> groupDtos = (groupIds != null && !groupIds.isEmpty()) | |
| ? groupService.findAllByIds(groupIds, Pageable.unpaged()) | |
| .stream() | |
| .map(group -> GroupDto.fromGroup(group, locale)) | |
| .collect(Collectors.toList()) | |
| : List.of(); | |
| UserDto result = UserDto.createUser(user, groupDtos); |
🤖 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/UserFactoryImpl.java`
around lines 26 - 27, In UserFactoryImpl.getUser, guard against a null
AbstractUser.getGroupIds() before calling groupService.findAllByIds: compute a
local Collection<Long> groupIds = (user.getGroupIds() == null ?
Collections.emptyList() : user.getGroupIds()), call
groupService.findAllByIds(groupIds, Pageable.unpaged()), map results with
GroupDto.fromGroup(locale) into a List<GroupDto> groups variable, then pass that
groups list into UserDto.createUser; this both prevents NPEs and makes the line
more readable.
Reordered parameters in `addUser` and `removeUser` methods across the codebase to consistently place `group` before `user`. This improves clarity and aligns method signatures for better maintainability and usage consistency.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java (2)
239-252:⚠️ Potential issue | 🟠 MajorNull-safe the bulk diff logic before creating
HashSets.
group.getMemberIds()and the incominguserIdsset can both benull. Passingnulltonew HashSet<>(null)throwsNullPointerException. Normalize both values before computing the diff.💡 Proposed fix
`@Override` public Group assignUsersToGroup(String groupId, Set<String> userIds) { Group group = this.findById(groupId); - Set<String> currentGroupMemberIds = group.getMemberIds(); + Set<String> currentGroupMemberIds = Optional.ofNullable(group.getMemberIds()).orElseGet(Collections::emptySet); + Set<String> requestedUserIds = Optional.ofNullable(userIds).orElseGet(Collections::emptySet); - Set<String> removableMemberIds = new HashSet<>(currentGroupMemberIds); - removableMemberIds.removeAll(userIds); + Set<String> removableMemberIds = new HashSet<>(currentGroupMemberIds); + removableMemberIds.removeAll(requestedUserIds); - Set<String> newMemberIds = new HashSet<>(userIds); + Set<String> newMemberIds = new HashSet<>(requestedUserIds); newMemberIds.removeAll(currentGroupMemberIds); removableMemberIds.forEach(toBeRemovedId -> removeUser(group, userService.findById(toBeRemovedId, group.getRealmId()))); newMemberIds.forEach(toBeAddedId -> addUser(group, userService.findById(toBeAddedId, group.getRealmId()))); return group; }🤖 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/GroupServiceImpl.java` around lines 239 - 252, Make the bulk-diff null-safe in assignUsersToGroup by normalizing group.getMemberIds() and the incoming userIds to empty sets before constructing removableMemberIds and newMemberIds; ensure you never call new HashSet<>(null) — use a safe empty Set fallback for currentGroupMemberIds and userIds, then compute removableMemberIds and newMemberIds and continue calling removeUser(...) and addUser(...) with userService.findById(...) as before.
366-380:⚠️ Potential issue | 🟠 MajorApply the same null-safety fix for
assignSubgroups.Same NPE risk as
assignUsersToGroup:parentGroup.getSubgroupIds()andchildGroupIdscan benull.💡 Proposed fix
`@Override` public Group assignSubgroups(String parentGroupId, Set<String> childGroupIds) { Group parentGroup = this.findById(parentGroupId); - Set<String> currentSubgroupIds = parentGroup.getSubgroupIds(); + Set<String> currentSubgroupIds = Optional.ofNullable(parentGroup.getSubgroupIds()).orElseGet(Collections::emptySet); + Set<String> requestedChildIds = Optional.ofNullable(childGroupIds).orElseGet(Collections::emptySet); - Set<String> removableGroupIds = new HashSet<>(currentSubgroupIds); - removableGroupIds.removeAll(childGroupIds); + Set<String> removableGroupIds = new HashSet<>(currentSubgroupIds); + removableGroupIds.removeAll(requestedChildIds); - Set<String> newSubgroupIds = new HashSet<>(childGroupIds); + Set<String> newSubgroupIds = new HashSet<>(requestedChildIds); newSubgroupIds.removeAll(currentSubgroupIds); ... }🤖 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/GroupServiceImpl.java` around lines 366 - 380, The assignSubgroups method can NPE when parentGroup.getSubgroupIds() or childGroupIds are null; mirror the fix used in assignUsersToGroup by normalizing both to non-null sets before computing removable/new sets. Specifically, in assignSubgroups retrieve parentGroup via findById, then replace parentGroup.getSubgroupIds() with a safeSet (e.g., currentSubgroupIds = Optional.ofNullable(parentGroup.getSubgroupIds()).orElseGet(HashSet::new)) and likewise ensure childGroupIds = Optional.ofNullable(childGroupIds).orElseGet(HashSet::new); then compute removableGroupIds/newSubgroupIds and call removeSubgroup/addSubgroup as before.application-engine/src/main/java/com/netgrif/application/engine/auth/web/GroupController.java (4)
92-94:⚠️ Potential issue | 🟠 MajorResolve owner within the requested realm.
The owner lookup at Line 92 uses
nullfor the realm parameter instead ofrequest.realmId(). This could allow creating a group in one realm with an owner from a different realm, breaking multi-tenancy isolation.-AbstractUser user = userService.findById(request.ownerId(), null); +AbstractUser user = userService.findById(request.ownerId(), request.realmId()); if (user == null) { - return ResponseEntity.status(HttpStatus.NOT_FOUND).body(ResponseMessage.createErrorMessage("User with id [%s] not found".formatted(request.ownerId()))); + return ResponseEntity.status(HttpStatus.NOT_FOUND).body(ResponseMessage.createErrorMessage("User with id [%s] not found in realm [%s]".formatted(request.ownerId(), request.realmId()))); }🤖 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/auth/web/GroupController.java` around lines 92 - 94, The owner lookup in GroupController uses userService.findById(request.ownerId(), null) which ignores realm isolation; update the lookup to pass the requested realm (use request.realmId()) so the owner is resolved within the same realm as the group creation (locate the userService.findById call inside the GroupController method handling the group create request and replace the null realm argument with request.realmId()).
67-70:⚠️ Potential issue | 🟠 MajorPagination metadata is lost when transforming to DTOs.
The
transformPageContenthelper at Line 450 usesgroups.size()as the total element count, which is just the current page size—not the actual total. This breaks pagination for clients, as they cannot determine total pages or navigate correctly.💡 Proposed fix
-private Page<GroupDto> transformPageContent(List<Group> groups, Pageable pageable, Locale locale) { - return new PageImpl<>(groups.stream().map(group -> GroupDto.fromGroup(group, locale)).toList(), pageable, groups.size()); +private Page<GroupDto> transformPageContent(Page<Group> groups, Locale locale) { + return new PageImpl<>( + groups.getContent().stream().map(group -> GroupDto.fromGroup(group, locale)).toList(), + groups.getPageable(), + groups.getTotalElements() + ); }Then update callers:
// In getAllGroupsOfRealm: Page<Group> groups = groupService.findAllFromRealm(realmId, pageable); -return ResponseEntity.ok(transformPageContent(groups.getContent(), pageable, locale)); +return ResponseEntity.ok(transformPageContent(groups, locale)); // In search: -List<Group> groups = groupService.search(query, pageable).getContent(); -return ResponseEntity.ok(transformPageContent(groups, pageable, locale)); +Page<Group> groups = groupService.search(query, pageable); +return ResponseEntity.ok(transformPageContent(groups, locale));Also applies to: 355-357, 450-452
🤖 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/auth/web/GroupController.java` around lines 67 - 70, The transformation loses pagination metadata because transformPageContent uses collection.size() instead of the Page total; update transformPageContent (used by methods like GroupController.getAllGroupsOfRealm and other callers) to accept the original Page<T> or an explicit totalElements parameter and use page.getTotalElements() (or the passed total) when constructing the DTO Page<Page<GroupDto>>/PageImpl so totalPages and navigation remain correct; then update all callers (e.g., getAllGroupsOfRealm) to pass the original Page or totalElements rather than relying on the page content size.
273-283:⚠️ Potential issue | 🟠 MajorBatch mutations are not atomic—partial updates persist on failure.
Multiple endpoints iterate over a set of IDs (e.g.,
roleIds.forEach(...)) and call individual service methods. If one ID is invalid, earlier operations have already been committed, leaving the group in an inconsistent state while returning a 400 error.Consider either:
- Validating all IDs upfront before any mutations
- Using a single transactional bulk operation in the service layer
- Accepting partial success and documenting it in the API
Example for
addRolesToGroup:// Option 1: Pre-validate Set<ProcessRole> roles = roleIds.stream() .map(id -> processRoleService.findById(new ProcessResourceId(id))) .peek(role -> { if (role == null) throw new IllegalArgumentException("Role not found"); }) .collect(Collectors.toSet()); // Then apply all at onceAlso applies to: 294-304, 315-325, 336-346, 417-427, 438-448
🤖 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/auth/web/GroupController.java` around lines 273 - 283, The loop in addRolesToGroup calls groupService.addRole per ID which allows partial commits on error; instead either validate all roleIds up front (e.g., resolve each ID to a domain Role and throw if any missing) before calling any mutation, or add a single transactional bulk service method (e.g., groupService.addRoles(groupId, Set<String>) or groupService.addRolesTransactional) that performs the whole operation inside a transaction and throws to trigger rollback; apply the same pattern for the other endpoints that iterate and call per-id service methods (the blocks using roleIds.forEach(...) / groupService.addRole and their counterparts).
96-98:⚠️ Potential issue | 🟡 MinorReturn HTTP 201 Created for successful group creation.
The
@ApiResponseat Line 77 documents response code 201 for successful creation, but the implementation at Line 98 returns200 OK. UseHttpStatus.CREATEDfor consistency with REST conventions and the documented API contract.groupService.create(request.identifier(), request.displayName(), user); - return ResponseEntity.ok(ResponseMessage.createSuccessMessage("Group created successfully")); + return ResponseEntity.status(HttpStatus.CREATED).body(ResponseMessage.createSuccessMessage("Group created successfully"));🤖 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/auth/web/GroupController.java` around lines 96 - 98, The controller currently returns ResponseEntity.ok(...) after calling groupService.create(request.identifier(), request.displayName(), user);; change the response to return HTTP 201 Created instead of 200 OK by replacing the ResponseEntity.ok(...) usage with a ResponseEntity constructed with HttpStatus.CREATED (while still returning ResponseMessage.createSuccessMessage("Group created successfully")). Update the return in the method that calls groupService.create(...) so it matches the `@ApiResponse` documentation.nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java (1)
208-215: 🧹 Nitpick | 🔵 TrivialJavadoc parameter name mismatch.
The
@paramtag at Line 212 referencesgroupIdentifier, but the actual method parameter is namedgroupId. This inconsistency can confuse developers using the API./** * Removes a user from a group specified by its unique identifier. * - * `@param` user the user to be removed - * `@param` groupIdentifier the unique identifier of the target group + * `@param` groupId the ID of the group + * `@param` user the user to be removed * `@return` the updated {`@link` Group} */ Group removeUser(String groupId, AbstractUser user);🤖 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/GroupService.java` around lines 208 - 215, The Javadoc for GroupService.removeUser references a non-existent parameter name `groupIdentifier`; update the Javadoc `@param` to match the actual method signature parameter `groupId` (or rename the method parameter to `groupIdentifier` if you prefer that name) so the `@param` tag and method parameter (GroupService.removeUser(String groupId, AbstractUser user)) are consistent and accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/auth/web/GroupController.java`:
- Around line 348-358: The search method in GroupController (public
ResponseEntity<Page<GroupDto>> search(...)) is missing an `@PreAuthorize`
restriction; add the same `@PreAuthorize` expression used by the other endpoints
in GroupController (e.g., the one applied to create/update/delete methods) to
restrict who can call search and prevent unrestricted group enumeration, ensure
the annotation is placed immediately above the `@Operation/`@GetMapping and import
the required Spring Security annotation.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java`:
- Around line 283-286: The method signature for removeUser in GroupServiceImpl
does not match the GroupService interface—change the signature from
removeUser(String userId, String groupId, String realmId) to removeUser(String
groupId, String userId, String realmId) so the `@Override` is correct, and ensure
the implementation calls removeUser(groupId, userService.findById(userId,
realmId)) (keeping the userService.findById call using the userId and realmId in
that order).
In
`@nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java`:
- Around line 252-266: Add Javadoc comments for the two overloaded methods in
the GroupService interface: addAuthority(Group group, Authority authority) and
removeAuthority(Group group, Authority authority). For each method, document the
purpose, parameters (describe Group and Authority parameters), and the return
value (the updated Group), following the style of the existing
removeAuthority(String, String) comment so API docs remain consistent and
complete.
---
Duplicate comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/auth/web/GroupController.java`:
- Around line 92-94: The owner lookup in GroupController uses
userService.findById(request.ownerId(), null) which ignores realm isolation;
update the lookup to pass the requested realm (use request.realmId()) so the
owner is resolved within the same realm as the group creation (locate the
userService.findById call inside the GroupController method handling the group
create request and replace the null realm argument with request.realmId()).
- Around line 67-70: The transformation loses pagination metadata because
transformPageContent uses collection.size() instead of the Page total; update
transformPageContent (used by methods like GroupController.getAllGroupsOfRealm
and other callers) to accept the original Page<T> or an explicit totalElements
parameter and use page.getTotalElements() (or the passed total) when
constructing the DTO Page<Page<GroupDto>>/PageImpl so totalPages and navigation
remain correct; then update all callers (e.g., getAllGroupsOfRealm) to pass the
original Page or totalElements rather than relying on the page content size.
- Around line 273-283: The loop in addRolesToGroup calls groupService.addRole
per ID which allows partial commits on error; instead either validate all
roleIds up front (e.g., resolve each ID to a domain Role and throw if any
missing) before calling any mutation, or add a single transactional bulk service
method (e.g., groupService.addRoles(groupId, Set<String>) or
groupService.addRolesTransactional) that performs the whole operation inside a
transaction and throws to trigger rollback; apply the same pattern for the other
endpoints that iterate and call per-id service methods (the blocks using
roleIds.forEach(...) / groupService.addRole and their counterparts).
- Around line 96-98: The controller currently returns ResponseEntity.ok(...)
after calling groupService.create(request.identifier(), request.displayName(),
user);; change the response to return HTTP 201 Created instead of 200 OK by
replacing the ResponseEntity.ok(...) usage with a ResponseEntity constructed
with HttpStatus.CREATED (while still returning
ResponseMessage.createSuccessMessage("Group created successfully")). Update the
return in the method that calls groupService.create(...) so it matches the
`@ApiResponse` documentation.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java`:
- Around line 239-252: Make the bulk-diff null-safe in assignUsersToGroup by
normalizing group.getMemberIds() and the incoming userIds to empty sets before
constructing removableMemberIds and newMemberIds; ensure you never call new
HashSet<>(null) — use a safe empty Set fallback for currentGroupMemberIds and
userIds, then compute removableMemberIds and newMemberIds and continue calling
removeUser(...) and addUser(...) with userService.findById(...) as before.
- Around line 366-380: The assignSubgroups method can NPE when
parentGroup.getSubgroupIds() or childGroupIds are null; mirror the fix used in
assignUsersToGroup by normalizing both to non-null sets before computing
removable/new sets. Specifically, in assignSubgroups retrieve parentGroup via
findById, then replace parentGroup.getSubgroupIds() with a safeSet (e.g.,
currentSubgroupIds =
Optional.ofNullable(parentGroup.getSubgroupIds()).orElseGet(HashSet::new)) and
likewise ensure childGroupIds =
Optional.ofNullable(childGroupIds).orElseGet(HashSet::new); then compute
removableGroupIds/newSubgroupIds and call removeSubgroup/addSubgroup as before.
In
`@nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java`:
- Around line 208-215: The Javadoc for GroupService.removeUser references a
non-existent parameter name `groupIdentifier`; update the Javadoc `@param` to
match the actual method signature parameter `groupId` (or rename the method
parameter to `groupIdentifier` if you prefer that name) so the `@param` tag and
method parameter (GroupService.removeUser(String groupId, AbstractUser user))
are consistent and accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0942a262-4e8a-4486-8c87-093332393f87
📒 Files selected for processing (6)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/auth/web/GroupController.javaapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.javanae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.javanae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java
Comment on lines
+348
to
+358
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "Groups retrieved successfully"), | ||
| @ApiResponse(responseCode = "400", description = "Invalid group data"), | ||
| @ApiResponse(responseCode = "500", description = "Internal server error") | ||
| }) | ||
| @Operation(summary = "Generic group search", security = {@SecurityRequirement(name = "X-Auth-Token")}) | ||
| @GetMapping(produces = MediaType.APPLICATION_JSON_VALUE) | ||
| public ResponseEntity<Page<GroupDto>> search(GroupSearchRequestDto query, Pageable pageable, Locale locale) { | ||
| List<Group> groups = groupService.search(query, pageable).getContent(); | ||
| return ResponseEntity.ok(transformPageContent(groups, pageable, locale)); | ||
| } |
There was a problem hiding this comment.
Missing authorization on search endpoint.
The search endpoint at Line 354 lacks @PreAuthorize annotation, unlike all other endpoints in this controller. This allows any authenticated user to search and enumerate groups, potentially exposing sensitive organizational structure. Add authorization if this is unintended.
+@PreAuthorize("@authorizationServiceImpl.hasAuthority('ADMIN')")
`@ApiResponses`(value = {
`@ApiResponse`(responseCode = "200", description = "Groups retrieved successfully"),
`@ApiResponse`(responseCode = "400", description = "Invalid group data"),
+ `@ApiResponse`(responseCode = "403", description = "Caller doesn't fulfill the authorisation requirements"),
`@ApiResponse`(responseCode = "500", description = "Internal server error")
})
`@Operation`(summary = "Generic group search", security = {`@SecurityRequirement`(name = "X-Auth-Token")})
`@GetMapping`(produces = MediaType.APPLICATION_JSON_VALUE)
public ResponseEntity<Page<GroupDto>> search(GroupSearchRequestDto query, Pageable pageable, Locale locale) {🤖 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/auth/web/GroupController.java`
around lines 348 - 358, The search method in GroupController (public
ResponseEntity<Page<GroupDto>> search(...)) is missing an `@PreAuthorize`
restriction; add the same `@PreAuthorize` expression used by the other endpoints
in GroupController (e.g., the one applied to create/update/delete methods) to
restrict who can call search and prevent unrestricted group enumeration, ensure
the annotation is placed immediately above the `@Operation/`@GetMapping and import
the required Spring Security annotation.
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java
Show resolved
Hide resolved
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java
Show resolved
Hide resolved
Introduced a new method to remove an authority from a Group object directly, enhancing flexibility in authority management. This complements the existing method by providing an additional, more intuitive way to update groups.
Introduced a new method `addAuthority(Group, Authority)` in `GroupService` to enable assigning a specific authority to a group. This enhances the service by allowing direct updates to a group's associated authorities.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java`:
- Around line 208-215: Update the Javadoc for GroupService.removeUser so the
`@param` name matches the method signature: replace the incorrect `@param`
groupIdentifier with `@param` groupId (or alternatively rename the method
parameter to groupIdentifier and update references); ensure the Javadoc text
describes the same value (the unique identifier of the target group) and that
the method signature Group removeUser(String groupId, AbstractUser user) stays
consistent with the documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94254b58-a55d-4da9-8a4f-fc855701abd6
📒 Files selected for processing (1)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java
Comment on lines
208
to
+215
| /** | ||
| * Removes a user from a group specified by identifier. | ||
| * Removes a user from a group specified by its unique identifier. | ||
| * | ||
| * @param user the user to remove | ||
| * @param groupIdentifier identifier of the target group | ||
| * @param user the user to be removed | ||
| * @param groupIdentifier the unique identifier of the target group | ||
| * @return the updated {@link Group} | ||
| */ | ||
| Group removeUser(AbstractUser user, String groupIdentifier); | ||
| Group removeUser(String groupId, AbstractUser user); |
There was a problem hiding this comment.
Javadoc parameter name doesn't match method signature.
The Javadoc on line 212 references @param groupIdentifier but the actual method parameter on line 215 is named groupId. This creates confusion for API consumers.
📝 Proposed fix
/**
* Removes a user from a group specified by its unique identifier.
*
- * `@param` user the user to be removed
- * `@param` groupIdentifier the unique identifier of the target group
+ * `@param` groupId the unique identifier of the target group
+ * `@param` user the user to be removed
* `@return` the updated {`@link` Group}
*/
Group removeUser(String groupId, AbstractUser user);🤖 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/GroupService.java`
around lines 208 - 215, Update the Javadoc for GroupService.removeUser so the
`@param` name matches the method signature: replace the incorrect `@param`
groupIdentifier with `@param` groupId (or alternatively rename the method
parameter to groupIdentifier and update references); ensure the Javadoc text
describes the same value (the unique identifier of the target group) and that
the method signature Group removeUser(String groupId, AbstractUser user) stays
consistent with the documentation.
Removed unnecessary blank lines to improve code readability and maintain consistent formatting in the GroupService class. No functionality was altered.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java`:
- Around line 198-205: Fix the Javadoc formatting for the removeUser method in
GroupService by ensuring each line in the comment block follows the standard
Javadoc pattern (a space after the leading asterisk). Update the lines
describing parameters so they start with " * " (e.g., " * `@param` groupId ...", "
* `@param` userId ...", " * `@param` realmId ...") to match the rest of the block
above the method Group removeUser(String groupId, String userId, String
realmId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 834b6b51-d01d-4c1b-8517-0c1d56b729ee
📒 Files selected for processing (1)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/GroupService.java
Comment on lines
198
to
+205
| /** | ||
| * Removes a user from a group specified by identifier. | ||
| * Removes a user from a group within a specific realm. | ||
| * @param groupId the unique identifier of the group | ||
| * @param userId the ID of the user to remove | ||
| * @param realmId the ID of the realm where the group exists | ||
| * @return the updated {@link Group} | ||
| */ | ||
| Group removeUser(String groupId, String userId, String realmId); |
There was a problem hiding this comment.
Minor Javadoc formatting inconsistency.
Lines 200-201 are missing the leading space before the asterisk, while the rest of the Javadoc block uses standard formatting.
📝 Suggested fix
/**
* Removes a user from a group within a specific realm.
- * `@param` groupId the unique identifier of the group
- * `@param` userId the ID of the user to remove
+ * `@param` groupId the unique identifier of the group
+ * `@param` userId the ID of the user to remove
* `@param` realmId the ID of the realm where the group exists
* `@return` the updated {`@link` Group}
*/📝 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
| /** | |
| * Removes a user from a group specified by identifier. | |
| * Removes a user from a group within a specific realm. | |
| * @param groupId the unique identifier of the group | |
| * @param userId the ID of the user to remove | |
| * @param realmId the ID of the realm where the group exists | |
| * @return the updated {@link Group} | |
| */ | |
| Group removeUser(String groupId, String userId, String realmId); | |
| /** | |
| * Removes a user from a group within a specific realm. | |
| * `@param` groupId the unique identifier of the group | |
| * `@param` userId the ID of the user to remove | |
| * `@param` realmId the ID of the realm where the group exists | |
| * `@return` the updated {`@link` Group} | |
| */ | |
| Group removeUser(String groupId, String userId, String realmId); |
🤖 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/GroupService.java`
around lines 198 - 205, Fix the Javadoc formatting for the removeUser method in
GroupService by ensuring each line in the comment block follows the standard
Javadoc pattern (a space after the leading asterisk). Update the lines
describing parameters so they start with " * " (e.g., " * `@param` groupId ...", "
* `@param` userId ...", " * `@param` realmId ...") to match the rest of the block
above the method Group removeUser(String groupId, String userId, String
realmId).
Ensure proper initialization of memberIds and subgroupIds to avoid null references. Adjust method parameter order in removeUser for consistency and readability.
Introduced a method to find groups by process roles in `GroupService` and `GroupRepository`. Refactored role deletion to also handle removal of process roles from groups alongside users. This ensures roles of deleted Petri nets are cleaned up consistently across users and groups.
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
Enhanced implementation of Group API, including Action API, REST API and Service API.
Implements NAE-2286
Dependencies
Added one new third party dependency
Third party dependencies
Blocking Pull requests
There are no dependencies on other PR.
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Changes
Documentation
Tests