Update /assets endpoint (#713)

* Assets: remove role validation in model

* AssetsController: update API endpoint and validation

The endpoint is updated to `/v2/admin/assets`, and validation is
performed within the router instead.

* Shift to `AdminAssetsController`

* Rename `AdminAssetsController` to `.exs`

* Assets: switch to `case` instead of `with`

Author: bnjmnt4n

lib/cadet/assets/assets.ex

@@ -5,17 +5,14 @@ defmodule Cadet.Assets.Assets do
   Assessments context contains domain logic for assets management
   for Source academy's game component
   """
-  @manage_assets_role ~w(staff admin)a
-
   @accessible_folders ~w(images locations objects avatars ui stories sfx bgm) ++
                         if(Mix.env() == :test, do: ["testFolder"], else: [])
   @accepted_file_types ~w(.jpg .jpeg .gif .png .wav .mp3 .txt)
 
-  def upload_to_s3(upload_params, folder_name, file_name, user) do
+  def upload_to_s3(upload_params, folder_name, file_name) do
     file_type = Path.extname(file_name)
 
-    with :ok <- validate_role(user.role),
-         :ok <- validate_file_name(file_name),
+    with :ok <- validate_file_name(file_name),
          :ok <- validate_folder_name(folder_name),
          :ok <- validate_file_type(file_type) do
       if object_exists?(folder_name, file_name) do
@@ -35,19 +32,21 @@ defmodule Cadet.Assets.Assets do
     end
   end
 
-  def list_assets(folder_name, user) do
-    with :ok <- validate_role(user.role),
-         :ok <- validate_folder_name(folder_name) do
-      bucket()
-      |> S3.list_objects(prefix: folder_name <> "/")
-      |> ExAws.stream!()
-      |> Enum.map(fn file -> file.key end)
+  def list_assets(folder_name) do
+    case validate_folder_name(folder_name) do
+      :ok ->
+        bucket()
+        |> S3.list_objects(prefix: folder_name <> "/")
+        |> ExAws.stream!()
+        |> Enum.map(fn file -> file.key end)
+
+      {:error, _} = error ->
+        error
     end
   end
 
-  def delete_object(folder_name, file_name, user) do
-    with :ok <- validate_role(user.role),
-         :ok <- validate_file_name(file_name),
+  def delete_object(folder_name, file_name) do
+    with :ok <- validate_file_name(file_name),
          :ok <- validate_folder_name(folder_name) do
       if object_exists?(folder_name, file_name) do
         bucket()
@@ -71,14 +70,6 @@ defmodule Cadet.Assets.Assets do
     end
   end
 
-  defp validate_role(role) do
-    if role in @manage_assets_role do
-      :ok
-    else
-      {:error, {:forbidden, "User not allowed to manage assets"}}
-    end
-  end
-
   defp validate_folder_name(folder_name) do
     if folder_name in @accessible_folders do
       :ok

lib/cadet_web/controllers/assets_controller.ex renamed to lib/cadet_web/admin_controllers/admin_assets_controller.ex

@@ -1,11 +1,11 @@
-defmodule CadetWeb.AssetsController do
+defmodule CadetWeb.AdminAssetsController do
   use CadetWeb, :controller
 
   use PhoenixSwagger
   alias Cadet.Assets.Assets
 
   def index(conn, _params = %{"foldername" => foldername}) do
-    case Assets.list_assets(foldername, conn.assigns.current_user) do
+    case Assets.list_assets(foldername) do
       {:error, {status, message}} -> conn |> put_status(status) |> text(message)
       assets -> render(conn, "index.json", assets: assets)
     end
@@ -15,7 +15,7 @@ defmodule CadetWeb.AssetsController do
   def delete(conn, _params = %{"foldername" => foldername, "filename" => filename}) do
     filename = Enum.join(filename, "/")
 
-    case Assets.delete_object(foldername, filename, conn.assigns.current_user) do
+    case Assets.delete_object(foldername, filename) do
       {:error, {status, message}} -> conn |> put_status(status) |> text(message)
       _ -> conn |> put_status(204) |> text('')
     end
@@ -28,7 +28,7 @@ defmodule CadetWeb.AssetsController do
       }) do
     filename = Enum.join(filename, "/")
 
-    case Assets.upload_to_s3(upload_params, foldername, filename, conn.assigns.current_user) do
+    case Assets.upload_to_s3(upload_params, foldername, filename) do
       {:error, {status, message}} -> conn |> put_status(status) |> text(message)
       resp -> render(conn, "show.json", resp: resp)
     end
@@ -60,7 +60,7 @@ defmodule CadetWeb.AssetsController do
   end
 
   swagger_path :index do
-    get("/assets/{foldername}")
+    get("/admin/assets/{foldername}")
 
     summary("Get a list of all assets in a folder")
 
@@ -78,7 +78,7 @@ defmodule CadetWeb.AssetsController do
   end
 
   swagger_path :delete do
-    PhoenixSwagger.Path.delete("/assets/{foldername}/{filename}")
+    PhoenixSwagger.Path.delete("/admin/assets/{foldername}/{filename}")
 
     summary("Delete a file from an asset folder")
 
@@ -90,16 +90,14 @@ defmodule CadetWeb.AssetsController do
 
     security([%{JWT: []}])
 
-    produces("application/json")
-
     response(204, "OK")
     response(400, "Invalid folder name, file name or file type")
     response(403, "User is not allowed to manage assets")
     response(404, "File not found")
   end
 
   swagger_path :upload do
-    post("/assets/{foldername}/{filename}")
+    post("/admin/assets/{foldername}/{filename}")
 
     summary("Upload a file to an asset folder")
 

lib/cadet_web/views/assets_view.ex renamed to lib/cadet_web/admin_views/admin_assets_view.ex

@@ -1,9 +1,9 @@
-defmodule CadetWeb.AssetsView do
+defmodule CadetWeb.AdminAssetsView do
   use CadetWeb, :view
   use Timex
 
   def render("index.json", %{assets: assets}) do
-    render_many(assets, CadetWeb.AssetsView, "show.json", as: :asset)
+    render_many(assets, CadetWeb.AdminAssetsView, "show.json", as: :asset)
   end
 
   def render("show.json", %{asset: asset}) do

lib/cadet_web/router.ex

@@ -66,10 +66,6 @@ defmodule CadetWeb.Router do
     delete("/stories/:storyid", StoriesController, :delete)
     post("/stories/:storyid", StoriesController, :update)
 
-    get("/assets/:foldername", AssetsController, :index)
-    post("/assets/:foldername/*filename", AssetsController, :upload)
-    delete("/assets/:foldername/*filename", AssetsController, :delete)
-
     get("/grading", GradingController, :index)
     get("/grading/summary", GradingController, :grading_summary)
     get("/grading/:submissionid", GradingController, :show)
@@ -115,6 +111,14 @@ defmodule CadetWeb.Router do
     delete("/goals/:uuid", AdminGoalsController, :delete)
   end
 
+  scope "/v2/admin", CadetWeb do
+    pipe_through([:api, :auth, :ensure_auth, :ensure_staff])
+
+    get("/assets/:foldername", AdminAssetsController, :index)
+    post("/assets/:foldername/*filename", AdminAssetsController, :upload)
+    delete("/assets/:foldername/*filename", AdminAssetsController, :delete)
+  end
+
   # Other scopes may use custom stacks.
   # scope "/api", CadetWeb do
   #   pipe_through :api

test/cadet/assets/assets_test.exs

@@ -1,58 +1,50 @@
 defmodule Cadet.Assets.AssetsTest do
   alias Cadet.Assets.Assets
-  alias Cadet.Accounts.User
 
   use ExUnit.Case, async: true
   use ExVCR.Mock, adapter: ExVCR.Adapter.Hackney
 
   describe "Manage assets" do
-    @tag authenticate: :staff
     test "accessible folder" do
       use_cassette "aws/model_list_assets#1" do
-        assert Assets.list_assets("testFolder", %User{role: :staff}) === [
+        assert Assets.list_assets("testFolder") === [
                  "testFolder/",
                  "testFolder/test.png",
                  "testFolder/test2.png"
                ]
       end
     end
 
-    @tag authenticate: :staff
     test "delete nonexistent file" do
       use_cassette "aws/model_delete_asset#1" do
-        assert Assets.delete_object("testFolder", "test4.png", %User{role: :staff}) ===
+        assert Assets.delete_object("testFolder", "test4.png") ===
                  {:error, {:not_found, "File not found"}}
       end
     end
 
-    @tag authenticate: :staff
     test "delete ok file" do
       use_cassette "aws/model_delete_asset#2" do
-        assert Assets.delete_object("testFolder", "test.png", %User{role: :staff}) === :ok
+        assert Assets.delete_object("testFolder", "test.png") === :ok
       end
     end
 
-    @tag authenticate: :staff
     test "upload existing file" do
       use_cassette "aws/model_upload_asset#1" do
         assert Assets.upload_to_s3(
                  build_upload("test/fixtures/upload.png"),
                  "testFolder",
-                 "test2.png",
-                 %User{role: :staff}
+                 "test2.png"
                ) ===
                  {:error, {:bad_request, "File already exists"}}
       end
     end
 
-    @tag authenticate: :staff
     test "upload ok file" do
       use_cassette "aws/model_upload_asset#2" do
         assert Assets.upload_to_s3(
                  build_upload("test/fixtures/upload.png"),
                  "testFolder",
-                 "test1.png",
-                 %User{role: :staff}
+                 "test1.png"
                ) ===
                  "https://source-academy-assets.s3.amazonaws.com/testFolder/test1.png"
       end

test/cadet_web/controllers/assets_controller_test.exs renamed to test/cadet_web/admin_controllers/assets_controller_test.exs

@@ -1,18 +1,18 @@
-defmodule CadetWeb.AssetsControllerTest do
+defmodule CadetWeb.AdminAssetsControllerTest do
   use CadetWeb.ConnCase
   use ExVCR.Mock, adapter: ExVCR.Adapter.Hackney
 
-  alias CadetWeb.AssetsController
+  alias CadetWeb.AdminAssetsController
 
   setup_all do
     HTTPoison.start()
   end
 
   test "swagger" do
-    AssetsController.swagger_definitions()
-    AssetsController.swagger_path_index(nil)
-    AssetsController.swagger_path_upload(nil)
-    AssetsController.swagger_path_delete(nil)
+    AdminAssetsController.swagger_definitions()
+    AdminAssetsController.swagger_path_index(nil)
+    AdminAssetsController.swagger_path_upload(nil)
+    AdminAssetsController.swagger_path_delete(nil)
   end
 
   describe "public access, unauthenticated" do
@@ -36,14 +36,14 @@ defmodule CadetWeb.AssetsControllerTest do
     @tag authenticate: :student
     test "GET /assets/:foldername", %{conn: conn} do
       conn = get(conn, build_url("testFolder"), %{})
-      assert response(conn, 403) =~ "User not allowed to manage assets"
+      assert response(conn, 403) =~ "Forbidden"
     end
 
     @tag authenticate: :student
     test "DELETE /assets/:foldername/*filename", %{conn: conn} do
       conn = delete(conn, build_url("testFolder/testFile.png"))
 
-      assert response(conn, 403) =~ "User not allowed to manage assets"
+      assert response(conn, 403) =~ "Forbidden"
     end
 
     @tag authenticate: :student
@@ -53,7 +53,7 @@ defmodule CadetWeb.AssetsControllerTest do
           :upload => build_upload("test/fixtures/upload.png")
         })
 
-      assert response(conn, 403) =~ "User not allowed to manage assets"
+      assert response(conn, 403) =~ "Forbidden"
     end
   end
 
@@ -170,7 +170,7 @@ defmodule CadetWeb.AssetsControllerTest do
     end
   end
 
-  defp build_url, do: "/v1/assets/"
+  defp build_url, do: "/v2/admin/assets/"
   defp build_url(url), do: "#{build_url()}/#{url}"
 
   defp build_upload(path, content_type \\ "image/png") do