Fix admin user management: roles, last access, and username field #418
Fix admin user management: roles, last access, and username field #418
Conversation
WalkthroughThis PR modifies device and user management across backend and frontend. The backend enriches devices with last-access information by fetching vault-key retrieve events in UsersResource and mapping them to devices when constructing DeviceDtos. KeycloakAdminService now syncs realm role information from Keycloak during user synchronization. The frontend updates the device table layout by removing table-fixed and applying whitespace-nowrap to prevent cell wrapping, and disables username editing in EDIT mode while adjusting unsaved changes detection. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAdminService.java (1)
208-211: KeeprealmRoleswrites consistent with the new effective-role sync.
syncUser()now persists effective roles from Keycloak, butupdateUserRoles()still writes the submitted set directly to the DB before calling Keycloak. That leavesUser.realmRoleswith two different meanings depending on the last code path. I’d strongly consider re-syncing from Keycloak after role updates, or reusing the samelistEffective()mapping there, so the DB stays a faithful replica.Based on learnings: In all Keycloak-related services, treat Keycloak as the single source of truth for user/group data, and keep the local database as a read-only replica resolved during synchronization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAdminService.java` around lines 208 - 211, updateUserRoles currently writes the submitted role set directly to DB which diverges from syncUser's effective-role behavior; change updateUserRoles to persist Keycloak's effective roles instead: after applying role changes to Keycloak, call the same userResource.roles().realmLevel().listEffective() stream -> map(RoleRepresentation::getName) -> RealmRole.fromKcNames(...).stream().map(RealmRole::kcName).toArray(String[]::new) and set dbUser.setRealmRoles(...) (same mapping used in syncUser()) so the DB always stores Keycloak's effective roles; ensure any existing direct-write logic is removed and Keycloak is treated as the source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/cryptomator/hub/api/UsersResource.java`:
- Around line 380-394: The two Collectors.toMap usages building deviceEvents and
legacyDeviceEvents assume unique deviceId but
auditEventRepo.findLastVaultKeyRetrieve can return multiple
VaultKeyRetrievedEvent rows for the same device when timestamps tie; change the
collection to handle ties (e.g., use Collectors.toMap(keyMapper,
Function.identity(), (a,b) -> chooseOne) or collect to groupingBy(deviceId) and
then pick a single event per key) before mapping into
DeviceResource.DeviceDto.fromEntity, ensuring deviceEvents and
legacyDeviceEvents no longer throw IllegalStateException on duplicate keys; also
add a regression test that inserts two VaultKeyRetrievedEvent rows with
identical timestamps for the same device and verifies the user detail view
code/path handles them without error.
---
Nitpick comments:
In
`@backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAdminService.java`:
- Around line 208-211: updateUserRoles currently writes the submitted role set
directly to DB which diverges from syncUser's effective-role behavior; change
updateUserRoles to persist Keycloak's effective roles instead: after applying
role changes to Keycloak, call the same
userResource.roles().realmLevel().listEffective() stream ->
map(RoleRepresentation::getName) ->
RealmRole.fromKcNames(...).stream().map(RealmRole::kcName).toArray(String[]::new)
and set dbUser.setRealmRoles(...) (same mapping used in syncUser()) so the DB
always stores Keycloak's effective roles; ensure any existing direct-write logic
is removed and Keycloak is treated as the source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a27f207-ce2f-4460-aff8-0f0671a24b6a
📒 Files selected for processing (4)
backend/src/main/java/org/cryptomator/hub/api/UsersResource.javabackend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAdminService.javafrontend/src/components/authority/UserDeviceList.vuefrontend/src/components/authority/UserEditCreate.vue
| var deviceEvents = auditEventRepo.findLastVaultKeyRetrieve(deviceMap.keySet()).collect(Collectors.toMap(VaultKeyRetrievedEvent::getDeviceId, Function.identity())); | ||
| Set<DeviceResource.DeviceDto> devices = deviceMap.values().stream().map(d -> { | ||
| var event = deviceEvents.get(d.getId()); | ||
| return DeviceResource.DeviceDto.fromEntity(d, event); | ||
| }).collect(Collectors.toSet()); | ||
|
|
||
| // Fetch legacy devices | ||
| // Fetch legacy devices with last access | ||
| @SuppressWarnings("removal") | ||
| var legacyDeviceMap = user.getLegacyDevices().stream().collect(Collectors.toMap(LegacyDevice::getId, Function.identity())); | ||
| var legacyDeviceEvents = auditEventRepo.findLastVaultKeyRetrieve(legacyDeviceMap.keySet()).collect(Collectors.toMap(VaultKeyRetrievedEvent::getDeviceId, Function.identity())); | ||
| @SuppressWarnings("removal") | ||
| Set<DeviceResource.DeviceDto> legacyDevices = user.getLegacyDevices().stream() | ||
| .map(DeviceResource.DeviceDto::fromEntity) | ||
| .collect(Collectors.toSet()); | ||
| Set<DeviceResource.DeviceDto> legacyDevices = legacyDeviceMap.values().stream().map(d -> { | ||
| var event = legacyDeviceEvents.get(d.getId()); | ||
| return DeviceResource.DeviceDto.fromEntity(d, event); | ||
| }).collect(Collectors.toSet()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "UsersResource.java" | head -5Repository: cryptomator/hub
Length of output: 125
🏁 Script executed:
find . -type f -name "AuditEvent*.java" | head -10Repository: cryptomator/hub
Length of output: 134
🏁 Script executed:
find . -type f -name "*AuditEventRepo*" -o -name "*AuditEventRepository*" | head -10Repository: cryptomator/hub
Length of output: 41
🏁 Script executed:
sed -n '370,400p' ./backend/src/main/java/org/cryptomator/hub/api/UsersResource.javaRepository: cryptomator/hub
Length of output: 1625
🏁 Script executed:
rg "findLastVaultKeyRetrieve" -A 10 -B 2Repository: cryptomator/hub
Length of output: 7007
🏁 Script executed:
cat -n ./backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.java | head -100Repository: cryptomator/hub
Length of output: 3478
🏁 Script executed:
rg "@NamedQuery|@NamedQueries" -A 3 ./backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.javaRepository: cryptomator/hub
Length of output: 396
🏁 Script executed:
rg "lastVaultKeyRetrieve|VaultKeyRetrievedEvent" -B 3 -A 3 backend/src/testRepository: cryptomator/hub
Length of output: 1599
🏁 Script executed:
rg "Collectors.toMap" backend/src/main/java -A 1 | head -50Repository: cryptomator/hub
Length of output: 4853
🏁 Script executed:
cat > /tmp/test_collectors.java << 'EOF'
import java.util.stream.Collectors;
import java.util.Map;
import java.util.Arrays;
public class test_collectors {
public static void main(String[] args) {
try {
// Simulating duplicate keys without merge function
var result = Arrays.asList(
new Pair("id1", "event1"),
new Pair("id1", "event2") // Duplicate key
).stream().collect(Collectors.toMap(
Pair::getKey,
Pair::getValue
));
} catch (IllegalStateException e) {
System.out.println("Exception thrown: " + e.getClass().getSimpleName() + " - " + e.getMessage());
}
}
static class Pair {
String key;
String value;
Pair(String k, String v) { key = k; value = v; }
String getKey() { return key; }
String getValue() { return value; }
}
}
EOF
cd /tmp && javac test_collectors.java && java test_collectorsRepository: cryptomator/hub
Length of output: 105
Handle tied "last access" events before collecting to a map.
AuditEvent.lastVaultKeyRetrieve can return multiple rows for the same device when events share the max timestamp. Both Collectors.toMap(...) calls will throw IllegalStateException on the duplicate deviceId, causing a 500 error on the admin detail view for that user.
🛠️ Minimal hardening
- var deviceEvents = auditEventRepo.findLastVaultKeyRetrieve(deviceMap.keySet()).collect(Collectors.toMap(VaultKeyRetrievedEvent::getDeviceId, Function.identity()));
+ var deviceEvents = auditEventRepo.findLastVaultKeyRetrieve(deviceMap.keySet())
+ .collect(Collectors.toMap(
+ VaultKeyRetrievedEvent::getDeviceId,
+ Function.identity(),
+ (left, right) -> left));
- var legacyDeviceEvents = auditEventRepo.findLastVaultKeyRetrieve(legacyDeviceMap.keySet()).collect(Collectors.toMap(VaultKeyRetrievedEvent::getDeviceId, Function.identity()));
+ var legacyDeviceEvents = auditEventRepo.findLastVaultKeyRetrieve(legacyDeviceMap.keySet())
+ .collect(Collectors.toMap(
+ VaultKeyRetrievedEvent::getDeviceId,
+ Function.identity(),
+ (left, right) -> left));Add a regression test that inserts two VaultKeyRetrievedEvent rows for the same device with identical timestamps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/java/org/cryptomator/hub/api/UsersResource.java` around
lines 380 - 394, The two Collectors.toMap usages building deviceEvents and
legacyDeviceEvents assume unique deviceId but
auditEventRepo.findLastVaultKeyRetrieve can return multiple
VaultKeyRetrievedEvent rows for the same device when timestamps tie; change the
collection to handle ties (e.g., use Collectors.toMap(keyMapper,
Function.identity(), (a,b) -> chooseOne) or collect to groupingBy(deviceId) and
then pick a single event per key) before mapping into
DeviceResource.DeviceDto.fromEntity, ensuring deviceEvents and
legacyDeviceEvents no longer throw IllegalStateException on duplicate keys; also
add a regression test that inserts two VaultKeyRetrievedEvent rows with
identical timestamps for the same device and verifies the user detail view
code/path handles them without error.
SailReal
left a comment
SailReal
left a comment
There was a problem hiding this comment.
To display devices we have now two database queries with a for each database query in each of the two queries. This is very inefficient.
We need to join the devices with the events in a database query.
|
Please create one PR per topic. Regarding the last access time: I don't think it is worth the complexity. n+1 queries per user is unacceptable. And even if refactored to a joined query, we would basically need a completely new entity class just this one property. Do we really need this? |
3 participants
Summary
for users. Now fetches effective realm-level roles from Keycloak and persists them.
queries audit events for both modern and legacy devices.
`UpdateUserDto` has no `name` field). The field is now disabled in edit mode.