Skip to content

[Fix-11031] AccessToken can only be used by the creator. #11032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
caishunfeng merged 5 commits into from
Jul 20, 2022
Merged

[Fix-11031] AccessToken can only be used by the creator. #11032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a948a1
AccessToken can only be used by the creator.
WangJPLeo Jul 18, 2022
7815776
supplement ut
WangJPLeo Jul 19, 2022
1d77425
ci run
WangJPLeo Jul 20, 2022
05cef6a
checkstyle fix
WangJPLeo Jul 20, 2022
f981ec0
e2e rerun
WangJPLeo Jul 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 28 additions & 18 deletions ...pi/src/main/java/org/apache/dolphinscheduler/api/service/impl/AccessTokenServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
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;
Expand All @@ -35,13 +36,10 @@

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;
Expand Down Expand Up @@ -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<AccessToken> pageInfo = new PageInfo<>(pageNo, pageSize);
Set<Integer> ids = resourcePermissionCheckService.userOwnedResourceIdsAcquisition(AuthorizationType.ACCESS_TOKEN, loginUser.getId(), logger);
if (!ids.isEmpty()) {
Page<AccessToken> page = new Page<>(pageNo, pageSize);
IPage<AccessToken> accessTokenList = accessTokenMapper.selectAccessTokenPage(page, new ArrayList<>(ids), searchVal);
pageInfo.setTotal((int) accessTokenList.getTotal());
pageInfo.setTotalList(accessTokenList.getRecords());
Page<AccessToken> page = new Page<>(pageNo, pageSize);
int userId = loginUser.getId();
if (loginUser.getUserType() == UserType.ADMIN_USER) {
userId = 0;
}
IPage<AccessToken> accessTokenList = accessTokenMapper.selectAccessTokenPage(page, searchVal, userId);
pageInfo.setTotal((int) accessTokenList.getTotal());
pageInfo.setTotalList(accessTokenList.getRecords());
result.setData(pageInfo);
putMsg(result, Status.SUCCESS);
return result;
Expand All @@ -98,11 +97,14 @@ public Result queryAccessTokenList(User loginUser, String searchVal, Integer pag
public Map<String, Object> queryAccessTokenByUser(User loginUser, Integer userId) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
List<AccessToken> accessTokenList = Collections.EMPTY_LIST;
Set<Integer> 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<AccessToken> accessTokenList = this.accessTokenMapper.queryAccessTokenByUser(userId);
result.put(Constants.DATA_LIST, accessTokenList);
this.putMsg(result, Status.SUCCESS);
return result;
Expand All @@ -111,7 +113,7 @@ public Map<String, Object> queryAccessTokenByUser(User loginUser, Integer userId
/**
* 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)
Expand Down Expand Up @@ -154,7 +156,6 @@ public Result createToken(User loginUser, int userId, String expireTime, String
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);
}
Expand Down Expand Up @@ -189,19 +190,23 @@ public Map<String, Object> generateToken(User loginUser, int userId, String expi
@Override
public Map<String, Object> delAccessTokenById(User loginUser, int id) {
Map<String, Object> 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;
Expand All @@ -221,7 +226,7 @@ public Map<String, Object> updateToken(User loginUser, int id, int userId, Strin
Map<String, Object> 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;
}
Expand All @@ -233,6 +238,11 @@ public Map<String, Object> updateToken(User loginUser, int id, int userId, Strin
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)) {
Expand Down
29 changes: 10 additions & 19 deletions ...ler-api/src/test/java/org/apache/dolphinscheduler/api/service/AccessTokenServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

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;
Expand All @@ -41,10 +42,8 @@
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;
Expand All @@ -54,7 +53,6 @@
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;

Expand All @@ -68,7 +66,6 @@
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;
Expand All @@ -84,20 +81,16 @@ public class AccessTokenServiceTest {
public void testQueryAccessTokenList() {
IPage<AccessToken> tokenPage = new Page<>();
tokenPage.setRecords(getList());
tokenPage.setTotal(1L);
User user = new User();
user.setId(1);
user.setUserType(UserType.ADMIN_USER);
Set<Integer> 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<AccessToken> pageInfo = (PageInfo<AccessToken>) 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<AccessToken>) result.getData();
logger.info(result.toString());
Expand All @@ -109,10 +102,7 @@ public void testQueryAccessTokenByUser() {
User user = this.getLoginUser();
user.setUserType(UserType.ADMIN_USER);
List<AccessToken> accessTokenList = Lists.newArrayList(this.getEntity());

Set<Integer> 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<String, Object> result = this.accessTokenService.queryAccessTokenByUser(user, 1);
logger.info(result.toString());
assertEquals(Status.SUCCESS, result.get(Constants.STATUS));
Expand Down Expand Up @@ -152,19 +142,20 @@ public void testDelAccessTokenById() {
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<String, Object> 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));
Expand All @@ -176,7 +167,7 @@ public void testUpdateToken() {
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<String, Object> result = accessTokenService.updateToken(getLoginUser(), 1,Integer.MAX_VALUE,getDate(),"token");
Expand All @@ -191,7 +182,7 @@ public void testUpdateToken() {
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));
Expand Down
6 changes: 3 additions & 3 deletions ...scheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ public interface AccessTokenMapper extends BaseMapper<AccessToken> {
* access token page
*
* @param page page
* @param tokenIds tokenIds
* @param userId userId
* @param userName userName
* @return access token Ipage
*/
IPage<AccessToken> selectAccessTokenPage(Page page,
@Param("ids") List<Integer> tokenIds,
@Param("userName") String userName
@Param("userName") String userName,
@Param("userId") int userId
);

/**
Expand Down
7 changes: 2 additions & 5 deletions ...duler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapper.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,8 @@
<if test="userName != null and userName != ''">
and u.user_name like concat ('%', #{userName}, '%')
</if>
<if test="ids != null and ids.size() > 0">
and t.id in
<foreach item="id" index="index" collection="ids" open="(" separator="," close=")">
#{id}
</foreach>
<if test="userId != 0">
and t.user_id = #{userId}
</if>
order by t.update_time desc
</select>
Expand Down
21 changes: 15 additions & 6 deletions ...duler-dao/src/test/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@
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;

Expand Down Expand Up @@ -86,7 +87,6 @@ public void testDeleteAccessTokenByUserId() {
Assert.assertEquals(insertCount, deleteCount);
}


/**
* test select by id
*
Expand Down Expand Up @@ -139,17 +139,26 @@ public void testSelectAccessTokenPage() throws Exception {
Integer size = 2;

Map<Integer, AccessToken> accessTokenMap = createAccessTokens(count, userName);
Set<Integer> 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<Integer> tokenIds = accessTokenMap.values().stream().map(AccessToken::getId).collect(Collectors.toList());
IPage<AccessToken> accessTokenPage = accessTokenMapper.selectAccessTokenPage(page, tokenIds, userName);

IPage<AccessToken> accessTokenPage = accessTokenMapper.selectAccessTokenPage(page, userName, createTokenUserId);
assertEquals(Integer.valueOf(accessTokenPage.getRecords().size()), size);

for (AccessToken accessToken : accessTokenPage.getRecords()) {
// admin user
IPage<AccessToken> 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<AccessToken> generalAccessTokenPage = accessTokenMapper.selectAccessTokenPage(page, userName, 1);
assertEquals(Integer.valueOf(generalAccessTokenPage.getRecords().size()), emptySize);
}


Expand Down