Merge pull request #3423 from magda-io/issue/3421

#3421, #3426 & #3424 and more...
magda-io · Nov 9, 2022 · 588ba63 · 588ba63
2 parents 6d691fa + 27a9db1
commit 588ba63
Show file tree

Hide file tree

Showing 15 changed files with 319 additions and 87 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -7,7 +7,13 @@
 - Upgraded indexer weekly reindexer trigger container to node14
 - Distribution page UI minor improvements: display file size (when available) & adjust margins between information blocks
 - Improve the error message for `false` unconditional decision.
-- #3420 add merge option support to PUT record API
+- #3420 add `merge` option support to PUT record API
+- #3421 Improve dataset updating UI logic using `merge` option of the PUT record API
+- Avoid sending unnecessary `aspects` field as the part of the context data created by RegistryExternalInterface
+- #3426 People with orgUnit constraint permission should only be able to perform read operation on datasets not assigned to an orgUnit
+- #3424 Set the default order by field for APIs
+- #3427 Dataset Editor: offer more details via error message box when encounter insufficient permission error
+- #3425 offer a button on dataset page to take the user back to data management tab
 
 ## v2.1.1
 

diff --git a/magda-authorization-api/src/apiRouters/createResourceApiRouter.ts b/magda-authorization-api/src/apiRouters/createResourceApiRouter.ts
@@ -74,7 +74,10 @@ export default function createResourceApiRouter(options: ApiRouterOptions) {
                             : (req?.query?.offset as string),
                         limit: returnCount
                             ? undefined
-                            : (req?.query?.limit as string)
+                            : (req?.query?.limit as string),
+                        orderBy: returnCount
+                            ? undefined
+                            : sqls`resources.uri ASC`
                     }
                 );
                 if (returnCount) {
@@ -434,7 +437,10 @@ export default function createResourceApiRouter(options: ApiRouterOptions) {
                             : (req?.query?.offset as string),
                         limit: returnCount
                             ? undefined
-                            : (req?.query?.limit as string)
+                            : (req?.query?.limit as string),
+                        orderBy: returnCount
+                            ? undefined
+                            : sqls`operations.uri ASC`
                     }
                 );
                 if (returnCount) {

diff --git a/magda-authorization-api/src/apiRouters/createRoleApiRouter.ts b/magda-authorization-api/src/apiRouters/createRoleApiRouter.ts
@@ -270,7 +270,10 @@ export default function createRoleApiRouter(options: ApiRouterOptions) {
                             : (req?.query?.offset as string),
                         limit: returnCount
                             ? undefined
-                            : (req?.query?.limit as string)
+                            : (req?.query?.limit as string),
+                        orderBy: returnCount
+                            ? undefined
+                            : sqls`permissions.create_time DESC`
                     }
                 );
                 if (returnCount) {
@@ -851,7 +854,10 @@ export default function createRoleApiRouter(options: ApiRouterOptions) {
                             : (req?.query?.offset as string),
                         limit: returnCount
                             ? undefined
-                            : (req?.query?.limit as string)
+                            : (req?.query?.limit as string),
+                        orderBy: returnCount
+                            ? undefined
+                            : sqls`roles.name, roles.create_time ASC`
                     }
                 );
                 if (returnCount) {

diff --git a/magda-opa/policies/common/verifyRecordPermission.rego b/magda-opa/policies/common/verifyRecordPermission.rego
@@ -28,14 +28,18 @@ verifyRecordPermission(inputOperationUri, inputObjectRefName) {
     input.user.managingOrgUnitIds[_] = input.object[inputObjectRefName]["access-control"].orgUnitId
 }
 
-# or when a user has org unit ownership constraint permission, he also can access all records with NO org unit assigned
+# or when a user has org unit ownership constraint permission, he also can access (read permission only) all records with NO org unit assigned
 verifyRecordPermission(inputOperationUri, inputObjectRefName) {
+    [resourceType, operationType, resourceUriPrefix] := breakdownOperationUri(inputOperationUri)
+    operationType == "read"
     hasOrgUnitConstaintPermission(inputOperationUri)
     # unfortunately, we can't use isEmpty to handle undefined value 
     not input.object[inputObjectRefName]["access-control"].orgUnitId
 }
 
 verifyRecordPermission(inputOperationUri, inputObjectRefName) {
+    [resourceType, operationType, resourceUriPrefix] := breakdownOperationUri(inputOperationUri)
+    operationType == "read"
     hasOrgUnitConstaintPermission(inputOperationUri)
     isEmpty(input.object[inputObjectRefName]["access-control"].orgUnitId)
 }

diff --git a/magda-opa/policies/common/verifyRecordWithPublishingStatusPermission.rego b/magda-opa/policies/common/verifyRecordWithPublishingStatusPermission.rego
@@ -82,10 +82,12 @@ verifyRecordWithPublishingStatusPermission(inputOperationUri, inputObjectRefName
 }
 
 # if find a permission with org unit ownership constraint, plus dataset have NOT been assigned org unit
-# e.g. anonymous users can access all datasets that not belongs to an org unit
+# e.g. anonymous users can access (`read` permission only) all datasets that not belongs to an org unit
 verifyRecordWithPublishingStatusPermission(inputOperationUri, inputObjectRefName) {
 	[resourceType, operationType, resourceUriPrefix] := breakdownOperationUri(inputOperationUri)
 
+	# we only want to allow this special permission grant for "read" operation (issue: #3426)
+	operationType == "read"
 	resourceType == "*"
 
 	input.user.permissions[i].userOwnershipConstraint = false
@@ -107,6 +109,8 @@ verifyRecordWithPublishingStatusPermission(inputOperationUri, inputObjectRefName
 verifyRecordWithPublishingStatusPermission(inputOperationUri, inputObjectRefName) {
 	[resourceType, operationType, resourceUriPrefix] := breakdownOperationUri(inputOperationUri)
 
+    # we only want to allow this special permission grant for "read" operation (issue: #3426)
+	operationType == "read"
 	resourceType == "*"
 
 	input.user.permissions[i].userOwnershipConstraint = false

diff --git a/magda-opa/policies/object/dataset/allow_test.rego b/magda-opa/policies/object/dataset/allow_test.rego
@@ -751,6 +751,47 @@ test_allow_empty_string_orgUnitId {
     }
 }
 
+test_not_allow_empty_string_orgUnitId_non_read_operation {
+    not allow with input as {
+        "operationUri": "object/dataset/published/update",
+        "object": {
+            "dataset": {
+                "access-control": {
+                    "ownerId": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxx",
+                    "orgUnitId": ""
+                },
+                "publishing": {
+                    "state": "published"
+                }
+            }
+        },
+        "user": {
+            "displayName": "Jacky Jiang",
+            "email": "[email protected]",
+            "id": "80a9dce4-91af-44e2-a2f4-9ddccb3f4c5e",
+            "managingOrgUnitIds": ["90a9dce4-91af-44e2-a2f4-9ddccb3f4c5f"],
+            "permissions": [
+                {
+                    "id": "e5ce2fc4-9f38-4f52-8190-b770ed2074e6",
+                    "name": "View Published Dataset",
+                    "operations": [
+                        {
+                            "id": "f1e2af3e-5d98-4081-a4ff-7016f43002fa",
+                            "name": "Read Publish Dataset",
+                            "uri": "object/dataset/published/read"
+                        }
+                    ],
+                    "orgUnitOwnershipConstraint": true,
+                    "preAuthorisedConstraint": false,
+                    "resourceId": "ef3b767f-d06b-46f4-9302-031ae5004275",
+                    "resourceUri": "object/dataset/published",
+                    "userOwnershipConstraint": false
+                }
+            ]
+        }
+    }
+}
+
 test_allow_undefined_orgUnitId {
     allow with input as {
         "operationUri": "object/dataset/published/read",
@@ -791,6 +832,46 @@ test_allow_undefined_orgUnitId {
     }
 }
 
+test_not_allow_undefined_orgUnitId_non_read {
+    not allow with input as {
+        "operationUri": "object/dataset/published/update",
+        "object": {
+            "dataset": {
+                "access-control": {
+                    "ownerId": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxx"
+                },
+                "publishing": {
+                    "state": "published"
+                }
+            }
+        },
+        "user": {
+            "displayName": "Jacky Jiang",
+            "email": "[email protected]",
+            "id": "80a9dce4-91af-44e2-a2f4-9ddccb3f4c5e",
+            "managingOrgUnitIds": ["90a9dce4-91af-44e2-a2f4-9ddccb3f4c5f"],
+            "permissions": [
+                {
+                    "id": "e5ce2fc4-9f38-4f52-8190-b770ed2074e6",
+                    "name": "View Published Dataset",
+                    "operations": [
+                        {
+                            "id": "f1e2af3e-5d98-4081-a4ff-7016f43002fa",
+                            "name": "Read Publish Dataset",
+                            "uri": "object/dataset/published/read"
+                        }
+                    ],
+                    "orgUnitOwnershipConstraint": true,
+                    "preAuthorisedConstraint": false,
+                    "resourceId": "ef3b767f-d06b-46f4-9302-031ae5004275",
+                    "resourceUri": "object/dataset/published",
+                    "userOwnershipConstraint": false
+                }
+            ]
+        }
+    }
+}
+
 test_allow_empty_string_orgUnitId_any_publishing_status {
     allow with input as {
         "operationUri": "object/dataset/*/read",
@@ -832,6 +913,47 @@ test_allow_empty_string_orgUnitId_any_publishing_status {
     }
 }
 
+test_not_allow_empty_string_orgUnitId_any_publishing_status_non_read {
+    not allow with input as {
+        "operationUri": "object/dataset/*/update",
+        "object": {
+            "dataset": {
+                "access-control": {
+                    "ownerId": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxx",
+                    "orgUnitId": ""
+                },
+                "publishing": {
+                    "state": "published"
+                }
+            }
+        },
+        "user": {
+            "displayName": "Jacky Jiang",
+            "email": "[email protected]",
+            "id": "80a9dce4-91af-44e2-a2f4-9ddccb3f4c5e",
+            "managingOrgUnitIds": ["90a9dce4-91af-44e2-a2f4-9ddccb3f4c5f"],
+            "permissions": [
+                {
+                    "id": "e5ce2fc4-9f38-4f52-8190-b770ed2074e6",
+                    "name": "View Published Dataset",
+                    "operations": [
+                        {
+                            "id": "f1e2af3e-5d98-4081-a4ff-7016f43002fa",
+                            "name": "Read Publish Dataset",
+                            "uri": "object/dataset/published/read"
+                        }
+                    ],
+                    "orgUnitOwnershipConstraint": true,
+                    "preAuthorisedConstraint": false,
+                    "resourceId": "ef3b767f-d06b-46f4-9302-031ae5004275",
+                    "resourceUri": "object/dataset/published",
+                    "userOwnershipConstraint": false
+                }
+            ]
+        }
+    }
+}
+
 test_allow_undefined_orgUnitId_any_publishing_status {
     allow with input as {
         "operationUri": "object/dataset/*/read",
@@ -870,4 +992,44 @@ test_allow_undefined_orgUnitId_any_publishing_status {
             ]
         }
     }
+}
+
+test_not_allow_undefined_orgUnitId_any_publishing_status_non_read {
+    not allow with input as {
+        "operationUri": "object/dataset/*/delete",
+        "object": {
+            "dataset": {
+                "access-control": {
+                    "ownerId": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxx"
+                },
+                "publishing": {
+                    "state": "published"
+                }
+            }
+        },
+        "user": {
+            "displayName": "Jacky Jiang",
+            "email": "[email protected]",
+            "id": "80a9dce4-91af-44e2-a2f4-9ddccb3f4c5e",
+            "managingOrgUnitIds": ["90a9dce4-91af-44e2-a2f4-9ddccb3f4c5f"],
+            "permissions": [
+                {
+                    "id": "e5ce2fc4-9f38-4f52-8190-b770ed2074e6",
+                    "name": "View Published Dataset",
+                    "operations": [
+                        {
+                            "id": "f1e2af3e-5d98-4081-a4ff-7016f43002fa",
+                            "name": "Read Publish Dataset",
+                            "uri": "object/dataset/published/read"
+                        }
+                    ],
+                    "orgUnitOwnershipConstraint": true,
+                    "preAuthorisedConstraint": false,
+                    "resourceId": "ef3b767f-d06b-46f4-9302-031ae5004275",
+                    "resourceUri": "object/dataset/published",
+                    "userOwnershipConstraint": false
+                }
+            ]
+        }
+    }
 }
diff --git a/magda-opa/policies/storage/bucket/allow.rego b/magda-opa/policies/storage/bucket/allow.rego
@@ -3,6 +3,7 @@ package storage.bucket
 import data.common.hasNoConstraintPermission
 import data.common.hasOrgUnitConstaintPermission
 import data.common.hasOwnerConstraintPermission
+import data.common.breakdownOperationUri
 import data.common.isEmpty
 
 default allow = false
@@ -29,16 +30,18 @@ allow {
 	input.user.managingOrgUnitIds[_] = input.storage.bucket.orgUnitId
 }
 
+# or when a user has org unit ownership constraint permission, he also can access (read only) all buckets with NO org unit assigned
 allow {
+	[resourceType, operationType, resourceUriPrefix] := breakdownOperationUri(input.operationUri)
+    operationType == "read"
 	hasOrgUnitConstaintPermission(input.operationUri)
-
-	# or when a user has org unit ownership constraint permission, he also can access all buckets with NO org unit assigned
 	not input.storage.bucket.orgUnitId
 }
 
+# or when a user has org unit ownership constraint permission, he also can access (read only) all buckets with NO org unit assigned
 allow {
+	[resourceType, operationType, resourceUriPrefix] := breakdownOperationUri(input.operationUri)
+    operationType == "read"
 	hasOrgUnitConstaintPermission(input.operationUri)
-
-	# or when a user has org unit ownership constraint permission, he also can access all buckets with NO org unit assigned
 	isEmpty(input.storage.bucket.orgUnitId)
 }
diff --git a/magda-opa/policies/storage/object/allow.rego b/magda-opa/policies/storage/object/allow.rego
@@ -28,15 +28,19 @@ allow {
     input.user.managingOrgUnitIds[_] = input.storage.object.orgUnitId
 }
 
+# or when a user has org unit ownership constraint permission, he also can access (read only) all objects with NO org unit assigned
 allow {
+    [resourceType, operationType, resourceUriPrefix] := breakdownOperationUri(input.operationUri)
+    operationType == "read"
     hasOrgUnitConstaintPermission(input.operationUri)
-    # or when a user has org unit ownership constraint permission, he also can access all objects with NO org unit assigned
     not input.storage.object.orgUnitId
 }
 
+# or when a user has org unit ownership constraint permission, he also can access (read only) all objects with NO org unit assigned
 allow {
+    [resourceType, operationType, resourceUriPrefix] := breakdownOperationUri(input.operationUri)
+    operationType == "read"
     hasOrgUnitConstaintPermission(input.operationUri)
-    # or when a user has org unit ownership constraint permission, he also can access all objects with NO org unit assigned
     isEmpty(input.storage.object.orgUnitId)
 }
 

diff --git a/...-scala-common/src/main/scala/au/csiro/data61/magda/client/RegistryExternalInterface.scala b/...-scala-common/src/main/scala/au/csiro/data61/magda/client/RegistryExternalInterface.scala
@@ -203,7 +203,7 @@ class RegistryExternalInterface(
                 case JsObject(v) => v.toSeq
                 case _           => Seq()
               }
-              JsObject((rawJsData.fields - "aspect") ++ aspects)
+              JsObject((rawJsData.fields - "aspects") ++ aspects)
             }
           case _ =>
             Unmarshal(response.entity).to[String].flatMap(onError(response))

diff --git a/magda-web-client/src/Components/Common/ErrorMessageBox.scss b/magda-web-client/src/Components/Common/ErrorMessageBox.scss
@@ -3,4 +3,10 @@
         margin-top: 10px;
         margin-bottom: 10px;
     }
+    .tooltip-container {
+        .tooltip {
+            padding-left: 0px !important;
+            left: 0px;
+        }
+    }
 }