From a1aa9708dfc4179801e53ca04763763d0074e9c7 Mon Sep 17 00:00:00 2001 From: Daniel Koch Date: Fri, 14 Jun 2024 17:06:54 +0200 Subject: [PATCH 1/2] fix: check against the client role instead of the realm role --- .../shogun/config/KeycloakConfig.java | 1 - .../lib/controller/WebhookController.java | 16 +----- .../shogun/lib/event/KeycloakEventType.java | 4 +- .../lib/listener/KeycloakEventListener.java | 5 -- .../keycloak/KeycloakRoleProviderService.java | 6 +- .../shogun/lib/util/KeycloakUtil.java | 56 ++++++++++++++++++- 6 files changed, 59 insertions(+), 29 deletions(-) diff --git a/shogun-config/src/main/java/de/terrestris/shogun/config/KeycloakConfig.java b/shogun-config/src/main/java/de/terrestris/shogun/config/KeycloakConfig.java index 096ed7aa3..0c025048a 100644 --- a/shogun-config/src/main/java/de/terrestris/shogun/config/KeycloakConfig.java +++ b/shogun-config/src/main/java/de/terrestris/shogun/config/KeycloakConfig.java @@ -59,5 +59,4 @@ public RealmResource getRealm(@Autowired Keycloak kc) { return kc.realm(keycloakProperties.getRealm()); } - } diff --git a/shogun-lib/src/main/java/de/terrestris/shogun/lib/controller/WebhookController.java b/shogun-lib/src/main/java/de/terrestris/shogun/lib/controller/WebhookController.java index d8b75dce6..485c03152 100644 --- a/shogun-lib/src/main/java/de/terrestris/shogun/lib/controller/WebhookController.java +++ b/shogun-lib/src/main/java/de/terrestris/shogun/lib/controller/WebhookController.java @@ -32,6 +32,7 @@ @RestController @RequestMapping("/webhooks") public class WebhookController { + @Autowired private ApplicationEventPublisher applicationEventPublisher; @@ -85,21 +86,6 @@ public void handleKeyCloakEvent(@RequestBody KeycloakEventDto event) { )); } } - case "REALM_ROLE" -> { - if (StringUtils.equals(eventType, "CREATE")) { - applicationEventPublisher.publishEvent(new KeycloakEvent( - this, - KeycloakEventType.REALM_ROLE_CREATED, - split[1] - )); - } else if (StringUtils.equals(eventType, "DELETE")) { - applicationEventPublisher.publishEvent(new KeycloakEvent( - this, - KeycloakEventType.REALM_ROLE_DELETED, - split[1] - )); - } - } case "REALM_ROLE_MAPPING", "CLIENT_ROLE_MAPPING" -> { if (split[0].equals("users")) { applicationEventPublisher.publishEvent(new KeycloakEvent( diff --git a/shogun-lib/src/main/java/de/terrestris/shogun/lib/event/KeycloakEventType.java b/shogun-lib/src/main/java/de/terrestris/shogun/lib/event/KeycloakEventType.java index 53817f618..7a02bd873 100644 --- a/shogun-lib/src/main/java/de/terrestris/shogun/lib/event/KeycloakEventType.java +++ b/shogun-lib/src/main/java/de/terrestris/shogun/lib/event/KeycloakEventType.java @@ -24,7 +24,5 @@ public enum KeycloakEventType { USER_GROUP_MEMBERSHIP_CHANGED, GROUP_CREATED, GROUP_DELETED, - GROUP_ROLES_CHANGED, - REALM_ROLE_CREATED, - REALM_ROLE_DELETED + GROUP_ROLES_CHANGED } diff --git a/shogun-lib/src/main/java/de/terrestris/shogun/lib/listener/KeycloakEventListener.java b/shogun-lib/src/main/java/de/terrestris/shogun/lib/listener/KeycloakEventListener.java index 9ffd42604..3fcb195d4 100644 --- a/shogun-lib/src/main/java/de/terrestris/shogun/lib/listener/KeycloakEventListener.java +++ b/shogun-lib/src/main/java/de/terrestris/shogun/lib/listener/KeycloakEventListener.java @@ -22,7 +22,6 @@ import de.terrestris.shogun.lib.event.OnRegistrationConfirmedEvent; import de.terrestris.shogun.lib.service.security.permission.UserInstancePermissionService; import de.terrestris.shogun.lib.service.security.provider.GroupProviderService; -import de.terrestris.shogun.lib.service.security.provider.RoleProviderService; import de.terrestris.shogun.lib.service.security.provider.UserProviderService; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.event.EventListener; @@ -37,9 +36,6 @@ public class KeycloakEventListener { @Autowired private GroupProviderService groupProviderService; - @Autowired - private RoleProviderService roleProviderService; - @Autowired protected UserInstancePermissionService userInstancePermissionService; @@ -48,7 +44,6 @@ public void onKeycloakEvent(KeycloakEvent event) { switch (event.getEventType()) { case USER_CREATED -> userProviderService.findOrCreateByProviderId(event.getKeycloakId()); case GROUP_CREATED -> groupProviderService.findOrCreateByProviderId(event.getKeycloakId()); - case REALM_ROLE_CREATED -> roleProviderService.findOrCreateByProviderId(event.getKeycloakId()); } } diff --git a/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/provider/keycloak/KeycloakRoleProviderService.java b/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/provider/keycloak/KeycloakRoleProviderService.java index e9c4c79e4..422777d0f 100644 --- a/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/provider/keycloak/KeycloakRoleProviderService.java +++ b/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/provider/keycloak/KeycloakRoleProviderService.java @@ -65,9 +65,9 @@ public List getRolesForUser(User user) { List roles = new ArrayList<>(); try { - List rolesA = keycloakUtil.getKeycloakUserRoles(user); + List roleRepresentations = keycloakUtil.getKeycloakUserRoles(user); - roles = rolesA.stream() + roles = roleRepresentations.stream() .map(role -> roleRepository.findByAuthProviderId(role.getId())) .filter(Optional::isPresent) .map(Optional::get) @@ -87,7 +87,7 @@ public Role findOrCreateByProviderId(String providerRoleId) Role role = roleOptional.orElse(null); if (role == null) { - role = new Role(providerRoleId, null); + role = new Role(providerRoleId, null); roleRepository.save(role); log.info("Role with Keycloak ID {} did not yet exist in the SHOGun DB and was therefore created.", providerRoleId); diff --git a/shogun-lib/src/main/java/de/terrestris/shogun/lib/util/KeycloakUtil.java b/shogun-lib/src/main/java/de/terrestris/shogun/lib/util/KeycloakUtil.java index eca6bd935..2115a52f5 100644 --- a/shogun-lib/src/main/java/de/terrestris/shogun/lib/util/KeycloakUtil.java +++ b/shogun-lib/src/main/java/de/terrestris/shogun/lib/util/KeycloakUtil.java @@ -19,6 +19,7 @@ import de.terrestris.shogun.lib.model.Group; import de.terrestris.shogun.lib.model.Role; import de.terrestris.shogun.lib.model.User; +import de.terrestris.shogun.properties.KeycloakProperties; import jakarta.ws.rs.WebApplicationException; import jakarta.ws.rs.core.Response; import lombok.extern.log4j.Log4j2; @@ -26,6 +27,7 @@ import org.keycloak.admin.client.CreatedResponseUtil; import org.keycloak.admin.client.resource.*; import org.keycloak.representations.IDToken; +import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.GroupRepresentation; import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.representations.idm.UserRepresentation; @@ -45,6 +47,9 @@ @Component public class KeycloakUtil { + @Autowired + private KeycloakProperties keycloakProperties; + @Autowired protected RealmResource keycloakRealm; @@ -211,7 +216,7 @@ public List getKeycloakUserGroups(User } /** - * Get the Keycloak RoleRepresentations from a user instance. + * Get the Keycloak RoleRepresentations (for the specified client) from a user instance. * * @param user * @return @@ -221,7 +226,13 @@ public List getKeycloakUserRoles(User us List roles = new ArrayList<>(); try { - roles = userResource.roles().getAll().getRealmMappings(); + ClientRepresentation clientRepresentation = getClientRepresentationFromClientId(); + + if (clientRepresentation == null) { + return roles; + } + + roles = userResource.roles().clientLevel(clientRepresentation.getId()).listEffective(); } catch (Exception e) { log.warn("Could not get the RoleMappingResource for the user with SHOGun ID {} and " + "Keycloak ID {}. This may happen if the user is not available in Keycloak.", @@ -232,6 +243,47 @@ public List getKeycloakUserRoles(User us return roles; } + /** + * Returns the client representation for the client the shogun instance is configured with (see keycloak.clientId + * in properties). + * + * @return + */ + public ClientRepresentation getClientRepresentationFromClientId() { + List clientRepresentations = keycloakRealm.clients().findByClientId(keycloakProperties.getClientId()); + + if (clientRepresentations.size() != 1) { + log.error("Could not find client with clientId {} in Keycloak. " + + "Expected to find exactly one client with this clientId, but found {} clients.", + keycloakProperties.getClientId(), clientRepresentations.size()); + + return null; + } + + return clientRepresentations.getFirst(); + } + + public RoleRepresentation getRoleByName(String roleName) { + ClientRepresentation clientRepresentation = getClientRepresentationFromClientId(); + + if (clientRepresentation == null) { + return null; + } + + RoleResource roleResource = keycloakRealm.clients().get(clientRepresentation.getId()).roles().get(roleName); + + RoleRepresentation roleRepresentation = null; + + try { + roleRepresentation = roleResource.toRepresentation(); + } catch (Exception e) { + log.error("Could not get the RoleRepresentation for role with name {}. This may happen if " + + "the role is not available in Keycloak.", roleName); + } + + return roleRepresentation; + } + /** * Return keycloak user id from {@link Authentication} object * - from {@link IDToken} From 9716cecf1099424fc008fcdccba04afbe8059d65 Mon Sep 17 00:00:00 2001 From: Daniel Koch Date: Fri, 14 Jun 2024 17:07:57 +0200 Subject: [PATCH 2/2] fix: remove role permissions before removing the role --- .../shogun/lib/service/RoleService.java | 15 +++++++++++++++ .../permission/RoleInstancePermissionService.java | 14 ++++++++++++++ .../permission/UserInstancePermissionService.java | 2 +- 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/RoleService.java b/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/RoleService.java index cb36a2b24..1300c404f 100644 --- a/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/RoleService.java +++ b/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/RoleService.java @@ -18,13 +18,16 @@ import de.terrestris.shogun.lib.model.Role; import de.terrestris.shogun.lib.repository.RoleRepository; +import de.terrestris.shogun.lib.service.security.permission.RoleClassPermissionService; import de.terrestris.shogun.lib.service.security.provider.RoleProviderService; import lombok.extern.log4j.Log4j2; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.jpa.domain.Specification; import org.springframework.security.access.prepost.PostAuthorize; import org.springframework.security.access.prepost.PostFilter; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Isolation; import org.springframework.transaction.annotation.Transactional; import java.util.List; @@ -37,6 +40,9 @@ public class RoleService extends BaseService { @Autowired RoleProviderService roleProviderService; + @Autowired + RoleClassPermissionService roleClassPermissionService; + @PostFilter("hasRole('ROLE_ADMIN') or hasPermission(filterObject, 'READ')") @Transactional(readOnly = true) @Override @@ -88,4 +94,13 @@ public Optional findByKeyCloakId(String keycloakId) { return role; } + @PreAuthorize("hasRole('ROLE_ADMIN') or hasPermission(#role, 'DELETE')") + @Transactional(isolation = Isolation.SERIALIZABLE) + public void delete(Role role) { + roleClassPermissionService.deleteAllFor(role); + roleInstancePermissionService.deleteAllFor(role); + + repository.delete(role); + } + } diff --git a/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/permission/RoleInstancePermissionService.java b/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/permission/RoleInstancePermissionService.java index c7c1e06c8..73de89881 100644 --- a/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/permission/RoleInstancePermissionService.java +++ b/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/permission/RoleInstancePermissionService.java @@ -252,6 +252,20 @@ public void deleteAllFor(BaseEntity persistedEntity) { log.trace("Deleted entity: {}", persistedEntity); } + /** + * Deletes all {@link RoleInstancePermission} for the given role. + * + * @param role The role to clear the permissions for. + */ + public void deleteAllFor(Role role) { + List roleInstancePermissions = this.findFor(role); + + repository.deleteAll(roleInstancePermissions); + + log.info("Successfully deleted all role instance permissions for role with ID {}", + role.getId()); + } + /** * Deletes the {@link RoleInstancePermission} for the given entity and role. * diff --git a/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/permission/UserInstancePermissionService.java b/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/permission/UserInstancePermissionService.java index 5ca32714b..d8f8cb299 100644 --- a/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/permission/UserInstancePermissionService.java +++ b/shogun-lib/src/main/java/de/terrestris/shogun/lib/service/security/permission/UserInstancePermissionService.java @@ -254,7 +254,7 @@ public void deleteAllFor(BaseEntity persistedEntity) { /** * Deletes all {@link UserInstancePermission} for the given user. * - * @param user The entity to clear the permissions for. + * @param user The user to clear the permissions for. */ public void deleteAllFor(User user) { List userInstancePermissions = this.findFor(user);