This is an automated email from the ASF dual-hosted git repository. xxyu pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kylin.git
The following commit(s) were added to refs/heads/master by this push: new 443c252 User info update logic is not correct 443c252 is described below commit 443c2523e27e86ed397c526f741db62a805b95c4 Author: yangjiang <yangji...@ebay.com> AuthorDate: Wed Sep 30 11:20:04 2020 +0800 User info update logic is not correct --- .../kylin/rest/controller/UserController.java | 1 + .../rest/security/KylinAuthenticationProvider.java | 26 +++++++++++++++++----- .../kylin/rest/security/KylinUserManager.java | 4 ++++ .../kylin/rest/service/KylinUserGroupService.java | 3 +++ .../kylin/rest/service/KylinUserService.java | 7 +++++- .../org/apache/kylin/rest/service/UserService.java | 2 ++ 6 files changed, 36 insertions(+), 7 deletions(-) diff --git a/server-base/src/main/java/org/apache/kylin/rest/controller/UserController.java b/server-base/src/main/java/org/apache/kylin/rest/controller/UserController.java index 6d62b38..83f6a29 100644 --- a/server-base/src/main/java/org/apache/kylin/rest/controller/UserController.java +++ b/server-base/src/main/java/org/apache/kylin/rest/controller/UserController.java @@ -217,6 +217,7 @@ public class UserController extends BasicController { throw new BadRequestException("pwd update error"); } + existing = userService.copyForWrite(existing); existing.setPassword(pwdEncode(user.getNewPassword())); existing.setDefaultPassword(false); logger.info("update password for user {}", user); diff --git a/server-base/src/main/java/org/apache/kylin/rest/security/KylinAuthenticationProvider.java b/server-base/src/main/java/org/apache/kylin/rest/security/KylinAuthenticationProvider.java index 590c15a..ed072a3 100644 --- a/server-base/src/main/java/org/apache/kylin/rest/security/KylinAuthenticationProvider.java +++ b/server-base/src/main/java/org/apache/kylin/rest/security/KylinAuthenticationProvider.java @@ -26,6 +26,7 @@ import java.util.Arrays; import javax.annotation.PostConstruct; import org.apache.kylin.common.KylinConfig; +import org.apache.kylin.rest.constant.Constant; import org.apache.kylin.rest.service.UserService; import org.slf4j.Logger; @@ -37,6 +38,7 @@ import org.springframework.cache.CacheManager; import org.springframework.security.authentication.AuthenticationProvider; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.userdetails.UserDetails; import org.springframework.security.core.userdetails.UsernameNotFoundException; @@ -113,8 +115,8 @@ public class KylinAuthenticationProvider implements AuthenticationProvider { logger.debug("User {} authorities : {}", username, user.getAuthorities()); if (!userService.userExists(username)) { userService.createUser(user); - } else if (needUpdateUser(user, username)) { - userService.updateUser(user); + } else { + updateUserIfNeeded(user, username); } cacheManager.getCache(USER_CACHE).put(userKey, authed); @@ -129,10 +131,22 @@ public class KylinAuthenticationProvider implements AuthenticationProvider { return authed; } - // in case ldap users changing. - private boolean needUpdateUser(ManagedUser user, String username) { - return KylinConfig.getInstanceFromEnv().getSecurityProfile().equals("ldap") - && !userService.loadUserByUsername(username).equals(user); + private void updateUserIfNeeded(ManagedUser newUser, String username) { + String securityProfile = KylinConfig.getInstanceFromEnv().getSecurityProfile(); + // in case ldap users changing. + if (securityProfile.equals("ldap") || securityProfile.equals("saml")) { + UserDetails existingUser = userService.loadUserByUsername(username); + SimpleGrantedAuthority groupAllUsersAuthority = new SimpleGrantedAuthority(Constant.GROUP_ALL_USERS); + if (existingUser.getAuthorities().contains(groupAllUsersAuthority)) { + if (!newUser.getAuthorities().contains(groupAllUsersAuthority)) { + newUser.getAuthorities().add(groupAllUsersAuthority); + } + } + if (!existingUser.equals(newUser)) { + logger.info("Going to update user info for {}", username); + userService.updateUser(newUser); + } + } } @Override diff --git a/server-base/src/main/java/org/apache/kylin/rest/security/KylinUserManager.java b/server-base/src/main/java/org/apache/kylin/rest/security/KylinUserManager.java index afa78b0..fef556f 100644 --- a/server-base/src/main/java/org/apache/kylin/rest/security/KylinUserManager.java +++ b/server-base/src/main/java/org/apache/kylin/rest/security/KylinUserManager.java @@ -139,4 +139,8 @@ public class KylinUserManager { return userMap.containsKey(username); } } + + public ManagedUser copyForWrite(ManagedUser user) { + return crud.copyForWrite(user); + } } diff --git a/server-base/src/main/java/org/apache/kylin/rest/service/KylinUserGroupService.java b/server-base/src/main/java/org/apache/kylin/rest/service/KylinUserGroupService.java index f5563c9..7af8d76 100644 --- a/server-base/src/main/java/org/apache/kylin/rest/service/KylinUserGroupService.java +++ b/server-base/src/main/java/org/apache/kylin/rest/service/KylinUserGroupService.java @@ -154,6 +154,7 @@ public class KylinUserGroupService extends UserGroupService { List<ManagedUser> managedUsers = userService.listUsers(); for (ManagedUser managedUser : managedUsers) { if (managedUser.getAuthorities().contains(new SimpleGrantedAuthority(name))) { + managedUser = userService.copyForWrite(managedUser); managedUser.removeAuthorities(name); userService.updateUser(managedUser); } @@ -181,12 +182,14 @@ public class KylinUserGroupService extends UserGroupService { for (String in : moveInUsers) { ManagedUser managedUser = (ManagedUser) userService.loadUserByUsername(in); + managedUser = userService.copyForWrite(managedUser); managedUser.addAuthorities(groupName); userService.updateUser(managedUser); } for (String out : moveOutUsers) { ManagedUser managedUser = (ManagedUser) userService.loadUserByUsername(out); + managedUser = userService.copyForWrite(managedUser); managedUser.removeAuthorities(groupName); userService.updateUser(managedUser); } diff --git a/server-base/src/main/java/org/apache/kylin/rest/service/KylinUserService.java b/server-base/src/main/java/org/apache/kylin/rest/service/KylinUserService.java index 1f93fc2..2d3151e 100644 --- a/server-base/src/main/java/org/apache/kylin/rest/service/KylinUserService.java +++ b/server-base/src/main/java/org/apache/kylin/rest/service/KylinUserService.java @@ -38,6 +38,7 @@ import org.apache.kylin.rest.msg.MsgPicker; import org.apache.kylin.rest.security.KylinUserManager; import org.apache.kylin.rest.security.ManagedUser; import org.apache.kylin.rest.util.AclEvaluate; +import org.apache.kylin.shaded.com.google.common.base.Preconditions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; @@ -47,7 +48,6 @@ import org.springframework.security.core.userdetails.UserDetails; import org.springframework.security.core.userdetails.UsernameNotFoundException; import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; -import org.apache.kylin.shaded.com.google.common.base.Preconditions; public class KylinUserService implements UserService { @@ -210,6 +210,11 @@ public class KylinUserService implements UserService { } @Override + public ManagedUser copyForWrite(ManagedUser user) { + return getKylinUserManager().copyForWrite(user); + } + + @Override public void completeUserInfo(ManagedUser user) { } diff --git a/server-base/src/main/java/org/apache/kylin/rest/service/UserService.java b/server-base/src/main/java/org/apache/kylin/rest/service/UserService.java index 1734be2..119e25a 100644 --- a/server-base/src/main/java/org/apache/kylin/rest/service/UserService.java +++ b/server-base/src/main/java/org/apache/kylin/rest/service/UserService.java @@ -34,6 +34,8 @@ public interface UserService extends UserDetailsManager { List<String> listAdminUsers() throws IOException; + ManagedUser copyForWrite(ManagedUser user); + //For performance consideration, list all users may be incomplete(eg. not load user's authorities until authorities has benn used). //So it's an extension point that can complete user's information latter. //loadUserByUsername() has guarantee that the return user is complete.