Fixing admin entity authorization bypass via client-controlled sectionCrumbs #97
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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()); | ||
|
Comment on lines
+655
to
+658
There was a problem hiding this comment. 📝 Info: LOOKUP_FOR_UPDATE still authorizes only against the owning entity, not the entity being modified For 
Was this helpful? React with 👍 or 👎 to provide feedback. There was a problem hiding this comment. Same as the other LOOKUP_FOR_UPDATE thread — noted as latent and left unchanged deliberately; see #97 (comment).
devin-ai-integration[bot] marked this conversation as resolved.
Comment on lines
+655
to
+658
There was a problem hiding this comment. 📝 Info: Security ceiling for lookup-for-update now derives from the current section rather than the first crumb
Was this helpful? React with 👍 or 👎 to provide feedback. There was a problem hiding this comment. Thanks — the analysis matches my reading. On the residual no-crumbs case: it only arises for |
||
| } | ||
|
|
||
| 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 (transactional) { | ||
| return service.update(pkg); | ||
| } else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔍 Sub-collection requests now authorize against the collection entity, not the owning section
The PR description states "nested-collection requests continue to authorize against the owning section's ceiling as before", but that is only true for the LOOKUP_FOR_UPDATE path that now explicitly sets a security ceiling (
admin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/service/AdminEntityServiceImpl.java:657). For ordinary sub-collection fetch/add/update/remove,PersistencePackageRequest.fromMetadatasets onlyceilingEntityClassnameto the collection ceiling (seeadmin/broadleaf-open-admin-platform/src/main/java/org/broadleafcommerce/openadmin/server/domain/PersistencePackageRequest.java:143-176andAdminEntityServiceImpl.java:395-437), andPersistencePackage.getSecurityCeilingEntityFullyQualifiedClassname()falls back to that ceiling. Previously the section crumb ceiling (the owning section entity) also satisfied the check via the OR loop; now it does not. OSS seed data incore/broadleaf-framework/src/main/resources/config/bc/sql/load_admin_permissions.sqldoes register most collection entities (CategoryXrefImpl, ProductOptionXref, SkuAttribute, ...), so out-of-the-box flows should keep working, but downstream projects with custom collections that were never registered in BLC_ADMIN_PERMISSION_ENTITY will start getting SecurityServiceException on collection screens. Worth calling out explicitly in release notes/upgrade docs.Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, and that tightening is inherent to the fix: any path that previously passed only because a crumb ceiling satisfied the OR now requires permission on the target ceiling. OSS seed data covers the standard collection entities, but downstream projects with unregistered custom collection entities will need BLC_ADMIN_PERMISSION_ENTITY records. I've called this out in the PR description as an upgrade note.