gh-275 Control access into non-secured domains contained inside other non-secured domains

mdm-core/grails-app/controllers/uk/ac/ox/softeng/maurodatamapper/core/facet/AnnotationController.groovy

@@ -59,7 +59,7 @@ class AnnotationController extends FacetController<Annotation> {
     protected Annotation createResource() {
         Annotation resource = super.createResource() as Annotation
         if (params.annotationId) {
-            annotationService.get(params.annotationId)?.addToChildAnnotations(resource)
+            annotationService.findByMultiFacetAwareItemIdAndId(params.multiFacetAwareItemId, params.annotationId)?.addToChildAnnotations(resource)
         }
         if (!resource.label && resource.parentAnnotation) {
             resource.label = "${resource.parentAnnotation.label} [${resource.parentAnnotation.childAnnotations.size()}]"

mdm-core/grails-app/controllers/uk/ac/ox/softeng/maurodatamapper/core/facet/AnnotationInterceptor.groovy

@@ -17,11 +17,14 @@
  */
 package uk.ac.ox.softeng.maurodatamapper.core.facet
 
+import uk.ac.ox.softeng.maurodatamapper.api.exception.ApiBadRequestException
 import uk.ac.ox.softeng.maurodatamapper.core.interceptor.FacetInterceptor
 import uk.ac.ox.softeng.maurodatamapper.util.Utils
 
 class AnnotationInterceptor extends FacetInterceptor {
 
+    AnnotationService annotationService
+
     @Override
     Class getFacetClass() {
         Annotation
@@ -32,6 +35,14 @@ class AnnotationInterceptor extends FacetInterceptor {
         Utils.toUuid(params, 'annotationId')
     }
 
+    @Override
+    void checkParentId() throws ApiBadRequestException {
+        if (params.annotationId && !annotationService.existsByMultiFacetAwareItemIdAndId(params.multiFacetAwareItemId, params.annotationId)) {
+            throw new ApiBadRequestException('AI01', 'Provided annotationId is not inside provided multiFacetAwareItemId')
+        }
+    }
+
+
     boolean before() {
         facetResourceChecks()
         checkActionAllowedOnFacet()

mdm-core/grails-app/controllers/uk/ac/ox/softeng/maurodatamapper/core/facet/rule/RuleRepresentationController.groovy

@@ -46,9 +46,9 @@ class RuleRepresentationController extends EditLoggingController<RuleRepresentat
         //Create the RuleRepresentation
         RuleRepresentation resource = super.createResource() as RuleRepresentation
         //Create an association between the Rule and RuleRepresentation
-        ruleService.get(params.ruleId)?.addToRuleRepresentations(resource)
+        ruleService.findByMultiFacetAwareItemIdAndId(params.multiFacetAwareItemId, params.ruleId)?.addToRuleRepresentations(resource)
         resource
-    }    
+    }
 
     @Override
     protected RuleRepresentation saveResource(RuleRepresentation resource) {
@@ -57,7 +57,7 @@ class RuleRepresentationController extends EditLoggingController<RuleRepresentat
         ruleService.
             addCreatedEditToMultiFacetAwareItemOfRule(currentUser, resource, params.multiFacetAwareItemDomainType, params.multiFacetAwareItemId)
         resource
-    }     
+    }
 
     @Override
     protected RuleRepresentation updateResource(RuleRepresentation resource) {
@@ -67,12 +67,12 @@ class RuleRepresentationController extends EditLoggingController<RuleRepresentat
         ruleService.
             addUpdatedEditToMultiFacetAwareItemOfRule(currentUser, resource, params.multiFacetAwareItemDomainType, params.multiFacetAwareItemId,
                                                       dirtyPropertyNames)
-    }        
+    }
 
     @Override
     void deleteResource(RuleRepresentation resource) {
         serviceDeleteResource(resource)
-    } 
+    }
 
     @Override
     void serviceDeleteResource(RuleRepresentation resource) {
@@ -85,6 +85,6 @@ class RuleRepresentationController extends EditLoggingController<RuleRepresentat
         ruleService.get(params.ruleId)?.removeFromRuleRepresentations(resource)
 
         //Delete the RuleRepresentation
-        ruleRepresentationService.delete(resource, true)           
+        ruleRepresentationService.delete(resource, true)
     }
 }

mdm-core/grails-app/controllers/uk/ac/ox/softeng/maurodatamapper/core/facet/rule/RuleRepresentationInterceptor.groovy

@@ -17,11 +17,15 @@
  */
 package uk.ac.ox.softeng.maurodatamapper.core.facet.rule
 
+import uk.ac.ox.softeng.maurodatamapper.api.exception.ApiBadRequestException
+import uk.ac.ox.softeng.maurodatamapper.core.facet.RuleService
 import uk.ac.ox.softeng.maurodatamapper.core.interceptor.FacetInterceptor
 import uk.ac.ox.softeng.maurodatamapper.util.Utils
 
 class RuleRepresentationInterceptor extends FacetInterceptor {
 
+    RuleService ruleService
+
     @Override
     Class getFacetClass() {
         RuleRepresentation
@@ -36,4 +40,11 @@ class RuleRepresentationInterceptor extends FacetInterceptor {
     void checkAdditionalIds() {
         Utils.toUuid(params, 'ruleId')
     }
+
+    @Override
+    void checkParentId() throws ApiBadRequestException {
+        if (!ruleService.existsByMultiFacetAwareItemIdAndId(params.multiFacetAwareItemId, params.ruleId)) {
+            throw new ApiBadRequestException('AI01', 'Provided ruleId is not inside provided multiFacetAwareItemId')
+        }
+    }
 }

mdm-core/grails-app/domain/uk/ac/ox/softeng/maurodatamapper/core/facet/Annotation.groovy

@@ -158,7 +158,7 @@ class Annotation implements MultiFacetItemAware, InformationAware, Diffable<Anno
         new DetachedCriteria<Annotation>(Annotation).inList('multiFacetAwareItemId', multiFacetAwareItemIds)
     }
 
-    static DetachedCriteria<Annotation> byyMultiFacetAwareItemIdAndId(Serializable multiFacetAwareItemId, Serializable resourceId) {
+    static DetachedCriteria<Annotation> byMultiFacetAwareItemIdAndId(Serializable multiFacetAwareItemId, Serializable resourceId) {
         byMultiFacetAwareItemId(multiFacetAwareItemId).idEq(Utils.toUuid(resourceId))
     }
 

mdm-core/grails-app/services/uk/ac/ox/softeng/maurodatamapper/core/facet/AnnotationService.groovy

@@ -78,9 +78,13 @@ class AnnotationService implements MultiFacetItemAwareService<Annotation> {
         annotation
     }
 
+    boolean existsByMultiFacetAwareItemIdAndId(UUID multiFacetAwareItemId, Serializable id) {
+        Annotation.byMultiFacetAwareItemIdAndId(multiFacetAwareItemId, id).count() == 1
+    }
+
     @Override
     Annotation findByMultiFacetAwareItemIdAndId(UUID multiFacetAwareItemId, Serializable id) {
-        Annotation.byyMultiFacetAwareItemIdAndId(multiFacetAwareItemId, id).get()
+        Annotation.byMultiFacetAwareItemIdAndId(multiFacetAwareItemId, id).get()
     }
 
     @Override

mdm-core/grails-app/services/uk/ac/ox/softeng/maurodatamapper/core/facet/RuleService.groovy

@@ -108,6 +108,11 @@ class RuleService implements MultiFacetItemAwareService<Rule> {
         true
     }
 
+
+    boolean existsByMultiFacetAwareItemIdAndId(UUID multiFacetAwareItemId, Serializable id) {
+        Rule.byMultiFacetAwareItemIdAndId(multiFacetAwareItemId, id).count() == 1
+    }
+
     @Override
     Rule findByMultiFacetAwareItemIdAndId(UUID multiFacetAwareItemId, Serializable id) {
         Rule.byMultiFacetAwareItemIdAndId(multiFacetAwareItemId, id).get()

mdm-core/src/main/groovy/uk/ac/ox/softeng/maurodatamapper/core/interceptor/FacetInterceptor.groovy

@@ -58,12 +58,17 @@ abstract class FacetInterceptor implements MdmInterceptor {
         // No-op
     }
 
+    void checkParentId() throws ApiBadRequestException {
+        // no op
+    }
+
     void facetResourceChecks() {
         Utils.toUuid(params, 'id')
-        params.multiFacetAwareItemDomainType = params.multiFacetAwareItemDomainType?: params.catalogueItemDomainType ?: params.containerDomainType
+        params.multiFacetAwareItemDomainType = params.multiFacetAwareItemDomainType ?: params.catalogueItemDomainType ?: params.containerDomainType
         params.multiFacetAwareItemId = params.multiFacetAwareItemId ?: params.catalogueItemId ?: params.containerId
         checkAdditionalIds()
         mapDomainTypeToClass(getOwningType(), true)
+        checkParentId()
     }
 
     boolean checkActionAllowedOnFacet() {

mdm-core/src/main/groovy/uk/ac/ox/softeng/maurodatamapper/core/interceptor/ModelItemInterceptor.groovy

@@ -0,0 +1,80 @@
+/*
+ * Copyright 2020-2022 University of Oxford and Health and Social Care Information Centre, also known as NHS Digital
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package uk.ac.ox.softeng.maurodatamapper.core.interceptor
+
+import uk.ac.ox.softeng.maurodatamapper.api.exception.ApiBadRequestException
+import uk.ac.ox.softeng.maurodatamapper.core.traits.controller.MdmInterceptor
+import uk.ac.ox.softeng.maurodatamapper.util.Utils
+
+/**
+ * @since 02/03/2022
+ */
+abstract class ModelItemInterceptor implements MdmInterceptor {
+
+    abstract Class getModelItemClass()
+
+    abstract Class getModelClass()
+
+    abstract String getModelIdParameterField()
+
+    abstract String getOtherModelIdParameterField()
+
+    void checkIds() {
+        Utils.toUuid(params, getModelIdParameterField())
+        Utils.toUuid(params, getOtherModelIdParameterField())
+        Utils.toUuid(params, 'id')
+    }
+
+    void checkModelId() throws ApiBadRequestException {
+        if (!params[modelIdParameterField]) throw new ApiBadRequestException('MII01', 'No Model Id provided against secured resource')
+    }
+
+    /**
+     * Check that when an is MI accessed through another MI the parent MI is contained inside the Model
+     * Should throw ApiBadRequestException
+     */
+    void checkParentModelItemId() throws ApiBadRequestException {
+    }
+
+    void performChecks() {
+        checkIds()
+        checkModelId()
+        checkParentModelItemId()
+    }
+
+    boolean checkStandardActions() {
+        checkActionAuthorisationOnUnsecuredResource(getModelItemClass(), params.id, getModelClass(), params[modelIdParameterField])
+    }
+
+    boolean canReadModel() {
+        currentUserSecurityPolicyManager.userCanReadSecuredResourceId(getModelClass(), params[modelIdParameterField]) ?:
+        notFound(getModelClass(), params[modelIdParameterField].toString()
+        )
+    }
+
+    boolean canEditModelAndReadOtherModel() {
+        boolean canRead = currentUserSecurityPolicyManager.userCanReadSecuredResourceId(getModelClass(), params[modelIdParameterField])
+        if (!currentUserSecurityPolicyManager.userCanEditSecuredResourceId(getModelClass(), params[modelIdParameterField])) {
+            return forbiddenOrNotFound(canRead, getModelClass(), params[modelIdParameterField])
+        }
+        if (!currentUserSecurityPolicyManager.userCanReadSecuredResourceId(getModelClass(), params[otherModelIdParameterField])) {
+            return notFound(getModelClass(), params[otherModelIdParameterField])
+        }
+        true
+    }
+}

mdm-core/src/test/groovy/uk/ac/ox/softeng/maurodatamapper/core/facet/NestedAnnotationControllerSpec.groovy

@@ -90,7 +90,7 @@ class NestedAnnotationControllerSpec extends ResourceControllerSpec<Annotation>
             {String domain, UUID bid -> basicModel.id == bid ? basicModel : null}
             findByMultiFacetAwareItemIdAndId(_, _) >> {UUID iid, Serializable mid ->
                 if (iid != basicModel.id) return null
-                mid == domain.id ? domain : null
+                mid == domain.id ? domain : Annotation.get(mid)
             }
             findAllWhereRootAnnotationOfMultiFacetAwareItemId(_, _) >> {
                 Annotation.whereRootAnnotationOfMultiFacetAwareItemId(basicModel.id).list()

mdm-plugin-dataflow/grails-app/controllers/uk/ac/ox/softeng/maurodatamapper/dataflow/component/DataClassComponentController.groovy

@@ -95,7 +95,7 @@ class DataClassComponentController extends EditLoggingController<DataClassCompon
     @Override
     protected DataClassComponent createResource() {
         DataClassComponent resource = super.createResource() as DataClassComponent
-        resource.dataFlow = dataFlowService.get(params.dataFlowId)
+        resource.dataFlow = dataFlowService.findByTargetDataModelIdAndId(params.dataModelId, params.dataFlowId)
         if (resource.targetDataClasses && resource.sourceDataClasses) {
             dataClassService.addDataClassesAreFromDataClasses(resource.targetDataClasses, resource.sourceDataClasses, currentUser)
         }

mdm-plugin-dataflow/grails-app/controllers/uk/ac/ox/softeng/maurodatamapper/dataflow/component/DataClassComponentInterceptor.groovy

@@ -17,13 +17,16 @@
  */
 package uk.ac.ox.softeng.maurodatamapper.dataflow.component
 
-
+import uk.ac.ox.softeng.maurodatamapper.api.exception.ApiBadRequestException
+import uk.ac.ox.softeng.maurodatamapper.dataflow.DataFlowService
 import uk.ac.ox.softeng.maurodatamapper.datamodel.DataModel
 import uk.ac.ox.softeng.maurodatamapper.datamodel.traits.controller.DataModelSecuredInterceptor
 import uk.ac.ox.softeng.maurodatamapper.util.Utils
 
 class DataClassComponentInterceptor extends DataModelSecuredInterceptor {
 
+    DataFlowService dataFlowService
+
     @Override
     Class getModelItemClass() {
         DataClassComponent
@@ -37,6 +40,13 @@ class DataClassComponentInterceptor extends DataModelSecuredInterceptor {
         Utils.toUuid(params, 'dataClassId')
     }
 
+    @Override
+    void checkParentModelItemId() throws ApiBadRequestException {
+        if (!dataFlowService.existsByTargetDataModelIdAndId(params.dataModelId, params.dataFlowId)) {
+            throw new ApiBadRequestException('DEI01', 'Provided dataFlowId is not inside provided dataModelId')
+        }
+    }
+
     boolean before() {
         performChecks()
 

mdm-plugin-dataflow/grails-app/controllers/uk/ac/ox/softeng/maurodatamapper/dataflow/component/DataElementComponentController.groovy

@@ -108,7 +108,7 @@ class DataElementComponentController extends EditLoggingController<DataElementCo
     @Override
     protected DataElementComponent createResource() {
         DataElementComponent resource = super.createResource() as DataElementComponent
-        resource.dataClassComponent = dataClassComponentService.get(params.dataClassComponentId)
+        resource.dataClassComponent = dataClassComponentService.findByDataFlowIdAndId(params.dataFlowId, params.dataClassComponentId)
 
         if (!resource.dataClassComponent) {
             resource.dataClassComponent = dataClassComponentService.findOrCreateDataClassComponentForDataElementComponent(resource, currentUser)

mdm-plugin-dataflow/grails-app/controllers/uk/ac/ox/softeng/maurodatamapper/dataflow/component/DataElementComponentInterceptor.groovy

@@ -17,12 +17,17 @@
  */
 package uk.ac.ox.softeng.maurodatamapper.dataflow.component
 
+import uk.ac.ox.softeng.maurodatamapper.api.exception.ApiBadRequestException
+import uk.ac.ox.softeng.maurodatamapper.dataflow.DataFlowService
 import uk.ac.ox.softeng.maurodatamapper.datamodel.DataModel
 import uk.ac.ox.softeng.maurodatamapper.datamodel.traits.controller.DataModelSecuredInterceptor
 import uk.ac.ox.softeng.maurodatamapper.util.Utils
 
 class DataElementComponentInterceptor extends DataModelSecuredInterceptor {
 
+    DataFlowService dataFlowService
+    DataClassComponentService dataClassComponentService
+
     @Override
     Class getModelItemClass() {
         DataElementComponent
@@ -38,6 +43,16 @@ class DataElementComponentInterceptor extends DataModelSecuredInterceptor {
         Utils.toUuid(params, 'dataClassId')
     }
 
+    @Override
+    void checkParentModelItemId() throws ApiBadRequestException {
+        if (!dataFlowService.existsByTargetDataModelIdAndId(params.dataModelId, params.dataFlowId)) {
+            throw new ApiBadRequestException('DEI01', 'Provided dataFlowId is not inside provided dataModelId')
+        }
+        if (!dataClassComponentService.existsByDataFlowIdAndId(params.dataFlowId, params.dataClassComponentId)) {
+            throw new ApiBadRequestException('DEI01', 'Provided dataClassComponentId is not inside provided dataFlowId')
+        }
+    }
+
     boolean before() {
         performChecks()
 

mdm-plugin-dataflow/grails-app/services/uk/ac/ox/softeng/maurodatamapper/dataflow/DataFlowService.groovy

@@ -121,6 +121,10 @@ class DataFlowService extends ModelItemService<DataFlow> {
         DataFlow.byTargetDataModelIdAndId(dataModelId, Utils.toUuid(id)).get()
     }
 
+    boolean existsByTargetDataModelIdAndId(UUID dataModelId, Serializable id) {
+        DataFlow.byTargetDataModelIdAndId(dataModelId, Utils.toUuid(id)).count() == 1
+    }
+
     List<DataFlow> findAllReadableByUser(UserSecurityPolicyManager userSecurityPolicyManager, Map pagination = [:]) {
         if (!userSecurityPolicyManager.listReadableSecuredResourceIds(DataModel)) return []
         DataFlow.byDataModelIdInList(

mdm-plugin-dataflow/grails-app/services/uk/ac/ox/softeng/maurodatamapper/dataflow/component/DataClassComponentService.groovy

@@ -144,6 +144,10 @@ class DataClassComponentService extends ModelItemService<DataClassComponent> {
         DataClassComponent.byDataFlowIdAndId(dataFlowId, Utils.toUuid(id)).get()
     }
 
+    boolean existsByDataFlowIdAndId(UUID dataFlowId, Serializable id) {
+        DataClassComponent.byDataFlowIdAndId(dataFlowId, Utils.toUuid(id)).count() == 1
+    }
+
     List<DataClassComponent> findAllByDataFlowId(UUID dataFlowId, Map pagination = [:]) {
         DataClassComponent.byDataFlowId(dataFlowId).list(pagination)
     }

mdm-plugin-dataflow/src/test/groovy/uk/ac/ox/softeng/maurodatamapper/dataflow/DataFlowInterceptorSpec.groovy

@@ -46,7 +46,7 @@ class DataFlowInterceptorSpec extends ContainedResourceInterceptorUnitSpec imple
 
     @Override
     String getExpectedExceptionCodeForNoContainingItem() {
-        'DMSI01'
+        'MII01'
     }
 
     @Override

mdm-plugin-dataflow/src/test/groovy/uk/ac/ox/softeng/maurodatamapper/dataflow/component/DataClassComponentInterceptorSpec.groovy

@@ -18,6 +18,7 @@
 package uk.ac.ox.softeng.maurodatamapper.dataflow.component
 
 import uk.ac.ox.softeng.maurodatamapper.dataflow.DataFlow
+import uk.ac.ox.softeng.maurodatamapper.dataflow.DataFlowService
 import uk.ac.ox.softeng.maurodatamapper.dataflow.component.DataClassComponent
 import uk.ac.ox.softeng.maurodatamapper.dataflow.component.DataElementComponent
 import uk.ac.ox.softeng.maurodatamapper.datamodel.DataModel
@@ -33,6 +34,9 @@ class DataClassComponentInterceptorSpec extends ContainedResourceInterceptorUnit
     def setup() {
         log.debug('Setting up DataClassComponentInterceptorSpec')
         mockDomains(DataModel, DataClass, DataClassComponent, DataElementComponent, DataFlow)
+        interceptor.dataFlowService = Stub(DataFlowService) {
+            existsByTargetDataModelIdAndId(_, _) >> true
+        }
     }
 
     @Override
@@ -48,7 +52,7 @@ class DataClassComponentInterceptorSpec extends ContainedResourceInterceptorUnit
 
     @Override
     String getExpectedExceptionCodeForNoContainingItem() {
-        'DMSI01'
+        'MII01'
     }
 
     @Override