This is an automated email from the ASF dual-hosted git repository. wuzhiguo pushed a commit to branch trunk in repository https://gitbox.apache.org/repos/asf/ambari.git
The following commit(s) were added to refs/heads/trunk by this push: new a4d579c67a AMBARI-25201: check acting users password on change password request (#3444) a4d579c67a is described below commit a4d579c67af031092873e07c31c5970c04a6cc2d Author: Zhiguo Wu <wuzhi...@apache.org> AuthorDate: Wed Nov 9 02:36:59 2022 +0800 AMBARI-25201: check acting users password on change password request (#3444) --- .../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 fc32d62818..d8d36c3e0a 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 java.util.regex.Pattern; @@ -1248,14 +1249,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"); } @@ -1273,6 +1273,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 2d065d3b82..da47027efa 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; @@ -50,6 +54,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; @@ -58,6 +63,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; @@ -164,6 +172,8 @@ public class TestUsers { users.grantAdminPrivilege(userEntity); users.addLocalAuthentication(userEntity, "admin"); + setAuthenticatedUser(userEntity); + userEntity = users.createUser("user", "user", "user"); users.addLocalAuthentication(userEntity, "user"); @@ -178,7 +188,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); @@ -204,7 +214,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 (IllegalArgumentException e) { assertEquals("The password does not meet the password policy requirements", e.getLocalizedMessage()); @@ -616,4 +626,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 b32f114a21..9a3d237a4d 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; @@ -50,23 +53,25 @@ 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.security.encryption.Encryptor; 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 com.google.inject.TypeLiteral; import com.google.inject.name.Names; -import junit.framework.Assert; - public class UsersTest extends EasyMockSupport { private static final String SERVICEOP_USER_NAME = "serviceopuser"; @@ -179,6 +184,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); @@ -190,6 +241,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)); } @@ -204,13 +284,19 @@ 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(new TypeLiteral<Encryptor<AmbariServerConfiguration>>() {}).annotatedWith(Names.named("AmbariServerConfigurationEncryptor")).toInstance(Encryptor.NONE); 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); } }); } --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@ambari.apache.org For additional commands, e-mail: commits-h...@ambari.apache.org