fix story and sourcecast role checking (#798)

Co-authored-by: angelsl <angelsl@in04.sg>

Author: YaleChen299

lib/cadet/courses/courses.ex

@@ -308,88 +308,68 @@ defmodule Cadet.Courses do
     end
   end
 
-  @upload_file_roles ~w(admin staff)a
-
   @doc """
   Upload a sourcecast file.
 
-  Note that there are no checks for whether the user belongs to the course, as this has been checked
-  inside a plug in the router.
+  Note that there are no checks for whether the user belongs to the course,
+  as this has been checked inside a plug in the router.
   """
   def upload_sourcecast_file(
-        _inserter = %CourseRegistration{user_id: user_id, course_id: course_id, role: role},
+        _inserter = %CourseRegistration{user_id: user_id, course_id: course_id},
         attrs = %{}
       ) do
-    if role in @upload_file_roles do
-      course_reg =
-        CourseRegistration
-        |> where(user_id: ^user_id)
-        |> where(course_id: ^course_id)
-        |> preload(:course)
-        |> preload(:user)
-        |> Repo.one()
-
-      changeset =
-        %Sourcecast{}
-        |> Sourcecast.changeset(attrs)
-        |> put_assoc(:uploader, course_reg.user)
-        |> put_assoc(:course, course_reg.course)
-
-      case Repo.insert(changeset) do
-        {:ok, sourcecast} ->
-          {:ok, sourcecast}
-
-        {:error, changeset} ->
-          {:error, {:bad_request, full_error_messages(changeset)}}
-      end
-    else
-      {:error, {:forbidden, "User is not permitted to upload"}}
-    end
-  end
+    changeset =
+      Sourcecast.changeset(%Sourcecast{uploader_id: user_id, course_id: course_id}, attrs)
 
-  @doc """
-  Upload a public sourcecast file.
+    case Repo.insert(changeset) do
+      {:ok, sourcecast} ->
+        {:ok, sourcecast}
 
-  Note that there are no checks for whether the user belongs to the course, as this has been checked
-  inside a plug in the router.
-  """
-  def upload_sourcecast_file_public(
-        inserter,
-        _inserter_course_reg = %CourseRegistration{role: role},
-        attrs = %{}
-      ) do
-    if role in @upload_file_roles do
-      changeset =
-        %Sourcecast{}
-        |> Sourcecast.changeset(attrs)
-        |> put_assoc(:uploader, inserter)
-
-      case Repo.insert(changeset) do
-        {:ok, sourcecast} ->
-          {:ok, sourcecast}
-
-        {:error, changeset} ->
-          {:error, {:bad_request, full_error_messages(changeset)}}
-      end
-    else
-      {:error, {:forbidden, "User is not permitted to upload"}}
+      {:error, changeset} ->
+        {:error, {:bad_request, full_error_messages(changeset)}}
     end
   end
 
+  # @doc """
+  # Upload a public sourcecast file.
+
+  # Note that there are no checks for whether the user belongs to the course,
+  # as this has been checked inside a plug in the router.
+  # unused in the current version
+  # """
+  # def upload_sourcecast_file_public(
+  #       inserter,
+  #       _inserter_course_reg = %CourseRegistration{role: role},
+  #       attrs = %{}
+  #     ) do
+  #   if role in @upload_file_roles do
+  #     changeset =
+  #       %Sourcecast{}
+  #       |> Sourcecast.changeset(attrs)
+  #       |> put_assoc(:uploader, inserter)
+
+  #     case Repo.insert(changeset) do
+  #       {:ok, sourcecast} ->
+  #         {:ok, sourcecast}
+
+  #       {:error, changeset} ->
+  #         {:error, {:bad_request, full_error_messages(changeset)}}
+  #     end
+  #   else
+  #     {:error, {:forbidden, "User is not permitted to upload"}}
+  #   end
+  # end
+
   @doc """
   Delete a sourcecast file
 
   Note that there are no checks for whether the user belongs to the course, as this has been checked
   inside a plug in the router.
   """
-  def delete_sourcecast_file(_deleter = %CourseRegistration{role: role}, sourcecast_id) do
-    if role in @upload_file_roles do
-      sourcecast = Repo.get(Sourcecast, sourcecast_id)
-      SourcecastUpload.delete({sourcecast.audio, sourcecast})
-      Repo.delete(sourcecast)
-    else
-      {:error, {:forbidden, "User is not permitted to delete"}}
-    end
+  def delete_sourcecast_file(sourcecast_id) do
+    sourcecast = Repo.get(Sourcecast, sourcecast_id)
+    SourcecastUpload.delete({sourcecast.audio, sourcecast})
+    Repo.delete(sourcecast)
   end
 
   @doc """
@@ -402,13 +382,14 @@ defmodule Cadet.Courses do
     |> Repo.preload(:uploader)
   end
 
-  def get_sourcecast_files do
-    Sourcecast
-    # Public sourcecasts are those without course_id
-    |> where([s], is_nil(s.course_id))
-    |> Repo.all()
-    |> Repo.preload(:uploader)
-  end
+  # unused in the current version
+  # def get_sourcecast_files do
+  #   Sourcecast
+  #   # Public sourcecasts are those without course_id
+  #   |> where([s], is_nil(s.course_id))
+  #   |> Repo.all()
+  #   |> Repo.preload(:uploader)
+  # end
 
   @spec assets_prefix(%Course{}) :: binary()
   def assets_prefix(course) do

lib/cadet/stories/stories.ex

@@ -7,16 +7,10 @@ defmodule Cadet.Stories.Stories do
   import Ecto.Query
 
   alias Cadet.Repo
-  alias Cadet.Accounts.CourseRegistration
   alias Cadet.Stories.Story
-  alias Cadet.Courses.Course
 
-  @manage_stories_role ~w(staff admin)a
-
-  def list_stories(
-        _user_course_registration = %CourseRegistration{course_id: course_id, role: role}
-      ) do
-    if role in @manage_stories_role do
+  def list_stories(course_id, list_all) do
+    if list_all do
       Story
       |> where(course_id: ^course_id)
       |> Repo.all()
@@ -29,66 +23,39 @@ defmodule Cadet.Stories.Stories do
     end
   end
 
-  def create_story(
-        attrs = %{},
-        _user_course_registration = %CourseRegistration{course_id: course_id, role: role}
-      ) do
-    if role in @manage_stories_role do
-      course =
-        Course
-        |> where(id: ^course_id)
-        |> Repo.one()
-
-      %Story{}
-      |> Story.changeset(Map.put(attrs, :course_id, course.id))
-      |> Repo.insert()
-    else
-      {:error, {:forbidden, "User not allowed to manage stories"}}
-    end
+  def create_story(attrs = %{}, course_id) do
+    %Story{}
+    |> Story.changeset(Map.put(attrs, :course_id, course_id))
+    |> Repo.insert()
   end
 
-  def update_story(
-        attrs = %{},
-        id,
-        _user_course_registration = %CourseRegistration{course_id: course_id, role: role}
-      ) do
-    if role in @manage_stories_role do
-      case Repo.get(Story, id) do
-        nil ->
-          {:error, {:not_found, "Story not found"}}
-
-        story ->
-          if story.course_id == course_id do
-            story
-            |> Story.changeset(attrs)
-            |> Repo.update()
-          else
-            {:error, {:forbidden, "User not allowed to manage stories from another course"}}
-          end
-      end
-    else
-      {:error, {:forbidden, "User not allowed to manage stories"}}
+  def update_story(attrs = %{}, id, course_id) do
+    case Repo.get(Story, id) do
+      nil ->
+        {:error, {:not_found, "Story not found"}}
+
+      story ->
+        if story.course_id == course_id do
+          story
+          |> Story.changeset(attrs)
+          |> Repo.update()
+        else
+          {:error, {:forbidden, "User not allowed to manage stories from another course"}}
+        end
     end
   end
 
-  def delete_story(
-        id,
-        _user_course_registration = %CourseRegistration{course_id: course_id, role: role}
-      ) do
-    if role in @manage_stories_role do
-      case Repo.get(Story, id) do
-        nil ->
-          {:error, {:not_found, "Story not found"}}
-
-        story ->
-          if story.course_id == course_id do
-            Repo.delete(story)
-          else
-            {:error, {:forbidden, "User not allowed to manage stories from another course"}}
-          end
-      end
-    else
-      {:error, {:forbidden, "User not allowed to manage stories"}}
+  def delete_story(id, course_id) do
+    case Repo.get(Story, id) do
+      nil ->
+        {:error, {:not_found, "Story not found"}}
+
+      story ->
+        if story.course_id == course_id do
+          Repo.delete(story)
+        else
+          {:error, {:forbidden, "User not allowed to manage stories from another course"}}
+        end
     end
   end
 end

lib/cadet_web/admin_controllers/admin_sourcecast_controller.ex

@@ -0,0 +1,93 @@
+defmodule CadetWeb.AdminSourcecastController do
+  use CadetWeb, :controller
+  use PhoenixSwagger
+
+  alias Cadet.Courses
+
+  def create(conn, %{"sourcecast" => sourcecast}) do
+    result = Courses.upload_sourcecast_file(conn.assigns.course_reg, sourcecast)
+
+    case result do
+      {:ok, _nil} ->
+        send_resp(conn, 200, "OK")
+
+      {:error, {status, message}} ->
+        conn
+        |> put_status(status)
+        |> text(message)
+    end
+  end
+
+  def create(conn, _params) do
+    send_resp(conn, :bad_request, "Missing or invalid parameter(s)")
+  end
+
+  def delete(conn, %{"id" => id}) do
+    result = Courses.delete_sourcecast_file(id)
+
+    case result do
+      {:ok, _nil} ->
+        send_resp(conn, 200, "OK")
+
+      {:error, {status, message}} ->
+        conn
+        |> put_status(status)
+        |> text(message)
+    end
+  end
+
+  swagger_path :create do
+    post("/sourcecast")
+    description("Uploads sourcecast")
+    summary("Upload sourcecast")
+    consumes("multipart/form-data")
+    security([%{JWT: []}])
+
+    parameters do
+      public(
+        :body,
+        :boolean,
+        "Uploads as public sourcecast when 'public' is specified regardless of truthy or falsy"
+      )
+
+      sourcecast(:body, Schema.ref(:Sourcecast), "sourcecast object", required: true)
+    end
+
+    response(200, "Success")
+    response(400, "Invalid or missing parameter(s)")
+    response(401, "Unauthorised")
+  end
+
+  swagger_path :delete do
+    PhoenixSwagger.Path.delete("/sourcecast/{id}")
+    description("Deletes sourcecast by id")
+    summary("Delete sourcecast")
+    security([%{JWT: []}])
+
+    parameters do
+      id(:path, :integer, "sourcecast id", required: true)
+    end
+
+    response(200, "Success")
+    response(400, "Invalid or missing parameter(s)")
+    response(401, "Unauthorised")
+  end
+
+  def swagger_definitions do
+    %{
+      Sourcecast:
+        swagger_schema do
+          properties do
+            title(:string, "title", required: true)
+            playbackData(:string, "playback data", required: true)
+            description(:string, "description", required: false)
+            uid(:string, "uid", required: false)
+
+            # Note: this is technically an invalid type in Swagger/OpenAPI 2.0,
+            # but represents that a string or integer could be returned.
+            audio(:file, "audio file", required: true)
+          end
+        end
+    }
+  end
+end