From 802397323c93d93cf392a3716d60d9370ac095c2 Mon Sep 17 00:00:00 2001 From: Riccardo Binetti Date: Mon, 2 Sep 2024 17:55:42 +0200 Subject: [PATCH] base_image: correctly scope upload to the BaseImageCollection bucket The extracted base_image_collection_id was nil in the current implementation, leading to the image being put in the wrong bucket directory. This makes the storage module behaviour more explicit in its requirement (instead of having to pass the whole BaseImage) and adds a test to verify that the base_image_collection_id received by the storage module actually matches the one from the Base Image Collection. Close #586 --- CHANGELOG.md | 4 ++++ .../base_images/base_image/base_image.ex | 2 +- .../base_image/changes/handle_file_upload.ex | 7 +++++-- .../lib/edgehog/base_images/bucket_storage.ex | 4 +++- backend/lib/edgehog/base_images/storage.ex | 6 +++++- .../mutation/create_base_image_test.exs | 20 +++++++++++-------- 6 files changed, 30 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f9000e59..e61ac2d6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.9.0] - Unreleased +### Fixed +- Correctly scope Base Image uploads to their Base Image Collection bucket. + ## [0.9.0-rc.1] - 2024-07-10 ### Fixed - Wrong input params used in GraphQL mutation when creating a base image, leading to a rejected operation ([#574](https://github.com/edgehog-device-manager/edgehog/pull/574)). diff --git a/backend/lib/edgehog/base_images/base_image/base_image.ex b/backend/lib/edgehog/base_images/base_image/base_image.ex index 164d21133..fb24479ae 100644 --- a/backend/lib/edgehog/base_images/base_image/base_image.ex +++ b/backend/lib/edgehog/base_images/base_image/base_image.ex @@ -71,7 +71,7 @@ defmodule Edgehog.BaseImages.BaseImage do description "A list of release display names in different languages." end - change Changes.HandleFileUpload + change Changes.HandleFileUpload, only_when_valid?: true change manage_relationship(:base_image_collection_id, :base_image_collection, type: :append) change {UpsertLocalizedAttribute, input_argument: :localized_descriptions, target_attribute: :description} diff --git a/backend/lib/edgehog/base_images/base_image/changes/handle_file_upload.ex b/backend/lib/edgehog/base_images/base_image/changes/handle_file_upload.ex index 98a544bd9..2902c7362 100644 --- a/backend/lib/edgehog/base_images/base_image/changes/handle_file_upload.ex +++ b/backend/lib/edgehog/base_images/base_image/changes/handle_file_upload.ex @@ -46,9 +46,12 @@ defmodule Edgehog.BaseImages.BaseImage.Changes.HandleFileUpload do end defp upload_file(changeset, file) do - {:ok, base_image} = Ash.Changeset.apply_attributes(changeset) + tenant_id = changeset.to_tenant - case @storage_module.store(base_image, file) do + {:ok, base_image_collection_id} = + Ash.Changeset.fetch_argument(changeset, :base_image_collection_id) + + case @storage_module.store(tenant_id, base_image_collection_id, file) do {:ok, file_url} -> changeset |> Ash.Changeset.force_change_attribute(:url, file_url) diff --git a/backend/lib/edgehog/base_images/bucket_storage.ex b/backend/lib/edgehog/base_images/bucket_storage.ex index 50dab1c3a..25c018bee 100644 --- a/backend/lib/edgehog/base_images/bucket_storage.ex +++ b/backend/lib/edgehog/base_images/bucket_storage.ex @@ -27,7 +27,9 @@ defmodule Edgehog.BaseImages.BucketStorage do alias Edgehog.BaseImages.Uploaders @impl Storage - def store(%BaseImage{} = scope, %Plug.Upload{} = upload) do + def store(tenant_id, base_image_collection_id, %Plug.Upload{} = upload) do + scope = %{tenant_id: tenant_id, base_image_collection_id: base_image_collection_id} + with {:ok, file_name} <- Uploaders.BaseImage.store({upload, scope}) do # TODO: investigate URL signing instead of public access file_url = Uploaders.BaseImage.url({file_name, scope}) diff --git a/backend/lib/edgehog/base_images/storage.ex b/backend/lib/edgehog/base_images/storage.ex index 9de4ee320..ed6b03d38 100644 --- a/backend/lib/edgehog/base_images/storage.ex +++ b/backend/lib/edgehog/base_images/storage.ex @@ -24,7 +24,11 @@ defmodule Edgehog.BaseImages.Storage do @type upload :: %Plug.Upload{} - @callback store(scope :: %BaseImage{}, file :: upload()) :: + @callback store( + tenant_id :: String.t() | integer(), + base_image_id :: String.t() | integer(), + file :: upload() + ) :: {:ok, file_url :: String.t()} | {:error, reason :: any} @callback delete(base_image :: %BaseImage{}) :: diff --git a/backend/test/edgehog_web/schema/mutation/create_base_image_test.exs b/backend/test/edgehog_web/schema/mutation/create_base_image_test.exs index 84b05e7b2..e426b87d5 100644 --- a/backend/test/edgehog_web/schema/mutation/create_base_image_test.exs +++ b/backend/test/edgehog_web/schema/mutation/create_base_image_test.exs @@ -28,21 +28,25 @@ defmodule EdgehogWeb.Schema.Mutation.CreateBaseImageTest do describe "createBaseImage mutation" do setup do StorageMock - |> stub(:store, fn _, _ -> {:ok, "https://example.com/ota.bin"} end) + |> stub(:store, fn _, _, _ -> {:ok, "https://example.com/ota.bin"} end) |> stub(:delete, fn _ -> :ok end) :ok end test "creates base image with valid data", %{tenant: tenant} do - base_image_collection_id = - [tenant: tenant] - |> base_image_collection_fixture() - |> AshGraphql.Resource.encode_relay_id() + base_image_collection = base_image_collection_fixture(tenant: tenant) + base_image_collection_id = AshGraphql.Resource.encode_relay_id(base_image_collection) file_url = "https://example.com/ota.bin" - expect(StorageMock, :store, fn _, _ -> {:ok, file_url} end) + expect(StorageMock, :store, fn tenant_id, base_image_collection_id, _upload -> + assert tenant_id == tenant.tenant_id + # This is the DB id + assert base_image_collection_id == base_image_collection.id + + {:ok, file_url} + end) result = create_base_image_mutation( @@ -195,14 +199,14 @@ defmodule EdgehogWeb.Schema.Mutation.CreateBaseImageTest do end test "doesn't upload the base image to the storage with invalid data", %{tenant: tenant} do - expect(StorageMock, :store, 0, fn _, _ -> {:error, :unreachable} end) + expect(StorageMock, :store, 0, fn _, _, _ -> {:error, :unreachable} end) result = create_base_image_mutation(tenant: tenant, version: "invalid") assert %{message: "is not a valid version"} = extract_error!(result) end test "returns error if the upload to the storage fails", %{tenant: tenant} do - expect(StorageMock, :store, fn _, _ -> {:error, :bucket_is_full} end) + expect(StorageMock, :store, fn _, _, _ -> {:error, :bucket_is_full} end) result = create_base_image_mutation(tenant: tenant) assert %{