Transfer groundControl (and admin panel) from staff to admin route

lib/cadet_web/router.ex

@@ -28,6 +28,10 @@ defmodule CadetWeb.Router do
     plug(:ensure_role, [:staff, :admin])
   end
 
+  pipeline :ensure_admin do
+    plug(:ensure_role, [:admin])
+  end
+
   scope "/", CadetWeb do
     get("/.well-known/jwks.json", JWKSController, :index)
   end
@@ -119,11 +123,18 @@ defmodule CadetWeb.Router do
     get("/team/:assessmentid", TeamController, :index)
   end
 
-  # Admin pages
-  scope "/v2/courses/:course_id/admin", CadetWeb do
-    pipe_through([:api, :auth, :ensure_auth, :course, :ensure_staff])
+  # Admin pages (Access: Course administrators only - these routes can cause substantial damage)
+  @doc """
+    NOTE: This scope must come before the routes for all staff below.
 
-    resources("/sourcecast", AdminSourcecastController, only: [:create, :delete])
+    This is due to the all-staff route "/grading/:submissionid/:questionid", which would pattern match
+    and overshadow "/grading/:assessmentid/publish_all_grades".
+
+    If an admin route will overshadow an all-staff route as well, a suggested better solution would be a
+    per-route permission level check.
+  """
+  scope "/v2/courses/:course_id/admin", CadetWeb do
+    pipe_through([:api, :auth, :ensure_auth, :course, :ensure_admin])
 
     get("/assets/:foldername", AdminAssetsController, :index)
     post("/assets/:foldername/*filename", AdminAssetsController, :upload)
@@ -133,6 +144,39 @@ defmodule CadetWeb.Router do
     post("/assessments/:assessmentid", AdminAssessmentsController, :update)
     delete("/assessments/:assessmentid", AdminAssessmentsController, :delete)
 
+    post(
+      "/grading/:assessmentid/publish_all_grades",
+      AdminGradingController,
+      :publish_all_grades
+    )
+
+    post(
+      "/grading/:assessmentid/unpublish_all_grades",
+      AdminGradingController,
+      :unpublish_all_grades
+    )
+
+    put("/users/:course_reg_id/role", AdminUserController, :update_role)
+    delete("/users/:course_reg_id", AdminUserController, :delete_user)
+
+    put("/config", AdminCoursesController, :update_course_config)
+    # TODO: Missing corresponding Swagger path entry
+    get("/config/assessment_configs", AdminCoursesController, :get_assessment_configs)
+    put("/config/assessment_configs", AdminCoursesController, :update_assessment_configs)
+    # TODO: Missing corresponding Swagger path entry
+    delete(
+      "/config/assessment_config/:assessment_config_id",
+      AdminCoursesController,
+      :delete_assessment_config
+    )
+  end
+
+  # Admin pages (Access: All staff)
+  scope "/v2/courses/:course_id/admin", CadetWeb do
+    pipe_through([:api, :auth, :ensure_auth, :course, :ensure_staff])
+
+    resources("/sourcecast", AdminSourcecastController, only: [:create, :delete])
+
     get(
       "/assessments/:assessmentid/popularVoteLeaderboard",
       AdminAssessmentsController,
@@ -148,14 +192,6 @@ defmodule CadetWeb.Router do
     get("/grading", AdminGradingController, :index)
     get("/grading/summary", AdminGradingController, :grading_summary)
 
-    post("/grading/:assessmentid/publish_all_grades", AdminGradingController, :publish_all_grades)
-
-    post(
-      "/grading/:assessmentid/unpublish_all_grades",
-      AdminGradingController,
-      :unpublish_all_grades
-    )
-
     get("/grading/:submissionid", AdminGradingController, :show)
     post("/grading/:submissionid/unsubmit", AdminGradingController, :unsubmit)
     post("/grading/:submissionid/unpublish_grades", AdminGradingController, :unpublish_grades)
@@ -184,8 +220,6 @@ defmodule CadetWeb.Router do
 
     # The admin route for getting total xp of a specific user
     get("/users/:course_reg_id/total_xp", AdminUserController, :combined_total_xp)
-    put("/users/:course_reg_id/role", AdminUserController, :update_role)
-    delete("/users/:course_reg_id", AdminUserController, :delete_user)
     get("/users/:course_reg_id/goals", AdminGoalsController, :index_goals_with_progress)
     post("/users/:course_reg_id/goals/:uuid/progress", AdminGoalsController, :update_progress)
 
@@ -202,17 +236,6 @@ defmodule CadetWeb.Router do
     delete("/stories/:storyid", AdminStoriesController, :delete)
     post("/stories/:storyid", AdminStoriesController, :update)
 
-    put("/config", AdminCoursesController, :update_course_config)
-    # TODO: Missing corresponding Swagger path entry
-    get("/config/assessment_configs", AdminCoursesController, :get_assessment_configs)
-    put("/config/assessment_configs", AdminCoursesController, :update_assessment_configs)
-    # TODO: Missing corresponding Swagger path entry
-    delete(
-      "/config/assessment_config/:assessment_config_id",
-      AdminCoursesController,
-      :delete_assessment_config
-    )
-
     get("/teams", AdminTeamsController, :index)
     post("/teams", AdminTeamsController, :create)
     delete("/teams/:teamid", AdminTeamsController, :delete)

priv/repo/seeds.exs

@@ -26,9 +26,9 @@
 # Cadet.Repo.insert!(%Cadet.Settings.Sublanguage{chapter: 1, variant: "default"})
 
 if Cadet.Env.env() == :dev do
-  number_of_students = 10
+  number_of_students = 100_000
   number_of_assessments = 5
-  number_of_questions = 3
+  number_of_questions = 13
 
   # Course
   admin_course =
@@ -84,7 +84,7 @@
   # Users
   Enum.each(groups_and_courses, fn {group, course} ->
     students =
       for i <- 1..number_of_students do
         student = insert(:user, %{latest_viewed_course: course})
 
         student_cr =
@@ -118,7 +118,7 @@
         })
       end)
 
     for i <- 1..number_of_assessments do
       assessment =
         insert(:assessment, %{
           is_published: true,

test/cadet_web/admin_controllers/admin_assessments_controller_test.exs

@@ -5,7 +5,7 @@
   import Ecto.Query
   import ExUnit.CaptureLog
 
   alias Cadet.{Assessments, Repo}
   alias Cadet.Accounts.CourseRegistration
   alias Cadet.Assessments.{Assessment, Submission}
   alias Cadet.Test.XMLGenerator
@@ -370,8 +370,34 @@
     end
   end
 
-  describe "POST /, staff only" do
+  describe "POST /, non-admin staff only" do
     @tag authenticate: :staff
+    test "unauthorized", %{conn: conn} do
+      test_cr = conn.assigns.test_cr
+      course = test_cr.course
+      config = insert(:assessment_config, %{course: course})
+
+      assessment =
+        build(:assessment,
+          course: course,
+          course_id: course.id,
+          config: config,
+          config_id: config.id,
+          is_published: true
+        )
+
+      questions = build_list(5, :question, assessment: nil)
+
+      xml = XMLGenerator.generate_xml_for(assessment, questions)
+      force_update = "false"
+      body = %{assessment: xml, forceUpdate: force_update, assessmentConfigId: config.id}
+      conn = post(conn, build_url(course.id), body)
+      assert response(conn, 403) == "Forbidden"
+    end
+  end
+
+  describe "POST /, admin only" do
+    @tag authenticate: :admin
     test "successful", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -429,7 +455,7 @@
       assert expected_assessment != nil
     end
 
-    @tag authenticate: :staff
+    @tag authenticate: :admin
     test "upload empty xml", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -487,6 +513,18 @@
 
   describe "DELETE /:assessment_id, staff only" do
     @tag authenticate: :staff
+    test "unauthorized", %{conn: conn} do
+      test_cr = conn.assigns.test_cr
+      course = test_cr.course
+      config = insert(:assessment_config, %{course: course})
+      assessment = insert(:assessment, %{course: course, config: config})
+      conn = delete(conn, build_url(course.id, assessment.id))
+      assert response(conn, 403) == "Forbidden"
+    end
+  end
+
+  describe "DELETE /:assessment_id, admin only" do
+    @tag authenticate: :admin
     test "successful", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -497,7 +535,7 @@
       assert is_nil(Repo.get(Assessment, assessment.id))
     end
 
-    @tag authenticate: :staff
+    @tag authenticate: :admin
     test "error due to different course", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -509,19 +547,6 @@
       assert response(conn, 403) == "User not allow to delete assessments from another course"
       refute is_nil(Repo.get(Assessment, assessment.id))
     end
-
-    # @tag authenticate: :staff
-    # test "error due to different course", %{conn: conn} do
-    #   test_cr = conn.assigns.test_cr
-    #   course = test_cr.course
-    #   another_course = insert(:course)
-    #   config = insert(:assessment_config, %{course: another_course})
-    #   assessment = insert(:assessment, %{course: another_course, config: config})
-
-    #   conn = delete(conn, build_url(course.id, assessment.id))
-    #   assert response(conn, 403) == "User not allow to delete assessments from another course"
-    #   refute is_nil(Repo.get(Assessment, assessment.id))
-    # end
   end
 
   describe "POST /:assessment_id, unauthenticated, publish" do
@@ -544,8 +569,20 @@
     end
   end
 
-  describe "POST /:assessment_id, staff only, publish" do
+  describe "POST /:assessment_id, non-admin staff only, publish" do
     @tag authenticate: :staff
+    test "forbidden", %{conn: conn} do
+      test_cr = conn.assigns.test_cr
+      course = test_cr.course
+      config = insert(:assessment_config, %{course: course})
+      assessment = insert(:assessment, %{course: course, config: config})
+      conn = post(conn, build_url(course.id, assessment.id), %{isPublished: true})
+      assert response(conn, 403) == "Forbidden"
+    end
+  end
+
+  describe "POST /:assessment_id, admin only, publish" do
+    @tag authenticate: :admin
     test "successful toggle from published to unpublished", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -557,7 +594,7 @@
       refute expected
     end
 
-    @tag authenticate: :staff
+    @tag authenticate: :admin
     test "successful toggle from unpublished to published", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -608,8 +645,38 @@
     end
   end
 
-  describe "POST /:assessment_id, staff only" do
+  describe "POST /:assessment_id, non-admin staff only" do
     @tag authenticate: :staff
+    test "forbidden", %{conn: conn} do
+      test_cr = conn.assigns.test_cr
+      course = test_cr.course
+      config = insert(:assessment_config, %{course: course})
+      assessment = insert(:assessment, %{course: course, config: config})
+
+      new_open_at =
+        Timex.now()
+        |> Timex.beginning_of_day()
+        |> Timex.shift(days: 3)
+        |> Timex.shift(hours: 4)
+
+      new_open_at_string =
+        new_open_at
+        |> Timex.format!("{ISO:Extended}")
+
+      new_close_at = Timex.shift(new_open_at, days: 7)
+
+      new_close_at_string =
+        new_close_at
+        |> Timex.format!("{ISO:Extended}")
+
+      new_dates = %{openAt: new_open_at_string, closeAt: new_close_at_string}
+      conn = post(conn, build_url(course.id, assessment.id), new_dates)
+      assert response(conn, 403) == "Forbidden"
+    end
+  end
+
+  describe "POST /:assessment_id, admin only" do
+    @tag authenticate: :admin
     test "successful", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -658,7 +725,7 @@
       assert [assessment.open_at, assessment.close_at] == [new_open_at, new_close_at]
     end
 
-    @tag authenticate: :staff
+    @tag authenticate: :admin
     test "allowed to change open time of opened assessments", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -703,7 +770,7 @@
       assert [assessment.open_at, assessment.close_at] == [new_open_at, close_at]
     end
 
-    @tag authenticate: :staff
+    @tag authenticate: :admin
     test "not allowed to set close time to before open time", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -748,7 +815,7 @@
       assert [assessment.open_at, assessment.close_at] == [open_at, close_at]
     end
 
-    @tag authenticate: :staff
+    @tag authenticate: :admin
     test "successful, set close time to before current time", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -793,7 +860,7 @@
       assert [assessment.open_at, assessment.close_at] == [open_at, new_close_at]
     end
 
-    @tag authenticate: :staff
+    @tag authenticate: :admin
     test "successful, set open time to before current time", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -838,7 +905,7 @@
       assert [assessment.open_at, assessment.close_at] == [new_open_at, close_at]
     end
 
-    @tag authenticate: :staff
+    @tag authenticate: :admin
     test "successful, set hasTokenCounter and hasVotingFeatures to true", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course
@@ -873,7 +940,7 @@
              ]
     end
 
-    @tag authenticate: :staff
+    @tag authenticate: :admin
     test "successful, set hasTokenCounter and hasVotingFeatures to false", %{conn: conn} do
       test_cr = conn.assigns.test_cr
       course = test_cr.course