From 88efae43286a19afd3b1f66aab9d1e37f3075d1b Mon Sep 17 00:00:00 2001 From: Radovan Semancik Date: Mon, 12 Feb 2018 14:44:24 +0100 Subject: [PATCH] Fixing authorizations of empty objects (MID-4369) --- .../intest/security/AbstractSecurityTest.java | 23 +++++++++++++++++++ .../intest/security/TestSecurityAdvanced.java | 22 ++++++++++++++++-- .../src/test/resources/logback-test.xml | 8 +++---- .../enforcer/impl/SecurityEnforcerImpl.java | 12 +++++++++- 4 files changed, 58 insertions(+), 7 deletions(-) diff --git a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/AbstractSecurityTest.java b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/AbstractSecurityTest.java index 26c66347879..26906054269 100644 --- a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/AbstractSecurityTest.java +++ b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/AbstractSecurityTest.java @@ -20,6 +20,7 @@ import static org.testng.AssertJUnit.assertNotNull; import static org.testng.AssertJUnit.assertNull; import static org.testng.AssertJUnit.assertTrue; +import static org.testng.AssertJUnit.assertFalse; import java.io.File; import java.io.FileInputStream; @@ -76,11 +77,14 @@ import com.evolveum.midpoint.schema.util.MiscSchemaUtil; import com.evolveum.midpoint.security.api.Authorization; import com.evolveum.midpoint.security.api.MidPointPrincipal; +import com.evolveum.midpoint.security.api.OwnerResolver; +import com.evolveum.midpoint.security.enforcer.api.AuthorizationParameters; import com.evolveum.midpoint.task.api.Task; import com.evolveum.midpoint.test.util.TestUtil; import com.evolveum.midpoint.util.FailableProcessor; import com.evolveum.midpoint.util.Holder; import com.evolveum.midpoint.util.MiscUtil; +import com.evolveum.midpoint.util.QNameUtil; import com.evolveum.midpoint.util.exception.CommunicationException; import com.evolveum.midpoint.util.exception.ConfigurationException; import com.evolveum.midpoint.util.exception.ExpressionEvaluationException; @@ -96,6 +100,7 @@ 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.AuthorizationDecisionType; +import com.evolveum.midpoint.xml.ns._public.common.common_3.AuthorizationPhaseType; import com.evolveum.midpoint.xml.ns._public.common.common_3.AuthorizationType; import com.evolveum.midpoint.xml.ns._public.common.common_3.CredentialsType; import com.evolveum.midpoint.xml.ns._public.common.common_3.FocusType; @@ -1374,6 +1379,24 @@ protected void logError(String action, Class type, Str LOGGER.info(msg); } + protected void assertIsAuthorized(String operationUrl, AuthorizationPhaseType phase, AuthorizationParameters params, OwnerResolver ownerResolver) throws SchemaException, ObjectNotFoundException, ExpressionEvaluationException, CommunicationException, ConfigurationException, SecurityViolationException { + Task task = taskManager.createTaskInstance(AbstractSecurityTest.class.getName() + ".assertIsAuthorized"); + OperationResult result = task.getResult(); + boolean authorized = securityEnforcer.isAuthorized(operationUrl, phase, params, ownerResolver, task, result); + assertTrue("Expected isAuthorized for "+QNameUtil.uriToQName(operationUrl).getLocalPart()+" with "+params+", but we are not authorized", authorized); + result.computeStatus(); + TestUtil.assertSuccess(result); + } + + protected void assertIsNotAuthorized(String operationUrl, AuthorizationPhaseType phase, AuthorizationParameters params, OwnerResolver ownerResolver) throws SchemaException, ObjectNotFoundException, ExpressionEvaluationException, CommunicationException, ConfigurationException, SecurityViolationException { + Task task = taskManager.createTaskInstance(AbstractSecurityTest.class.getName() + ".assertIsAuthorized"); + OperationResult result = task.getResult(); + boolean authorized = securityEnforcer.isAuthorized(operationUrl, phase, params, ownerResolver, task, result); + assertFalse("Expected not isAuthorized for "+QNameUtil.uriToQName(operationUrl).getLocalPart()+" with "+params+", but we are authorized", authorized); + result.computeStatus(); + TestUtil.assertSuccess(result); + } + protected void asAdministrator(Attempt attempt) throws Exception { Task task = taskManager.createTaskInstance(AbstractSecurityTest.class.getName() + ".asAdministrator"); OperationResult result = task.getResult(); diff --git a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/TestSecurityAdvanced.java b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/TestSecurityAdvanced.java index 703eb64df7e..6fec6248801 100644 --- a/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/TestSecurityAdvanced.java +++ b/model/model-intest/src/test/java/com/evolveum/midpoint/model/intest/security/TestSecurityAdvanced.java @@ -15,6 +15,8 @@ */ 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.assertNull; import static org.testng.AssertJUnit.assertEquals; @@ -52,6 +54,7 @@ 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.IntegrationTestTools; import com.evolveum.midpoint.util.exception.CommunicationException; @@ -66,6 +69,7 @@ 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; @@ -2124,7 +2128,6 @@ public void test262AutzJackLimitedRoleAdministratorAndAssignApplicationRoles() t (task, result) -> deleteObject(RoleType.class, ROLE_EMPTY_OID)); // MID-4443 - display("TTT1"); assertAddAllow(ROLE_EMPTY_FILE); asAdministrator( @@ -2139,9 +2142,24 @@ public void test262AutzJackLimitedRoleAdministratorAndAssignApplicationRoles() t roleEmpty2.asObjectable().getAssignment().add(appliationRoleAssignment); // MID-4443 - display("TTT2"); assertAllow("Add empty role with application role assignment", (task, result) -> addObject(roleEmpty2)); + + // MID-4369 + // Empty role as object. Really empty: no items there at all. + PrismObject testRoleObject = prismContext.getSchemaRegistry().findObjectDefinitionByCompileTimeClass(RoleType.class).instantiate(); + // There are no items in this empty role. Therefore there is no item that is allowed. But also no item that + // is denied or not allowed. This is a corner case. But this approach is often used by GUI to determine if + // a specific class of object is allowed, e.g. if it is allowed to create (some) roles. This is used to + // determine whether to display a particular menu item. + assertTrue(testRoleObject.isEmpty()); + assertIsAuthorized(ModelAuthorizationAction.ADD.getUrl(), AuthorizationPhaseType.REQUEST, + AuthorizationParameters.Builder.buildObject(testRoleObject), null); + + testRoleObject.asObjectable().setRiskLevel("hazardous"); + assertFalse(testRoleObject.isEmpty()); + assertIsNotAuthorized(ModelAuthorizationAction.ADD.getUrl(), AuthorizationPhaseType.REQUEST, + AuthorizationParameters.Builder.buildObject(testRoleObject), null); assertGlobalStateUntouched(); } diff --git a/model/model-intest/src/test/resources/logback-test.xml b/model/model-intest/src/test/resources/logback-test.xml index d6d966ba111..2db0155f8b0 100644 --- a/model/model-intest/src/test/resources/logback-test.xml +++ b/model/model-intest/src/test/resources/logback-test.xml @@ -82,7 +82,7 @@ - + @@ -94,8 +94,8 @@ - - + + @@ -105,7 +105,7 @@ - + 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 d4d7536ceb7..b18d589fc4a 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 @@ -298,7 +298,17 @@ private AccessDecision decideAllowedItems(final ItemPath itemPath, final AutzIte } private AccessDecision determineObjectDecision(PrismContainer object, ItemDecisionFunction itemDecitionFunction) { - return determineContainerDecision(object.getValue(), itemDecitionFunction, false, "object"); + AccessDecision containerDecision = determineContainerDecision(object.getValue(), itemDecitionFunction, false, "object"); + if (containerDecision == null && object.isEmpty()) { + // There are no items in the object. Therefore there is no item that is allowed. Therefore decision is DEFAULT. + // But also there is no item that is denied or not allowed. + // This is a corner case. But this approach is often used by GUI to determine if + // a specific class of object is allowed, e.g. if it is allowed to create (some) roles. This is used to + // determine whether to display a particular menu item. + // Therefore we should allow such cases. + return AccessDecision.ALLOW; + } + return containerDecision; } private AccessDecision determineContainerDeltaDecision(ContainerDelta cdelta, PrismObject currentObject, ItemDecisionFunction itemDecitionFunction) {