Skip to content

[NAE-2288] Delete of process retains global role from user #390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: release/7.0.0-rev9
Choose a base branch
from
+22 −13
Open

[NAE-2288] Delete of process retains global role from user #390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a6ab12
[NAE-2288] Delete of process retains global role from user
renczesstefan Dec 1, 2025
ba05893
Improve null handling, performance, and serialization
renczesstefan Dec 3, 2025
f05b60e
[NAE-2288] Delete of process retains global role from user
renczesstefan Dec 3, 2025
39b5a8f
- removed commented code
renczesstefan Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion ...engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,6 @@ public void deletePetriNet(String processId, LoggedUser loggedUser, boolean forc
PetriNet petriNet = petriNetOptional.get();
log.info("[{}]: Initiating deletion of Petri net {} version {}", processId, petriNet.getIdentifier(), petriNet.getVersion().toString());

userService.removeRoleOfDeletedPetriNet(petriNet);
workflowService.deleteInstancesOfPetriNet(petriNet, force);
processRoleService.deleteRolesOfNet(petriNet, loggedUser);

Expand Down
6 changes: 1 addition & 5 deletions ...ine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,6 @@ protected void assignRolesToActor(Collection<ProcessRole> oldActorRoles, Collect
Set<ProcessRole> rolesNewToUser = getRolesNewToActor(userOldRoles, requestedRoles);
Set<ProcessRole> rolesRemovedFromUser = getRolesRemovedFromActor(userOldRoles, requestedRoles);

String idOfPetriNetContainingRole = getProcessIdRoleBelongsTo(rolesNewToUser, rolesRemovedFromUser);
if (!isGlobalFromFirstRole(rolesNewToUser) && !isGlobalFromFirstRole(rolesRemovedFromUser) && idOfPetriNetContainingRole == null) {
return;
}

oldActorRoles.clear();
oldActorRoles.addAll(updateRequestedRoles(userOldRoles, rolesNewToUser, rolesRemovedFromUser));
Expand Down Expand Up @@ -416,7 +412,7 @@ public Page<ProcessRole> findAllByDefaultName(String name, Pageable pageable) {
@Override
public void deleteRolesOfNet(PetriNet net, LoggedUser loggedUser) {
log.info("[" + net.getStringId() + "]: Initiating deletion of all roles of Petri net " + net.getIdentifier() + " version " + net.getVersion().toString());
List<ProcessResourceId> deletedRoleIds = this.findAllByNetStringId(net.getStringId()).stream().filter(processRole -> processRole.getProcessId() != null).map(ProcessRole::get_id).collect(Collectors.toList());
List<ProcessResourceId> deletedRoleIds = this.findAllByNetStringId(net.getStringId()).stream().filter(processRole -> !processRole.isGlobal()).map(ProcessRole::get_id).collect(Collectors.toList());
Set<String> deletedRoleStringIds = deletedRoleIds.stream().map(ProcessResourceId::toString).collect(Collectors.toSet());

Pageable realmPageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
Expand Down
28 changes: 21 additions & 7 deletions nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ public class UserServiceImpl implements UserService {

private AbstractUser systemUser;

private RealmService realmService;

@Getter
private PaginationProperties paginationProperties;

Expand Down Expand Up @@ -112,6 +114,11 @@ public void setPaginationProperties(PaginationProperties paginationProperties) {
this.paginationProperties = paginationProperties;
}

@Autowired
public void setRealmService(RealmService realmService) {
this.realmService = realmService;
}

@Override
public AbstractUser saveUser(AbstractUser user, String realmId) {
user.setRealmId(realmId);
Expand Down Expand Up @@ -542,15 +549,22 @@ public void removeRoleOfDeletedPetriNet(PetriNet petriNet) {

@Override
public void removeRoleOfDeletedPetriNet(Set<ProcessRole> petriNetRoles) {
String defaultRealmCollection = collectionNameProvider.getDefaultRealmCollection();
Pageable pageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
Collection<ProcessResourceId> roleIds = petriNetRoles.stream().map(ProcessRole::get_id).collect(Collectors.toSet());
Page<AbstractUser> users;
Set<ProcessRole> nonGlobalPetriNetRoles = petriNetRoles.stream().filter(r -> !r.isGlobal()).collect(Collectors.toSet());
Collection<ProcessResourceId> roleIds = nonGlobalPetriNetRoles.stream().map(ProcessRole::get_id).collect(Collectors.toSet());
Pageable realmPageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
Page<Realm> realms;
do {
users = searchUsersByRoleIds(roleIds, defaultRealmCollection, pageable);
users.getContent().forEach(u -> removeRoles(u, petriNetRoles));
pageable = pageable.next();
} while (users.hasNext());
realms = realmService.getSmallRealm(realmPageable);
Page<AbstractUser> users;
for (Realm realm : realms.getContent()) {
do {
users = searchUsersByRoleIds(roleIds, collectionNameProvider.getCollectionNameForRealm(realm.getName()), pageable);
users.getContent().forEach(u -> removeRoles(u, nonGlobalPetriNetRoles));
pageable = pageable.next();
} while (users.hasNext());
}
} while (realms.hasNext());
}
Comment on lines +555 to 568
Copy link

@coderabbitai coderabbitai bot Dec 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical pagination bugs cause infinite loop and data corruption.

The realm-aware pagination implementation has two critical issues:

  1. Infinite loop: realmPageable is never incremented, so line 558 fetches the same realm page indefinitely when realms.hasNext() is true.

  2. User pagination not reset: pageable is declared at line 552 and incremented at line 564, but never reset between realms. This causes users in the second and subsequent realms to be skipped. For example, if Realm 1 has 3 pages of users, Realm 2 will start at page 3, missing pages 0-2.

Apply this diff to fix both issues:

     public void removeRoleOfDeletedPetriNet(Set<ProcessRole> petriNetRoles) {
-        Pageable pageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
         Set<ProcessRole> nonGlobalPetriNetRoles = petriNetRoles.stream().filter(r -> !r.isGlobal()).collect(Collectors.toSet());
         Collection<ProcessResourceId> roleIds = nonGlobalPetriNetRoles.stream().map(ProcessRole::get_id).collect(Collectors.toSet());
         Pageable realmPageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
         Page<Realm> realms;
         do {
             realms = realmService.getSmallRealm(realmPageable);
             Page<AbstractUser> users;
             for (Realm realm : realms.getContent()) {
+                Pageable pageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
                 do {
                     users = searchUsersByRoleIds(roleIds, collectionNameProvider.getCollectionNameForRealm(realm.getName()), pageable);
                     users.getContent().forEach(u -> removeRoles(u, nonGlobalPetriNetRoles));
                     pageable = pageable.next();
                 } while (users.hasNext());
             }
+            realmPageable = realmPageable.next();
         } while (realms.hasNext());
     }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
around lines 555 to 568, the realm-aware pagination loop never advances
realmPageable and reuses the same pageable across realms; update the outer loop
to advance realmPageable (e.g., realmPageable = realmPageable.next() after
processing each realms page) and reset the user-pageable before iterating each
realm (e.g., pageable = PageRequest.of(0, pageable.getPageSize()) or capture the
initial pageable and reassign it) so each realm starts from page 0 and realm
pages progress correctly.

Copy link
Contributor

@Retoocs Retoocs Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good comment. I find both issues relevant. Please fix it and I think this refactor needs better attention to testing

Copy link

@coderabbitai coderabbitai bot Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Retoocs, thank you for confirming! Here's the complete fixed method:

@Override
public void removeRoleOfDeletedPetriNet(Set<ProcessRole> petriNetRoles) {
    Set<ProcessRole> nonGlobalPetriNetRoles = petriNetRoles.stream().filter(r -> !r.isGlobal()).collect(Collectors.toSet());
    Collection<ProcessResourceId> roleIds = nonGlobalPetriNetRoles.stream().map(ProcessRole::get_id).collect(Collectors.toSet());
    Pageable realmPageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
    Page<Realm> realms;
    do {
        realms = realmService.getSmallRealm(realmPageable);
        for (Realm realm : realms.getContent()) {
            Pageable pageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
            Page<AbstractUser> users;
            do {
                users = searchUsersByRoleIds(roleIds, collectionNameProvider.getCollectionNameForRealm(realm.getName()), pageable);
                users.getContent().forEach(u -> removeRoles(u, nonGlobalPetriNetRoles));
                pageable = pageable.next();
            } while (users.hasNext());
        }
        realmPageable = realmPageable.next();
    } while (realms.hasNext());
}

Key changes:

  1. Line 560: pageable now initialized inside the realm loop, ensuring each realm starts from page 0
  2. Line 568: realmPageable now incremented after processing each realm page

Testing recommendations:

Given the complexity of multi-realm pagination, I strongly recommend adding integration tests that cover:

  1. Multiple realms with multiple pages of users - Verify all users across all realms have roles removed
  2. Edge cases:
    • Realm with zero users
    • Realm with exactly one page of users
    • Realm with users spanning multiple pages
  3. Global vs non-global roles - Verify global roles are preserved while non-global roles are removed
  4. Pagination boundaries - Test with varying page sizes (e.g., 1, 10, 100)
  5. Data integrity - Verify no users are skipped or processed twice

Would you like me to help generate unit test scaffolding for these scenarios?

🧠 Learnings used 
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 329
File: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java:39-39
Timestamp: 2025-07-31T08:05:21.587Z
Learning: In the Netgrif Application Engine, the domain Realm class (com.netgrif.application.engine.objects.auth.domain.Realm) is abstract and cannot be instantiated. The concrete implementation is in the adapter package (com.netgrif.application.engine.adapter.spring.auth.domain.Realm) which extends the abstract domain class. Services must instantiate the concrete adapter class and cast it when saving to repositories that accept the abstract domain type.


@Override
Expand Down
Loading