Skip to content

fix scope grants after creating new members #129

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

Merged
raphael-frank merged 1 commit into from
Jul 14, 2026
Merged

fix scope grants after creating new members #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 42 additions & 1 deletion services/spring-member/src/main/java/tum/devoops/memberservice/service/KeycloakService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import java.net.URI;
import java.time.Instant;
import java.util.Arrays;
import java.util.List;
import java.util.UUID;

Expand All @@ -23,6 +24,10 @@ public class KeycloakService {

private static final long TOKEN_REFRESH_SKEW_SECONDS = 30L;

// Baseline realm role every member needs; without it, hasAnyRole('member', 'admin')
// rejects them with 403 on first login even though their Keycloak account exists.
private static final String MEMBER_ROLE_NAME = "member";

private final RestClient restClient;
private final String realm;
private final String serviceAccountClientId;
Expand Down Expand Up @@ -79,7 +84,41 @@ public UUID createUser(MemberCreate member) throws Exception {
}

String path = location.getPath();
return UUID.fromString(path.substring(path.lastIndexOf("/") + 1));
UUID userId = UUID.fromString(path.substring(path.lastIndexOf("/") + 1));

assignMemberRealmRole(userId);

return userId;
}

// Looked up via the user's own "available realm roles" endpoint rather than the
// realm-wide /roles/{role-name} resource: the org-role-sync service account only
// holds the manage-users client role (see realm-config.json), which covers a
// user's own role-mappings sub-resource but not the separate view-realm permission
// that GET /roles/{role-name} would require.
private void assignMemberRealmRole(UUID userId) {
try {
RoleRepresentation[] availableRoles = restClient.get()
.uri("/admin/realms/{realm}/users/{id}/role-mappings/realm/available", realm, userId)
.header(HttpHeaders.AUTHORIZATION, authorizationHeader())
.retrieve()
.body(RoleRepresentation[].class);

RoleRepresentation memberRole = Arrays.stream(availableRoles != null ? availableRoles : new RoleRepresentation[0])
.filter(role -> MEMBER_ROLE_NAME.equals(role.name()))
.findFirst()
.orElseThrow(() -> new IllegalStateException("Keycloak realm role not found: " + MEMBER_ROLE_NAME));

restClient.post()
.uri("/admin/realms/{realm}/users/{id}/role-mappings/realm", realm, userId)
.header(HttpHeaders.AUTHORIZATION, authorizationHeader())
.contentType(MediaType.APPLICATION_JSON)
.body(List.of(memberRole))
.retrieve()
.toBodilessEntity();
} catch (HttpClientErrorException.Forbidden e) {
throw new SecurityException("Insufficient permissions to assign the member realm role");
}
Comment on lines +87 to +121

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find callers of createUser to see how failures/rollback are handled
rg -nP --type=java -C4 '\bcreateUser\s*\(' -g '!**/test/**'

Repository: AET-DevOps26/team-devoops

Length of output: 163

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the service implementation around createUser and role assignment
FILE="services/spring-member/src/main/java/tum/devoops/memberservice/service/KeycloakService.java"
wc -l "$FILE"
sed -n '1,260p' "$FILE"

# Find any usages of KeycloakService and createUser in the repo
rg -n --hidden --glob '!**/test/**' 'KeycloakService|createUser\s*\(' services . || true

Repository: AET-DevOps26/team-devoops

Length of output: 11917

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="services/spring-member/src/main/java/tum/devoops/memberservice/service/MemberService.java"
wc -l "$FILE"
sed -n '1,220p' "$FILE"

# Check whether the member creation flow compensates for a Keycloak failure
rg -n --hidden --glob '!**/test/**' 'deleteUser\s*\(|createUser\(|`@Transactional`|rollback|catch .*createUser|KeycloakService' services/spring-member/src/main/java

Repository: AET-DevOps26/team-devoops

Length of output: 5907

Handle cleanup when Keycloak role assignment fails

assignMemberRealmRole() can throw after the Keycloak user has already been created, leaving an orphaned account with no local member record. MemberService.createMember() wraps that failure as a BadRequestException, so the flow is not retry-safe. Delete the new user on failure or make the create/assign flow idempotent.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@services/spring-member/src/main/java/tum/devoops/memberservice/service/KeycloakService.java`
around lines 87 - 121, Handle failures from assignMemberRealmRole in
MemberService.createMember by deleting the newly created Keycloak user before
propagating the error, ensuring cleanup occurs for any assignment failure and
preserving the original exception. Use the existing Keycloak user identifier and
deletion client/service method, and keep successful creation behavior unchanged;
alternatively, make both creation and role assignment idempotent so retries do
not leave orphaned users.

}

public void updateUser(Member member) throws HttpClientErrorException {
Expand Down Expand Up @@ -181,6 +220,8 @@ private record UserUpdateRepresentation(

private record Credential(String type, String value, boolean temporary) {}

private record RoleRepresentation(String id, String name) {}

private record TokenResponse(
@JsonProperty("access_token") String accessToken,
@JsonProperty("expires_in") Long expiresIn
Expand Down
49 changes: 49 additions & 0 deletions ...es/spring-member/src/test/java/tum/devoops/memberservice/service/KeycloakServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class KeycloakServiceTest {
private static final String SERVICE_ACCOUNT_TOKEN = "service-account-token";
private static final String TOKEN_URI = BASE_URL + "/realms/" + REALM + "/protocol/openid-connect/token";
private static final String USERS_URI = BASE_URL + "/admin/realms/" + REALM + "/users";
private static final String MEMBER_ROLE_ID = "member-role-id";

private MockRestServiceServer server;
private KeycloakService keycloakService;
Expand Down Expand Up @@ -68,13 +69,15 @@ void setUp() {
}

// Verifies that a successful creation returns the id parsed from the Location header
// and assigns the "member" realm role so the account isn't stuck at 403 on first login
@Test
void createUserReturnsIdFromLocationHeader() throws Exception {
expectServiceAccountTokenRequest();
server.expect(requestTo(USERS_URI))
.andExpect(header(HttpHeaders.AUTHORIZATION, "Bearer " + SERVICE_ACCOUNT_TOKEN))
.andRespond(withStatus(HttpStatus.CREATED)
.header(HttpHeaders.LOCATION, USERS_URI + "/" + id));
expectMemberRoleAssignment(id);

UUID result = keycloakService.createUser(memberCreate);

Expand All @@ -91,12 +94,45 @@ void createUserWithoutEmailReturnsId() throws Exception {
.andExpect(header(HttpHeaders.AUTHORIZATION, "Bearer " + SERVICE_ACCOUNT_TOKEN))
.andRespond(withStatus(HttpStatus.CREATED)
.header(HttpHeaders.LOCATION, USERS_URI + "/" + id));
expectMemberRoleAssignment(id);

UUID result = keycloakService.createUser(memberCreate);

assertEquals(id, result);
}

// Verifies that a 403 while assigning the member realm role is translated into a SecurityException
@Test
void createUserThrowsOnForbiddenRoleAssignment() {
expectServiceAccountTokenRequest();
server.expect(requestTo(USERS_URI))
.andExpect(header(HttpHeaders.AUTHORIZATION, "Bearer " + SERVICE_ACCOUNT_TOKEN))
.andRespond(withStatus(HttpStatus.CREATED)
.header(HttpHeaders.LOCATION, USERS_URI + "/" + id));
server.expect(requestTo(USERS_URI + "/" + id + "/role-mappings/realm/available"))
.andExpect(header(HttpHeaders.AUTHORIZATION, "Bearer " + SERVICE_ACCOUNT_TOKEN))
.andRespond(withStatus(HttpStatus.FORBIDDEN));

assertThrows(SecurityException.class, () -> keycloakService.createUser(memberCreate));
}

// Verifies that a missing "member" realm role fails loudly instead of silently leaving the user unassigned
@Test
void createUserThrowsWhenMemberRoleNotFound() {
expectServiceAccountTokenRequest();
server.expect(requestTo(USERS_URI))
.andExpect(header(HttpHeaders.AUTHORIZATION, "Bearer " + SERVICE_ACCOUNT_TOKEN))
.andRespond(withStatus(HttpStatus.CREATED)
.header(HttpHeaders.LOCATION, USERS_URI + "/" + id));
server.expect(requestTo(USERS_URI + "/" + id + "/role-mappings/realm/available"))
.andExpect(header(HttpHeaders.AUTHORIZATION, "Bearer " + SERVICE_ACCOUNT_TOKEN))
.andRespond(withStatus(HttpStatus.OK)
.contentType(MediaType.APPLICATION_JSON)
.body("[]"));

assertThrows(IllegalStateException.class, () -> keycloakService.createUser(memberCreate));
}

// Verifies that a 409 conflict is translated into an IllegalAccessException
@Test
void createUserThrowsOnConflict() {
Expand Down Expand Up @@ -196,6 +232,19 @@ void deleteUserThrowsOnForbidden() {
assertThrows(SecurityException.class, () -> keycloakService.deleteUser(id));
}

private void expectMemberRoleAssignment(UUID userId) {
server.expect(requestTo(USERS_URI + "/" + userId + "/role-mappings/realm/available"))
.andExpect(header(HttpHeaders.AUTHORIZATION, "Bearer " + SERVICE_ACCOUNT_TOKEN))
.andRespond(withStatus(HttpStatus.OK)
.contentType(MediaType.APPLICATION_JSON)
.body("[{\"id\":\"" + MEMBER_ROLE_ID + "\",\"name\":\"member\"}]"));

server.expect(requestTo(USERS_URI + "/" + userId + "/role-mappings/realm"))
.andExpect(header(HttpHeaders.AUTHORIZATION, "Bearer " + SERVICE_ACCOUNT_TOKEN))
.andExpect(content().json("[{\"id\":\"" + MEMBER_ROLE_ID + "\",\"name\":\"member\"}]"))
.andRespond(withStatus(HttpStatus.NO_CONTENT));
}

private void expectServiceAccountTokenRequest() {
MultiValueMap<String, String> form = new LinkedMultiValueMap<>();
form.add("grant_type", "client_credentials");
Expand Down
Loading