Skip to content

Commit

Permalink
CB-5051 handle internal calls with internal actor correctly
Browse files Browse the repository at this point in the history
  • Loading branch information
horadla23 committed Jun 29, 2020
1 parent 3f1a4c7 commit 34f2165
Show file tree
Hide file tree
Showing 80 changed files with 169 additions and 110 deletions.
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package com.sequenceiq.cloudbreak.audit.util;

import static com.sequenceiq.cloudbreak.auth.altus.GrpcUmsClient.INTERNAL_ACTOR_CRN;
import static com.sequenceiq.cloudbreak.auth.ThreadBasedUserCrnProvider.INTERNAL_ACTOR_CRN;

import org.springframework.stereotype.Component;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package com.sequenceiq.cloudbreak.audit.util;

import static com.sequenceiq.cloudbreak.auth.altus.GrpcUmsClient.INTERNAL_ACTOR_CRN;
import static com.sequenceiq.cloudbreak.auth.ThreadBasedUserCrnProvider.INTERNAL_ACTOR_CRN;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
import com.sequenceiq.cloudbreak.auth.altus.config.UmsConfig;
import com.sequenceiq.cloudbreak.auth.altus.exception.UmsOperationException;
import com.sequenceiq.cloudbreak.auth.altus.model.AltusCredential;
import com.sequenceiq.cloudbreak.auth.security.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.auth.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.grpc.ManagedChannelWrapper;
import com.sequenceiq.cloudbreak.logger.MDCUtils;
import com.sequenceiq.common.api.telemetry.model.AnonymizationRule;
Expand All @@ -51,8 +51,6 @@
@Component
public class GrpcUmsClient {

public static final String INTERNAL_ACTOR_CRN = new InternalCrnBuilder(Crn.Service.IAM).getInternalCrnForServiceAsString();

private static final Logger LOGGER = LoggerFactory.getLogger(GrpcUmsClient.class);

private static final String ACCOUNT_IN_IAM_CRNS = "altus";
Expand Down Expand Up @@ -588,7 +586,7 @@ public String getIdentityProviderMetadataXml(String accountId, String actorCrn)
// @CacheEvict(cacheNames = {"umsUserRightsCache", "umsUserRoleAssigmentsCache", "umsResourceAssigneesCache"}, key = "#userCrn")
public void assignResourceRole(String userCrn, String resourceCrn, String resourceRoleCrn, Optional<String> requestId) {
try (ManagedChannelWrapper channelWrapper = makeWrapper()) {
UmsClient client = makeClient(channelWrapper.getChannel(), INTERNAL_ACTOR_CRN);
UmsClient client = makeClient(channelWrapper.getChannel(), ThreadBasedUserCrnProvider.INTERNAL_ACTOR_CRN);
LOGGER.info("Assigning {} role for resource {} to user {}", resourceRoleCrn, resourceCrn, userCrn);
client.assignResourceRole(requestId.orElse(UUID.randomUUID().toString()), userCrn, resourceCrn, resourceRoleCrn);
LOGGER.info("Assigned {} role for resource {} to user {}", resourceRoleCrn, resourceCrn, userCrn);
Expand All @@ -614,7 +612,7 @@ public void assignResourceOwnerRoleIfEntitled(String userCrn, String resourceCrn
// @CacheEvict(cacheNames = {"umsUserRightsCache", "umsUserRoleAssigmentsCache", "umsResourceAssigneesCache"}, key = "#userCrn")
public void unassignResourceRole(String userCrn, String resourceCrn, String resourceRoleCrn, Optional<String> requestId) {
try (ManagedChannelWrapper channelWrapper = makeWrapper()) {
UmsClient client = makeClient(channelWrapper.getChannel(), INTERNAL_ACTOR_CRN);
UmsClient client = makeClient(channelWrapper.getChannel(), ThreadBasedUserCrnProvider.INTERNAL_ACTOR_CRN);
LOGGER.info("Unassigning {} role for resource {} from user {}", resourceRoleCrn, resourceCrn, userCrn);
client.unassignResourceRole(requestId.orElse(UUID.randomUUID().toString()), userCrn, resourceCrn, resourceRoleCrn);
LOGGER.info("Unassigned {} role for resource {} from user {}", resourceRoleCrn, resourceCrn, userCrn);
Expand All @@ -626,7 +624,7 @@ public void unassignResourceRole(String userCrn, String resourceCrn, String reso
public void notifyResourceDeleted(String resourceCrn, Optional<String> requestId) {
try (ManagedChannelWrapper channelWrapper = makeWrapper()) {
LOGGER.debug("Notify UMS about resource ('{}') was deleted", resourceCrn);
UmsClient client = makeClient(channelWrapper.getChannel(), INTERNAL_ACTOR_CRN);
UmsClient client = makeClient(channelWrapper.getChannel(), ThreadBasedUserCrnProvider.INTERNAL_ACTOR_CRN);
client.notifyResourceDeleted(requestId.orElse(UUID.randomUUID().toString()), resourceCrn);
LOGGER.info("Notified UMS about deletion of resource {}", resourceCrn);
} catch (Exception e) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.sequenceiq.cloudbreak.auth.altus;

import static com.sequenceiq.cloudbreak.auth.ThreadBasedUserCrnProvider.INTERNAL_ACTOR_CRN;

import java.util.HashMap;
import java.util.Map;

Expand All @@ -10,8 +12,6 @@
import org.springframework.stereotype.Component;
import org.springframework.util.StringUtils;

import com.sequenceiq.cloudbreak.auth.altus.Crn.Service;
import com.sequenceiq.cloudbreak.auth.security.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.logger.MDCUtils;

import io.grpc.Status.Code;
Expand All @@ -21,8 +21,6 @@
public class VirtualGroupService {
private static final Logger LOGGER = LoggerFactory.getLogger(VirtualGroupService.class);

private static final String IAM_INTERNAL_ACTOR_CRN = new InternalCrnBuilder(Service.IAM).getInternalCrnForServiceAsString();

@Inject
private GrpcUmsClient grpcUmsClient;

Expand Down Expand Up @@ -50,7 +48,7 @@ public void cleanupVirtualGroups(String accountId, String environmentCrn) {
for (UmsRight right : UmsRight.values()) {
try {
LOGGER.debug("Start deleting virtual groups from UMS for environment '{}'", environmentCrn);
grpcUmsClient.deleteWorkloadAdministrationGroupName(IAM_INTERNAL_ACTOR_CRN, accountId,
grpcUmsClient.deleteWorkloadAdministrationGroupName(INTERNAL_ACTOR_CRN, accountId,
MDCUtils.getRequestId(), right.getRight(), environmentCrn);
LOGGER.debug("Virtual groups deletion from UMS has been finished successfully for environment '{}'", environmentCrn);
} catch (RuntimeException ex) {
Expand All @@ -62,14 +60,14 @@ public void cleanupVirtualGroups(String accountId, String environmentCrn) {
private String createOrGetVirtualGroup(String accountId, String environmentCrn, String right) {
String virtualGroup = "";
try {
virtualGroup = grpcUmsClient.getWorkloadAdministrationGroupName(IAM_INTERNAL_ACTOR_CRN, accountId, MDCUtils.getRequestId(), right, environmentCrn);
virtualGroup = grpcUmsClient.getWorkloadAdministrationGroupName(INTERNAL_ACTOR_CRN, accountId, MDCUtils.getRequestId(), right, environmentCrn);
} catch (StatusRuntimeException ex) {
if (Code.NOT_FOUND != ex.getStatus().getCode()) {
throw ex;
}
}
if (StringUtils.isEmpty(virtualGroup)) {
virtualGroup = grpcUmsClient.setWorkloadAdministrationGroupName(IAM_INTERNAL_ACTOR_CRN, accountId,
virtualGroup = grpcUmsClient.setWorkloadAdministrationGroupName(INTERNAL_ACTOR_CRN, accountId,
MDCUtils.getRequestId(), right, environmentCrn);
LOGGER.info("{} workloadAdministrationGroup is created for {} right on {} environment", virtualGroup, right, environmentCrn);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import org.springframework.security.core.userdetails.UsernameNotFoundException;
import org.springframework.stereotype.Service;

import com.sequenceiq.cloudbreak.auth.CrnUser;
import com.sequenceiq.cloudbreak.auth.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.auth.altus.Crn;
import com.sequenceiq.cloudbreak.auth.altus.GrpcUmsClient;
import com.sequenceiq.cloudbreak.auth.security.authentication.UmsAuthenticationService;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.stereotype.Service;

import com.sequenceiq.cloudbreak.auth.security.CrnUser;
import com.sequenceiq.cloudbreak.auth.CrnUser;
import com.sequenceiq.cloudbreak.common.user.CloudbreakUser;

@Service
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
import com.sequenceiq.cloudbreak.auth.altus.CrnParseException;
import com.sequenceiq.cloudbreak.auth.altus.GrpcUmsClient;
import com.sequenceiq.cloudbreak.auth.altus.exception.UmsAuthenticationException;
import com.sequenceiq.cloudbreak.auth.security.CrnUser;
import com.sequenceiq.cloudbreak.auth.security.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.auth.CrnUser;
import com.sequenceiq.cloudbreak.auth.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.common.user.CloudbreakUser;
import com.sequenceiq.cloudbreak.logger.LoggerContextKey;
import com.sequenceiq.cloudbreak.logger.MDCBuilder;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@

import com.sequenceiq.cloudbreak.auth.ThreadBasedUserCrnProvider;
import com.sequenceiq.cloudbreak.auth.altus.Crn;
import com.sequenceiq.cloudbreak.auth.security.CrnUser;
import com.sequenceiq.cloudbreak.auth.security.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.auth.CrnUser;
import com.sequenceiq.cloudbreak.auth.InternalCrnBuilder;

@Service
public class InternalCrnModifier {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package com.sequenceiq.cloudbreak.auth.security.internal;

import com.sequenceiq.cloudbreak.auth.security.CrnUser;
import com.sequenceiq.cloudbreak.auth.CrnUser;

public class InternalUserModifier {

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.sequenceiq.cloudbreak.auth.altus;

import static com.sequenceiq.cloudbreak.auth.ThreadBasedUserCrnProvider.INTERNAL_ACTOR_CRN;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.anyString;
Expand All @@ -20,17 +21,12 @@
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

import com.sequenceiq.cloudbreak.auth.altus.Crn.Service;
import com.sequenceiq.cloudbreak.auth.security.InternalCrnBuilder;

import io.grpc.Status;
import io.grpc.StatusRuntimeException;

@RunWith(MockitoJUnitRunner.class)
public class VirtualGroupServiceTest {

private static final String IAM_INTERNAL_ACTOR_CRN = new InternalCrnBuilder(Service.IAM).getInternalCrnForServiceAsString();

private static final String MOCK_VIRTUAL_GROUP = "mock_virtual_group";

private static final String ACCOUNT_ID = "accountId";
Expand Down Expand Up @@ -61,7 +57,7 @@ public void testCreateVirtualGroupsWithNonExistingGroups() {
assertEquals(UmsRight.values().length, result.size());
verify(grpcUmsClient, times(UmsRight.values().length)).setWorkloadAdministrationGroupName(crnCaptor.capture(), eq(ACCOUNT_ID),
eq(Optional.empty()), rightCaptor.capture(), eq(ENV_CRN));
assertEquals(IAM_INTERNAL_ACTOR_CRN, crnCaptor.getValue());
assertEquals(INTERNAL_ACTOR_CRN, crnCaptor.getValue());
for (UmsRight umsRight : UmsRight.values()) {
assertEquals(MOCK_VIRTUAL_GROUP, result.get(umsRight));
assertTrue(rightCaptor.getAllValues().contains(umsRight.getRight()));
Expand Down Expand Up @@ -95,7 +91,7 @@ public void testGetVirtualGroupWithNoAdminGroupProvidedAndNewGroupNeedsToBeCreat

verify(grpcUmsClient, times(1)).setWorkloadAdministrationGroupName(crnCaptor.capture(), eq(ACCOUNT_ID),
eq(Optional.empty()), rightCaptor.capture(), eq(ENV_CRN));
assertEquals(IAM_INTERNAL_ACTOR_CRN, crnCaptor.getValue());
assertEquals(INTERNAL_ACTOR_CRN, crnCaptor.getValue());
assertEquals(MOCK_VIRTUAL_GROUP, result);
assertTrue(rightCaptor.getAllValues().contains(UmsRight.ENVIRONMENT_ACCESS.getRight()));
}
Expand All @@ -112,7 +108,7 @@ public void testGetVirtualGroupWithEmptyGroupProvidedAndNewGroupNeedsToBeCreated

verify(grpcUmsClient, times(1)).setWorkloadAdministrationGroupName(crnCaptor.capture(), eq(ACCOUNT_ID),
eq(Optional.empty()), rightCaptor.capture(), eq(ENV_CRN));
assertEquals(IAM_INTERNAL_ACTOR_CRN, crnCaptor.getValue());
assertEquals(INTERNAL_ACTOR_CRN, crnCaptor.getValue());
assertEquals(MOCK_VIRTUAL_GROUP, result);
assertTrue(rightCaptor.getAllValues().contains(UmsRight.ENVIRONMENT_ACCESS.getRight()));
}
Expand All @@ -135,7 +131,7 @@ public void testCleanupVirtualGroups() {

verify(grpcUmsClient, times(UmsRight.values().length)).deleteWorkloadAdministrationGroupName(crnCaptor.capture(), eq(ACCOUNT_ID),
eq(Optional.empty()), rightCaptor.capture(), eq(ENV_CRN));
assertEquals(IAM_INTERNAL_ACTOR_CRN, crnCaptor.getValue());
assertEquals(INTERNAL_ACTOR_CRN, crnCaptor.getValue());
for (UmsRight right : UmsRight.values()) {
assertTrue(rightCaptor.getAllValues().contains(right.getRight()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.springframework.security.core.userdetails.UserDetails;

import com.cloudera.thunderhead.service.usermanagement.UserManagementProto.User;
import com.sequenceiq.cloudbreak.auth.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.auth.altus.Crn;
import com.sequenceiq.cloudbreak.auth.altus.GrpcUmsClient;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import org.junit.Test;

import com.sequenceiq.cloudbreak.auth.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.auth.altus.Crn;

public class InternalCrnBuilderTest {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import org.mockito.junit.MockitoJUnitRunner;

import com.sequenceiq.cloudbreak.auth.ThreadBasedUserCrnProvider;
import com.sequenceiq.cloudbreak.auth.security.CrnUser;
import com.sequenceiq.cloudbreak.auth.CrnUser;

@RunWith(MockitoJUnitRunner.class)
public class InternalCrnModifierTest {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.sequenceiq.authorization.annotation;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;


@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
public @interface InternalOnly {
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import com.sequenceiq.authorization.annotation.CheckPermissionByResourceObject;
import com.sequenceiq.authorization.annotation.DisableCheckPermissions;
import com.sequenceiq.authorization.annotation.FilterListBasedOnPermissions;
import com.sequenceiq.authorization.annotation.InternalOnly;

public class AuthorizationAnnotationUtils {

Expand All @@ -24,6 +25,6 @@ public static List<Class<? extends Annotation>> getPossibleMethodAnnotations() {
return List.of(CheckPermissionByResourceCrn.class, CheckPermissionByResourceName.class, CheckPermissionByAccount.class,
DisableCheckPermissions.class, CheckPermissionByResourceCrnList.class, CheckPermissionByResourceNameList.class,
CheckPermissionByResourceObject.class, CheckPermissionByEnvironmentCrn.class, CheckPermissionByEnvironmentName.class,
FilterListBasedOnPermissions.class);
FilterListBasedOnPermissions.class, InternalOnly.class);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@
import com.sequenceiq.authorization.annotation.AuthorizationResource;
import com.sequenceiq.authorization.annotation.DisableCheckPermissions;
import com.sequenceiq.authorization.annotation.FilterListBasedOnPermissions;
import com.sequenceiq.authorization.annotation.InternalOnly;
import com.sequenceiq.authorization.service.list.ListPermissionChecker;
import com.sequenceiq.authorization.util.AuthorizationAnnotationUtils;
import com.sequenceiq.cloudbreak.auth.ThreadBasedUserCrnProvider;
import com.sequenceiq.cloudbreak.auth.security.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.auth.InternalCrnBuilder;

@Service
public class PermissionCheckService {
Expand Down Expand Up @@ -73,6 +74,11 @@ protected Object checkPermission(ProceedingJoinPoint proceedingJoinPoint, Method
.filter(Objects::nonNull)
.collect(Collectors.toList());

if (annotations.stream().anyMatch(annotation -> annotation instanceof InternalOnly) &&
!InternalCrnBuilder.isInternalCrn(ThreadBasedUserCrnProvider.getUserCrn())) {
getAccessDeniedAndLogInternalActorRestriction(methodSignature);
}

if (annotations.stream().anyMatch(annotation -> annotation instanceof DisableCheckPermissions)) {
return commonPermissionCheckingUtils.proceed(proceedingJoinPoint, methodSignature, startTime);
} else if (annotations.stream().anyMatch(annotation -> annotation instanceof FilterListBasedOnPermissions)) {
Expand All @@ -93,4 +99,10 @@ private AccessDeniedException getAccessDeniedAndLogMissingAnnotation(Class<?> re
AuthorizationResource.class.getName());
return new AccessDeniedException("You have no access to this resource.");
}

private AccessDeniedException getAccessDeniedAndLogInternalActorRestriction(MethodSignature methodSignature) {
LOGGER.error("Method {} should be called by internal actor only.",
methodSignature.getMethod().getDeclaringClass().getSimpleName() + "#" + methodSignature.getMethod().getName());
return new AccessDeniedException("You have no access to this resource.");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public void checkCallerIsSelfOrHasRight(String actorCrnStr, String targetUserCrn
throw new AccessDeniedException(unauthorizedMessage);
}
validateAction(action);
if (!umsClient.checkRight(GrpcUmsClient.INTERNAL_ACTOR_CRN, actorCrn.toString(), umsRightProvider.getRight(action), getRequestId())) {
if (!umsClient.checkRight(ThreadBasedUserCrnProvider.INTERNAL_ACTOR_CRN, actorCrn.toString(), umsRightProvider.getRight(action), getRequestId())) {
String unauthorizedMessage = String.format("You have no right to perform %s on user %s.", umsRightProvider.getRight(action), targetUserCrnStr);
LOGGER.error(unauthorizedMessage);
throw new AccessDeniedException(unauthorizedMessage);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import com.sequenceiq.authorization.annotation.EnvironmentCrn;
import com.sequenceiq.authorization.annotation.EnvironmentName;
import com.sequenceiq.authorization.annotation.FilterListBasedOnPermissions;
import com.sequenceiq.authorization.annotation.InternalOnly;
import com.sequenceiq.authorization.annotation.ResourceCrn;
import com.sequenceiq.authorization.annotation.ResourceCrnList;
import com.sequenceiq.authorization.annotation.ResourceName;
Expand Down Expand Up @@ -86,6 +87,7 @@ public class EnforceAuthorizationLogicsUtil {
.put(CheckPermissionByResourceNameList.class, anyCollectionFrom(ResourceNameList.class, list(String.class), set(String.class)))
.put(DisableCheckPermissions.class, noRestriction())
.put(CheckPermissionByAccount.class, noRestriction())
.put(InternalOnly.class, noRestriction())
.put(FilterListBasedOnPermissions.class,
returnsAnyOf(
list(ResourceCrnAwareApiModel.class),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import org.springframework.context.annotation.Configuration;

import com.sequenceiq.cloudbreak.auth.altus.Crn.Service;
import com.sequenceiq.cloudbreak.auth.security.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.auth.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.client.CloudbreakInternalCrnClient;
import com.sequenceiq.cloudbreak.client.CloudbreakServiceUserCrnClient;
import com.sequenceiq.cloudbreak.client.CloudbreakUserCrnClientBuilder;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import org.springframework.scheduling.concurrent.ThreadPoolExecutorFactoryBean;
import org.springframework.test.context.TestPropertySource;

import com.sequenceiq.cloudbreak.auth.security.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.auth.InternalCrnBuilder;
import com.sequenceiq.cloudbreak.common.service.Clock;
import com.sequenceiq.periscope.aspects.RequestLogging;
import com.sequenceiq.periscope.monitor.ClusterManagerHostHealthMonitor;
Expand Down
3 changes: 3 additions & 0 deletions common/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ dependencies {
}
compile group: 'javax.validation', name: 'validation-api', version: javaxValidationVersion

compile group: 'org.springframework.security', name: 'spring-security-jwt', version: '1.0.8.RELEASE'
compile group: 'org.springframework.security', name: 'spring-security-core', version: springFrameworkVersion

testCompile group: 'org.springframework.boot', name: 'spring-boot-starter-test', version: springBootVersion
testCompile group: 'org.springframework.boot', name: 'spring-boot-starter-validation', version: springBootVersion
}
Expand Down
Loading

0 comments on commit 34f2165

Please sign in to comment.