diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 2b105d49..e050a713 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -19,38 +19,43 @@ jobs: - id: vars run: | echo "tag=${{ github.head_ref || github.ref_name }}-$(git rev-parse --short HEAD)" >> $GITHUB_OUTPUT - echo "repository=us-central1-docker.pkg.dev/cdip-78ca/gundi/admin-portal" >> $GITHUB_OUTPUT + echo "repository=europe-west3-docker.pkg.dev/serca-artifact-registry/gundi/admin-portal" >> $GITHUB_OUTPUT build: - uses: PADAS/gundi-workflows/.github/workflows/build_docker.yml@v7 + uses: PADAS/gundi-workflows/.github/workflows/build_docker.yml@v11 needs: vars with: - environment: stage + workload_identity_provider: "projects/563316706574/locations/global/workloadIdentityPools/github-actions/providers/github-oidc" # pragma: allowlist secret repository: ${{ needs.vars.outputs.repository }} tag: ${{ needs.vars.outputs.tag }} + build-args: BASE_REGISTRY=europe-west3-docker.pkg.dev/serca-artifact-registry/virtual-docker/ deploy_dev: - uses: PADAS/gundi-workflows/.github/workflows/deploy_k8s.yml@v7 + # gundi-dev is reconciled by ArgoCD. This job bumps the image tag in + # PADAS/serca-argocd-apps:gundi/admin-portal/values-gundi-dev.yaml and lets + # ArgoCD's automated selfHeal pick up the change. + uses: PADAS/gundi-workflows/.github/workflows/deploy_argocd.yml@v8 if: startsWith(github.ref, 'refs/heads/main') needs: [vars, build] with: - environment: dev - chart_name: admin-portal - chart_version: '1.3.6' - repository: ${{ needs.vars.outputs.repository }} - tag: ${{ needs.vars.outputs.tag }} + app-name: admin-portal + environment: gundi-dev + image-repository: ${{ needs.vars.outputs.repository }} + image-tag: ${{ needs.vars.outputs.tag }} secrets: inherit deploy_stage: - uses: PADAS/gundi-workflows/.github/workflows/deploy_k8s.yml@v7 + # gundi-stage is reconciled by ArgoCD. This job bumps the image tag in + # PADAS/serca-argocd-apps:gundi/admin-portal/values-gundi-stage.yaml and lets + # ArgoCD's automated selfHeal pick up the change. + uses: PADAS/gundi-workflows/.github/workflows/deploy_argocd.yml@v8 if: startsWith(github.ref, 'refs/heads/release') needs: [vars, build] with: - environment: stage - chart_name: admin-portal - chart_version: '1.3.6' - repository: ${{ needs.vars.outputs.repository }} - tag: ${{ needs.vars.outputs.tag }} + app-name: admin-portal + environment: gundi-stage + image-repository: ${{ needs.vars.outputs.repository }} + image-tag: ${{ needs.vars.outputs.tag }} secrets: inherit deploy_dev_legacy: @@ -60,7 +65,7 @@ jobs: needs: [vars, build] strategy: matrix: - deployment: + deployment: - cdip-portal - celery-beat - celery-deployments-worker @@ -75,25 +80,35 @@ jobs: namespace: cdip-v1 secrets: inherit - deploy_prod: - uses: PADAS/gundi-workflows/.github/workflows/deploy_k8s.yml@v7 + approve_prod: + runs-on: ubuntu-latest + environment: prod if: startsWith(github.ref, 'refs/heads/release') needs: [vars, build, deploy_stage] + steps: + - run: echo "Approved for prod" + + deploy_prod: + # gundi-prod is reconciled by ArgoCD. This job bumps the image tag in + # PADAS/serca-argocd-apps:gundi/admin-portal/values-gundi-prod.yaml and lets + # ArgoCD's automated selfHeal pick up the change. + uses: PADAS/gundi-workflows/.github/workflows/deploy_argocd.yml@v8 + if: startsWith(github.ref, 'refs/heads/release') + needs: [vars, build, deploy_stage, approve_prod] with: - environment: prod - chart_name: admin-portal - chart_version: '1.3.6' - repository: ${{ needs.vars.outputs.repository }} - tag: ${{ needs.vars.outputs.tag }} + app-name: admin-portal + environment: gundi-prod + image-repository: ${{ needs.vars.outputs.repository }} + image-tag: ${{ needs.vars.outputs.tag }} secrets: inherit deploy_prod_legacy: - uses: PADAS/gundi-workflows/.github/workflows/update_k8s_image.yml@v7 + uses: PADAS/gundi-workflows/.github/workflows/update_k8s_image.yml@v12 if: startsWith(github.ref, 'refs/heads/release') - needs: [vars, build, deploy_stage] + needs: [vars, build, deploy_stage, approve_prod] strategy: matrix: - deployment: + deployment: - cdip-portal - celery-beat - celery-deployments-worker diff --git a/cdip_admin/api/v2/serializers.py b/cdip_admin/api/v2/serializers.py index 8224b696..e0a22626 100644 --- a/cdip_admin/api/v2/serializers.py +++ b/cdip_admin/api/v2/serializers.py @@ -487,6 +487,14 @@ class IntegrationConfigurationCreateUpdateSerializer(serializers.ModelSerializer class Meta: model = IntegrationConfiguration fields = ["id", "integration", "action", "data"] + # The model's UniqueConstraint(fields=["integration", "action"]) makes + # DRF auto-generate a UniqueTogetherValidator, which enforces both + # fields as required regardless of the explicit required=False above. + # The parent IntegrationCreateUpdateSerializer injects `integration` + # from the URL after validation, and `update_or_create(id=...)` + # plus the DB-level UniqueConstraint enforce uniqueness, so the + # serializer-level check is redundant. + validators = [] class IntegrationCreateUpdateSerializer(serializers.ModelSerializer): @@ -517,13 +525,42 @@ def validate(self, data): """ Validate the configurations """ + seen_action_ids = set() for configuration in data.get("configurations", []): if self.instance and "id" in configuration: # Integration Update - action = IntegrationConfiguration.objects.get(id=configuration["id"]).action + # Scope the lookup to this integration's configurations so + # an id from a different integration doesn't get repointed + # (and a missing id surfaces as a 400, not a 500 from + # DoesNotExist). + try: + existing_config = self.instance.configurations.get(id=configuration["id"]) + except IntegrationConfiguration.DoesNotExist: + raise drf_exceptions.ValidationError( + f"Configuration '{configuration['id']}' was not found on this integration." + ) + action = existing_config.action else: # Create a new integration or new config if "action" not in configuration: raise drf_exceptions.ValidationError("The action id is required.") action = configuration["action"] + # On PATCH, a new (no-id) item for an action that already + # has a configuration would collide with the (integration, + # action) UniqueConstraint at the DB layer and surface as + # a 500. Surface it as a friendly 400 here. The validator- + # level UniqueTogetherValidator that would normally catch + # this is disabled in IntegrationConfigurationCreateUpdate + # Serializer.Meta because it incorrectly required the + # `integration` field that the URL implies. + if self.instance and self.instance.configurations.filter(action=action).exists(): + raise drf_exceptions.ValidationError( + f"A configuration for action '{action.value}' already exists on this " + f"integration. Include 'id' in the payload to update it." + ) + if action.id in seen_action_ids: + raise drf_exceptions.ValidationError( + f"Duplicate configurations for action '{action.value}' in the request." + ) + seen_action_ids.add(action.id) configuration_schema = action.schema if configuration_schema and not configuration: # Blank or None raise drf_exceptions.ValidationError("The configuration can't be null or empty") @@ -570,6 +607,14 @@ def update(self, instance, validated_data): # Update or Create nested configurations if provided for config_data in configurations: # Usually less than 5-10 configs config_data["integration"] = self.instance + if config_data.get("id"): + # When updating by id, the row's `action` is immutable — + # repointing it would (a) collide with the (integration, + # action) UniqueConstraint, and (b) bypass the schema + # validation in validate() which keyed off the existing + # action. The portal sometimes echoes `action` back for + # client convenience; ignore it on id-updates. + config_data.pop("action", None) IntegrationConfiguration.objects.update_or_create( id=config_data.get("id"), defaults=config_data diff --git a/cdip_admin/api/v2/tests/test_integrations_api.py b/cdip_admin/api/v2/tests/test_integrations_api.py index e195b9a5..ac0b6fc6 100644 --- a/cdip_admin/api/v2/tests/test_integrations_api.py +++ b/cdip_admin/api/v2/tests/test_integrations_api.py @@ -1499,6 +1499,157 @@ def test_update_or_create_integration_config_as_org_admin( ) +def test_patch_integration_config_with_portal_payload_shape( + api_client, org_admin_user, organization, provider_lotek_panthera, + lotek_action_auth, +): + # The portal omits `integration` from each configuration item (it is + # implied by the URL). Regression test for the UniqueTogetherValidator + # auto-generated from the model's UniqueConstraint(integration, action) + # rejecting the request with `{"integration": ["This field is required."]}`. + lotek_auth_config = lotek_action_auth.configurations_by_action.get(integration=provider_lotek_panthera) + api_client.force_authenticate(org_admin_user) + response = api_client.patch( + reverse("integrations-detail", kwargs={"pk": provider_lotek_panthera.id}), + data={ + "configurations": [ + { + "id": str(lotek_auth_config.id), + "action": str(lotek_action_auth.id), + "data": {"username": "user@lotek.com", "password": "NewPassword"}, + }, + ], + }, + format="json", + ) + assert response.status_code == status.HTTP_200_OK, response.json() + lotek_auth_config.refresh_from_db() + assert lotek_auth_config.data == {"username": "user@lotek.com", "password": "NewPassword"} + + +def test_patch_integration_config_rejects_new_item_for_existing_action( + api_client, org_admin_user, organization, provider_lotek_panthera, + lotek_action_auth, +): + # A PATCH that adds a new (no-id) configuration for an action that + # already has a config would otherwise hit the (integration, action) + # DB UniqueConstraint mid-update and surface as a 500. Should be a 400. + api_client.force_authenticate(org_admin_user) + response = api_client.patch( + reverse("integrations-detail", kwargs={"pk": provider_lotek_panthera.id}), + data={ + "configurations": [ + { + "action": str(lotek_action_auth.id), + "data": {"username": "x", "password": "y"}, + }, + ], + }, + format="json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "already exists" in response.json()["non_field_errors"][0] + + +def test_patch_integration_config_rejects_duplicate_actions_in_payload( + api_client, org_admin_user, organization, provider_lotek_panthera, + lotek_action_auth, lotek_action_list_devices, +): + # Two items targeting the same action in one request would silently + # last-write-wins (in the natural-key sense) or hit IntegrityError. + # Should be a 400. + api_client.force_authenticate(org_admin_user) + response = api_client.patch( + reverse("integrations-detail", kwargs={"pk": provider_lotek_panthera.id}), + data={ + "configurations": [ + {"action": str(lotek_action_list_devices.id), "data": {"group_id": "1"}}, + {"action": str(lotek_action_list_devices.id), "data": {"group_id": "2"}}, + ], + }, + format="json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Duplicate" in response.json()["non_field_errors"][0] + + +def test_patch_integration_config_rejects_unknown_config_id( + api_client, org_admin_user, organization, provider_lotek_panthera, +): + # A PATCH referencing a configuration id that doesn't exist on this + # integration should return 400, not 500 from DoesNotExist. + api_client.force_authenticate(org_admin_user) + response = api_client.patch( + reverse("integrations-detail", kwargs={"pk": provider_lotek_panthera.id}), + data={ + "configurations": [ + { + "id": "00000000-0000-0000-0000-000000000000", + "data": {"username": "x", "password": "y"}, + }, + ], + }, + format="json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "not found" in response.json()["non_field_errors"][0] + + +def test_patch_integration_config_rejects_id_from_different_integration( + api_client, org_admin_user, organization, provider_lotek_panthera, provider_ats, +): + # A PATCH referencing a configuration id that belongs to a different + # integration must not repoint that row — it should 400 cleanly. + other_config = provider_ats.configurations.first() + assert other_config is not None + api_client.force_authenticate(org_admin_user) + response = api_client.patch( + reverse("integrations-detail", kwargs={"pk": provider_lotek_panthera.id}), + data={ + "configurations": [ + {"id": str(other_config.id), "data": {"hijacked": True}}, + ], + }, + format="json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "not found" in response.json()["non_field_errors"][0] + # The other integration's config must remain untouched. + other_config.refresh_from_db() + assert other_config.integration_id == provider_ats.id + assert other_config.data != {"hijacked": True} + + +def test_patch_integration_config_ignores_action_on_id_update( + api_client, org_admin_user, organization, provider_lotek_panthera, + lotek_action_auth, lotek_action_pull_positions, +): + # The portal sometimes echoes back `action` alongside `id` for client + # convenience. The row's action must not be repointed via update_or_create + # defaults — that would collide with the (integration, action) constraint. + lotek_auth_config = lotek_action_auth.configurations_by_action.get(integration=provider_lotek_panthera) + api_client.force_authenticate(org_admin_user) + response = api_client.patch( + reverse("integrations-detail", kwargs={"pk": provider_lotek_panthera.id}), + data={ + "configurations": [ + { + "id": str(lotek_auth_config.id), + # Try to repoint at a different action — must be ignored. + "action": str(lotek_action_pull_positions.id), + "data": {"username": "u", "password": "p"}, + }, + ], + }, + format="json", + ) + assert response.status_code == status.HTTP_200_OK, response.json() + lotek_auth_config.refresh_from_db() + # The row's action stayed put; only data updated. + assert lotek_auth_config.action_id == lotek_action_auth.id + assert lotek_auth_config.data == {"username": "u", "password": "p"} + + def _test_get_integration_api_key( api_client, user, integration ):