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

* Create a new staff scope

* Move Admin Panel requests into admin scope

* Change appropriate routes into admin scope

* Find-replace galore

* Fix linting

* Linting does not work :(

* Revert "Find-replace galore"

This reverts commit e77aa05.

* Revert "Change appropriate routes into admin scope"

This reverts commit 18dc689.

* Revert "Create a new staff scope"

This reverts commit 6b7e54e.

* Move dangerous routes into a new scope

* Fix linting

* Linting works in mysterious ways

* One more formatting change

* Swap order of all-staff and admin-only routes

This swap prevents the all-staff route,
"/grading/:submissionid/:questionid", from pattern
matching and overshadowing the admin-only route
"/grading/:assessmentid/publish_all_grades". Thankfully, no admin routes
overshadow staff routes, so a quick fix can be done here.

* Update error message for grading routes

* Update error messages for users

* Add test cases for assets for staff

Create test cases to indicate that non-admin staff can only read assets,
but not create, modify, or delete them.

* Update test auth to admin for assets

* Update and add tests for course config routes

Updates positive test auth from staff to admin, adds negative tests to
ensure that non-admin staff are unable to read, update, create, or
delete course configs.

* Update and add tests for assessment-level routes

Update the modification / deletion test auth from staff to admin, and
create tests to ensure that non-admin staff are not able to delete /
unpublish them

* Fix sourcecast error

* Revert "Fix sourcecast error"

This reverts commit 831ca60.

* Transfer asset routes to admin

* Revert accidental formatting changes

Author: josh1248

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 @@ alias Cadet.Accounts.{
 # 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 =

test/cadet_web/admin_controllers/admin_assessments_controller_test.exs

@@ -370,8 +370,34 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
     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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
       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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
 
   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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
       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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
       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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
     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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
       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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
     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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
       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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
       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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
       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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
       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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
       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 @@ defmodule CadetWeb.AdminAssessmentsControllerTest do
              ]
     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