From 22aee8d5d18cac980ee878edaac256c228a0f90e Mon Sep 17 00:00:00 2001 From: Radovan Semancik Date: Wed, 12 Sep 2018 17:47:55 +0200 Subject: [PATCH] Fixes for improved multitenant authorizations (MID-4882) --- .../security/TestSecurityMultitenant.java | 163 +++++++++++------- .../security/multitenant/user-dmurr.xml | 29 ++++ .../security/multitenant/user-duncan.xml | 29 ++++ .../security/multitenant/user-piter.xml | 29 ++++ .../enforcer/impl/SecurityEnforcerImpl.java | 49 +++--- 5 files changed, 216 insertions(+), 83 deletions(-) create mode 100644 model/model-intest/src/test/resources/security/multitenant/user-dmurr.xml create mode 100644 model/model-intest/src/test/resources/security/multitenant/user-duncan.xml create mode 100644 model/model-intest/src/test/resources/security/multitenant/user-piter.xml diff --git a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/TestSecurityMultitenant.java b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/TestSecurityMultitenant.java index 907034a9e47..e54aaa26dd1 100644 --- a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/TestSecurityMultitenant.java +++ b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/TestSecurityMultitenant.java @@ -15,81 +15,21 @@ */ package com.evolveum.midpoint.model.intest.security; -import static org.testng.AssertJUnit.assertFalse; -import static org.testng.AssertJUnit.assertTrue; -import static org.testng.AssertJUnit.assertNotNull; -import static org.testng.AssertJUnit.assertEquals; - import java.io.File; -import java.io.IOException; -import java.util.Collection; -import java.util.List; - -import javax.xml.datatype.XMLGregorianCalendar; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.annotation.DirtiesContext.ClassMode; import org.springframework.test.context.ContextConfiguration; import org.testng.annotations.Test; -import com.evolveum.midpoint.model.api.ModelAuthorizationAction; -import com.evolveum.midpoint.model.api.RoleSelectionSpecification; -import com.evolveum.midpoint.prism.PrismContainer; -import com.evolveum.midpoint.prism.PrismObject; -import com.evolveum.midpoint.prism.PrismObjectDefinition; -import com.evolveum.midpoint.prism.PrismReferenceValue; -import com.evolveum.midpoint.prism.delta.ObjectDelta; -import com.evolveum.midpoint.prism.path.ItemPath; -import com.evolveum.midpoint.prism.path.NameItemPathSegment; -import com.evolveum.midpoint.prism.query.ObjectFilter; -import com.evolveum.midpoint.prism.query.ObjectQuery; -import com.evolveum.midpoint.prism.query.TypeFilter; -import com.evolveum.midpoint.prism.util.PrismAsserts; -import com.evolveum.midpoint.prism.util.PrismTestUtil; -import com.evolveum.midpoint.prism.xml.XmlTypeConverter; -import com.evolveum.midpoint.schema.GetOperationOptions; -import com.evolveum.midpoint.schema.SelectorOptions; -import com.evolveum.midpoint.schema.constants.SchemaConstants; import com.evolveum.midpoint.schema.result.OperationResult; import com.evolveum.midpoint.schema.util.MiscSchemaUtil; -import com.evolveum.midpoint.security.api.MidPointPrincipal; -import com.evolveum.midpoint.security.enforcer.api.AuthorizationParameters; import com.evolveum.midpoint.task.api.Task; -import com.evolveum.midpoint.test.DummyResourceContoller; -import com.evolveum.midpoint.test.IntegrationTestTools; -import com.evolveum.midpoint.util.exception.CommunicationException; -import com.evolveum.midpoint.util.exception.ConfigurationException; -import com.evolveum.midpoint.util.exception.ExpressionEvaluationException; -import com.evolveum.midpoint.util.exception.ObjectAlreadyExistsException; -import com.evolveum.midpoint.util.exception.ObjectNotFoundException; -import com.evolveum.midpoint.util.exception.PolicyViolationException; -import com.evolveum.midpoint.util.exception.SchemaException; -import com.evolveum.midpoint.util.exception.SecurityViolationException; import com.evolveum.midpoint.xml.ns._public.common.api_types_3.ImportOptionsType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ActivationStatusType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ActivationType; import com.evolveum.midpoint.xml.ns._public.common.common_3.AssignmentPolicyEnforcementType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.AssignmentType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.AuthorizationPhaseType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ConstructionType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ExclusionPolicyConstraintType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.FocusType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.LookupTableType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.MappingType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.MetadataType; import com.evolveum.midpoint.xml.ns._public.common.common_3.ModelExecuteOptionsType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ObjectReferenceType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ObjectType; import com.evolveum.midpoint.xml.ns._public.common.common_3.OrgType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.PasswordType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.PolicyConstraintsType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.PolicyExceptionType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.PolicyRuleType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ResourceAttributeDefinitionType; import com.evolveum.midpoint.xml.ns._public.common.common_3.RoleType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.ShadowType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.SystemConfigurationType; -import com.evolveum.midpoint.xml.ns._public.common.common_3.SystemObjectsType; import com.evolveum.midpoint.xml.ns._public.common.common_3.UserType; /** @@ -109,30 +49,54 @@ public class TestSecurityMultitenant extends AbstractSecurityTest { protected static final String ROLE_TENANT_ADMIN_OID = "00000000-8888-6666-a000-100000000000"; protected static final String ORG_GUILD_OID = "00000000-8888-6666-a001-000000000000"; + protected static final String ROLE_GUILD_BROKEN_ADMIN_OID = "00000000-8888-6666-a001-100000000001"; + protected static final String USER_EDRIC_OID = "00000000-8888-6666-a001-200000000000"; protected static final String USER_EDRIC_NAME = "edric"; protected static final String USER_EDRIC_FULL_NAME = "Navigator Edric"; + protected static final File USER_DMURR_FILE = new File(TEST_DIR, "user-dmurr.xml"); + protected static final String USER_DMURR_OID = "00000000-8888-6666-a001-200000000001"; + protected static final String USER_DMURR_NAME = "dmurr"; + protected static final String USER_DMURR_FULL_NAME = "D'murr Pilru"; + protected static final String ORG_CORRINO_OID = "00000000-8888-6666-a100-000000000000"; + protected static final String ROLE_CORRINO_ADMIN_OID = "00000000-8888-6666-a100-100000000000"; + protected static final String USER_SHADDAM_CORRINO_OID = "00000000-8888-6666-a100-200000000000"; protected static final String USER_SHADDAM_CORRINO_NAME = "shaddam"; protected static final String USER_SHADDAM_CORRINO_FULL_NAME = "Padishah Emperor Shaddam IV"; protected static final String ORG_ATREIDES_OID = "00000000-8888-6666-a200-000000000000"; + protected static final String ROLE_ATREIDES_ADMIN_OID = "00000000-8888-6666-a200-100000000000"; + protected static final String USER_LETO_ATREIDES_OID = "00000000-8888-6666-a200-200000000000"; protected static final String USER_LETO_ATREIDES_NAME = "leto"; protected static final String USER_LETO_ATREIDES_FULL_NAME = "Duke Leto Atreides"; + protected static final String USER_PAUL_ATREIDES_OID = "00000000-8888-6666-a200-200000000001"; protected static final String USER_PAUL_ATREIDES_NAME = "paul"; protected static final String USER_PAUL_ATREIDES_FULL_NAME = "Paul Atreides"; + protected static final File USER_DUNCAN_FILE = new File(TEST_DIR, "user-duncan.xml"); + protected static final String USER_DUNCAN_OID = "00000000-8888-6666-a200-200000000002"; + protected static final String USER_DUNCAN_NAME = "duncan"; + protected static final String USER_DUNCAN_FULL_NAME = "Duncan Idaho"; + protected static final String ORG_HARKONNEN_OID = "00000000-8888-6666-a300-000000000000"; + protected static final String ROLE_HARKONNEN_ADMIN_OID = "00000000-8888-6666-a300-100000000000"; + protected static final String USER_VLADIMIR_HARKONNEN_OID = "00000000-8888-6666-a300-200000000000"; + protected static final File USER_PITER_FILE = new File(TEST_DIR, "user-piter.xml"); + protected static final String USER_PITER_OID = "00000000-8888-6666-a300-200000000001"; + protected static final String USER_PITER_NAME = "piter"; + protected static final String USER_PITER_FULL_NAME = "Piter De Vries"; + @Override public void initSystem(Task initTask, OperationResult initResult) throws Exception { super.initSystem(initTask, initResult); @@ -324,8 +288,51 @@ public void test100AutzLetoRead() throws Exception { * MID-4882 */ @Test - public void test102AutzLetoModify() throws Exception { - final String TEST_NAME = "test102AutzLetoModify"; + public void test102AutzLetoAdd() throws Exception { + final String TEST_NAME = "test102AutzLetoAdd"; + displayTestTitle(TEST_NAME); + // GIVEN + cleanupAutzTest(null); + + login(USER_LETO_ATREIDES_NAME); + + // WHEN + displayWhen(TEST_NAME); + + // Matching tenant + assertAddAllow(USER_DUNCAN_FILE); + + // Wrong tenant + assertAddDeny(USER_PITER_FILE); + + // No tenant + assertAddDeny(USER_DMURR_FILE); + + // THEN + displayThen(TEST_NAME); + + login(USER_ADMINISTRATOR_USERNAME); + + assertUserAfter(USER_DUNCAN_OID) + .assertName(USER_DUNCAN_NAME) + .assertFullName(USER_DUNCAN_FULL_NAME) + .assignments() + .assertOrg(ORG_ATREIDES_OID) + .assertNoRole() + .end() + .assertTenantRef(ORG_ATREIDES_OID) + .assertParentOrgRefs(ORG_ATREIDES_OID) + .assertLinks(0); + + assertGlobalStateUntouched(); + } + + /** + * MID-4882 + */ + @Test + public void test104AutzLetoModify() throws Exception { + final String TEST_NAME = "test104AutzLetoModify"; displayTestTitle(TEST_NAME); // GIVEN cleanupAutzTest(null); @@ -348,7 +355,37 @@ public void test102AutzLetoModify() throws Exception { displayThen(TEST_NAME); assertGlobalStateUntouched(); - } + } + + /** + * MID-4882 + */ + @Test + public void test109AutzLetoDelete() throws Exception { + final String TEST_NAME = "test109AutzLetoDelete"; + displayTestTitle(TEST_NAME); + // GIVEN + cleanupAutzTest(null); + + login(USER_LETO_ATREIDES_NAME); + + // WHEN + displayWhen(TEST_NAME); + + // Matching tenant + assertDeleteAllow(UserType.class, USER_DUNCAN_OID); + + // Wrong tenant + assertDeleteDeny(UserType.class, USER_PITER_OID); + + // No tenant + assertDeleteDeny(UserType.class, USER_DMURR_OID); + + // THEN + displayThen(TEST_NAME); + + assertGlobalStateUntouched(); + } /** * Edric is part of Spacing Guld. But the Guild is not tenant. diff --git a/model/model-intest/src/test/resources/security/multitenant/user-dmurr.xml b/model/model-intest/src/test/resources/security/multitenant/user-dmurr.xml new file mode 100644 index 00000000000..dfb3f0a6775 --- /dev/null +++ b/model/model-intest/src/test/resources/security/multitenant/user-dmurr.xml @@ -0,0 +1,29 @@ + + + + + dmurr + D'murr + Pilru + D'murr Pilru + + + + diff --git a/model/model-intest/src/test/resources/security/multitenant/user-duncan.xml b/model/model-intest/src/test/resources/security/multitenant/user-duncan.xml new file mode 100644 index 00000000000..f745b4a531d --- /dev/null +++ b/model/model-intest/src/test/resources/security/multitenant/user-duncan.xml @@ -0,0 +1,29 @@ + + + + + duncan + Duncan + Idaho + Duncan Idaho + + + + diff --git a/model/model-intest/src/test/resources/security/multitenant/user-piter.xml b/model/model-intest/src/test/resources/security/multitenant/user-piter.xml new file mode 100644 index 00000000000..108f088e097 --- /dev/null +++ b/model/model-intest/src/test/resources/security/multitenant/user-piter.xml @@ -0,0 +1,29 @@ + + + + + piter + Piter + De Vries + De Vries + + + + diff --git a/repo/security-enforcer-impl/src/main/java/com/evolveum/midpoint/security/enforcer/impl/SecurityEnforcerImpl.java b/repo/security-enforcer-impl/src/main/java/com/evolveum/midpoint/security/enforcer/impl/SecurityEnforcerImpl.java index 5149bb9a09a..85348119926 100644 --- a/repo/security-enforcer-impl/src/main/java/com/evolveum/midpoint/security/enforcer/impl/SecurityEnforcerImpl.java +++ b/repo/security-enforcer-impl/src/main/java/com/evolveum/midpoint/security/enforcer/impl/SecurityEnforcerImpl.java @@ -270,7 +270,10 @@ private AccessDecision isAuthorized ItemDecisionFunction itemDecisionFunction = (itemPath, removingContainer) -> decideAllowedItems(itemPath, allowedItems, phase, removingContainer); AccessDecision itemsDecision = null; if (params.hasDelta()) { - itemsDecision = determineDeltaDecision(params.getDelta(), params.getObject(), itemDecisionFunction); + // Behave as if this is execution phase for delete delta authorizations. We do not want to avoid deleting objects just because there + // are automatic/operational items that were generated by midPoint. Otherwise we won't be really able to delete any object. + ItemDecisionFunction itemDecisionFunctionDelete = (itemPath, removingContainer) -> decideAllowedItems(itemPath, allowedItems, AuthorizationPhaseType.EXECUTION, removingContainer); + itemsDecision = determineDeltaDecision(params.getDelta(), params.getObject(), itemDecisionFunction, itemDecisionFunctionDelete); } else if (params.hasObject()) { itemsDecision = determineObjectDecision(params.getObject(), itemDecisionFunction); } @@ -454,9 +457,12 @@ private AccessDecision determineContainerDecision(PrismContainerValue cval, I * The currentObject parameter is the state of the object as we have seen it (the more recent the better). * This is used to check authorization for id-only delete deltas and replace deltas for containers. */ - private AccessDecision determineDeltaDecision(ObjectDelta delta, PrismObject currentObject, ItemDecisionFunction itemDecisionFunction) { + private AccessDecision determineDeltaDecision(ObjectDelta delta, PrismObject currentObject, + ItemDecisionFunction itemDecisionFunction, ItemDecisionFunction itemDecisionFunctionDelete) { if (delta.isAdd()) { return determineObjectDecision(delta.getObjectToAdd(), itemDecisionFunction); + } else if (delta.isDelete()) { + return determineObjectDecision(currentObject, itemDecisionFunctionDelete); } else { AccessDecision decision = null; for (ItemDelta itemDelta: delta.getModifications()) { @@ -1729,24 +1735,27 @@ public MidPointPrincipal createDonorPrincipal(MidPointPrincipal attorneyPrincipa public AccessDecision determineSubitemDecision( ObjectSecurityConstraints securityConstraints, ObjectDelta delta, PrismObject currentObject, String operationUrl, AuthorizationPhaseType phase, ItemPath subitemRootPath) { - return determineDeltaDecision(delta, currentObject, - (nameOnlyItemPath, removingContainer) -> { - if (removingContainer && isInList(nameOnlyItemPath, AuthorizationConstants.OPERATIONAL_ITEMS_ALLOWED_FOR_CONTAINER_DELETE)) { - return null; - } - if (AuthorizationPhaseType.EXECUTION.equals(phase) && isInList(nameOnlyItemPath, AuthorizationConstants.EXECUTION_ITEMS_ALLOWED_BY_DEFAULT)) { - return null; - } - if (subitemRootPath != null && !subitemRootPath.isSubPathOrEquivalent(nameOnlyItemPath)) { -// LOGGER.trace("subitem decision: {} <=> {} (not under root) : {}", subitemRootPath, nameOnlyItemPath, null); - return null; - } - - AuthorizationDecisionType authorizationDecisionType = securityConstraints.findItemDecision(nameOnlyItemPath, operationUrl, phase); - AccessDecision decision = AccessDecision.translate(authorizationDecisionType); -// LOGGER.trace("subitem decision: {} <=> {} : {}", subitemRootPath, nameOnlyItemPath, decision); - return decision; - }); + ItemDecisionFunction itemDecisionFunction = (nameOnlyItemPath, removingContainer) -> subitemDecide(nameOnlyItemPath, removingContainer, securityConstraints, operationUrl, phase, subitemRootPath); + ItemDecisionFunction itemDecisionFunctionDelete = (nameOnlyItemPath, removingContainer) -> subitemDecide(nameOnlyItemPath, removingContainer, securityConstraints, operationUrl, AuthorizationPhaseType.EXECUTION, subitemRootPath); + return determineDeltaDecision(delta, currentObject, itemDecisionFunction, itemDecisionFunctionDelete); + } + + private AccessDecision subitemDecide(ItemPath nameOnlyItemPath, boolean removingContainer, ObjectSecurityConstraints securityConstraints, String operationUrl, AuthorizationPhaseType phase, ItemPath subitemRootPath) { + if (removingContainer && isInList(nameOnlyItemPath, AuthorizationConstants.OPERATIONAL_ITEMS_ALLOWED_FOR_CONTAINER_DELETE)) { + return null; + } + if (AuthorizationPhaseType.EXECUTION.equals(phase) && isInList(nameOnlyItemPath, AuthorizationConstants.EXECUTION_ITEMS_ALLOWED_BY_DEFAULT)) { + return null; + } + if (subitemRootPath != null && !subitemRootPath.isSubPathOrEquivalent(nameOnlyItemPath)) { +// LOGGER.trace("subitem decision: {} <=> {} (not under root) : {}", subitemRootPath, nameOnlyItemPath, null); + return null; + } + + AuthorizationDecisionType authorizationDecisionType = securityConstraints.findItemDecision(nameOnlyItemPath, operationUrl, phase); + AccessDecision decision = AccessDecision.translate(authorizationDecisionType); +// LOGGER.trace("subitem decision: {} <=> {} : {}", subitemRootPath, nameOnlyItemPath, decision); + return decision; } @Override