diff --git a/admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/security/remote/AdminSecurityServiceRemote.java b/admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/security/remote/AdminSecurityServiceRemote.java index 9072be39c27..73c3f6f2979 100644 --- a/admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/security/remote/AdminSecurityServiceRemote.java +++ b/admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/security/remote/AdminSecurityServiceRemote.java @@ -31,7 +31,6 @@ import org.broadleafcommerce.openadmin.dto.Entity; import org.broadleafcommerce.openadmin.dto.PersistencePackage; import org.broadleafcommerce.openadmin.dto.Property; -import org.broadleafcommerce.openadmin.dto.SectionCrumb; import org.broadleafcommerce.openadmin.server.security.domain.AdminPermission; import org.broadleafcommerce.openadmin.server.security.domain.AdminRole; import org.broadleafcommerce.openadmin.server.security.domain.AdminUser; @@ -40,8 +39,6 @@ import org.broadleafcommerce.openadmin.server.security.service.type.PermissionType; import org.broadleafcommerce.openadmin.server.service.ValidationException; import org.broadleafcommerce.openadmin.server.service.persistence.validation.GlobalValidationResult; -import org.springframework.cglib.core.CollectionUtils; -import org.springframework.cglib.core.Transformer; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; @@ -49,8 +46,6 @@ import org.springframework.stereotype.Service; import java.util.Arrays; -import java.util.HashSet; -import java.util.Set; import jakarta.annotation.Resource; @@ -133,17 +128,10 @@ public AdminUser getPersistentAdminUser() { @Override public void securityCheck(PersistencePackage persistencePackage, EntityOperationType operationType) throws ServiceException { - Set ceilingNames = new HashSet<>(); - ceilingNames.add(persistencePackage.getSecurityCeilingEntityFullyQualifiedClassname()); - if (!ArrayUtils.isEmpty(persistencePackage.getSectionCrumbs())) { - ceilingNames.addAll(CollectionUtils.transform(Arrays.asList(persistencePackage.getSectionCrumbs()), - new Transformer() { - @Override - public Object transform(Object o) { - return ((SectionCrumb) o).getSectionIdentifier(); - } - })); - } + //Authorization is performed exclusively against the security ceiling of the entity actually being operated + //on. Section crumbs are client supplied navigation state and must never widen the set of ceilings the user + //is authorized against. + String securityCeiling = persistencePackage.getSecurityCeilingEntityFullyQualifiedClassname(); Entity entity = persistencePackage.getEntity(); @@ -186,7 +174,7 @@ public Object transform(Object o) { } } - securityCheck(ceilingNames.toArray(new String[ceilingNames.size()]), operationType); + securityCheck(securityCeiling, operationType); } @Override @@ -194,6 +182,10 @@ public void securityCheck(String ceilingEntityFullyQualifiedName, EntityOperatio securityCheck(new String[]{ceilingEntityFullyQualifiedName}, operationType); } + /** + * Verifies the current admin user is qualified for the given operation on every supplied ceiling. A user + * must never gain access to one ceiling by virtue of holding a permission on another. + */ protected void securityCheck(String[] ceilingNames, EntityOperationType operationType) throws ServiceException { if (ArrayUtils.isEmpty(ceilingNames)) { throw new SecurityServiceException("Security Check Failed: ceilingNames not specified"); @@ -228,25 +220,23 @@ protected void securityCheck(String[] ceilingNames, EntityOperationType operatio } SecurityServiceException primaryException = null; - boolean isQualified = false; + String unqualifiedCeiling = null; for (String ceilingEntityFullyQualifiedName : ceilingNames) { - isQualified = securityService.isUserQualifiedForOperationOnCeilingEntity( + boolean isQualified = securityService.isUserQualifiedForOperationOnCeilingEntity( persistentAdminUser, permissionType, ceilingEntityFullyQualifiedName ); if (!isQualified) { - if (primaryException == null) { - primaryException = new SecurityServiceException("Security Check Failed for entity operation: " - + operationType.toString() + " (" + ceilingEntityFullyQualifiedName + ")"); - } - } else { + unqualifiedCeiling = ceilingEntityFullyQualifiedName; + primaryException = new SecurityServiceException("Security Check Failed for entity operation: " + + operationType.toString() + " (" + ceilingEntityFullyQualifiedName + ")"); break; } } - if (!isQualified) { + if (primaryException != null) { //check if the requested entity is not configured and warn - if (!securityService.doesOperationExistForCeilingEntity(permissionType, ceilingNames[0])) { + if (!securityService.doesOperationExistForCeilingEntity(permissionType, unqualifiedCeiling)) { if (LOG.isWarnEnabled()) { - LOG.warn("Detected security request for an unregistered ceiling entity (" + StringUtil.sanitize(ceilingNames[0]) + "). " + + LOG.warn("Detected security request for an unregistered ceiling entity (" + StringUtil.sanitize(unqualifiedCeiling) + "). " + "As a result, the request failed. Please make sure to configure security for any ceiling entities " + "referenced via the admin. This is usually accomplished by adding records in the " + "BLC_ADMIN_PERMISSION_ENTITY table. Note, depending on how the entity in question is used, you " + diff --git a/admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/service/AdminEntityServiceImpl.java b/admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/service/AdminEntityServiceImpl.java index 5cabce51ced..8a7ae3ea92c 100644 --- a/admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/service/AdminEntityServiceImpl.java +++ b/admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/service/AdminEntityServiceImpl.java @@ -652,6 +652,10 @@ public PersistenceResponse addSubCollectionEntity( if (fmd.getAddMethodType().equals(AddMethodType.LOOKUP_FOR_UPDATE)) { ppr.setUpdateLookupType(true); + //the operation updates the looked up member of a collection owned by the entity currently being + //managed, so authorize against that owning entity's ceiling, as derived from server side metadata + ppr.withSecurityCeilingEntityClassname(StringUtils.isNotBlank(mainMetadata.getSecurityCeilingType()) + ? mainMetadata.getSecurityCeilingType() : mainMetadata.getCeilingType()); } Property fp = new Property(); @@ -1003,12 +1007,6 @@ public PersistenceResponse add(PersistencePackageRequest request, boolean transa PersistencePackage pkg = persistencePackageFactory.create(request); try { if (request.isUpdateLookupType()) { - if (pkg.getSectionCrumbs() != null && pkg.getSectionCrumbs().length > 0) { - SectionCrumb sc = pkg.getSectionCrumbs()[0]; - if (StringUtils.isNotBlank(sc.getSectionIdentifier())) { - pkg.setSecurityCeilingEntityFullyQualifiedClassname(sc.getSectionIdentifier()); - } - } if (transactional) { return service.update(pkg); } else { diff --git a/admin/broadleaf-open-admin-platform/src/test/groovy/org/broadleafcommerce/openadmin/spec/AdminSecurityServiceRemoteSpec.groovy b/admin/broadleaf-open-admin-platform/src/test/groovy/org/broadleafcommerce/openadmin/spec/AdminSecurityServiceRemoteSpec.groovy new file mode 100644 index 00000000000..628dabc1b81 --- /dev/null +++ b/admin/broadleaf-open-admin-platform/src/test/groovy/org/broadleafcommerce/openadmin/spec/AdminSecurityServiceRemoteSpec.groovy @@ -0,0 +1,114 @@ +/*- + * #%L + * BroadleafCommerce Open Admin Platform + * %% + * Copyright (C) 2009 - 2026 Broadleaf Commerce + * %% + * Licensed under the Broadleaf Fair Use License Agreement, Version 1.0 + * (the "Fair Use License" located at http://license.broadleafcommerce.org/fair_use_license-1.0.txt) + * unless the restrictions on use therein are violated and require payment to Broadleaf in which case + * the Broadleaf End User License Agreement (EULA), Version 1.1 + * (the "Commercial License" located at http://license.broadleafcommerce.org/commercial_license-1.1.txt) + * shall apply. + * + * Alternatively, the Commercial License may be replaced with a mutually agreed upon license (the "Custom License") + * between you and Broadleaf Commerce. You may not use this file except in compliance with the applicable license. + * #L% + */ +package org.broadleafcommerce.openadmin.spec + +import org.broadleafcommerce.common.exception.SecurityServiceException +import org.broadleafcommerce.openadmin.dto.PersistencePackage +import org.broadleafcommerce.openadmin.dto.SectionCrumb +import org.broadleafcommerce.openadmin.server.security.domain.AdminUser +import org.broadleafcommerce.openadmin.server.security.extension.AdminSecurityCheckExtensionManager +import org.broadleafcommerce.openadmin.server.security.remote.AdminSecurityServiceRemote +import org.broadleafcommerce.openadmin.server.security.remote.EntityOperationType +import org.broadleafcommerce.openadmin.server.security.service.AdminSecurityService +import org.broadleafcommerce.openadmin.server.security.service.RowLevelSecurityService +import org.broadleafcommerce.openadmin.server.security.service.type.PermissionType +import spock.lang.Specification + +/** + * Verifies that admin entity authorization is performed against the security ceiling of the entity actually being + * operated on and cannot be satisfied by client supplied section crumbs. + */ +class AdminSecurityServiceRemoteSpec extends Specification { + + static final String TARGET_CEILING = "org.broadleafcommerce.openadmin.server.security.domain.AdminUser" + static final String AUTHORIZED_CEILING = "org.broadleafcommerce.core.catalog.domain.Product" + + AdminSecurityService securityService + AdminUser adminUser + AdminSecurityServiceRemote remoteService + + def setup() { + securityService = Mock(AdminSecurityService) + adminUser = Mock(AdminUser) + + //a real manager with no registered handlers always reports NOT_HANDLED + AdminSecurityCheckExtensionManager extensionManager = new AdminSecurityCheckExtensionManager() + + AdminUser currentUser = adminUser + remoteService = new AdminSecurityServiceRemote() { + @Override + AdminUser getPersistentAdminUser() { + return currentUser + } + } + remoteService.securityService = securityService + remoteService.securityCheckExtensionManager = extensionManager + remoteService.rowLevelSecurityService = Mock(RowLevelSecurityService) + } + + def "section crumbs cannot authorize an operation on an unrelated entity"() { + given: + PersistencePackage pkg = new PersistencePackage() + pkg.setCeilingEntityFullyQualifiedClassname(TARGET_CEILING) + pkg.setSectionCrumbs([crumb(AUTHORIZED_CEILING)] as SectionCrumb[]) + + when: + remoteService.securityCheck(pkg, EntityOperationType.FETCH) + + then: + 1 * securityService.isUserQualifiedForOperationOnCeilingEntity(adminUser, PermissionType.READ, TARGET_CEILING) >> false + 0 * securityService.isUserQualifiedForOperationOnCeilingEntity(adminUser, _, AUTHORIZED_CEILING) + thrown(SecurityServiceException) + } + + def "a permission on the target ceiling authorizes the operation"() { + given: + PersistencePackage pkg = new PersistencePackage() + pkg.setCeilingEntityFullyQualifiedClassname(TARGET_CEILING) + pkg.setSectionCrumbs([crumb(AUTHORIZED_CEILING)] as SectionCrumb[]) + + when: + remoteService.securityCheck(pkg, EntityOperationType.UPDATE) + + then: + 1 * securityService.isUserQualifiedForOperationOnCeilingEntity(adminUser, PermissionType.UPDATE, TARGET_CEILING) >> true + notThrown(SecurityServiceException) + } + + def "the explicit security ceiling takes precedence over the ceiling entity"() { + given: + PersistencePackage pkg = new PersistencePackage() + pkg.setCeilingEntityFullyQualifiedClassname(AUTHORIZED_CEILING) + pkg.setSecurityCeilingEntityFullyQualifiedClassname(TARGET_CEILING) + + when: + remoteService.securityCheck(pkg, EntityOperationType.ADD) + + then: + 1 * securityService.isUserQualifiedForOperationOnCeilingEntity(adminUser, PermissionType.CREATE, TARGET_CEILING) >> false + thrown(SecurityServiceException) + } + + protected SectionCrumb crumb(String sectionIdentifier) { + SectionCrumb crumb = new SectionCrumb() + crumb.setSectionIdentifier(sectionIdentifier) + crumb.setSectionId("1") + return crumb + } + +}