From fd59f0bb326899f7298a33b01e271070ceba112c Mon Sep 17 00:00:00 2001 From: WangJPLeo <103574007+WangJPLeo@users.noreply.github.com> Date: Wed, 20 Jul 2022 16:32:03 +0800 Subject: [PATCH] [Fix-11031] AccessToken can only be used by the creator. (#11032) * AccessToken can only be used by the creator. * supplement ut * checkstyle fix --- .../service/impl/AccessTokenServiceImpl.java | 46 +++++++++++-------- .../api/service/AccessTokenServiceTest.java | 29 ++++-------- .../dao/mapper/AccessTokenMapper.java | 6 +-- .../dao/mapper/AccessTokenMapper.xml | 7 +-- .../dao/mapper/AccessTokenMapperTest.java | 21 ++++++--- 5 files changed, 58 insertions(+), 51 deletions(-) diff --git a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/AccessTokenServiceImpl.java b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/AccessTokenServiceImpl.java index 792138ff4e..141f9cd672 100644 --- a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/AccessTokenServiceImpl.java +++ b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/AccessTokenServiceImpl.java @@ -27,6 +27,7 @@ import org.apache.dolphinscheduler.api.utils.PageInfo; import org.apache.dolphinscheduler.api.utils.Result; import org.apache.dolphinscheduler.common.Constants; import org.apache.dolphinscheduler.common.enums.AuthorizationType; +import org.apache.dolphinscheduler.common.enums.UserType; import org.apache.dolphinscheduler.common.utils.DateUtils; import org.apache.dolphinscheduler.common.utils.EncryptionUtils; import org.apache.dolphinscheduler.dao.entity.AccessToken; @@ -35,13 +36,10 @@ import org.apache.dolphinscheduler.dao.mapper.AccessTokenMapper; import org.apache.commons.lang3.StringUtils; -import java.util.ArrayList; -import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Set; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -75,13 +73,14 @@ public class AccessTokenServiceImpl extends BaseServiceImpl implements AccessTok public Result queryAccessTokenList(User loginUser, String searchVal, Integer pageNo, Integer pageSize) { Result result = new Result(); PageInfo pageInfo = new PageInfo<>(pageNo, pageSize); - Set ids = resourcePermissionCheckService.userOwnedResourceIdsAcquisition(AuthorizationType.ACCESS_TOKEN, loginUser.getId(), logger); - if (!ids.isEmpty()) { - Page page = new Page<>(pageNo, pageSize); - IPage accessTokenList = accessTokenMapper.selectAccessTokenPage(page, new ArrayList<>(ids), searchVal); - pageInfo.setTotal((int) accessTokenList.getTotal()); - pageInfo.setTotalList(accessTokenList.getRecords()); + Page page = new Page<>(pageNo, pageSize); + int userId = loginUser.getId(); + if (loginUser.getUserType() == UserType.ADMIN_USER) { + userId = 0; } + IPage accessTokenList = accessTokenMapper.selectAccessTokenPage(page, searchVal, userId); + pageInfo.setTotal((int) accessTokenList.getTotal()); + pageInfo.setTotalList(accessTokenList.getRecords()); result.setData(pageInfo); putMsg(result, Status.SUCCESS); return result; @@ -98,11 +97,14 @@ public class AccessTokenServiceImpl extends BaseServiceImpl implements AccessTok public Map queryAccessTokenByUser(User loginUser, Integer userId) { Map result = new HashMap<>(); result.put(Constants.STATUS, false); - List accessTokenList = Collections.EMPTY_LIST; - Set ids = resourcePermissionCheckService.userOwnedResourceIdsAcquisition(AuthorizationType.ACCESS_TOKEN, loginUser.getId(), logger); - if (!ids.isEmpty()) { - accessTokenList = this.accessTokenMapper.selectBatchIds(ids); + // no permission + if (loginUser.getUserType().equals(UserType.GENERAL_USER) && loginUser.getId() != userId) { + putMsg(result, Status.USER_NO_OPERATION_PERM); + return result; } + userId = loginUser.getUserType().equals(UserType.ADMIN_USER) ? 0 : userId; + // query access token for specified user + List accessTokenList = this.accessTokenMapper.queryAccessTokenByUser(userId); result.put(Constants.DATA_LIST, accessTokenList); this.putMsg(result, Status.SUCCESS); return result; @@ -111,7 +113,7 @@ public class AccessTokenServiceImpl extends BaseServiceImpl implements AccessTok /** * create token * - * @param loginUser + * @param loginUser loginUser * @param userId token for user * @param expireTime token expire time * @param token token string (if it is absent, it will be automatically generated) @@ -154,7 +156,6 @@ public class AccessTokenServiceImpl extends BaseServiceImpl implements AccessTok if (insert > 0) { result.setData(accessToken); putMsg(result, Status.SUCCESS); - resourcePermissionCheckService.postHandle(AuthorizationType.ACCESS_TOKEN, loginUser.getId(), new ArrayList<>(accessToken.getId()), logger); } else { putMsg(result, Status.CREATE_ACCESS_TOKEN_ERROR); } @@ -189,19 +190,23 @@ public class AccessTokenServiceImpl extends BaseServiceImpl implements AccessTok @Override public Map delAccessTokenById(User loginUser, int id) { Map result = new HashMap<>(); - if (!canOperatorPermissions(loginUser,new Object[]{id},AuthorizationType.ACCESS_TOKEN,ACCESS_TOKEN_DELETE)) { + if (!canOperatorPermissions(loginUser, null, AuthorizationType.ACCESS_TOKEN,ACCESS_TOKEN_DELETE)) { putMsg(result, Status.USER_NO_OPERATION_PERM); return result; } AccessToken accessToken = accessTokenMapper.selectById(id); - if (accessToken == null) { logger.error("access token not exist, access token id {}", id); putMsg(result, Status.ACCESS_TOKEN_NOT_EXIST); return result; } + // admin can operate all, non-admin can operate their own + if (accessToken.getUserId() != loginUser.getId() && !loginUser.getUserType().equals(UserType.ADMIN_USER)) { + putMsg(result, Status.USER_NO_OPERATION_PERM); + return result; + } accessTokenMapper.deleteById(id); putMsg(result, Status.SUCCESS); return result; @@ -221,7 +226,7 @@ public class AccessTokenServiceImpl extends BaseServiceImpl implements AccessTok Map result = new HashMap<>(); // 1. check permission - if (!canOperatorPermissions(loginUser,new Object[]{id},AuthorizationType.ACCESS_TOKEN,ACCESS_TOKEN_UPDATE)) { + if (!canOperatorPermissions(loginUser, null,AuthorizationType.ACCESS_TOKEN,ACCESS_TOKEN_UPDATE)) { putMsg(result, Status.USER_NO_OPERATION_PERM); return result; } @@ -233,6 +238,11 @@ public class AccessTokenServiceImpl extends BaseServiceImpl implements AccessTok putMsg(result, Status.ACCESS_TOKEN_NOT_EXIST); return result; } + // admin can operate all, non-admin can operate their own + if (accessToken.getUserId() != loginUser.getId() && !loginUser.getUserType().equals(UserType.ADMIN_USER)) { + putMsg(result, Status.USER_NO_OPERATION_PERM); + return result; + } // 3. generate access token if absent if (StringUtils.isBlank(token)) { diff --git a/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/AccessTokenServiceTest.java b/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/AccessTokenServiceTest.java index 41d68accb8..35e5d65a7f 100644 --- a/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/AccessTokenServiceTest.java +++ b/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/AccessTokenServiceTest.java @@ -22,6 +22,7 @@ import static org.apache.dolphinscheduler.api.constants.ApiFuncIdentificationCon import static org.junit.Assert.assertEquals; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.when; import org.apache.dolphinscheduler.api.enums.Status; @@ -41,10 +42,8 @@ import org.apache.dolphinscheduler.dao.mapper.AccessTokenMapper; import java.util.ArrayList; import java.util.Calendar; import java.util.Date; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; import org.assertj.core.util.Lists; import org.junit.Assert; @@ -54,7 +53,6 @@ import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; -import org.powermock.api.mockito.PowerMockito; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -68,7 +66,6 @@ import com.baomidou.mybatisplus.extension.plugins.pagination.Page; public class AccessTokenServiceTest { private static final Logger baseServiceLogger = LoggerFactory.getLogger(BaseServiceImpl.class); private static final Logger logger = LoggerFactory.getLogger(AccessTokenServiceTest.class); - private static final Logger serviceLogger = LoggerFactory.getLogger(AccessTokenServiceImpl.class); @InjectMocks private AccessTokenServiceImpl accessTokenService; @@ -84,20 +81,16 @@ public class AccessTokenServiceTest { public void testQueryAccessTokenList() { IPage tokenPage = new Page<>(); tokenPage.setRecords(getList()); - tokenPage.setTotal(1L); User user = new User(); user.setId(1); user.setUserType(UserType.ADMIN_USER); - Set tokenIds = new HashSet<>(); - tokenIds.add(1); - when(resourcePermissionCheckService.userOwnedResourceIdsAcquisition(AuthorizationType.ACCESS_TOKEN, user.getId(), serviceLogger)).thenReturn(new HashSet()); + when(accessTokenMapper.selectAccessTokenPage(any(Page.class), eq("zhangsan"), eq(0))).thenReturn(tokenPage); Result result = accessTokenService.queryAccessTokenList(user, "zhangsan", 1, 10); PageInfo pageInfo = (PageInfo) result.getData(); assertEquals(0, (int) pageInfo.getTotal()); - PowerMockito.when(resourcePermissionCheckService.userOwnedResourceIdsAcquisition(AuthorizationType.ACCESS_TOKEN, user.getId(), serviceLogger)).thenReturn(tokenIds); - Mockito.when(accessTokenMapper.selectAccessTokenPage(Mockito.any(Page.class), Mockito.anyList(),Mockito.eq("zhangsan"))).thenReturn(tokenPage); - + tokenPage.setTotal(1L); + when(accessTokenMapper.selectAccessTokenPage(any(Page.class), eq("zhangsan"), eq(0))).thenReturn(tokenPage); result = accessTokenService.queryAccessTokenList(user, "zhangsan", 1, 10); pageInfo = (PageInfo) result.getData(); logger.info(result.toString()); @@ -109,10 +102,7 @@ public class AccessTokenServiceTest { User user = this.getLoginUser(); user.setUserType(UserType.ADMIN_USER); List accessTokenList = Lists.newArrayList(this.getEntity()); - - Set tokenIds = new HashSet<>(); - tokenIds.add(1); - Mockito.when(resourcePermissionCheckService.userOwnedResourceIdsAcquisition(AuthorizationType.ACCESS_TOKEN, user.getId(), serviceLogger)).thenReturn(tokenIds); + Mockito.when(this.accessTokenMapper.queryAccessTokenByUser(Mockito.anyInt())).thenReturn(accessTokenList); Map result = this.accessTokenService.queryAccessTokenByUser(user, 1); logger.info(result.toString()); assertEquals(Status.SUCCESS, result.get(Constants.STATUS)); @@ -152,19 +142,20 @@ public class AccessTokenServiceTest { userLogin.setId(1); userLogin.setUserType(UserType.ADMIN_USER); Mockito.when(resourcePermissionCheckService.operationPermissionCheck(AuthorizationType.ACCESS_TOKEN, 1, ACCESS_TOKEN_DELETE, baseServiceLogger)).thenReturn(true); - Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.ACCESS_TOKEN, new Object[]{0}, 0, baseServiceLogger)).thenReturn(true); + Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.ACCESS_TOKEN, null, 0, baseServiceLogger)).thenReturn(true); // not exist Map result = accessTokenService.delAccessTokenById(userLogin, 0); logger.info(result.toString()); assertEquals(Status.ACCESS_TOKEN_NOT_EXIST, result.get(Constants.STATUS)); // no operate + userLogin.setId(2); result = accessTokenService.delAccessTokenById(userLogin, 1); logger.info(result.toString()); assertEquals(Status.USER_NO_OPERATION_PERM, result.get(Constants.STATUS)); //success userLogin.setId(1); userLogin.setUserType(UserType.ADMIN_USER); - Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.ACCESS_TOKEN, new Object[]{1}, 0, baseServiceLogger)).thenReturn(true); + Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.ACCESS_TOKEN, null, 0, baseServiceLogger)).thenReturn(true); result = accessTokenService.delAccessTokenById(userLogin, 1); logger.info(result.toString()); assertEquals(Status.SUCCESS, result.get(Constants.STATUS)); @@ -176,7 +167,7 @@ public class AccessTokenServiceTest { user.setId(1); user.setUserType(UserType.ADMIN_USER); Mockito.when(resourcePermissionCheckService.operationPermissionCheck(AuthorizationType.ACCESS_TOKEN, 1, ACCESS_TOKEN_UPDATE, baseServiceLogger)).thenReturn(true); - Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.ACCESS_TOKEN, new Object[]{1}, 0, baseServiceLogger)).thenReturn(true); + Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.ACCESS_TOKEN, null, 0, baseServiceLogger)).thenReturn(true); // Given Token when(accessTokenMapper.selectById(1)).thenReturn(getEntity()); Map result = accessTokenService.updateToken(getLoginUser(), 1,Integer.MAX_VALUE,getDate(),"token"); @@ -191,7 +182,7 @@ public class AccessTokenServiceTest { Assert.assertNotNull(result.get(Constants.DATA_LIST)); // ACCESS_TOKEN_NOT_EXIST - Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.ACCESS_TOKEN, new Object[]{2}, 0, baseServiceLogger)).thenReturn(true); + Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.ACCESS_TOKEN, null, 0, baseServiceLogger)).thenReturn(true); result = accessTokenService.updateToken(getLoginUser(), 2,Integer.MAX_VALUE,getDate(),"token"); logger.info(result.toString()); assertEquals(Status.ACCESS_TOKEN_NOT_EXIST, result.get(Constants.STATUS)); diff --git a/dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapper.java b/dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapper.java index 926a1f5a44..b6fabb66c1 100644 --- a/dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapper.java +++ b/dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapper.java @@ -37,13 +37,13 @@ public interface AccessTokenMapper extends BaseMapper { * access token page * * @param page page - * @param tokenIds tokenIds + * @param userId userId * @param userName userName * @return access token Ipage */ IPage selectAccessTokenPage(Page page, - @Param("ids") List tokenIds, - @Param("userName") String userName + @Param("userName") String userName, + @Param("userId") int userId ); /** diff --git a/dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapper.xml b/dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapper.xml index b6baff29db..969572b1de 100644 --- a/dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapper.xml +++ b/dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapper.xml @@ -26,11 +26,8 @@ and u.user_name like concat ('%', #{userName}, '%') - - and t.id in - - #{id} - + + and t.user_id = #{userId} order by t.update_time desc diff --git a/dolphinscheduler-dao/src/test/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapperTest.java b/dolphinscheduler-dao/src/test/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapperTest.java index 38d34d1815..c569b3b926 100644 --- a/dolphinscheduler-dao/src/test/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapperTest.java +++ b/dolphinscheduler-dao/src/test/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapperTest.java @@ -30,9 +30,10 @@ import org.apache.dolphinscheduler.dao.BaseDaoTest; import org.apache.dolphinscheduler.dao.entity.AccessToken; import org.apache.dolphinscheduler.dao.entity.User; +import java.util.ArrayList; import java.util.HashMap; -import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.ThreadLocalRandom; import java.util.stream.Collectors; @@ -86,7 +87,6 @@ public class AccessTokenMapperTest extends BaseDaoTest { Assert.assertEquals(insertCount, deleteCount); } - /** * test select by id * @@ -139,17 +139,26 @@ public class AccessTokenMapperTest extends BaseDaoTest { Integer size = 2; Map accessTokenMap = createAccessTokens(count, userName); + Set userIds = accessTokenMap.values().stream().map(AccessToken::getUserId).collect(Collectors.toSet()); + Integer createTokenUserId = new ArrayList<>(userIds).get(0); + // general user and create token user Page page = new Page(offset, size); - List tokenIds = accessTokenMap.values().stream().map(AccessToken::getId).collect(Collectors.toList()); - IPage accessTokenPage = accessTokenMapper.selectAccessTokenPage(page, tokenIds, userName); - + IPage accessTokenPage = accessTokenMapper.selectAccessTokenPage(page, userName, createTokenUserId); assertEquals(Integer.valueOf(accessTokenPage.getRecords().size()), size); - for (AccessToken accessToken : accessTokenPage.getRecords()) { + // admin user + IPage adminAccessTokenPage = accessTokenMapper.selectAccessTokenPage(page, userName, 0); + assertEquals(Integer.valueOf(adminAccessTokenPage.getRecords().size()), size); + for (AccessToken accessToken : adminAccessTokenPage.getRecords()) { AccessToken resultAccessToken = accessTokenMap.get(accessToken.getId()); assertEquals(accessToken, resultAccessToken); } + + // general user + Integer emptySize = 0; + IPage generalAccessTokenPage = accessTokenMapper.selectAccessTokenPage(page, userName, 1); + assertEquals(Integer.valueOf(generalAccessTokenPage.getRecords().size()), emptySize); }