- change kebab-case to camelCase for webhook table column names (as w…

…e didn't quoted the column name it's actually all lowercase --- which is consisten with all other legecy code) - fix typescript test case - in CI pipeline run `cd ..` as a new command so other commands after it would ends up run in wrong directory when previous tests case failed
magdabot · Jan 26, 2022 · 849b3de · 849b3de
1 parent 0d670b1
commit 849b3de
Show file tree

Hide file tree

Showing 9 changed files with 72 additions and 287 deletions.
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
@@ -59,9 +59,12 @@ build-builder-image:
     - docker:dind
   script: |
     # Build all three builder images and push to gitlab registry
-    cd magda-builder-nodejs && docker buildx build --push -t $CI_REGISTRY/magda-data/magda/data61/magda-builder-nodejs:$CI_COMMIT_REF_SLUG --platform linux/arm64,linux/amd64 -f Dockerfile . && cd ..
-    cd magda-builder-docker && docker buildx build --push -t $CI_REGISTRY/magda-data/magda/data61/magda-builder-docker:$CI_COMMIT_REF_SLUG --platform linux/arm64,linux/amd64 -f Dockerfile . && cd ..
-    cd magda-builder-scala && docker buildx build --push -t $CI_REGISTRY/magda-data/magda/data61/magda-builder-scala:$CI_COMMIT_REF_SLUG --platform linux/arm64,linux/amd64 -f Dockerfile . && cd ..
+    cd magda-builder-nodejs && docker buildx build --push -t $CI_REGISTRY/magda-data/magda/data61/magda-builder-nodejs:$CI_COMMIT_REF_SLUG --platform linux/arm64,linux/amd64 -f Dockerfile . 
+    cd .. 
+    cd magda-builder-docker && docker buildx build --push -t $CI_REGISTRY/magda-data/magda/data61/magda-builder-docker:$CI_COMMIT_REF_SLUG --platform linux/arm64,linux/amd64 -f Dockerfile . 
+    cd .. 
+    cd magda-builder-scala && docker buildx build --push -t $CI_REGISTRY/magda-data/magda/data61/magda-builder-scala:$CI_COMMIT_REF_SLUG --platform linux/arm64,linux/amd64 -f Dockerfile . 
+    cd .. 
 
 
 # Make sure sbt depenencies, plugins are in place, cached (only for this job) and pass to following stage as artifacts
@@ -249,7 +252,8 @@ buildtest:ui:
   script:
     - yarn install
     - yarn run eslint
-    - cd magda-typescript-common && yarn build && cd ..
+    - cd magda-typescript-common && yarn build
+    - cd ..
     - yarn run in-submodules -- -f categories.ui=true -- run build --include-filtered-dependencies
     - yarn run in-submodules -- -f categories.ui=true -- run test
   artifacts:
@@ -313,8 +317,10 @@ buildtest:typescript-apis-stateless:
     - build-builder-image
     - registry-typescript-api
   script:
-    - cd magda-typescript-common && yarn build && yarn test && cd ..
-    - cd magda-minion-framework && yarn build && yarn test && cd ..
+    - cd magda-typescript-common && yarn build && yarn test
+    - cd ..
+    - cd magda-minion-framework && yarn build && yarn test
+    - cd ..
     - yarn run in-submodules -- -f language=typescript -f categories.api=true -f categories.stateless=true -- run build --include-filtered-dependencies
     - yarn run in-submodules -- -f language=typescript -f categories.api=true -f categories.stateless=true -- run test --include-filtered-dependencies
     - yarn run in-submodules -- -f categories.npmPackage=true -f categories.useCommonLib=true -- run build
@@ -363,8 +369,10 @@ buildtest:typescript-apis-with-pg:
     - cd deploy/helm/internal-charts/opa
     - docker-compose up -d
     - cd ../../../../
-    - cd magda-typescript-common && yarn build && yarn test && cd ..
-    - cd magda-minion-framework && yarn build && yarn test && cd ..
+    - cd magda-typescript-common && yarn build && yarn test
+    - cd ..
+    - cd magda-minion-framework && yarn build && yarn test
+    - cd ..
     - yarn run in-submodules -- -f language=typescript -f categories.api=true -f categories.uses-pg=true -- run build --include-filtered-dependencies
     - yarn run in-submodules -- -f language=typescript -f categories.api=true -f categories.uses-pg=true -- run test --include-filtered-dependencies
     - yarn run in-submodules -- -f categories.npmPackage=true -f categories.useAuthApi=true -- run build

diff --git a/deploy/helm/internal-charts/opa/policies/object/webhook/allow.rego b/deploy/helm/internal-charts/opa/policies/object/webhook/allow.rego
@@ -15,5 +15,7 @@ allow {
 allow {
     hasOwnerConstaintPermission(input.operationUri)
     # webhook field name should match table column name
-    input.object.webhook.owner_id = input.user.id
+    # as we didn't quoted column name when create the table
+    # either uppercase or lowercase will work
+    input.object.webhook.ownerId = input.user.id
 }
diff --git a/magda-migrator-registry-db/sql/V2_6__generic_auth_ model.sql b/magda-migrator-registry-db/sql/V2_6__generic_auth_ model.sql
@@ -1,11 +1,11 @@
 -- Add owner_id, creator_id & editor_id field to webhooks table
 ALTER TABLE webhooks
-ADD COLUMN "owner_id" uuid DEFAULT NULL,
-ADD COLUMN "creator_id" uuid DEFAULT NULL,
-ADD COLUMN "editor_id" uuid DEFAULT NULL,
-ADD COLUMN "create_time" timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP,
-ADD COLUMN "edit_time" timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP;
+ADD COLUMN ownerId uuid DEFAULT NULL,
+ADD COLUMN creatorId uuid DEFAULT NULL,
+ADD COLUMN editorId uuid DEFAULT NULL,
+ADD COLUMN createTime timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP,
+ADD COLUMN editTime timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP;
 
-CREATE INDEX webhooks_owner_id_idx ON webhooks USING btree ("owner_id");
-CREATE INDEX webhooks_create_time_idx ON webhooks USING btree ("create_time");
-CREATE INDEX webhooks_create_time_idx ON webhooks USING btree ("edit_time");
+CREATE INDEX webhooks_owner_id_idx ON webhooks USING btree (ownerId);
+CREATE INDEX webhooks_create_time_idx ON webhooks USING btree (createTime);
+CREATE INDEX webhooks_create_time_idx ON webhooks USING btree (editTime);
diff --git a/magda-minion-framework/src/registerWebhook.ts b/magda-minion-framework/src/registerWebhook.ts
@@ -35,7 +35,12 @@ export default async function registerNewWebhook(
         lastRetryTime: null,
         retryCount: 0,
         isRunning: null,
-        isProcessing: null
+        isProcessing: null,
+        ownerId: undefined,
+        createTime: undefined,
+        creatorId: undefined,
+        editTime: undefined,
+        editorId: undefined
     };
 
     return registry.putHook(newWebHook);

diff --git a/magda-minion-framework/src/test/registry.spec.ts b/magda-minion-framework/src/test/registry.spec.ts
@@ -259,6 +259,11 @@ function buildWebHook(
         lastRetryTime: null,
         retryCount: 0,
         isRunning: null,
-        isProcessing: null
+        isProcessing: null,
+        ownerId: undefined,
+        createTime: undefined,
+        creatorId: undefined,
+        editTime: undefined,
+        editorId: undefined
     };
 }
diff --git a/magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/Directives.scala b/magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/Directives.scala
@@ -555,7 +555,7 @@ object Directives extends Protocols with SprayJsonSupport {
       blocking {
         val hookData = HookPersistence
           .getById(webhookId, UnconditionalTrueDecision)
-          .map(_.toPolicyEngineContextData)
+          .map(_.toJson.asJsObject)
         if (hookData.isEmpty) {
           throw NoRecordFoundException(webhookId)
         } else {
@@ -646,7 +646,7 @@ object Directives extends Protocols with SprayJsonSupport {
           input = Some(
             JsObject(
               "object" -> JsObject(
-                "webhook" -> newWebhook.toPolicyEngineContextData
+                "webhook" -> newWebhook.toJson
               )
             )
           )
@@ -658,7 +658,7 @@ object Directives extends Protocols with SprayJsonSupport {
           input = Some(
             JsObject(
               "object" -> JsObject(
-                "webhook" -> newWebhook.toPolicyEngineContextData
+                "webhook" -> newWebhook.toJson
               )
             )
           )
@@ -708,230 +708,4 @@ object Directives extends Protocols with SprayJsonSupport {
           )
         }
       }
-
-//  /**
-//    * Returns true if the configuration says to skip the opa query.
-//    */
-//  private def skipOpaQuery(implicit system: ActorSystem, config: Config) = {
-//    val skip = config.hasPath("authorization.skipOpaQuery") && config
-//      .getBoolean(
-//        "authorization.skipOpaQuery"
-//      )
-//
-//    if (skip) {
-//      system.log.warning(
-//        "WARNING: Skip OPA querying is turned on! This is fine for testing or playing around, but this should NOT BE TURNED ON FOR PRODUCTION!"
-//      )
-//    }
-//
-//    skip
-//  }
-//
-//  /**
-//    * Determines OPA policies that apply, queries OPA and parses the policies for translation to SQL or some other query language.
-//    *
-//    * @param operationType The operation being performed (read, edit etc)
-//    * @param recordPersistence A RecordPersistence instance to look up record policies etc through
-//    * @param authApiClient An auth api client to query OPA through
-//    * @param recordId A record id - optional, will be used to narrow down the policies returned to only those that apply to this record.
-//    * @param noPoliciesResponse What to respond with if there are no valid policies to query for
-//    * @return - If no auth should be applied, None
-//    *         - If auth should be applied, Some, with a List of tuples - _1 in the tuple is the id of the policy, _2 is the parsed query
-//    *              that pertain to that policy. So a query can be made along the lines of
-//    *              (policyId = $policy1 AND (<policy1 details)) OR (policyId = $policy2 AND (<policy2 details>))
-//    */
-//  def withRecordOpaQuery(
-//      operationType: AuthOperations.OperationType,
-//      recordPersistence: RecordPersistence,
-//      authApiClient: RegistryAuthApiClient,
-//      recordId: Option[String] = None,
-//      noPoliciesResponse: => ToResponseMarshallable
-//  )(
-//      implicit config: Config,
-//      system: ActorSystem,
-//      materializer: Materializer,
-//      ec: ExecutionContext
-//  ): Directive1[Option[List[(String, List[List[OpaQuery]])]]] = {
-//    if (skipOpaQuery) {
-//      provide(None)
-//    } else {
-//      getJwt().flatMap { jwt =>
-//        val recordPolicyIds = DB readOnly { implicit session =>
-//          recordPersistence
-//            .getPolicyIds(operationType, recordId.map(Set(_)))
-//        } get
-//
-//        if (recordPolicyIds.isEmpty) {
-//          system.log.warning(
-//            s"""Could not find any policy for operation $operationType on
-//            ${if (recordId.isDefined) s"record $recordId" else "records"}.
-//            This will result in the record being completely inaccessible -
-//            if this isn't what you want, define a default policy in config,
-//            or set one for all records"""
-//          )
-//        }
-//
-//        val recordFuture =
-//          authApiClient
-//            .queryRecord(
-//              jwt,
-//              operationType,
-//              recordPolicyIds.toList
-//            )
-//
-//        onSuccess(recordFuture).flatMap { queryResults =>
-//          if (queryResults.isEmpty) {
-//            complete(noPoliciesResponse)
-//          } else {
-//            provide(Some(queryResults))
-//          }
-//        }
-//      }
-//    }
-//  }
-//
-//  /**
-//    * Determines OPA policies that apply, queries OPA and parses the policies for translation to SQL or some other query language.
-//    * As opposed to withRecordOpaQuery, this also takes into account links within records.
-//    *
-//    * @param operationType The operation being performed (read, edit etc)
-//    * @param recordPersistence A RecordPersistence instance to look up record policies etc through
-//    * @param authApiClient An auth api client to query OPA through
-//    * @param recordId A record id - optional, will be used to narrow down the policies returned to only those that apply to this record.
-//    * @param aspectIds An optional iterable of the aspects being requested, to narrow down the total scope of potential policies to query
-//    * @return A tuple with two values, both matching the following structure:
-//    *         - If no auth should be applied, None
-//    *         - If auth should be applied, Some, with a List of tuples - _1 in the tuple is the id of the policy, _2 is the parsed query
-//    *           that pertain to that policy. So a query can be made along the lines of
-//    *           (policyId = $policy1 AND (<policy1 details)) OR (policyId = $policy2 AND (<policy2 details>))
-//    *
-//    *           The first value in the tuple is to be applied for outer records, the second value is to be supplied for records that the
-//    *           outer record links to.
-//    */
-//  def withRecordOpaQueryIncludingLinks(
-//      operationType: AuthOperations.OperationType,
-//      recordPersistence: RecordPersistence,
-//      authApiClient: RegistryAuthApiClient,
-//      recordId: Option[String] = None,
-//      aspectIds: Iterable[String] = List(),
-//      noPoliciesResponse: => ToResponseMarshallable
-//  )(
-//      implicit config: Config,
-//      system: ActorSystem,
-//      materializer: Materializer,
-//      ec: ExecutionContext
-//  ): Directive1[
-//    (
-//        Option[List[(String, List[List[OpaQuery]])]],
-//        Option[List[(String, List[List[OpaQuery]])]]
-//    )
-//  ] = {
-//    if (skipOpaQuery) {
-//      provide((None, None))
-//    } else {
-//      getJwt().flatMap { jwt =>
-//        val recordPolicyIds = DB readOnly { implicit session =>
-//          recordPersistence
-//            .getPolicyIds(operationType, recordId.map(Set(_)))
-//        } get
-//
-//        val linkedRecordIds = recordId.map(
-//          innerRecordId =>
-//            DB readOnly { implicit session =>
-//              recordPersistence.getLinkedRecordIds(
-//                innerRecordId,
-//                aspectIds
-//              )
-//            } get
-//        )
-//
-//        val linkedRecordPolicyIds =
-//          DB readOnly { implicit session =>
-//            recordPersistence
-//              .getPolicyIds(
-//                operationType,
-//                linkedRecordIds.map(_.toSet)
-//              )
-//          } get
-//
-//        val allPolicyIds = recordPolicyIds.toSet ++ linkedRecordPolicyIds
-//
-//        val recordFuture =
-//          authApiClient
-//            .queryRecord(
-//              jwt,
-//              operationType,
-//              allPolicyIds.toList
-//            )
-//
-//        onSuccess(recordFuture).flatMap { queryResults =>
-//          if (queryResults.isEmpty) {
-//            complete(noPoliciesResponse)
-//          } else {
-//            val queryResultLookup = queryResults.toMap
-//            val fullRecordPolicyIds = recordPolicyIds
-//              .map(policyId => (policyId, queryResultLookup(policyId)))
-//            val fullLinkedRecordPolicyIds = linkedRecordPolicyIds
-//              .map(policyId => (policyId, queryResultLookup(policyId)))
-//
-//            provide(
-//              (Some(fullRecordPolicyIds), Some(fullLinkedRecordPolicyIds))
-//            )
-//          }
-//        }
-//      }
-//    }
-//  }
-//
-//  /**
-//    * Checks whether the user making this call can access events for the provided record id.
-//    *
-//    * This will complete with 404 if:
-//    *   - The record never existed OR
-//    *   - The record existed and has been deleted, and the user is NOT an admin
-//    *
-//    * This will complete with 200 if:
-//    *   - The record exists and the user is allowed to access it in its current form OR
-//    *   - The record existed in the past and has been deleted, but the user is an admin
-//    *
-//    * @param recordPersistence A RecordPersistence instance to look up record policies etc through
-//    * @param authApiClient An auth api client to query OPA through
-//    * @param recordId The id of the record in question
-//    * @param tenantId tenantId
-//    * @param notFoundResponse What to respond with if there are no valid policies to query for (defaults to 404)
-//    * @return If the record exists and the user is allowed to access it, will pass through, otherwise will complete with 404.
-//    */
-//  def checkUserCanAccessRecordEvents(
-//      recordPersistence: RecordPersistence,
-//      authApiClient: RegistryAuthApiClient,
-//      recordId: String,
-//      tenantId: au.csiro.data61.magda.model.TenantId.TenantId,
-//      notFoundResponse: => ToResponseMarshallable
-//  )(
-//      implicit config: Config,
-//      system: ActorSystem,
-//      materializer: Materializer,
-//      ec: ExecutionContext
-//  ): Directive1[Option[List[(String, List[List[OpaQuery]])]]] = {
-//    provideUser(authApiClient) flatMap {
-//      // If the user is an admin, we should allow this even if they can't access the record
-//      case Some(User(_, true)) => provide(None)
-//      case _ =>
-//        withRecordOpaQuery(
-//          AuthOperations.read,
-//          recordPersistence,
-//          authApiClient,
-//          Some(recordId),
-//          notFoundResponse
-//        ) flatMap { recordQueries =>
-//          DB readOnly { implicit session =>
-//            recordPersistence
-//              .getById(tenantId, recordQueries, recordId) match {
-//              case Some(record) => provide(recordQueries)
-//              case None         => complete(notFoundResponse)
-//            }
-//          }
-//        }
-//    }
-//  }
 }