Merge pull request #3334 from magda-io/issue/3330

Issue/3330 Metadata creation tool & Storage api

CHANGES.md

@@ -14,6 +14,13 @@
 - #3308 Policy enforcement on auth objects APIs
 - related #3250: Rewrite decision enforcement logic for search API
 - #3326 Build OPA docker image with builtin policies & Run OPA as a side car
+- #3330 Fine-tune Metadata creation tool workflow for the new auth model
+- #3332 Policy Enforcement on Storage API using new auth model
+- #3333 Rename access-control aspect orgUnitOwnerId field to orgUnitId
+- #3335 Reshape Storage API (in progress)
+- #3340 OPA AST parser takes too long to evaluate large response
+- Increase registry default request timeout to 60s (from 30s)
+- Bump Dataset index version to 49 & publishers index version to 7
 
 ## 1.2.1
 

deploy/helm/internal-charts/registry-api/README.md

@@ -23,13 +23,13 @@ Kubernetes: `>= 1.14.0-0`
 | defaultImage.pullPolicy | string | `"IfNotPresent"` |  |
 | defaultImage.pullSecrets | bool | `false` |  |
 | defaultImage.repository | string | `"docker.io/data61"` |  |
-| deployments.full | object | `{"idleTimeout":"60s","replicas":1,"requestTimeout":"30s"}` | deployment config for full registry instance. You can also specify different `resources` config under this key. |
+| deployments.full | object | `{"idleTimeout":"60s","replicas":1,"requestTimeout":"60s"}` | deployment config for full registry instance. You can also specify different `resources` config under this key. |
 | deployments.full.idleTimeout | string | `"60s"` | Default idle timeout for full instance. Make sure `idleTimeout` is longer than `requestTimeout` |
-| deployments.full.requestTimeout | string | `"30s"` | Default request timeout for full instance |
-| deployments.readOnly | object | `{"enable":false,"idleTimeout":"60s","replicas":1,"requestTimeout":"30s"}` | deployment config for readonly registry instances. You can also specify different `resources` config under this key. |
+| deployments.full.requestTimeout | string | `"60s"` | Default request timeout for full instance |
+| deployments.readOnly | object | `{"enable":false,"idleTimeout":"60s","replicas":1,"requestTimeout":"60s"}` | deployment config for readonly registry instances. You can also specify different `resources` config under this key. |
 | deployments.readOnly.idleTimeout | string | `"60s"` | Default idle timeout for readonly instance. Make sure `idleTimeout` is longer than `requestTimeout` |
 | deployments.readOnly.replicas | int | `1` | no. of replicates. Its value must no lower than `minReplicas` |
-| deployments.readOnly.requestTimeout | string | `"30s"` | Default request timeout for readonly instance |
+| deployments.readOnly.requestTimeout | string | `"60s"` | Default request timeout for readonly instance |
 | image.name | string | `"magda-registry-api"` |  |
 | livenessProbe | object | `{}` |  |
 | printSQlInConsole | bool | `false` | Whether print all SQL in console. For DEBUG only |

deploy/helm/internal-charts/registry-api/values.yaml

@@ -31,7 +31,7 @@ deployments:
     replicas: 1
 
     # -- Default request timeout for full instance
-    requestTimeout: 30s
+    requestTimeout: 60s
 
     # -- Default idle timeout for full instance.
     # Make sure `idleTimeout` is longer than `requestTimeout`
@@ -45,7 +45,7 @@ deployments:
     replicas: 1
 
     # -- Default request timeout for readonly instance
-    requestTimeout: 30s
+    requestTimeout: 60s
 
     # -- Default idle timeout for readonly instance.
     # Make sure `idleTimeout` is longer than `requestTimeout`

deploy/helm/internal-charts/storage-api/README.md

@@ -36,4 +36,5 @@ Kubernetes: `>= 1.14.0-0`
 | minioRegion | string | "unspecified-region" | specify bucket region |
 | resources.limits.cpu | string | `"50m"` |  |
 | resources.requests.cpu | string | `"10m"` |  |
-| resources.requests.memory | string | `"30Mi"` |  |
+| resources.requests.memory | string | `"30Mi"` |  |
+| skipAuth | bool | `false` | when set to true, API will not query policy engine for auth decision but assume it's always permitted.  It's for debugging only. |

deploy/helm/internal-charts/storage-api/templates/deployment.yaml

@@ -33,6 +33,9 @@ spec:
             "--minioPort", "9000",
 {{- if .Values.minioRegion }}
             "--minioRegion", {{ .Values.minioRegion | quote }},
+{{- end }}
+{{- if .Values.skipAuth }}
+            "--skipAuth", "true",
 {{- end }}
             "--authApiUrl", "http://authorization-api/v0",
             "--tenantId", "0",
@@ -56,6 +59,8 @@ spec:
             secretKeyRef:
               name: storage-secrets
               key: accesskey
+        - name: USER_ID
+          value: {{ .Values.global.defaultAdminUserId }}
         - name: JWT_SECRET
           valueFrom:
             secretKeyRef:

deploy/helm/internal-charts/storage-api/values.yaml

@@ -38,6 +38,10 @@ defaultBuckets: []
 # @default -- "unspecified-region"
 minioRegion: ""
 
+# -- when set to true, API will not query policy engine for auth decision but assume it's always permitted. 
+# It's for debugging only.
+skipAuth: false
+
 minio:
   host: "minio"
   port: 9000

deploy/helm/local-auth-test-deployment/policies/object/registry/record/esri_owner_groups_test.rego

@@ -292,7 +292,7 @@ test_deny_read_if_permission_is_incorrect {
             "registry": {
                 "record": {
                     "esri-access-control": {
-                        "orgUnitOwnerId": ["G1", "G2"],
+                        "orgUnitId": ["G1", "G2"],
                         "expiration": 9569380434535153100
                     }
                 }

deploy/helm/local-auth-test-deployment/policies/object/registry/record/orgunit.rego

@@ -2,5 +2,5 @@ package object.registry.record
 
 # "access-control" is the aspect id defined in the registry database.
 orgunit {
-    input.object.registry.record["access-control"].orgUnitOwnerId == input.user.managingOrgUnitIds[_]
+    input.object.registry.record["access-control"].orgUnitId == input.user.managingOrgUnitIds[_]
 }

deploy/helm/local-auth-test-deployment/policies/object/registry/record/orgunit_test.rego

@@ -9,7 +9,7 @@ test_allow_correct_orgunit {
             "registry": {
                 "record": {
                     "access-control": {
-                        "orgUnitOwnerId": "3"
+                        "orgUnitId": "3"
                     }
                 }
             }
@@ -26,7 +26,7 @@ test_deny_wrong_orgunit {
             "registry": {
                 "record": {
                     "access-control": {
-                        "orgUnitOwnerId": "5"
+                        "orgUnitId": "5"
                     }
                 }
             }
@@ -57,7 +57,7 @@ test_deny_empty_managing_orgunit_ids {
             "registry": {
                 "record": {
                     "access-control": {
-                        "orgUnitOwnerId": "5"
+                        "orgUnitId": "5"
                     }
                 }
             }

deploy/helm/local-auth-test-deployment/policies/object/registry/record/owner_only_test.rego

@@ -21,7 +21,7 @@ test_allow_read_if_owner_and_permission_are_correct_regardless_orgunit {
                 "record": {
                     "access-control": {
                         "ownerId": "personA",
-                        "orgUnitOwnerId": "3"
+                        "orgUnitId": "3"
                     }
                 }
             }
@@ -50,7 +50,7 @@ test_deny_read_if_owner_and_permission_are_incorrect {
                 "record": {
                     "access-control": {
                         "ownerId": "personB",
-                        "orgUnitOwnerId": "3"
+                        "orgUnitId": "3"
                     }
                 }
             }

deploy/helm/local-auth-test-deployment/policies/object/registry/record/owner_orgunit_test.rego

@@ -29,7 +29,7 @@ test_allow_read_if_owner_and_permission_are_correct {
                 "record": {
                     "access-control": {
                         "ownerId": "personA",
-                        "orgUnitOwnerId": "3"
+                        "orgUnitId": "3"
                     }
                 }
             }
@@ -59,7 +59,7 @@ test_allow_read_if_orgunit_and_permission_are_correct {
                 "record": {
                     "access-control": {
                     "ownerId": "personB",
-                        "orgUnitOwnerId": "3"
+                        "orgUnitId": "3"
                     }
                 }
             }
@@ -89,7 +89,7 @@ test_deny_read_if_both_owner_and_orgunit_are_incorrect {
                 "record": {
                     "access-control": {
                         "ownerId": "personB",
-                        "orgUnitOwnerId": "3"
+                        "orgUnitId": "3"
                     }
                 }
             }
@@ -119,7 +119,7 @@ test_deny_read_if_permission_is_incorrect {
                 "record": {
                     "access-control": {
                         "ownerId": "personA",
-                        "orgUnitOwnerId": "3"
+                        "orgUnitId": "3"
                     }
                 }
             }

deploy/helm/preview.yml

@@ -113,9 +113,9 @@ magda:
       fallbackUrl: "https://data.gov.au"
       featureFlags:
         cataloguing: true
-        publishToDga: true
+        publishToDga: false
         placeholderWorkflowsOn: false
-        datasetApprovalWorkflowOn: true
+        datasetApprovalWorkflowOn: false
         useStorageApi: true
       vocabularyApiEndpoints:
         - "https://vocabs.ands.org.au/repository/api/lda/abares/australian-land-use-and-management-classification/version-8/concept.json"

magda-authorization-api/src/NestedSetModelQueryer.ts

@@ -165,9 +165,14 @@ class NestedSetModelQueryer {
     async getNodes(
         nodesQuery: NodesQuery = {},
         fields: string[] = null,
-        client: pg.Client = null
+        client: pg.Client = null,
+        authDecision: AuthDecision = UnconditionalTrueDecision
     ): Promise<NodeRecord[]> {
+        const authConditions = authDecision.toSql({
+            prefixes: ["input.authObject.orgUnit"]
+        });
         const clauses = [
+            authConditions,
             nodesQuery.name
                 ? sqls`"name" = ${nodesQuery.name}`
                 : SQLSyntax.empty,
@@ -225,7 +230,7 @@ class NestedSetModelQueryer {
 
     /**
      * Get the root node of the tree
-     * Return null if empty tree
+     * Return null if empty tree or the user has no access to the root node
      *
      * @param {string[]} [fields=null] Selected Fields; If null, use this.defaultSelectFieldList
      * @param {pg.Client} [client=null] Optional pg client; Use supplied client connection for query rather than a random connection from Pool
@@ -277,7 +282,7 @@ class NestedSetModelQueryer {
     ): Promise<NodeRecord[]> {
         const authConditions = authDecision.toSql({
             prefixes: ["input.authObject.orgUnit"],
-            tableRef: "Children"
+            tableRef: "children"
         });
         const tbl = escapeIdentifier(this.tableName);
         const conditions = [
@@ -320,7 +325,7 @@ class NestedSetModelQueryer {
     ): Promise<NodeRecord[]> {
         const authConditions = authDecision.toSql({
             prefixes: ["input.authObject.orgUnit"],
-            tableRef: "Parents"
+            tableRef: "parents"
         });
         const tbl = escapeIdentifier(this.tableName);
         const conditions = [
@@ -360,7 +365,7 @@ class NestedSetModelQueryer {
     ): Promise<NodeRecord[]> {
         const authConditions = authDecision.toSql({
             prefixes: ["input.authObject.orgUnit"],
-            tableRef: "Children"
+            tableRef: "children"
         });
         const tbl = escapeIdentifier(this.tableName);
         const result = await (client ? client : this.pool).query(
@@ -401,7 +406,7 @@ class NestedSetModelQueryer {
     ): Promise<Maybe<NodeRecord>> {
         const authConditions = authDecision.toSql({
             prefixes: ["input.authObject.orgUnit"],
-            tableRef: "Parents"
+            tableRef: "parents"
         });
         const tbl = escapeIdentifier(this.tableName);
         const result = await (client ? client : this.pool).query(
@@ -474,7 +479,7 @@ class NestedSetModelQueryer {
     ): Promise<number> {
         const authConditions = authDecision.toSql({
             prefixes: ["input.authObject.orgUnit"],
-            tableRef: "Parents"
+            tableRef: "parents"
         });
         const tbl = escapeIdentifier(this.tableName);
         const result = await this.pool.query(
@@ -548,7 +553,7 @@ class NestedSetModelQueryer {
     ): Promise<Maybe<NodeRecord>> {
         const authConditions = authDecision.toSql({
             prefixes: ["input.authObject.orgUnit"],
-            tableRef: "Children"
+            tableRef: "children"
         });
         const tbl = escapeIdentifier(this.tableName);
         const result = await this.pool.query(
@@ -579,7 +584,7 @@ class NestedSetModelQueryer {
     ): Promise<Maybe<NodeRecord>> {
         const authConditions = authDecision.toSql({
             prefixes: ["input.authObject.orgUnit"],
-            tableRef: "Children"
+            tableRef: "children"
         });
         const tbl = escapeIdentifier(this.tableName);
         const result = await this.pool.query(
@@ -599,9 +604,11 @@ class NestedSetModelQueryer {
 
     /**
      * Get all nodes on the top to down path between the `higherNode` to the `lowerNode`
-     * Sort from higher level nodes to lower level node
-     * If a path doesn't exist, null will be returned
-     * If you pass a lower node to the `higherNodeId` and a higher node to `lowerNodeId`, null will be returned
+     * Sort from higher level nodes to lower level node.
+     * Result will include `higherNode` and the `lowerNode`.
+     * If `higherNode` and the `lowerNode` is the same node, an array contains the single node will be return.
+     * If a path doesn't exist, empty array (`[]`) will be returned
+     * If you pass a lower node to the `higherNodeId` and a higher node to `lowerNodeId`, empty array will be returned
      *
      * @param {string} higherNodeId
      * @param {string} lowerNodeId
@@ -612,7 +619,7 @@ class NestedSetModelQueryer {
         higherNodeId: string,
         lowerNodeId: string,
         authDecision: AuthDecision = UnconditionalTrueDecision
-    ): Promise<Maybe<NodeRecord[]>> {
+    ): Promise<NodeRecord[]> {
         const authConditions = authDecision.toSql({
             prefixes: ["input.authObject.orgUnit"],
             tableRef: "t2"
@@ -631,9 +638,11 @@ class NestedSetModelQueryer {
             }
             ORDER BY (t2.right-t2.left) DESC`.toQuery()
         );
-        if (!result || !result.rows || !result.rows.length)
-            return Maybe.nothing();
-        return Maybe.just(result.rows);
+        if (!result?.rows?.length) {
+            return [];
+        } else {
+            return result.rows;
+        }
     }
 
     /**