[NAE-2286] Group API - #423
Open
renczesstefan wants to merge 123 commits into
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. 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
+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?
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
…Id` and deprecate `findByNetworkIdAndObjectId`.
…te management and enhanced executor configuration. - Added methods to `ActionDelegate` to manage asynchronous execution lifecycle (`retainForAsyncExecution`, `releaseAfterAsyncExecution`, `clearAfterExecution`). - Enhanced `AsyncRunner` with delegate state tracking and custom `actionsExecutor`. - Added `async_run.xml` test Petri net and corresponding test cases to validate asynchronous action handling.
…te management and enhanced executor configuration. - Added methods to `ActionDelegate` to manage asynchronous execution lifecycle (`retainForAsyncExecution`, `releaseAfterAsyncExecution`, `clearAfterExecution`). - Enhanced `AsyncRunner` with delegate state tracking and custom `actionsExecutor`. - Added `async_run.xml` test Petri net and corresponding test cases to validate asynchronous action handling.
- Update `server-patterns` to replace `/manage/**` with `/manage/health` - Configure `management.endpoint.shutdown.enabled` as `false` across properties - Expand `management.endpoints.web.exposure.include` for additional actuator endpoints
…logic; consolidate MongoTemplate usage to default bean.
# Conflicts: # application-engine/src/test/groovy/com/netgrif/application/engine/TestHelper.groovy # application-engine/src/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovy
… and `TestHelper` for consistency and clarity.
…d` and deprecate `findByNetworkIdAndObjectId`; update tests and enums accordingly.
…ariable naming consistency.
…tringQuery` with `BoolQuery`, introduce `FullTextField` model, and enhance wildcard handling.
…ce static imports with dynamic discovery, introduce `ACTION_IMPORT_PACKAGES`, and optimize class loading.
[NAE-2464] Release 1.0.1 Bugfixes
…ce static imports with dynamic discovery, introduce `ACTION_IMPORT_PACKAGES`, and optimize class loading.
- Update version to 7.0.2 in all affected `pom.xml` files - Modify `Dockerfile` and `Dockerfile.multi-stage` to use version 7.0.2 - Adjust `docker-compose.yml` to reflect the new image version
Remove the dedicated actions executor bean Log ambiguous automatic Groovy action imports instead of silently skipping them Prevent dev profile from dropping MongoDB and Elasticsearch data by default Update deprecated case repository methods to version 7.0.2 Remove obsolete migration MongoTemplate bean configuration Validate Elasticsearch full-text field boost values before applying them
…r regex handling consistency in full-text search normalization methods.
…ull-text search behavior and edge cases.
…r string normalization in full-text search
…ion for method usage
…e string replacement in full-text search normalization
[NAE-2464] Post release fixes
…s filtering, improve action import handling, and remove unused `ProcessResourceId` logic.
…cement` usage in full-text search normalization
…chRequestDto` to use class-based implementation, and extend `AuthorityDto` with `stringId`.
…d add realm validation for group hierarchy integrity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
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