This is an automated email from the ASF dual-hosted git repository. benyoka pushed a commit to branch branch-2.7 in repository https://gitbox.apache.org/repos/asf/ambari.git
The following commit(s) were added to refs/heads/branch-2.7 by this push: new 1d58a2a AMBARI-25201 check acting users password on change password request (#2924) 1d58a2a is described below commit 1d58a2ad29cd9ee2ffb1a7c0cc2beda7c13effd1 Author: benyoka <beny...@users.noreply.github.com> AuthorDate: Fri Apr 12 19:32:50 2019 +0200 AMBARI-25201 check acting users password on change password request (#2924) * AMBARI-25201 check acting users password on change password request * AMBARI-25201 fix failing unit tests (benyoka) --- .../server/security/authorization/Users.java | 23 ++++-- .../server/security/authorization/TestUsers.java | 24 +++++- .../server/security/authorization/UsersTest.java | 92 +++++++++++++++++++++- 3 files changed, 126 insertions(+), 13 deletions(-) diff --git a/ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java b/ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java index 3e750c6..0974a72 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java @@ -26,6 +26,7 @@ import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import javax.persistence.EntityManager; @@ -1230,14 +1231,13 @@ public class Users { if (userAuthenticationEntity != null) { if (userAuthenticationEntity.getAuthenticationType() == UserAuthenticationType.LOCAL) { - // If the authentication record represents a local password and the authenticated user is - // changing the password for himself, ensure the old key value matches the current key value - // If the authenticated user is can manager users and is not changing his own password, there - // is no need to check that the authenticated user knows the current password - just update it. - if (isSelf && - (StringUtils.isEmpty(currentKey) || !passwordEncoder.matches(currentKey, userAuthenticationEntity.getAuthenticationKey()))) { - // The authenticated user is the same user as subject user and the correct current password - // was not supplied. + + String expectedCurrentKey = isSelf + ? userAuthenticationEntity.getAuthenticationKey() + : getAuthenticatedUserLocalAuthenticationMethod(). + orElseThrow(() -> new AmbariException("Authentication error")).getAuthenticationKey(); + + if (StringUtils.isEmpty(currentKey) || !passwordEncoder.matches(currentKey, expectedCurrentKey)) { throw new AmbariException("Wrong current password provided"); } @@ -1255,6 +1255,13 @@ public class Users { } } + private Optional<AuthenticationMethod> getAuthenticatedUserLocalAuthenticationMethod() { + User authenticatedUser = getUser(AuthorizationHelper.getAuthenticatedId()); + return authenticatedUser.getAuthenticationMethods().stream() + .filter(am -> UserAuthenticationType.LOCAL.equals(am.getAuthenticationType())) + .findAny(); + } + public void removeAuthentication(String username, Long authenticationId) { removeAuthentication(getUserEntity(username), authenticationId); } diff --git a/ambari-server/src/test/java/org/apache/ambari/server/security/authorization/TestUsers.java b/ambari-server/src/test/java/org/apache/ambari/server/security/authorization/TestUsers.java index f7174f5..0c1ce05 100644 --- a/ambari-server/src/test/java/org/apache/ambari/server/security/authorization/TestUsers.java +++ b/ambari-server/src/test/java/org/apache/ambari/server/security/authorization/TestUsers.java @@ -17,6 +17,10 @@ */ package org.apache.ambari.server.security.authorization; +import static java.util.Collections.emptySet; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.mock; +import static org.easymock.EasyMock.replay; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNotSame; @@ -49,6 +53,7 @@ import org.apache.ambari.server.orm.entities.ResourceEntity; import org.apache.ambari.server.orm.entities.ResourceTypeEntity; import org.apache.ambari.server.orm.entities.UserAuthenticationEntity; import org.apache.ambari.server.orm.entities.UserEntity; +import org.apache.ambari.server.security.authentication.AmbariUserDetailsImpl; import org.apache.ambari.server.security.ldap.LdapBatchDto; import org.apache.ambari.server.security.ldap.LdapGroupDto; import org.apache.ambari.server.security.ldap.LdapUserDto; @@ -57,6 +62,9 @@ import org.easymock.EasyMock; import org.junit.After; import org.junit.Before; import org.junit.Test; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContext; +import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.crypto.password.PasswordEncoder; import com.google.inject.Guice; @@ -161,6 +169,8 @@ public class TestUsers { users.grantAdminPrivilege(userEntity); users.addLocalAuthentication(userEntity, "admin"); + setAuthenticatedUser(userEntity); + userEntity = users.createUser("user", "user", "user"); users.addLocalAuthentication(userEntity, "user"); @@ -175,7 +185,7 @@ public class TestUsers { foundUserEntity = userDAO.findUserByName("admin"); assertNotNull(foundUserEntity); - users.modifyAuthentication(foundLocalAuthenticationEntity, "user", "user_new_password", false); + users.modifyAuthentication(foundLocalAuthenticationEntity, "admin", "user_new_password", false); foundUserEntity = userDAO.findUserByName("user"); assertNotNull(foundUserEntity); @@ -201,7 +211,7 @@ public class TestUsers { assertTrue(passwordEncoder.matches("user", foundLocalAuthenticationEntity.getAuthenticationKey())); try { - users.modifyAuthentication(foundLocalAuthenticationEntity, "user", null, false); + users.modifyAuthentication(foundLocalAuthenticationEntity, "user", null, true); fail("Null password should not be allowed"); } catch (AmbariException e) { assertEquals("The new password does not meet the Ambari password requirements", e.getLocalizedMessage()); @@ -602,4 +612,14 @@ public class TestUsers { return null; } + + private void setAuthenticatedUser(UserEntity userEntity) { + AmbariUserDetailsImpl principal = new AmbariUserDetailsImpl(new User(userEntity), "", emptySet()); + Authentication auth = mock(Authentication.class); + expect(auth.getPrincipal()).andReturn(principal).anyTimes(); + SecurityContext securityContext = mock(SecurityContext.class); + expect(securityContext.getAuthentication()).andReturn(auth).anyTimes(); + replay(auth, securityContext); + SecurityContextHolder.setContext(securityContext); + } } diff --git a/ambari-server/src/test/java/org/apache/ambari/server/security/authorization/UsersTest.java b/ambari-server/src/test/java/org/apache/ambari/server/security/authorization/UsersTest.java index cdb10be..d895cbd 100644 --- a/ambari-server/src/test/java/org/apache/ambari/server/security/authorization/UsersTest.java +++ b/ambari-server/src/test/java/org/apache/ambari/server/security/authorization/UsersTest.java @@ -18,6 +18,7 @@ package org.apache.ambari.server.security.authorization; +import static org.easymock.EasyMock.anyInt; import static org.easymock.EasyMock.anyObject; import static org.easymock.EasyMock.anyString; import static org.easymock.EasyMock.capture; @@ -25,11 +26,13 @@ import static org.easymock.EasyMock.eq; import static org.easymock.EasyMock.expect; import static org.easymock.EasyMock.expectLastCall; import static org.easymock.EasyMock.newCapture; +import static org.junit.Assert.assertEquals; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.Objects; import javax.annotation.Nullable; import javax.persistence.EntityManager; @@ -49,20 +52,22 @@ import org.apache.ambari.server.orm.entities.MemberEntity; import org.apache.ambari.server.orm.entities.PermissionEntity; import org.apache.ambari.server.orm.entities.PrincipalEntity; import org.apache.ambari.server.orm.entities.PrivilegeEntity; +import org.apache.ambari.server.orm.entities.UserAuthenticationEntity; import org.apache.ambari.server.orm.entities.UserEntity; import org.apache.ambari.server.state.stack.OsFamily; import org.easymock.Capture; import org.easymock.EasyMock; import org.easymock.EasyMockSupport; +import org.junit.Assert; import org.junit.Test; import org.springframework.security.crypto.password.PasswordEncoder; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.inject.AbstractModule; import com.google.inject.Guice; import com.google.inject.Injector; -import junit.framework.Assert; - public class UsersTest extends EasyMockSupport { private static final String SERVICEOP_USER_NAME = "serviceopuser"; @@ -175,6 +180,52 @@ public class UsersTest extends EasyMockSupport { users.createUser(SERVICEOP_USER_NAME, SERVICEOP_USER_NAME, SERVICEOP_USER_NAME); } + @Test + public void modifyAuthentication_local_bySameUser() throws AmbariException { + // given + UserAuthenticationEntity entity = initForModifyAuthentication(); + + // when + Users users = injector.getInstance(Users.class); + users.modifyAuthentication(entity, "hello", "world", true); + + // then + assertEquals("world", entity.getAuthenticationKey()); + } + + @Test(expected = AmbariException.class) + public void modifyAuthentication_local_bySameUser_wrongPassword() throws AmbariException { + // given + UserAuthenticationEntity entity = initForModifyAuthentication(); + + // when + Users users = injector.getInstance(Users.class); + users.modifyAuthentication(entity, "12345", "world", true); + } + + @Test + public void modifyAuthentication_local_byAdminUser() throws AmbariException { + // given + UserAuthenticationEntity entity = initForModifyAuthentication(); + + // when + Users users = injector.getInstance(Users.class); + users.modifyAuthentication(entity, "admin1234", "world", false); + + // then + assertEquals("world", entity.getAuthenticationKey()); + } + + @Test(expected = AmbariException.class) + public void modifyAuthentication_local_byAdminUser_wrongPassword() throws AmbariException { + // given + UserAuthenticationEntity entity = initForModifyAuthentication(); + + // when + Users users = injector.getInstance(Users.class); + users.modifyAuthentication(entity, "wrong password", "world", false); + } + private void initForCreateUser(@Nullable UserEntity existingUser) { UserDAO userDao = createStrictMock(UserDAO.class); expect(userDao.findUserByName(anyString())).andReturn(existingUser); @@ -186,6 +237,35 @@ public class UsersTest extends EasyMockSupport { createInjector(userDao, entityManager); } + private UserAuthenticationEntity initForModifyAuthentication() { + UserAuthenticationEntity userEntity = new UserAuthenticationEntity(); + userEntity.setAuthenticationKey("hello"); + userEntity.setAuthenticationType(UserAuthenticationType.LOCAL); + + EntityManager manager = mock(EntityManager.class); + expect(manager.merge(userEntity)).andReturn(userEntity).once(); + + UserDAO dao = createMock(UserDAO.class); + UserEntity admin = new UserEntity(); + admin.setUserId(-1); + admin.setUserName("admin"); + PrincipalEntity principalEntity = new PrincipalEntity(); + admin.setPrincipal(principalEntity); + PrivilegeEntity privilegeEntity = new PrivilegeEntity(); + principalEntity.setPrivileges(ImmutableSet.of(privilegeEntity)); + PermissionEntity permissionEntity = new PermissionEntity(); + privilegeEntity.setPermission(permissionEntity); + permissionEntity.setPermissionName(PermissionEntity.AMBARI_ADMINISTRATOR_PERMISSION_NAME); + UserAuthenticationEntity adminAuthentication = new UserAuthenticationEntity(); + admin.setAuthenticationEntities(ImmutableList.of(adminAuthentication)); + adminAuthentication.setAuthenticationKey("admin1234"); + expect(dao.findByPK(anyInt())).andReturn(admin).anyTimes(); + + createInjector(dao, manager); + replayAll(); + return userEntity; + } + private void createInjector() { createInjector(createMock(UserDAO.class), createMock(EntityManager.class)); } @@ -200,12 +280,18 @@ public class UsersTest extends EasyMockSupport { bind(UserDAO.class).toInstance(mockUserDao); bind(MemberDAO.class).toInstance(createMock(MemberDAO.class)); bind(PrivilegeDAO.class).toInstance(createMock(PrivilegeDAO.class)); - bind(PasswordEncoder.class).toInstance(createMock(PasswordEncoder.class)); bind(HookService.class).toInstance(createMock(HookService.class)); bind(HookContextFactory.class).toInstance(createMock(HookContextFactory.class)); bind(PrincipalDAO.class).toInstance(createMock(PrincipalDAO.class)); bind(Configuration.class).toInstance(createNiceMock(Configuration.class)); bind(AmbariLdapConfigurationProvider.class).toInstance(createMock(AmbariLdapConfigurationProvider.class)); + + PasswordEncoder nopEncoder = createMock(PasswordEncoder.class); + expect(nopEncoder.matches(anyString(), anyString())).andAnswer( + () -> Objects.equals(EasyMock.getCurrentArguments()[0], EasyMock.getCurrentArguments()[1])).anyTimes(); + expect(nopEncoder.encode(anyString())).andAnswer( + () -> (String)EasyMock.getCurrentArguments()[0]).anyTimes(); + bind(PasswordEncoder.class).toInstance(nopEncoder); } }); }