From b50d481b10b99c5f7da162c78917113aa2d0980f Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Thu, 9 May 2024 19:10:20 -0300 Subject: [PATCH] Make sure organization groups can not be managed but when managing an organization Closes #29431 Signed-off-by: Pedro Igor --- .../org/keycloak/models/jpa/GroupAdapter.java | 13 + .../keycloak/models/jpa/JpaRealmProvider.java | 43 ++- .../org/keycloak/models/jpa/RealmAdapter.java | 6 +- .../org/keycloak/models/jpa/UserAdapter.java | 7 +- .../jpa/JpaOrganizationProvider.java | 94 ++++--- .../jpa/JpaOrganizationProviderFactory.java | 12 + .../organization/jpa/OrganizationAdapter.java | 13 + .../java/org/keycloak/models/GroupModel.java | 123 ++++++++- .../admin/resource/OrganizationResource.java | 6 +- .../model/OrganizationAwareRealmBean.java | 17 ++ .../organization/utils/Organizations.java | 44 +++ .../resources/admin/GroupResource.java | 33 +-- .../resources/admin/GroupsResource.java | 14 +- .../resources/admin/RealmAdminResource.java | 5 - .../resources/admin/UserResource.java | 5 - .../org/keycloak/utils/OrganizationUtils.java | 74 ----- .../admin/OrganizationGroupTest.java | 256 ++++++++++++++++++ .../organization/admin/OrganizationTest.java | 154 ----------- 18 files changed, 580 insertions(+), 339 deletions(-) create mode 100644 services/src/main/java/org/keycloak/organization/utils/Organizations.java delete mode 100644 services/src/main/java/org/keycloak/utils/OrganizationUtils.java create mode 100755 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationGroupTest.java diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/GroupAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/GroupAdapter.java index 988fbefb22..436e1f279d 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/GroupAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/GroupAdapter.java @@ -77,6 +77,7 @@ public class GroupAdapter implements GroupModel , JpaModel { @Override public void setName(String name) { group.setName(name); + fireGroupUpdatedEvent(); } @Override @@ -105,6 +106,7 @@ public class GroupAdapter implements GroupModel , JpaModel { GroupEntity parentEntity = toEntity(parent, em); group.setParentId(parentEntity.getId()); } + fireGroupUpdatedEvent(); } @Override @@ -113,6 +115,7 @@ public class GroupAdapter implements GroupModel , JpaModel { return; } subGroup.setParent(this); + fireGroupUpdatedEvent(); } @Override @@ -121,6 +124,7 @@ public class GroupAdapter implements GroupModel , JpaModel { return; } subGroup.setParent(null); + fireGroupUpdatedEvent(); } @Override @@ -176,10 +180,12 @@ public class GroupAdapter implements GroupModel , JpaModel { } if (found) { + fireGroupUpdatedEvent(); return; } persistAttributeValue(name, value); + fireGroupUpdatedEvent(); } @Override @@ -213,6 +219,7 @@ public class GroupAdapter implements GroupModel , JpaModel { em.remove(attr); } } + fireGroupUpdatedEvent(); } @Override @@ -264,6 +271,7 @@ public class GroupAdapter implements GroupModel , JpaModel { em.persist(entity); em.flush(); em.detach(entity); + fireGroupUpdatedEvent(); } @Override @@ -293,6 +301,7 @@ public class GroupAdapter implements GroupModel , JpaModel { em.remove(entity); } em.flush(); + fireGroupUpdatedEvent(); } @Override @@ -318,4 +327,8 @@ public class GroupAdapter implements GroupModel , JpaModel { public boolean escapeSlashesInGroupPath() { return KeycloakModelUtils.escapeSlashesInGroupPath(session); } + + private void fireGroupUpdatedEvent() { + GroupUpdatedEvent.fire(this, session); + } } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java index 3fea3c0438..ce9bf4c6e0 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java @@ -54,6 +54,9 @@ import org.keycloak.models.ClientScopeModel; import org.keycloak.models.ClientScopeProvider; import org.keycloak.models.DeploymentStateProvider; import org.keycloak.models.GroupModel; +import org.keycloak.models.GroupModel.GroupCreatedEvent; +import org.keycloak.models.GroupModel.GroupPathChangeEvent; +import org.keycloak.models.GroupModel.GroupUpdatedEvent; import org.keycloak.models.GroupProvider; import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelDuplicateException; @@ -524,29 +527,8 @@ public class JpaRealmProvider implements RealmProvider, ClientProvider, ClientSc String newPath = KeycloakModelUtils.buildGroupPath(group); String previousPath = KeycloakModelUtils.buildGroupPath(group, previousParent); - GroupModel.GroupPathChangeEvent event = - new GroupModel.GroupPathChangeEvent() { - @Override - public RealmModel getRealm() { - return realm; - } - - @Override - public String getNewPath() { - return newPath; - } - - @Override - public String getPreviousPath() { - return previousPath; - } - - @Override - public KeycloakSession getKeycloakSession() { - return session; - } - }; - session.getKeycloakSessionFactory().publish(event); + GroupPathChangeEvent.fire(group, newPath, previousPath, session); + fireGroupUpdatedEvent(group); } @Override @@ -733,12 +715,17 @@ public class JpaRealmProvider implements RealmProvider, ClientProvider, ClientSc em.persist(groupEntity); em.flush(); - return new GroupAdapter(session, realm, em, groupEntity); + GroupAdapter group = new GroupAdapter(session, realm, em, groupEntity); + + fireGroupCreatedEvent(group); + + return group; } @Override public void addTopLevelGroup(RealmModel realm, GroupModel subGroup) { subGroup.setParent(null); + fireGroupUpdatedEvent(subGroup); } public void preRemove(RealmModel realm, RoleModel role) { @@ -1262,4 +1249,12 @@ public class JpaRealmProvider implements RealmProvider, ClientProvider, ClientSc public Set getClientSearchableAttributes() { return clientSearchableAttributes; } + + private void fireGroupUpdatedEvent(GroupModel group) { + GroupUpdatedEvent.fire(group, session); + } + + private void fireGroupCreatedEvent(GroupAdapter group) { + GroupCreatedEvent.fire(group, session); + } } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java index 20a730e752..0156d20bd8 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java @@ -28,6 +28,7 @@ import org.keycloak.common.util.Time; import org.keycloak.component.ComponentFactory; import org.keycloak.component.ComponentModel; import org.keycloak.models.*; +import org.keycloak.models.GroupModel.GroupUpdatedEvent; import org.keycloak.models.jpa.entities.*; import org.keycloak.models.utils.ComponentUtil; import org.keycloak.models.utils.KeycloakModelUtils; @@ -722,6 +723,7 @@ public class RealmAdapter implements StorageProviderRealmModel, JpaModel getRolesStream() { return session.roles().getRealmRolesStream(this); } - + @Override public Stream getRolesStream(Integer first, Integer max) { return session.roles().getRealmRolesStream(this, first, max); } - + @Override public Stream searchForRolesStream(String search, Integer first, Integer max) { return session.roles().searchForRolesStream(this, search, first, max); diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java index 2e82897741..d93d32df7e 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java @@ -22,6 +22,8 @@ import org.keycloak.common.util.ObjectUtil; import org.keycloak.credential.UserCredentialManager; import org.keycloak.models.ClientModel; import org.keycloak.models.GroupModel; +import org.keycloak.models.GroupModel.GroupMemberJoinEvent; +import org.keycloak.models.GroupModel.GroupMemberLeaveEvent; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; @@ -412,6 +414,7 @@ public class UserAdapter implements UserModel, JpaModel { em.persist(entity); em.flush(); em.detach(entity); + GroupMemberJoinEvent.fire(group, session); } @@ -427,7 +430,7 @@ public class UserAdapter implements UserModel, JpaModel { em.remove(entity); } em.flush(); - + GroupMemberLeaveEvent.fire(group, session); } @Override @@ -544,6 +547,4 @@ public class UserAdapter implements UserModel, JpaModel { public int hashCode() { return getId().hashCode(); } - - } diff --git a/model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java b/model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java index 779d8f47bd..43c16d6cf3 100644 --- a/model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java +++ b/model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java @@ -64,8 +64,10 @@ public class JpaOrganizationProvider implements OrganizationProvider { private final GroupProvider groupProvider; private final UserProvider userProvider; private final RealmModel realm; + private final KeycloakSession session; public JpaOrganizationProvider(KeycloakSession session) { + this.session = session; em = session.getProvider(JpaConnectionProvider.class).getEntityManager(); groupProvider = session.groups(); userProvider = session.users(); @@ -85,21 +87,22 @@ public class JpaOrganizationProvider implements OrganizationProvider { throw new ModelDuplicateException("A organization with the same name already exists."); } - OrganizationEntity entity = new OrganizationEntity(); - entity.setId(KeycloakModelUtils.generateId()); + OrganizationAdapter adapter = new OrganizationAdapter(realm, this); - GroupModel group = createOrganizationGroup(entity.getId()); + try { + session.setAttribute(OrganizationModel.class.getName(), adapter); + GroupModel group = createOrganizationGroup(adapter.getId()); - entity.setGroupId(group.getId()); - entity.setRealmId(realm.getId()); - entity.setName(name); - entity.setEnabled(true); + adapter.setGroupId(group.getId()); + adapter.setName(name); + adapter.setEnabled(true); - em.persist(entity); + em.persist(adapter.getEntity()); - OrganizationAdapter adapter = new OrganizationAdapter(realm, entity, this); - - adapter.setDomains(domains.stream().map(OrganizationDomainModel::new).collect(Collectors.toSet())); + adapter.setDomains(domains.stream().map(OrganizationDomainModel::new).collect(Collectors.toSet())); + } finally { + session.removeAttribute(OrganizationModel.class.getName()); + } return adapter; } @@ -107,17 +110,22 @@ public class JpaOrganizationProvider implements OrganizationProvider { @Override public boolean remove(OrganizationModel organization) { OrganizationEntity entity = getEntity(organization.getId()); - GroupModel group = getOrganizationGroup(entity); - if (group != null) { - //TODO: won't scale, requires a better mechanism for bulk deleting users - userProvider.getGroupMembersStream(realm, group).forEach(userModel -> removeMember(organization, userModel)); - groupProvider.removeGroup(realm, group); - organization.getIdentityProviders().forEach((model) -> removeIdentityProvider(organization, model)); + try { + session.setAttribute(OrganizationModel.class.getName(), organization); + GroupModel group = getOrganizationGroup(entity); + + if (group != null) { + //TODO: won't scale, requires a better mechanism for bulk deleting users + userProvider.getGroupMembersStream(realm, group).forEach(userModel -> removeMember(organization, userModel)); + groupProvider.removeGroup(realm, group); + organization.getIdentityProviders().forEach((model) -> removeIdentityProvider(organization, model)); + } + em.remove(entity); + } finally { + session.removeAttribute(OrganizationModel.class.getName()); } - em.remove(entity); - return true; } @@ -133,18 +141,30 @@ public class JpaOrganizationProvider implements OrganizationProvider { throwExceptionIfObjectIsNull(user, "User"); OrganizationEntity entity = getEntity(organization.getId()); - GroupModel group = getOrganizationGroup(entity); + OrganizationModel current = (OrganizationModel) session.getAttribute(OrganizationModel.class.getName()); - if (user.isMemberOf(group)) { - return false; + if (current == null) { + session.setAttribute(OrganizationModel.class.getName(), organization); } - if (user.getFirstAttribute(ORGANIZATION_ATTRIBUTE) != null) { - throw new ModelException("User [" + user.getId() + "] is a member of a different organization"); - } + try { + GroupModel group = getOrganizationGroup(entity); - user.joinGroup(group); - user.setSingleAttribute(ORGANIZATION_ATTRIBUTE, entity.getId()); + if (user.isMemberOf(group)) { + return false; + } + + if (user.getFirstAttribute(ORGANIZATION_ATTRIBUTE) != null) { + throw new ModelException("User [" + user.getId() + "] is a member of a different organization"); + } + + user.joinGroup(group); + user.setSingleAttribute(ORGANIZATION_ATTRIBUTE, entity.getId()); + } finally { + if (current == null) { + session.removeAttribute(OrganizationModel.class.getName()); + } + } return true; } @@ -333,10 +353,22 @@ public class JpaOrganizationProvider implements OrganizationProvider { if (isManagedMember(organization, member)) { userProvider.removeUser(realm, member); } else { - List organizations = member.getAttributes().get(ORGANIZATION_ATTRIBUTE); - organizations.remove(organization.getId()); - member.setAttribute(ORGANIZATION_ATTRIBUTE, organizations); - member.leaveGroup(getOrganizationGroup(organization)); + OrganizationModel current = (OrganizationModel) session.getAttribute(OrganizationModel.class.getName()); + + if (current == null) { + session.setAttribute(OrganizationModel.class.getName(), organization); + } + + try { + List organizations = member.getAttributes().get(ORGANIZATION_ATTRIBUTE); + organizations.remove(organization.getId()); + member.setAttribute(ORGANIZATION_ATTRIBUTE, organizations); + member.leaveGroup(getOrganizationGroup(organization)); + } finally { + if (current == null) { + session.removeAttribute(OrganizationModel.class.getName()); + } + } } return true; diff --git a/model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProviderFactory.java b/model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProviderFactory.java index ee2b26f1f3..a47789d352 100644 --- a/model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProviderFactory.java +++ b/model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProviderFactory.java @@ -18,6 +18,9 @@ package org.keycloak.organization.jpa; import org.keycloak.Config.Scope; +import org.keycloak.models.GroupModel; +import org.keycloak.models.GroupModel.GroupEvent; +import org.keycloak.models.ModelValidationException; import org.keycloak.organization.authentication.authenticators.broker.IdpOrganizationAuthenticatorFactory; import org.keycloak.organization.authentication.authenticators.browser.OrganizationAuthenticatorFactory; import org.keycloak.models.AuthenticationExecutionModel; @@ -29,6 +32,7 @@ import org.keycloak.models.RealmModel.RealmPostCreateEvent; import org.keycloak.models.RealmModel.RealmRemovedEvent; import org.keycloak.organization.OrganizationProvider; import org.keycloak.organization.OrganizationProviderFactory; +import org.keycloak.organization.utils.Organizations; import org.keycloak.provider.ProviderEvent; public class JpaOrganizationProviderFactory implements OrganizationProviderFactory { @@ -68,6 +72,14 @@ public class JpaOrganizationProviderFactory implements OrganizationProviderFacto OrganizationProvider provider = session.getProvider(OrganizationProvider.class); provider.removeAll(); } + if (event instanceof GroupEvent) { + GroupEvent groupEvent = (GroupEvent) event; + KeycloakSession session = groupEvent.getKeycloakSession(); + GroupModel group = groupEvent.getGroup(); + if (!Organizations.canManageOrganizationGroup(session, group)) { + throw new ModelValidationException("Can not update organization group"); + } + } } private void configureAuthenticationFlows(RealmModel realm) { diff --git a/model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java b/model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java index c9a020d61f..adcca93c43 100644 --- a/model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java @@ -36,6 +36,7 @@ import org.keycloak.models.UserModel; import org.keycloak.models.jpa.JpaModel; import org.keycloak.models.jpa.entities.OrganizationDomainEntity; import org.keycloak.models.jpa.entities.OrganizationEntity; +import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.organization.OrganizationProvider; import org.keycloak.utils.EmailValidationUtil; import org.keycloak.utils.StringUtil; @@ -47,6 +48,14 @@ public final class OrganizationAdapter implements OrganizationModel, JpaModel COMPARE_BY_NAME = Comparator.comparing(GroupModel::getName); diff --git a/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationResource.java b/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationResource.java index 129b357cc8..d13bfe27ed 100644 --- a/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationResource.java +++ b/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationResource.java @@ -164,9 +164,7 @@ public class OrganizationResource { @Path("{id}/members") public OrganizationMemberResource members(@PathParam("id") String id) { - OrganizationModel organization = getOrganization(id); - session.setAttribute(OrganizationModel.class.getName(), organization); - return new OrganizationMemberResource(session, organization, auth, adminEvent); + return new OrganizationMemberResource(session, getOrganization(id), auth, adminEvent); } @Path("{id}/identity-providers") @@ -188,6 +186,8 @@ public class OrganizationResource { throw ErrorResponse.error("Organization not found.", Response.Status.NOT_FOUND); } + session.setAttribute(OrganizationModel.class.getName(), model); + return model; } diff --git a/services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareRealmBean.java b/services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareRealmBean.java index e22e6bcfb3..3d69c48c69 100644 --- a/services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareRealmBean.java +++ b/services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareRealmBean.java @@ -1,3 +1,20 @@ +/* + * Copyright 2024 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.keycloak.organization.forms.login.freemarker.model; import org.keycloak.forms.login.freemarker.model.RealmBean; diff --git a/services/src/main/java/org/keycloak/organization/utils/Organizations.java b/services/src/main/java/org/keycloak/organization/utils/Organizations.java new file mode 100644 index 0000000000..685720d594 --- /dev/null +++ b/services/src/main/java/org/keycloak/organization/utils/Organizations.java @@ -0,0 +1,44 @@ +/* + * Copyright 2024 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.organization.utils; + +import org.keycloak.common.Profile; +import org.keycloak.common.Profile.Feature; +import org.keycloak.models.GroupModel; +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.OrganizationModel; +import org.keycloak.utils.StringUtil; + +public class Organizations { + + public static boolean canManageOrganizationGroup(KeycloakSession session, GroupModel group) { + if (Profile.isFeatureEnabled(Feature.ORGANIZATION)) { + Object organization = session.getAttribute(OrganizationModel.class.getName()); + + if (organization != null) { + return true; + } + + String orgId = group.getFirstAttribute(OrganizationModel.ORGANIZATION_ATTRIBUTE); + + return StringUtil.isBlank(orgId); + } + + return true; + } +} diff --git a/services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java b/services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java index 6ea630043b..afd283ab2e 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java @@ -28,6 +28,7 @@ import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; import org.keycloak.models.Constants; import org.keycloak.models.GroupModel; +import org.keycloak.models.GroupModel.GroupPathChangeEvent; import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.RealmModel; @@ -61,7 +62,6 @@ import java.util.Set; import java.util.stream.Stream; import org.keycloak.utils.GroupUtils; -import static org.keycloak.utils.OrganizationUtils.checkForOrgRelatedGroupRep; import static org.keycloak.utils.StreamsUtil.paginatedStream; /** @@ -118,11 +118,14 @@ public class GroupResource { this.auth.groups().requireManage(group); String groupName = rep.getName(); + if (ObjectUtil.isBlank(groupName)) { throw ErrorResponse.error("Group name is missing", Response.Status.BAD_REQUEST); } - checkForOrgRelatedGroupRep(session, rep); + if (rep.getId() != null && !group.getId().equals(rep.getId())) { + throw ErrorResponse.error("Invalid group id", Response.Status.BAD_REQUEST); + } if (!Objects.equals(groupName, group.getName())) { boolean exists = siblings().filter(s -> !Objects.equals(s.getId(), group.getId())) @@ -197,8 +200,6 @@ public class GroupResource { throw ErrorResponse.error("Group name is missing", Response.Status.BAD_REQUEST); } - checkForOrgRelatedGroupRep(session, rep); - try { Response.ResponseBuilder builder = Response.status(204); GroupModel child = null; @@ -246,29 +247,7 @@ public class GroupResource { String newPath = KeycloakModelUtils.buildGroupPath(model); - GroupModel.GroupPathChangeEvent event = - new GroupModel.GroupPathChangeEvent() { - @Override - public RealmModel getRealm() { - return realm; - } - - @Override - public String getNewPath() { - return newPath; - } - - @Override - public String getPreviousPath() { - return previousPath; - } - - @Override - public KeycloakSession getKeycloakSession() { - return session; - } - }; - session.getKeycloakSessionFactory().publish(event); + GroupPathChangeEvent.fire(model, newPath, previousPath, session); } } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java b/services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java index 3b1965113a..0fb22906ab 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java @@ -32,6 +32,8 @@ import java.util.HashMap; import java.util.Map; import java.util.Objects; import java.util.stream.Stream; + +import jakarta.ws.rs.core.Response.Status; import org.eclipse.microprofile.openapi.annotations.Operation; import org.eclipse.microprofile.openapi.annotations.extensions.Extension; import org.eclipse.microprofile.openapi.annotations.tags.Tag; @@ -44,6 +46,7 @@ import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.RealmModel; import org.keycloak.models.utils.ModelToRepresentation; +import org.keycloak.organization.utils.Organizations; import org.keycloak.representations.idm.GroupRepresentation; import org.keycloak.services.ErrorResponse; import org.keycloak.services.resources.KeycloakOpenAPI; @@ -52,10 +55,6 @@ import org.keycloak.services.resources.admin.permissions.GroupPermissionEvaluato import org.keycloak.utils.GroupUtils; import org.keycloak.utils.SearchQueryUtils; -import static org.keycloak.utils.OrganizationUtils.checkForOrgRelatedGroupModel; -import static org.keycloak.utils.OrganizationUtils.checkForOrgRelatedGroupRep; - - /** * @resource Groups * @author Bill Burke @@ -124,11 +123,14 @@ public class GroupsResource { @Path("{group-id}") public GroupResource getGroupById(@PathParam("group-id") String id) { GroupModel group = realm.getGroupById(id); + if (group == null) { throw new NotFoundException("Could not find group by id"); } - checkForOrgRelatedGroupModel(session, group); + if (!Organizations.canManageOrganizationGroup(session, group)) { + throw ErrorResponse.error("Cannot manage organization related group via non Organization API.", Status.BAD_REQUEST); + } return new GroupResource(realm, group, session, this.auth, adminEvent); } @@ -181,8 +183,6 @@ public class GroupsResource { throw ErrorResponse.error("Group name is missing", Response.Status.BAD_REQUEST); } - checkForOrgRelatedGroupRep(session, rep); - try { if (rep.getId() != null) { child = realm.getGroupById(rep.getId()); diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java index 43d570636e..cb4f64cc20 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java @@ -17,7 +17,6 @@ package org.keycloak.services.resources.admin; import static org.keycloak.util.JsonSerialization.readValue; -import static org.keycloak.utils.OrganizationUtils.checkForOrgRelatedGroupModel; import java.io.InputStream; import java.security.cert.X509Certificate; @@ -1056,8 +1055,6 @@ public class RealmAdminResource { throw new NotFoundException("Group not found"); } - checkForOrgRelatedGroupModel(session, group); - realm.addDefaultGroup(group); adminEvent.operation(OperationType.CREATE).resource(ResourceType.GROUP).resourcePath(session.getContext().getUri()).success(); @@ -1076,8 +1073,6 @@ public class RealmAdminResource { throw new NotFoundException("Group not found"); } - checkForOrgRelatedGroupModel(session, group); - realm.removeDefaultGroup(group); adminEvent.operation(OperationType.DELETE).resource(ResourceType.GROUP).resourcePath(session.getContext().getUri()).success(); diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java index 0bc280f86d..22cd2e640f 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java @@ -128,7 +128,6 @@ import java.util.stream.Stream; import static org.keycloak.models.ImpersonationSessionNote.IMPERSONATOR_ID; import static org.keycloak.models.ImpersonationSessionNote.IMPERSONATOR_USERNAME; import static org.keycloak.userprofile.UserProfileContext.USER_API; -import static org.keycloak.utils.OrganizationUtils.checkForOrgRelatedGroupModel; /** * Base resource for managing users @@ -1018,8 +1017,6 @@ public class UserResource { } auth.groups().requireManageMembership(group); - checkForOrgRelatedGroupModel(session, group); - try { if (user.isMemberOf(group)){ user.leaveGroup(group); @@ -1048,8 +1045,6 @@ public class UserResource { } auth.groups().requireManageMembership(group); - checkForOrgRelatedGroupModel(session, group); - if (!RoleUtils.isDirectMember(user.getGroupsStream(),group)){ user.joinGroup(group); adminEvent.operation(OperationType.CREATE).resource(ResourceType.GROUP_MEMBERSHIP).representation(ModelToRepresentation.toRepresentation(group, true)).resourcePath(session.getContext().getUri()).success(); diff --git a/services/src/main/java/org/keycloak/utils/OrganizationUtils.java b/services/src/main/java/org/keycloak/utils/OrganizationUtils.java deleted file mode 100644 index 91320ff01d..0000000000 --- a/services/src/main/java/org/keycloak/utils/OrganizationUtils.java +++ /dev/null @@ -1,74 +0,0 @@ -/* - * Copyright 2024 Red Hat, Inc. and/or its affiliates - * and other contributors as indicated by the @author tags. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.keycloak.utils; - -import jakarta.ws.rs.core.Response; -import org.keycloak.models.GroupModel; -import org.keycloak.models.KeycloakSession; -import org.keycloak.models.OrganizationModel; -import org.keycloak.organization.OrganizationProvider; -import org.keycloak.representations.idm.GroupRepresentation; -import org.keycloak.services.ErrorResponse; - -import java.util.List; -import java.util.Map; - -public class OrganizationUtils { - - public static void checkForOrgRelatedGroupRep(KeycloakSession session, GroupRepresentation rep) { - if (isOrgsEnabled(session)) { - checkRep(rep); - } - } - - public static void checkForOrgRelatedGroupModel(KeycloakSession session, GroupModel model) { - if (isOrgsEnabled(session)) { - checkModel(model); - } - } - - private static boolean isOrgsEnabled(KeycloakSession session) { - OrganizationProvider orgProvider = session.getProvider(OrganizationProvider.class); - return orgProvider != null && orgProvider.isEnabled(); - } - - private static boolean isOrganizationRelatedGroup(Object o) { - if (o instanceof GroupRepresentation rep) { - return attributeContains(rep.getAttributes()); - } else if (o instanceof GroupModel model) { - return attributeContains(model.getAttributes()); - } - return false; - } - - private static boolean attributeContains(Map> attributes) { - return attributes != null && attributes.containsKey(OrganizationModel.ORGANIZATION_ATTRIBUTE); - } - - private static void checkModel(GroupModel model) { - if (isOrganizationRelatedGroup(model)) { - throw ErrorResponse.error("Cannot manage organization related group via non Organization API.", Response.Status.FORBIDDEN); - } - } - - private static void checkRep(GroupRepresentation rep) { - if (isOrganizationRelatedGroup(rep)) { - throw ErrorResponse.error("Cannot use group attribute reserved for organizations.", Response.Status.FORBIDDEN); - } - } -} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationGroupTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationGroupTest.java new file mode 100755 index 0000000000..15124f3e4f --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationGroupTest.java @@ -0,0 +1,256 @@ +/* + * Copyright 2024 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.testsuite.organization.admin; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.fail; +import static org.keycloak.testsuite.admin.group.GroupSearchTest.buildSearchQuery; + +import java.util.ArrayList; +import java.util.List; + +import jakarta.ws.rs.BadRequestException; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.Response.Status; +import org.junit.Test; +import org.keycloak.admin.client.resource.GroupResource; +import org.keycloak.common.Profile.Feature; +import org.keycloak.models.GroupModel; +import org.keycloak.models.ModelValidationException; +import org.keycloak.models.OrganizationModel; +import org.keycloak.models.RealmModel; +import org.keycloak.models.UserModel; +import org.keycloak.organization.OrganizationProvider; +import org.keycloak.representations.idm.GroupRepresentation; +import org.keycloak.representations.idm.ManagementPermissionRepresentation; +import org.keycloak.representations.idm.OrganizationRepresentation; +import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.testsuite.arquillian.annotation.EnableFeature; +import org.keycloak.testsuite.runonserver.RunOnServer; + +@EnableFeature(Feature.ORGANIZATION) +public class OrganizationGroupTest extends AbstractOrganizationTest { + + @Test + public void testManageOrgGroupsViaDifferentAPIs() { + // test realm contains some groups initially + List getAllBefore = testRealm().groups().groups(); + long countBefore = testRealm().groups().count().get("count"); + + List orgIds = new ArrayList<>(); + // create 5 organizations + for (int i = 0; i < 5; i++) { + OrganizationRepresentation expected = createOrganization("myorg" + i); + OrganizationRepresentation existing = testRealm().organizations().get(expected.getId()).toRepresentation(); + orgIds.add(expected.getId()); + assertNotNull(existing); + } + + // create one top-level group and one subgroup + GroupRepresentation topGroup = createGroup(testRealm(), "top"); + GroupRepresentation level2Group = new GroupRepresentation(); + level2Group.setName("level2"); + testRealm().groups().group(topGroup.getId()).subGroup(level2Group); + + // check that count queries include org related groups + assertEquals(countBefore + 7, (long) testRealm().groups().count().get("count")); + + // check that search queries include org related groups but those can't be updated + assertEquals(getAllBefore.size() + 6, testRealm().groups().groups().size()); + // we need to pull full representation of the group, otherwise org related attributes are lost in the representation + List groups = testRealm().groups().query(buildSearchQuery(OrganizationModel.ORGANIZATION_ATTRIBUTE, orgIds.get(0)), false, 0, 10, false); + assertEquals(1, groups.size()); + GroupRepresentation orgGroupRep = groups.get(0); + GroupResource group = testRealm().groups().group(orgGroupRep.getId()); + + try { + // group to be updated is organization related group + group.update(topGroup); + fail("Expected BadRequestException"); + } catch (BadRequestException ex) { + // success, the group could not be updated + } + + try { + // cannot update a group with the attribute reserved for organization related groups + testRealm().groups().group(topGroup.getId()).update(orgGroupRep); + fail("Expected BadRequestException"); + } catch (BadRequestException ex) { + // success, the group could not be updated + } + + try { + // cannot remove organization related group + group.remove(); + fail("Expected BadRequestException"); + } catch (BadRequestException ex) { + // success, the group could not be removed + } + + try { + // cannot manage organization related group permissions + group.setPermissions(new ManagementPermissionRepresentation(true)); + fail("Expected BadRequestException"); + } catch (BadRequestException ex) { + // success, the group's permissions cannot be managed + } + + // try to add subgroup to an org related group + try (Response response = group.subGroup(topGroup)) { + assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus()); + } + + // try to add org related group as a subgroup to a group + try (Response response = testRealm().groups().group(topGroup.getId()).subGroup(orgGroupRep)) { + assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus()); + } + + try { + // cannot manage organization related group role mappers + group.roles().realmLevel().add(null); + fail("Expected BadRequestException"); + } catch (BadRequestException ex) { + // success + } + + try { + // cannot manage organization related group role mappers + group.roles().realmLevel().remove(null); + fail("Expected BadRequestException"); + } catch (BadRequestException ex) { + // success + } + + try { + // cannot manage organization related group role mappers + group.roles().clientLevel(testRealm().clients().findByClientId("test-app").get(0).getId()).add(null); + fail("Expected BadRequestException"); + } catch (BadRequestException ex) { + // success + } + + try { + // cannot manage organization related group role mappers + group.roles().clientLevel(testRealm().clients().findByClientId("test-app").get(0).getId()).remove(null); + fail("Expected BadRequestException"); + } catch (BadRequestException ex) { + // success + } + + // cannot add top level group with reserved attribute for organizations + String id = orgGroupRep.getId(); + String name = orgGroupRep.getName(); + orgGroupRep.setId(null); + orgGroupRep.setName(null); + try (Response response = testRealm().groups().add(orgGroupRep)) { + assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus()); + } finally { + orgGroupRep.setId(id); + orgGroupRep.setName(name); + } + + try { + // cannot add organization related group as a default group + testRealm().addDefaultGroup(orgGroupRep.getId()); + fail("Expected BadRequestException"); + } catch (BadRequestException ex) { + // success + } + + OrganizationRepresentation org = createOrganization(); + UserRepresentation userRep = addMember(testRealm().organizations().get(org.getId())); + + try { + // cannot join organization related group + testRealm().users().get(userRep.getId()).joinGroup(orgGroupRep.getId()); + fail("Expected BadRequestException"); + } catch (BadRequestException ex) { + // success + } + } + + @Test + public void testManagingOrganizationGroupNotInOrganizationScope() { + String id = createOrganization().getId(); + String memberId = addMember(testRealm().organizations().get(id)).getId(); + + getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) session -> { + OrganizationProvider provider = session.getProvider(OrganizationProvider.class); + OrganizationModel organization = provider.getById(id); + RealmModel realm = session.getContext().getRealm(); + GroupModel orgGroup = session.groups().getGroupByName(realm, null, organization.getId()); + + try { + orgGroup.setName("fail"); + fail("can not manage"); + } catch (ModelValidationException ignore) { + } + + try { + orgGroup.setSingleAttribute(OrganizationModel.ORGANIZATION_ATTRIBUTE, "fail"); + fail("can not manage"); + } catch (ModelValidationException ignore) { + } + + GroupModel child = realm.createGroup("child"); + + try { + orgGroup.addChild(child); + fail("can not manage"); + } catch (ModelValidationException ignore) { + } + + GroupModel parent = realm.createGroup("parent"); + + try { + realm.moveGroup(orgGroup, parent); + fail("can not manage"); + } catch (ModelValidationException ignore) { + } + + try { + realm.removeGroup(orgGroup); + fail("can not manage"); + } catch (ModelValidationException ignore) { + } + + try { + realm.addDefaultGroup(orgGroup); + fail("can not manage"); + } catch (ModelValidationException ignore) { + } + + UserModel user = session.users().getUserByUsername(realm, "john-doh@localhost"); + assertNotNull(user); + try { + user.joinGroup(orgGroup); + fail("can not manage"); + } catch (ModelValidationException ignore) { + } + + UserModel member = session.users().getUserById(realm, memberId); + assertNotNull(user); + try { + member.leaveGroup(orgGroup); + fail("can not manage"); + } catch (ModelValidationException ignore) { + } + }); + } +} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationTest.java index 31777690e8..4920e0f41d 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationTest.java @@ -31,7 +31,6 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; -import static org.keycloak.testsuite.admin.group.GroupSearchTest.buildSearchQuery; import java.util.ArrayList; import java.util.Comparator; @@ -39,20 +38,14 @@ import java.util.HashMap; import java.util.List; import java.util.stream.Collectors; -import jakarta.ws.rs.ForbiddenException; import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.Response.Status; import org.junit.Test; -import org.keycloak.admin.client.resource.GroupResource; import org.keycloak.admin.client.resource.OrganizationResource; import org.keycloak.common.Profile.Feature; -import org.keycloak.models.OrganizationModel; -import org.keycloak.representations.idm.GroupRepresentation; -import org.keycloak.representations.idm.ManagementPermissionRepresentation; import org.keycloak.representations.idm.OrganizationDomainRepresentation; import org.keycloak.representations.idm.OrganizationRepresentation; -import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.arquillian.annotation.EnableFeature; @EnableFeature(Feature.ORGANIZATION) @@ -374,151 +367,4 @@ public class OrganizationTest extends AbstractOrganizationTest { assertEquals(1, existing.getDomains().size()); assertNotNull(existing.getDomain("acme.com")); } - - @Test - public void testManageOrgGroupsViaDifferentAPIs() { - // test realm contains some groups initially - List getAllBefore = testRealm().groups().groups(); - long countBefore = testRealm().groups().count().get("count"); - - List orgIds = new ArrayList<>(); - // create 5 organizations - for (int i = 0; i < 5; i++) { - OrganizationRepresentation expected = createOrganization("myorg" + i); - OrganizationRepresentation existing = testRealm().organizations().get(expected.getId()).toRepresentation(); - orgIds.add(expected.getId()); - assertNotNull(existing); - } - - // create one top-level group and one subgroup - GroupRepresentation topGroup = createGroup(testRealm(), "top"); - GroupRepresentation level2Group = new GroupRepresentation(); - level2Group.setName("level2"); - testRealm().groups().group(topGroup.getId()).subGroup(level2Group); - - // check that count queries include org related groups - assertEquals(countBefore + 7, (long) testRealm().groups().count().get("count")); - - // check that search queries include org related groups but those can't be updated - assertEquals(getAllBefore.size() + 6, testRealm().groups().groups().size()); - // we need to pull full representation of the group, otherwise org related attributes are lost in the representation - List groups = testRealm().groups().query(buildSearchQuery(OrganizationModel.ORGANIZATION_ATTRIBUTE, orgIds.get(0)), false, 0, 10, false); - assertEquals(1, groups.size()); - GroupRepresentation orgGroupRep = groups.get(0); - GroupResource group = testRealm().groups().group(orgGroupRep.getId()); - - try { - // group to be updated is organization related group - group.update(topGroup); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success, the group could not be updated - } - - try { - // cannot update a group with the attribute reserved for organization related groups - testRealm().groups().group(topGroup.getId()).update(orgGroupRep); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success, the group could not be updated - } - - try { - // cannot remove organization related group - group.remove(); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success, the group could not be removed - } - - try { - // cannot manage organization related group permissions - group.setPermissions(new ManagementPermissionRepresentation(true)); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success, the group's permissions cannot be managed - } - - // try to add subgroup to an org related group - try (Response response = group.subGroup(topGroup)) { - assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); - } - - // try to add org related group as a subgroup to a group - try (Response response = testRealm().groups().group(topGroup.getId()).subGroup(orgGroupRep)) { - assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); - } - - try { - // cannot manage organization related group role mappers - group.roles().realmLevel().add(null); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success - } - - try { - // cannot manage organization related group role mappers - group.roles().realmLevel().remove(null); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success - } - - try { - // cannot manage organization related group role mappers - group.roles().clientLevel(testRealm().clients().findByClientId("test-app").get(0).getId()).add(null); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success - } - - try { - // cannot manage organization related group role mappers - group.roles().clientLevel(testRealm().clients().findByClientId("test-app").get(0).getId()).remove(null); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success - } - - // cannot add top level group with reserved attribute for organizations - try (Response response = testRealm().groups().add(orgGroupRep)) { - assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); - } - - try { - // cannot add organization related group as a default group - testRealm().addDefaultGroup(orgGroupRep.getId()); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success - } - - try { - // cannot remove organization related group as a default group - testRealm().removeDefaultGroup(orgGroupRep.getId()); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success - } - - OrganizationRepresentation org = createOrganization(); - UserRepresentation userRep = addMember(testRealm().organizations().get(org.getId())); - - try { - // cannot join organization related group - testRealm().users().get(userRep.getId()).joinGroup(orgGroupRep.getId()); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success - } - - try { - // cannot leave organization related group - testRealm().users().get(userRep.getId()).leaveGroup(orgGroupRep.getId()); - fail("Expected ForbiddenException"); - } catch (ForbiddenException ex) { - // success - } - } }