Merge pull request #1289 from navi-infra/INFRA-3970-11
INFRA-3970 | Dhruv | fix manifest read condition
This commit is contained in:
@@ -2,6 +2,7 @@ package com.navi.infra.portal.service.user;
|
||||
|
||||
import static com.navi.infra.portal.v2.privilege.Action.APPROVAL_WRITE;
|
||||
import static com.navi.infra.portal.v2.privilege.Action.KUBE_DELETE;
|
||||
import static com.navi.infra.portal.v2.privilege.Action.MANIFEST_READ;
|
||||
import static com.navi.infra.portal.v2.privilege.Action.MANIFEST_SECRET_READ;
|
||||
import static com.navi.infra.portal.v2.privilege.Action.MANIFEST_SECRET_WRITE;
|
||||
import static com.navi.infra.portal.v2.privilege.Action.MANIFEST_SUBSTITUTE_SECRETS;
|
||||
@@ -38,7 +39,7 @@ public class PrivilegeUtilService {
|
||||
|
||||
public boolean canReadManifest(String team, String environment) {
|
||||
return authorizationFilter.hasPermissions(MANIFEST, team, environment,
|
||||
MANIFEST_SECRET_WRITE);
|
||||
MANIFEST_READ);
|
||||
}
|
||||
|
||||
public boolean canSubstituteEnvironmentVariable(Manifest manifest) {
|
||||
|
||||
@@ -22,6 +22,7 @@ import com.navi.infra.portal.domain.manifest.Manifest;
|
||||
import com.navi.infra.portal.domain.user.User;
|
||||
import com.navi.infra.portal.security.authorization.AuthorizationContext;
|
||||
import com.navi.infra.portal.service.manifest.ManifestService;
|
||||
import com.navi.infra.portal.service.user.PrivilegeUtilService;
|
||||
import com.navi.infra.portal.service.user.UserService;
|
||||
import com.navi.infra.portal.util.MapUtil;
|
||||
import com.navi.infra.portal.util.manifest.ChangeRequestUtils;
|
||||
@@ -69,6 +70,7 @@ public class ChangeRequestServiceImpl implements ChangeRequestService {
|
||||
private final MapUtil mapUtil;
|
||||
private final AuthUtil authUtil;
|
||||
private final Set<String> ignoredFieldsForMerge;
|
||||
private final PrivilegeUtilService privilegeUtilService;
|
||||
|
||||
public ChangeRequestServiceImpl(
|
||||
ChangeRequestRepository repository,
|
||||
@@ -79,6 +81,7 @@ public class ChangeRequestServiceImpl implements ChangeRequestService {
|
||||
ObjectMapper objectMapper,
|
||||
MapUtil mapUtil,
|
||||
AuthUtil authUtil,
|
||||
PrivilegeUtilService privilegeUtilService,
|
||||
@Value("version") Set<String> ignoredFieldsForMerge
|
||||
) {
|
||||
this.repository = repository;
|
||||
@@ -89,6 +92,7 @@ public class ChangeRequestServiceImpl implements ChangeRequestService {
|
||||
this.objectMapper = objectMapper;
|
||||
this.mapUtil = mapUtil;
|
||||
this.authUtil = authUtil;
|
||||
this.privilegeUtilService = privilegeUtilService;
|
||||
this.ignoredFieldsForMerge = unmodifiableSet(
|
||||
requireNonNullElse(ignoredFieldsForMerge, emptySet()));
|
||||
}
|
||||
@@ -199,17 +203,17 @@ public class ChangeRequestServiceImpl implements ChangeRequestService {
|
||||
.orElseThrow(() -> new NotFoundException("Change request not found"));
|
||||
final var approvals = approvalService.findAllByRequestTypeAndRequestIds(
|
||||
RequestType.CHANGE_REQUEST, List.of(requestId));
|
||||
|
||||
final var canView = approvalService.canApproveSomeApprovalRequest(
|
||||
AuthorizationContext.getUserId(), approvals);
|
||||
Manifest manifest = manifestService.fetchByIdWithoutAuthorization(
|
||||
changeRequest.getManifestId());
|
||||
var canView = approvalService.
|
||||
canApproveSomeApprovalRequest(AuthorizationContext.getUserId(), approvals) ||
|
||||
privilegeUtilService.canReadManifest(manifest.getTeam(), manifest.getEnvironment());
|
||||
if (!canView) {
|
||||
log.warn("User {} does not have access to view CR: {}",
|
||||
AuthorizationContext.getUserEmail(),
|
||||
requestId);
|
||||
return null;
|
||||
}
|
||||
Manifest manifest = manifestService.fetchByIdWithoutAuthorization(
|
||||
changeRequest.getManifestId());
|
||||
return createCrDto(List.of(manifest), List.of(changeRequest), approvals).get(0);
|
||||
}
|
||||
|
||||
|
||||
@@ -16,6 +16,7 @@ import com.navi.infra.portal.domain.user.Team;
|
||||
import com.navi.infra.portal.domain.user.User;
|
||||
import com.navi.infra.portal.exceptions.ManifestNotFoundException;
|
||||
import com.navi.infra.portal.service.manifest.ManifestService;
|
||||
import com.navi.infra.portal.service.user.PrivilegeUtilService;
|
||||
import com.navi.infra.portal.service.user.UserService;
|
||||
import com.navi.infra.portal.util.MapUtil;
|
||||
import com.navi.infra.portal.v2.approvalflow.dto.ApprovalRequestDto;
|
||||
@@ -63,6 +64,8 @@ class ChangeRequestServiceImplTest {
|
||||
@Mock
|
||||
private ManifestService manifestService;
|
||||
@Mock
|
||||
private PrivilegeUtilService privilegeUtilService;
|
||||
@Mock
|
||||
private ChangeRequestSlackService changeRequestSlackService;
|
||||
@Mock
|
||||
private UserService userService;
|
||||
@@ -92,7 +95,7 @@ class ChangeRequestServiceImplTest {
|
||||
service = new ChangeRequestServiceImpl(repo, approvalRequestService,
|
||||
changeRequestSlackService,
|
||||
manifestService, userService, new ObjectMapper(),
|
||||
new MapUtil(), null, null);
|
||||
new MapUtil(), null, privilegeUtilService, null);
|
||||
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user