sec: harden portlet-only gated methods in RoleAjax #36345
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🟠 [High] Over-restrictive role requirement in ToolGroupResource
The PR adds CMS_ADMINISTRATOR_ROLE to @RolesAllowed annotation, requiring admin privileges for a method that should be accessible to non-admin users with proper portlet permissions. This breaks existing access control logic by introducing an unnecessary admin role requirement, as evidenced by the test cases in ToolGroupResourceTest which don't validate admin-only access.