diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/v1/portlet/ToolGroupResource.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/portlet/ToolGroupResource.java index ece7ceb94695..ea02cc8ebf4f 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/api/v1/portlet/ToolGroupResource.java +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/portlet/ToolGroupResource.java @@ -7,6 +7,7 @@ import com.dotmarketing.business.APILocator; import com.dotmarketing.business.ApiProvider; import com.dotmarketing.business.Layout; +import com.dotmarketing.business.Role; import com.dotmarketing.exception.DotDataException; import com.dotmarketing.exception.DotSecurityException; import com.liferay.portal.model.User; @@ -111,6 +112,7 @@ public final Response addToolGroupToUser(@Context final HttpServletRequest reque final User loggedInUser = new WebResource.InitBuilder(webResource) .requiredBackendUser(true) + .requiredRoles(Role.CMS_ADMINISTRATOR_ROLE) .requestAndResponse(request, response) .rejectWhenNoUser(true) .init() diff --git a/dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java b/dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java index 2229f4f41822..1b5aacbbf10e 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java @@ -570,11 +570,11 @@ public void saveRoleLayouts(String roleId, String[] layoutIds) throws DotDataExc //Validate if this logged in user has the required permissions to access the roles portlet validateRolesPortletPermissions(getLoggedInUser()); + getAdminUser(); LayoutAPI layoutAPI = APILocator.getLayoutAPI(); RoleAPI roleAPI = APILocator.getRoleAPI(); Role role = roleAPI.loadRoleById(roleId); - User user = getAdminUser(); List layouts = layoutAPI.loadLayoutsForRole(role); //Looking for removed layouts @@ -803,6 +803,7 @@ public List> getRolePermissions(String roleId) throws DotDat public void saveRolePermission(String roleId, String folderHostId, Map permissions, boolean cascade) throws DotDataException, DotSecurityException, PortalException, SystemException { //Validate if this logged in user has the required permissions to access the roles portlet validateRolesPortletPermissions(getLoggedInUser()); + getAdminUser(); Logger.info(this, "Applying role permissions for role " + roleId + " and folder/host id " + folderHostId); diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java index 4c48d54ccd2a..fdaccff000c8 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java @@ -108,6 +108,7 @@ import com.dotcms.rest.api.v1.folder.FolderResourceTest; import com.dotcms.rest.api.v1.maintenance.ClusterLogCollectorTest; import com.dotcms.rest.api.v1.menu.MenuResourceTest; +import com.dotcms.rest.api.v1.portlet.ToolGroupResourceTest; import com.dotcms.rest.api.v1.publishing.BundleManagementResourceIntegrationTest; import com.dotcms.rest.api.v1.publishing.PublishingResourceIntegrationTest; import com.dotcms.rest.api.v1.pushpublish.PushPublishFilterResourceTest; @@ -137,6 +138,7 @@ import com.dotmarketing.business.VersionableFactoryImplTest; import com.dotmarketing.business.helper.PermissionHelperTest; import com.dotmarketing.common.db.DBTimeZoneCheckTest; +import com.dotmarketing.business.ajax.RoleAjaxSecurityTest; import com.dotmarketing.filters.AutoLoginFilterTest; import com.dotmarketing.filters.CMSUrlUtilIntegrationTest; import com.dotmarketing.image.focalpoint.FocalPointAPITest; @@ -367,6 +369,7 @@ Task220606UpdatePushNowActionletNameTest.class, BundlerUtilTest.class, MenuResourceTest.class, + ToolGroupResourceTest.class, AWSS3PublisherTest.class, ContentTypeInitializerTest.class, CSSPreProcessServletIT.class, @@ -463,6 +466,7 @@ com.dotmarketing.business.IdentifierAPITest.class, com.dotmarketing.business.CommitListenerCacheWrapperTest.class, com.dotmarketing.business.RoleAPITest.class, + RoleAjaxSecurityTest.class, com.dotmarketing.business.IdentifierConsistencyIntegrationTest.class, com.dotmarketing.business.LayoutAPITest.class, com.dotmarketing.business.PermissionAPIIntegrationTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java new file mode 100644 index 000000000000..7a791dea136f --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java @@ -0,0 +1,106 @@ +package com.dotcms.rest.api.v1.portlet; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; + +import com.dotcms.IntegrationTestBase; +import com.dotcms.datagen.LayoutDataGen; +import com.dotcms.datagen.TestUserUtils; +import com.dotcms.datagen.UserDataGen; +import com.dotcms.mock.request.MockAttributeRequest; +import com.dotcms.mock.request.MockHttpRequestIntegrationTest; +import com.dotcms.mock.response.MockHttpResponse; +import com.dotcms.rest.exception.SecurityException; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.business.Layout; +import com.liferay.portal.model.User; +import com.liferay.portal.util.WebKeys; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.ws.rs.core.Response; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for {@link ToolGroupResource}. + * Verifies that the {@code /_addtouser} endpoint enforces CMS Administrator access, + * closing the privilege-escalation path documented in private-issues#642. + */ +public class ToolGroupResourceTest extends IntegrationTestBase { + + private static ToolGroupResource resource; + private static HttpServletResponse mockResponse; + private static User adminUser; + private static User backendUser; + private static Layout testLayout; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + + resource = new ToolGroupResource(); + mockResponse = new MockHttpResponse().response(); + + adminUser = TestUserUtils.getAdminUser(); + + backendUser = new UserDataGen().nextPersisted(); + APILocator.getRoleAPI().addRoleToUser( + APILocator.getRoleAPI().loadBackEndUserRole(), backendUser); + + testLayout = new LayoutDataGen().name("ToolGroupResourceTest-layout").nextPersisted(); + } + + @AfterClass + public static void cleanUp() throws Exception { + if (testLayout != null) { + try { + APILocator.getRoleAPI().removeLayoutFromRole( + testLayout, adminUser.getUserRole()); + } catch (Exception e) { + // layout may not have been associated if the admin test did not run + } + APILocator.getLayoutAPI().removeLayout(testLayout); + } + if (backendUser != null) { + APILocator.getUserAPI().delete(backendUser, APILocator.systemUser(), false); + } + } + + /** + * Method to test: {@link ToolGroupResource#addToolGroupToUser} + * Given Scenario: A low-privilege backend user (non-admin) calls {@code PUT /{layoutId}/_addtouser} + * Expected Result: A {@link SecurityException} is thrown and the user does NOT gain the layout, + * closing the first step of private-issues#642 privilege escalation chain. + */ + @Test(expected = SecurityException.class) + public void test_addToolGroupToUser_lowPrivilegeUser_throwsSecurityException() throws Exception { + final HttpServletRequest request = requestFor(backendUser); + resource.addToolGroupToUser(request, mockResponse, testLayout.getId(), null); + } + + /** + * Method to test: {@link ToolGroupResource#addToolGroupToUser} + * Given Scenario: A CMS Administrator calls {@code PUT /{layoutId}/_addtouser} + * Expected Result: HTTP 200 OK is returned and the layout is added to the admin's user role. + */ + @Test + public void test_addToolGroupToUser_adminUser_succeeds() throws Exception { + final HttpServletRequest request = requestFor(adminUser); + final Response response = resource.addToolGroupToUser( + request, mockResponse, testLayout.getId(), null); + + assertNotNull("Response must not be null", response); + assertEquals("Admin should get HTTP 200", Response.Status.OK.getStatusCode(), + response.getStatus()); + } + + private static HttpServletRequest requestFor(final User user) { + final HttpServletRequest request = new MockAttributeRequest( + new MockHttpRequestIntegrationTest("localhost", "/api/v1/toolgroups").request() + ).request(); + request.setAttribute(WebKeys.USER, user); + return request; + } +} diff --git a/dotcms-integration/src/test/java/com/dotmarketing/business/ajax/RoleAjaxSecurityTest.java b/dotcms-integration/src/test/java/com/dotmarketing/business/ajax/RoleAjaxSecurityTest.java new file mode 100644 index 000000000000..0bc22ab64f24 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotmarketing/business/ajax/RoleAjaxSecurityTest.java @@ -0,0 +1,113 @@ +package com.dotmarketing.business.ajax; + +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.dotcms.IntegrationTestBase; +import com.dotcms.datagen.LayoutDataGen; +import com.dotcms.datagen.UserDataGen; +import com.dotcms.repackage.org.directwebremoting.WebContext; +import com.dotcms.repackage.org.directwebremoting.WebContextFactory; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.business.Layout; +import com.dotmarketing.exception.DotSecurityException; +import com.liferay.portal.model.User; +import com.liferay.portal.util.WebKeys; +import com.liferay.util.servlet.SessionMessages; +import java.util.LinkedHashMap; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Regression tests for the privilege-escalation fix in {@link RoleAjax#addUserToRole}. + * + * The attack chain from private-issues#642 requires that a non-admin backend user first + * self-assigns a layout that includes the "roles" portlet via the {@code _addtouser} endpoint + * (now blocked), and then calls {@code addUserToRole} to add themselves to CMS Administrator. + * This test verifies that step 2 is independently blocked by the {@code getAdminUser()} check, + * even when the caller already holds roles-portlet access. + */ +public class RoleAjaxSecurityTest extends IntegrationTestBase { + + private static User backendUser; + private static Layout rolesPortletLayout; + private static String cmsAdminRoleId; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + + backendUser = new UserDataGen().nextPersisted(); + APILocator.getRoleAPI().addRoleToUser( + APILocator.getRoleAPI().loadBackEndUserRole(), backendUser); + + // Simulate the post-step-1 state: give the backend user access to the roles portlet + // (this is what _addtouser would have provided before it was hardened). + rolesPortletLayout = new LayoutDataGen().name("RoleAjaxSecurityTest-roles-layout") + .portletIds("roles", "users") + .nextPersisted(); + APILocator.getRoleAPI().addLayoutToRole(rolesPortletLayout, backendUser.getUserRole()); + + assertTrue("backend user should now have roles portlet access", + APILocator.getLayoutAPI().doesUserHaveAccessToPortlet("roles", backendUser)); + + cmsAdminRoleId = APILocator.getRoleAPI().loadCMSAdminRole().getId(); + } + + @AfterClass + public static void cleanUp() throws Exception { + if (rolesPortletLayout != null) { + APILocator.getRoleAPI().removeLayoutFromRole(rolesPortletLayout, + backendUser.getUserRole()); + APILocator.getLayoutAPI().removeLayout(rolesPortletLayout); + } + if (backendUser != null) { + APILocator.getUserAPI().delete(backendUser, APILocator.systemUser(), false); + } + } + + /** + * Method to test: {@link RoleAjax#addUserToRole} + * Given Scenario: A low-privilege backend user has already obtained roles-portlet access + * (simulating a successful step 1), then calls the DWR endpoint to add + * themselves to the CMS Administrator role (step 2 of the attack chain). + * Expected Result: {@link DotSecurityException} is thrown by the {@code getAdminUser()} + * admin check — the self-escalation is blocked. + */ + @Test(expected = DotSecurityException.class) + public void test_addUserToRole_withRolesPortletAccess_cannotSelfEscalateToAdmin() + throws Exception { + setUpDwrContext(backendUser); + + final RoleAjax roleAjax = new RoleAjax(); + roleAjax.addUserToRole(backendUser.getUserId(), cmsAdminRoleId); + } + + private static void setUpDwrContext(final User user) { + final HttpSession session = mock(HttpSession.class); + when(session.getAttribute(SessionMessages.KEY)).thenReturn(new LinkedHashMap<>()); + + final HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getSession()).thenReturn(session); + when(request.getAttribute(WebKeys.USER)).thenReturn(user); + + final WebContext webContext = mock(WebContext.class); + when(webContext.getHttpServletRequest()).thenReturn(request); + + final WebContextFactory.WebContextBuilder builderMock = + mock(WebContextFactory.WebContextBuilder.class); + when(builderMock.get()).thenReturn(webContext); + + final com.dotcms.repackage.org.directwebremoting.Container containerMock = + mock(com.dotcms.repackage.org.directwebremoting.Container.class); + when(containerMock.getBean(WebContextFactory.WebContextBuilder.class)) + .thenReturn(builderMock); + + WebContextFactory.attach(containerMock); + } +}