'Tomcat' role user can gain access to adding and deleting users

Matt Raible added a comment - 14/Jun/05 7:16 PM Fixed in CVS using AOP on the UserManager.saveUser and removeUser calls: File Changes: Directory: /appfuse/ ==================== File [changed] : build.xml Url: https://appfuse.dev.java.net/source/browse/appfuse/build.xml?r1=1.92&r2=1.93 Delta lines: +1 -0 ------------------- — build.xml 26 May 2005 09:03:12 -0000 1.92 +++ build.xml 15 Jun 2005 00:11:25 -0000 1.93 @@ -1299,6 +1299,7 @@ <exclude name="src/* / .java"/> <exclude name="src/* / .xml"/> <exclude name="test/* / .java"/> + <exclude name="test/* / .xml"/> <exclude name="extras/* / .java"/> <exclude name="extras/* / .xml"/> <exclude name="metadata/web/global-exceptions.xml"/> Directory: /appfuse/extras/webwork/src/web/org/appfuse/webapp/action/ ===================================================================== File [changed] : UserAction.java Url: https://appfuse.dev.java.net/source/browse/appfuse/extras/webwork/src/web/org/appfuse/webapp/action/UserAction.java?r1=1.8&r2=1.9 Delta lines: +1 -8 ------------------- — UserAction.java 23 Jan 2005 17:28:20 -0000 1.8 +++ UserAction.java 15 Jun 2005 00:11:25 -0000 1.9 @@ -48,13 +48,6 @@ } public String delete() throws IOException { // only allow administrators to delete if (getRequest().getRemoteUser() != null && // be nice to unit tests !getRequest().isUserInRole(Constants.ADMIN_ROLE)) { - getResponse().sendError(HttpServletResponse.SC_FORBIDDEN); - return null; - } - userManager.removeUser(user.getUsername()); List args = new ArrayList(); args.add(user.getFullName()); Directory: /appfuse/src/service/org/appfuse/service/ ==================================================== File [changed] : applicationContext-service.xml Url: https://appfuse.dev.java.net/source/browse/appfuse/src/service/org/appfuse/service/applicationContext-service.xml?r1=1.20&r2=1.21 Delta lines: +19 -1 -------------------- — applicationContext-service.xml 11 Mar 2005 13:19:39 -0000 1.20 +++ applicationContext-service.xml 15 Jun 2005 00:11:25 -0000 1.21 @@ -47,8 +47,26 @@ <prop key="*">PROPAGATION_REQUIRED,readOnly</prop> </props> </property> + <property name="preInterceptors"> + <list> + <ref bean="userSecurityInterceptor"/> + </list> + </property> </bean> + <!-- This interceptor insures that that users can only update themselves, not other users --> + <bean id="userSecurityInterceptor" class="org.springframework.aop.support.RegexpMethodPointcutAdvisor"> + <property name="advice" ref="userSecurityAdvice"/> + <property name="patterns"> + <list> + <value>.*saveUser</value> + <value>.*removeUser</value> + </list> + </property> + </bean> + + <bean id="userSecurityAdvice" class="org.appfuse.service.UserSecurityAdvice"/> + <bean id="roleManager" parent="txProxyTemplate"> <property name="target"> <bean class="org.appfuse.service.impl.RoleManagerImpl"> File [added] : UserSecurityAdvice.java Url: https://appfuse.dev.java.net/source/browse/appfuse/src/service/org/appfuse/service/UserSecurityAdvice.java?rev=1.1&content-type=text/vnd.viewcvs-markup Added lines: 67 --------------- package org.appfuse.service; import org.springframework.aop.MethodBeforeAdvice; import org.appfuse.model.User; import org.appfuse.Constants; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import java.lang.reflect.Method; import net.sf.acegisecurity.context.security.SecureContext; import net.sf.acegisecurity.context.ContextHolder; import net.sf.acegisecurity.*; public class UserSecurityAdvice implements MethodBeforeAdvice { public final static String ACCESS_DENIED = "Access Denied: Only administrators are allowed to modify other users."; protected final Log log = LogFactory.getLog(UserSecurityAdvice.class); public void before(Method method, Object[] args, Object target) throws Throwable { SecureContext ctx = (SecureContext) ContextHolder.getContext(); if (ctx != null) { Authentication auth = ctx.getAuthentication(); String username = null; boolean removeUser = false; if (args [0] instanceof User) { username = ((User) args[0]).getUsername(); // saveUser } else { username = (String) args[0]; // removeUser removeUser = true; } String currentUser = null; if (auth.getPrincipal() instanceof UserDetails) { currentUser = ((UserDetails) auth.getPrincipal()).getUsername(); } else { currentUser = String.valueOf(auth.getPrincipal()); } if (auth.isAuthenticated() && (!username.equals(currentUser))) { AuthenticationTrustResolver resolver = new AuthenticationTrustResolverImpl(); // allow new users to signup - this is OK b/c Signup doesn't allow setting of roles boolean signupUser = resolver.isAnonymous(auth); if (!signupUser || removeUser) { if (log.isDebugEnabled()) { log.debug("Verifying that '" + currentUser + "' can modify '" + username + "'"); } boolean allowedToModify = false; GrantedAuthority[] roles = auth.getAuthorities(); for (int i=0; i < roles.length; i++) { if (roles [i] .getAuthority().equals(Constants.ADMIN_ROLE)) { allowedToModify = true; break; } } if (!allowedToModify) { log.warn("Access Denied: '" + currentUser + "' tried to modify '" + username + "'!"); throw new AccessDeniedException(ACCESS_DENIED); } } else { if (log.isDebugEnabled()) { log.debug("Registering new user '" + username + "'"); } } } } } } Directory: /appfuse/src/web/org/appfuse/webapp/action/ ====================================================== File [changed] : UserAction.java Url: https://appfuse.dev.java.net/source/browse/appfuse/src/web/org/appfuse/webapp/action/UserAction.java?r1=1.31&r2=1.32 Delta lines: +0 -6 ------------------- — UserAction.java 8 Jun 2005 13:51:11 -0000 1.31 +++ UserAction.java 15 Jun 2005 00:11:26 -0000 1.32 @@ -95,12 +95,6 @@ if (log.isDebugEnabled()) { log.debug("Entering 'delete' method"); } - // only allow administrators to delete if (!request.isUserInRole(Constants.ADMIN_ROLE)) { - response.sendError(HttpServletResponse.SC_FORBIDDEN); - return null; - } // Extract attributes and parameters we will need ActionMessages messages = new ActionMessages(); Directory: /appfuse/test/service/org/appfuse/service/ ===================================================== File [changed] : UserExistsExceptionTest.java Url: https://appfuse.dev.java.net/source/browse/appfuse/test/service/org/appfuse/service/UserExistsExceptionTest.java?r1=1.4&r2=1.5 Delta lines: +1 -1 ------------------- — UserExistsExceptionTest.java 28 Apr 2005 09:47:13 -0000 1.4 +++ UserExistsExceptionTest.java 15 Jun 2005 00:11:26 -0000 1.5 @@ -19,7 +19,7 @@ static { String pkg = ClassUtils.classPackageAsResourcePath(Constants.class); String[] paths = {"classpath*:/" + pkg + "/dao/applicationContext-*.xml", - "classpath*:/" + pkg + "/service/applicationContext-*.xml", + "classpath*:/" + pkg + "/service/applicationContext-service.xml", "classpath*:META-INF/applicationContext-*.xml"} ; ctx = new ClassPathXmlApplicationContext(paths); } File [added] : applicationContext-test.xml Url: https://appfuse.dev.java.net/source/browse/appfuse/test/service/org/appfuse/service/applicationContext-test.xml?rev=1.1&content-type=text/vnd.viewcvs-markup Added lines: 33 --------------- <?xml version="1.0" encoding="UTF-8"?> <!DOCTYPE beans PUBLIC "-//SPRING//DTD BEAN//EN" "http://www.springframework.org/dtd/spring-beans.dtd"> <beans> <!-- AOP Advisor --> <bean id="userSecurityInterceptor" class="org.springframework.aop.support.RegexpMethodPointcutAdvisor"> <property name="advice" ref="userSecurityAdvice"/> <property name="patterns"> <list> <value>.*saveUser</value> <value>.*removeUser</value> </list> </property> </bean> <!-- Advice Class --> <bean id="userSecurityAdvice" class="org.appfuse.service.UserSecurityAdvice"/> <bean id="userManager" class="org.appfuse.service.impl.UserManagerImpl"/> <bean id="target" class="org.springframework.aop.framework.ProxyFactoryBean"> <property name="proxyInterfaces" value="org.appfuse.service.UserManager"/> <property name="interceptorNames"> <list> <idref local="userSecurityInterceptor"/> <idref local="userManager"/> </list> </property> </bean> </beans> File [added] : UserSecurityAdviceTest.java Url: https://appfuse.dev.java.net/source/browse/appfuse/test/service/org/appfuse/service/UserSecurityAdviceTest.java?rev=1.1&content-type=text/vnd.viewcvs-markup Added lines: 102 ---------------- package org.appfuse.service; import net.sf.acegisecurity.AccessDeniedException; import net.sf.acegisecurity.Authentication; import net.sf.acegisecurity.GrantedAuthority; import net.sf.acegisecurity.GrantedAuthorityImpl; import net.sf.acegisecurity.context.ContextHolder; import net.sf.acegisecurity.context.security.SecureContext; import net.sf.acegisecurity.context.security.SecureContextImpl; import net.sf.acegisecurity.providers.UsernamePasswordAuthenticationToken; import org.appfuse.Constants; import org.appfuse.dao.UserDAO; import org.appfuse.model.User; import org.jmock.Mock; import org.springframework.context.ApplicationContext; import org.springframework.context.support.ClassPathXmlApplicationContext; public class UserSecurityAdviceTest extends BaseManagerTestCase { Mock userDAO = null; protected void setUp() throws Exception { super.setUp(); SecureContext context = new SecureContextImpl(); UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("user", "password", new GrantedAuthority[] {new GrantedAuthorityImpl(Constants.USER_ROLE)} ); token.setAuthenticated(true); context.setAuthentication(token); ContextHolder.setContext(context); } public void testAddUserWithoutAdminRole() throws Exception { Authentication auth = ((SecureContext) ContextHolder.getContext()).getAuthentication(); assertTrue(auth.isAuthenticated()); UserManager userManager = (UserManager) makeInterceptedTarget(); User user = new User(); user.setUsername("admin"); try { userManager.saveUser(user); fail("AccessDeniedException not thrown"); } catch (AccessDeniedException expected) { assertNotNull(expected); assertEquals(expected.getMessage(), UserSecurityAdvice.ACCESS_DENIED); } } public void testAddUserAsAdmin() throws Exception { SecureContext context = new SecureContextImpl(); UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("admin", "password", new GrantedAuthority[] {new GrantedAuthorityImpl(Constants.ADMIN_ROLE)}); token.setAuthenticated(true); context.setAuthentication(token); ContextHolder.setContext(context); UserManager userManager = (UserManager) makeInterceptedTarget(); User user = new User(); user.setUsername("admin"); userDAO.expects(once()).method("saveUser"); userManager.saveUser(user); userDAO.verify(); } public void testUpdateUserProfile() throws Exception { UserManager userManager = (UserManager) makeInterceptedTarget(); User user = new User(); user.setUsername("user"); userDAO.expects(once()).method("saveUser"); userManager.saveUser(user); userDAO.verify(); } public void testRemoveUserWithoutAdminRole() throws Exception { Authentication auth = ((SecureContext) ContextHolder.getContext()).getAuthentication(); assertTrue(auth.isAuthenticated()); UserManager userManager = (UserManager) makeInterceptedTarget(); try { userManager.removeUser("admin"); fail("AccessDeniedException not thrown"); } catch (AccessDeniedException expected) { assertNotNull(expected); assertEquals(expected.getMessage(), UserSecurityAdvice.ACCESS_DENIED); } } private UserManager makeInterceptedTarget() { ApplicationContext context = new ClassPathXmlApplicationContext( "org/appfuse/service/applicationContext-test.xml"); UserManager userManager = (UserManager) context.getBean("target"); // Mock the userDAO userDAO = new Mock(UserDAO.class); userManager.setUserDAO((UserDAO) userDAO.proxy()); return userManager; } }