Development: Let admins manage every user from one place - #2487
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…WithPassword Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…in delete Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…embers Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d-members Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
📊 Client Test Coverage Too Low 🔍 View coverage locally: npm run test:ci
open build/test-results/vitest/coverage/index.html🌐 View coverage from GitHub: |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 5 high |
| Complexity | 5 medium |
🟢 Metrics 185 complexity
Metric Results Complexity 185
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
📊 Server Test Coverage Too Low 🔍 View coverage locally: ./gradlew test jacocoTestReport
open build/reports/jacoco/test/html/index.html🌐 View coverage from GitHub: |
The detail view streams `user.researchGroupRoles` to derive the primary role, but `findById` returns a User whose roles collection is lazy. By the time the stream runs the Hibernate session is gone and the lookup 500s. Use the existing JOIN-FETCH finder used by UserRetentionService. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
📊 Client Test Coverage Too Low 🔍 View coverage locally: npm run test:ci
open build/test-results/vitest/coverage/index.html🌐 View coverage from GitHub: |
|
🤖 No OpenAPI or client changes needed. |
|
📊 Server Test Coverage Too Low 🔍 View coverage locally: ./gradlew test jacocoTestReport
open build/reports/jacoco/test/html/index.html🌐 View coverage from GitHub: |
|
📊 Client Test Coverage Too Low 🔍 View coverage locally: npm run test:ci
open build/test-results/vitest/coverage/index.html🌐 View coverage from GitHub: |
|
🤖 No OpenAPI or client changes needed. |
|
📊 Server Test Coverage Too Low 🔍 View coverage locally: ./gradlew test jacocoTestReport
open build/reports/jacoco/test/html/index.html🌐 View coverage from GitHub: |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eate-field branches Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
…in-page # Conflicts: # openapi/openapi.yaml # src/main/java/de/tum/cit/aet/usermanagement/repository/UserRepository.java # src/main/java/de/tum/cit/aet/usermanagement/service/ResearchGroupService.java # src/main/webapp/app/generated/.openapi-generator/FILES
|
🤖 No OpenAPI or client changes needed. |
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
Adapt the Manage Users admin page to the internal user management introduced in #2590, and resolve the conflicts in KeycloakUserService and UserService. #2590 removed the external Keycloak realm: Keycloak is now used only for TUM members, and the whole external-realm write surface was deleted. This branch still drove Keycloak for admin user CRUD, so after the merge it no longer compiled - create, import and delete each called a method that no longer exists. Conflict resolution - KeycloakUserService: took main's version. This branch's only additions (findKeycloakUserById, createUserWithPassword, deleteUser) were all built on externalKeycloak/externalRealm, which main deleted. - UserService: kept both sides of the constructor - this branch's researchGroupRepository and main's passwordEncoder. Admin user management, rewired - create now provisions an internally managed user (provisionExternalUser plus setLocalPassword) instead of creating a Keycloak user. Admin-created users are always internal. - import now re-resolves the TUM member server-side via findUserByUniversityId instead of trusting a client-supplied Keycloak UUID, and the client picks the user through the existing Keycloak search picker rather than pasting a UUID. - delete no longer removes the user from Keycloak; only the local record is anonymised, so a later login re-provisions a fresh row. The admin feature's only remaining Keycloak call is a read. Password handling Local passwords are now refused for TUM members at the single point where passwordHash is written, which also closes the same gap on PUT /api/users/password - its Javadoc claimed the restriction but nothing enforced it. Admin create additionally rejects an email that already belongs to an account. provisionExternalUser resolves by email and emails are not unique across TUM and applicant rows, so without this an admin could set a password on, and mark verified, somebody else's account - including a TUM member, granting them password login. create is now transactional and applies optional fields before the password check, so a supplied universityId cannot be written after the guard and slip past it. Tests UserAdminResourceTest covered neither create nor import, so the suite stayed green while none of this was exercised. Added coverage for the internal create path (asserting Keycloak is untouched), duplicate-email and TUM-password rejection, and import by university ID including the unknown-id and blank-id cases. The setLocalPassword guard itself is not directly covered: UserService is a mock in resource tests and this repo keeps server coverage at the resource layer. Verified on the merged tree: 804 server tests, 2063 client tests, client typecheck, client production build, eslint (0 errors), a11y lint and prettier. Co-Authored-By: Claude <noreply@anthropic.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 2 high |
| CodeStyle | 4 minor |
| Complexity | 4 medium |
🟢 Metrics 276 complexity
Metric Results Complexity 276
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
🤖 No OpenAPI or client changes needed. |
- manage-user-form spec: built the second candidate explicitly instead of
spreading the first. The client guidelines call out the spread operator with
this exact pattern as the counter-example, and it was one of only two uses
across the spec suite.
- UserAdminResource: the delete endpoint's javadoc still claimed the user is
removed from Keycloak. That stopped being true when the Keycloak call was
dropped from the flow, and it contradicted the service javadoc directly below
it. It now says what the endpoint does, which is anonymise the local record
and leave the Keycloak identity alone.
- UserAdminResourceTest: the create and import payloads went from untyped maps
to the CreateUserDTO and ImportUserDTO records, matching the sibling
UpdateUser tests and the rest of the resource tests.
One payload stays a map deliberately. ImportUserDTO is @JsonInclude(NON_EMPTY),
so new ImportUserDTO("") serialises to {} and the blank-value test would still
pass while no longer sending a blank value. There is now a comment saying so.
Verified: 804 server tests, 2063 client tests, typecheck, eslint with no errors
and prettier.
Co-Authored-By: Claude <noreply@anthropic.com>
|
🤖 No OpenAPI or client changes needed. |
…in-page # Conflicts: # src/main/webapp/app/generated/.openapi-generator/FILES # src/main/webapp/app/usermanagement/research-group/research-group-add-members/research-group-add-members.component.ts # src/main/webapp/i18n/de/global.json # src/main/webapp/i18n/en/global.json
|
🤖 No OpenAPI or client changes needed. |
…heir role Setting a role deleted every role mapping the user had and wrote one back, so an admin changing someone's role in one group silently removed them from every other group they belonged to. The form says "set the role", not "remove from all other groups". A group-bound role now only replaces the role held in the group it names. A role that belongs to no group still clears the group-bound ones, since that is a demotion out of every group rather than a change within one. Covering this meant the test context could no longer replace UserService with a plain mock, because the endpoint an admin actually calls delegates to it and nothing ran. It is a spy now, so unstubbed methods run against the real database and the two new tests assert what the role change leaves behind. That change makes when(...) on this service dangerous: stubbing that way calls the real method first, which counted an extra invocation in one test and would have written a real password hash while setting up another. The twenty existing stubs are doReturn/doThrow now, which do not. Co-Authored-By: Claude <noreply@anthropic.com>
|
🤖 No OpenAPI or client changes needed. |
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
Checklist
General
Server
Client
Motivation and Context
Closes #2486.
Administrators previously had no UI to oversee, create, edit, or delete users. This change adds a dedicated Manage Users admin page, plus a small role-picker addition to the existing research-group add-members flow.
This branch was originally built against the old Keycloak-backed user management and has since been adapted to the internal user management introduced in #2590. Keycloak is now used only for TUM members, and the admin page no longer writes to it at all: its single remaining Keycloak call is a read used to look up TUM members for import.
Description
Server
/api/admin/userscontroller with list / view / create / import / update / delete endpoints (UserAdminResource+UserAdminService).UserService.provisionExternalUser+setLocalPassword). Admin-created users are always internal; nothing is written to Keycloak.KeycloakUserService.findUserByUniversityId(read-only) rather than trusting an id supplied by the client, so a picked entry cannot be used to fabricate a user.passwordHashis written. This also closes the same gap onPUT /api/users/password, whose Javadoc claimed the restriction but which enforced nothing.provisionExternalUserresolves by email and emails are not unique across TUM and applicant rows, so without this an admin could set a password on — and mark verified — somebody else's account, including a TUM member.createis transactional and applies optional fields before the password check, so a supplieduniversityIdcannot be written after the guard and slip past it.UserRetentionService.deleteUserByAdmin— public single-user delete that reuses the existing per-category anonymisation pipeline (admins included).UserRepository.findAllUsersForAdminJPQL projection with role + research-group filter, search across name / email / university id.AdminUserOverviewDTO,AdminUserDetailDTO,CreateUserDTO,UpdateUserDTO,ImportUserDTO(carriesuniversityId).AddMembersToResearchGroupDTOwith an optionalrolefield (default EMPLOYEE for back-compat);ResearchGroupService.addMembersToResearchGrouppropagates it (only PROFESSOR or EMPLOYEE accepted).OperationNotAllowedException).Client
/manage-users,/manage-users/create,/manage-users/:userId(admin only).ManageUsersPageComponentmirrors the recent admin-list pattern (signals, computed filter labels, no method-calls in templates).ManageUserFormComponent— single shared form with three modes: create / edit / import. Reactive Forms + signals./api/users/available-for-research-group, no target group). Users without auniversityIdare filtered out and cannot be selected, since only TUM members are importable. Submitting posts{ universityId }.ResearchGroupAddMembersComponentgets a role-picker dropdown (PROFESSOR / EMPLOYEE, default EMPLOYEE).manageUsersPagei18n keys (en + de) plus route titles. German uses neutral plural style ("Nutzende verwalten", "Mitarbeitende", "Professoren").What changed during review
Role assignment moved out of the admin service.
UserAdminServicewas reaching forResearchGroupRepositoryitself to write role mappings. That logic is nowUserService.setPrimaryRole, so the admin service goes through the service that owns users rather than around it, and the same rule is available to anything else that needs it.The delete endpoint's Javadoc claimed it deleted from Keycloak. It never did after the move to internal user management — it anonymises the local record only. Corrected, since a stale contract on a destructive endpoint is worth more than a stale comment elsewhere.
Added resource tests for the update and delete flows, which had none, and folded the create/import test fixtures into the resource test rather than a shared holder.
Page titles picked up
{siteName}when main's rename landed, so this page follows the configurable platform name like the rest of the app.A role change no longer empties the user's other research groups. Setting a role deleted every mapping the user had and wrote one back, so changing someone's role in one group silently removed them from every other group — while the form says "set the role", not "remove from all other groups". A group-bound role now replaces only the role held in the group it names. A role that belongs to no group still clears the group-bound ones, because that is a demotion out of every group rather than a change within one.
The test context stopped replacing
UserServicewith a plain mock. The endpoint an admin calls delegates to that service, so with a mock in place nothing ran and the behaviour above was unreachable from any test. It is a spy now: unstubbed methods run against the real database, and two tests assert what a role change leaves behind. One of them fails against the old behaviour, so it is a regression guard rather than a description of whatever the code happens to do.Twenty stubs moved from
when(...)todoReturn(...). Stubbing a spy withwhen(spy.method())calls the real method first. That showed up immediately as a double-counted invocation, and would also have written a real password hash while setting up an unrelated test.Steps for Testing
Prerequisites:
http://localhost:9080(deployed Keycloak URL is set via theKEYCLOAK_URLGitHub variable). Still required — the import picker reads TUM members from Keycloak.Steps:
/manage-usersloads with the table populated./manage-users/:id, the local DB row has apassword_hashand the APPLICANT role, and — importantly — no user is created in Keycloak.university_idset. Confirm users without a university id do not appear as selectable options, and that Submit stays disabled until a user is picked.PUT /api/users/password. Confirm it is refused and nopassword_hashis written.UserResearchGroupRolerow has PROFESSOR.UserResearchGroupRolerow is written./manage-users→ 403 / redirect.Review Progress
Code Review
Manual Tests
/manage-usersScreenshots
To be added before merge.
Test Coverage
Client
Server
Last updated: 2026-07-30 10:59:57 UTC