From feaca0339edba2cb090c62917a154574c5bde5ed Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Wed, 25 Mar 2026 15:37:02 +0800 Subject: [PATCH 1/4] fix(ai): Create initial implementation --- lib/cadet/accounts/course_registrations.ex | 7 ++ lib/cadet/accounts/teams.ex | 9 ++ lib/cadet/assessments/assessments.ex | 63 +++++++++- .../admin_assessments_controller.ex | 25 ++-- .../admin_goals_controller.ex | 16 ++- .../admin_grading_controller.ex | 26 +++++ .../admin_teams_controller.ex | 23 ++-- .../admin_user_controller.ex | 10 +- .../controllers/generate_ai_comments.ex | 24 ++-- lib/cadet_web/plug/ensure_resource_scope.ex | 110 ++++++++++++++++++ 10 files changed, 266 insertions(+), 47 deletions(-) create mode 100644 lib/cadet_web/plug/ensure_resource_scope.ex diff --git a/lib/cadet/accounts/course_registrations.ex b/lib/cadet/accounts/course_registrations.ex index b87a72870..ff8623ec4 100644 --- a/lib/cadet/accounts/course_registrations.ex +++ b/lib/cadet/accounts/course_registrations.ex @@ -100,6 +100,13 @@ defmodule Cadet.Accounts.CourseRegistrations do |> Repo.all() end + def get_course_reg_in_course(course_reg_id, course_id) + when is_ecto_id(course_reg_id) and is_ecto_id(course_id) do + CourseRegistration + |> where(id: ^course_reg_id, course_id: ^course_id) + |> Repo.one() + end + def get_staffs(course_id) do CourseRegistration |> where(course_id: ^course_id) diff --git a/lib/cadet/accounts/teams.ex b/lib/cadet/accounts/teams.ex index 8cd0a64a8..82f2cdba1 100644 --- a/lib/cadet/accounts/teams.ex +++ b/lib/cadet/accounts/teams.ex @@ -39,6 +39,15 @@ defmodule Cadet.Accounts.Teams do teams end + def get_team_in_course(team_id, course_id) + when is_ecto_id(team_id) and is_ecto_id(course_id) do + Team + |> where(id: ^team_id) + |> join(:inner, [t], a in assoc(t, :assessment)) + |> where([_, a], a.course_id == ^course_id) + |> Repo.one() + end + @doc """ Creates a new team and assigns an assessment and team members to it. diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 426fc8914..b92e7a7c0 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -1248,6 +1248,23 @@ defmodule Cadet.Assessments do |> Repo.one() end + def get_assessment_in_course(assessment_id, course_id) + when is_ecto_id(assessment_id) and is_ecto_id(course_id) do + Assessment + |> where(id: ^assessment_id, course_id: ^course_id) + |> Repo.one() + end + + def get_question_in_course(question_id, course_id) + when is_ecto_id(question_id) and is_ecto_id(course_id) do + Question + |> where(id: ^question_id) + |> join(:inner, [q], a in assoc(q, :assessment)) + |> where([_, a], a.course_id == ^course_id) + |> preload([_, a], assessment: a) + |> Repo.one() + end + def delete_question(id) when is_ecto_id(id) do Logger.info("Deleting question #{id}") @@ -1405,6 +1422,16 @@ defmodule Cadet.Assessments do |> Repo.one() end + def get_submission_in_course(submission_id, course_id) + when is_ecto_id(submission_id) and is_ecto_id(course_id) do + Submission + |> where(id: ^submission_id) + |> join(:inner, [s], a in assoc(s, :assessment)) + |> where([_, a], a.course_id == ^course_id) + |> preload([_, a], assessment: a) + |> Repo.one() + end + def finalise_submission(submission = %Submission{}) do Logger.info( "Finalizing submission #{submission.id} for assessment #{submission.assessment_id}" @@ -3093,6 +3120,21 @@ defmodule Cadet.Assessments do end end + def get_answer_in_course(answer_id, course_id) + when is_ecto_id(answer_id) and is_ecto_id(course_id) do + answer_query = + Answer + |> where(id: ^answer_id) + |> join(:inner, [a], q in assoc(a, :question)) + |> join(:inner, [_, q], ast in assoc(q, :assessment)) + |> where([_, _, ast], ast.course_id == ^course_id) + + case Repo.exists?(answer_query) do + true -> get_answer(answer_id) + false -> {:error, {:forbidden, "Forbidden"}} + end + end + @spec get_answers_in_submission(integer() | String.t()) :: {:ok, {[Answer.t()], Assessment.t()}} | {:error, {:bad_request, String.t()}} @@ -3200,7 +3242,7 @@ defmodule Cadet.Assessments do def update_grading_info( %{submission_id: submission_id, question_id: question_id}, attrs, - cr = %CourseRegistration{id: grader_id} + cr = %CourseRegistration{id: grader_id, course_id: course_id} ) when is_ecto_id(submission_id) and is_ecto_id(question_id) do attrs = Map.put(attrs, "grader_id", grader_id) @@ -3213,7 +3255,9 @@ defmodule Cadet.Assessments do answer_query = answer_query |> join(:inner, [a], s in assoc(a, :submission)) - |> preload([_, s], submission: s) + |> join(:inner, [_, s], asst in assoc(s, :assessment)) + |> where([_, _, asst], asst.course_id == ^course_id) + |> preload([_, s, asst], submission: {s, assessment: asst}) answer = Repo.one(answer_query) @@ -3267,10 +3311,16 @@ defmodule Cadet.Assessments do {:ok, nil} | {:error, {:forbidden | :not_found, String.t()}} def force_regrade_submission( submission_id, - _requesting_user = %CourseRegistration{id: grader_id} + _requesting_user = %CourseRegistration{id: grader_id, course_id: course_id} ) when is_ecto_id(submission_id) do - with {:get, sub} when not is_nil(sub) <- {:get, Repo.get(Submission, submission_id)}, + submission_query = + Submission + |> where(id: ^submission_id) + |> join(:inner, [s], asst in assoc(s, :assessment)) + |> where([_, asst], asst.course_id == ^course_id) + + with {:get, sub} when not is_nil(sub) <- {:get, Repo.one(submission_query)}, {:status, true} <- {:status, sub.student_id == grader_id or sub.status == :submitted} do GradingJob.force_grade_individual_submission(sub, true) {:ok, nil} @@ -3296,12 +3346,15 @@ defmodule Cadet.Assessments do def force_regrade_answer( submission_id, question_id, - _requesting_user = %CourseRegistration{id: grader_id} + _requesting_user = %CourseRegistration{id: grader_id, course_id: course_id} ) when is_ecto_id(submission_id) and is_ecto_id(question_id) do answer = Answer |> where(submission_id: ^submission_id, question_id: ^question_id) + |> join(:inner, [a], q in assoc(a, :question)) + |> join(:inner, [_, q], asst in assoc(q, :assessment)) + |> where([_, _, asst], asst.course_id == ^course_id) |> preload([:question, :submission]) |> Repo.one() diff --git a/lib/cadet_web/admin_controllers/admin_assessments_controller.ex b/lib/cadet_web/admin_controllers/admin_assessments_controller.ex index 862ad7444..6310e03d3 100644 --- a/lib/cadet_web/admin_controllers/admin_assessments_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_assessments_controller.ex @@ -9,18 +9,29 @@ defmodule CadetWeb.AdminAssessmentsController do alias CadetWeb.AssessmentsHelpers alias Cadet.Assessments.{Question, Assessment} alias Cadet.{Assessments, Repo} - alias Cadet.Accounts.CourseRegistration - def index(conn, %{"course_reg_id" => course_reg_id}) do - course_reg = Repo.get(CourseRegistration, course_reg_id) + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :course_reg, param: "course_reg_id", assign: :target_course_reg] + when action in [:index, :get_assessment] + ) + + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :assessment, param: "assessmentid", assign: :scoped_assessment] + when action in [:update, :calculate_contest_score, :dispatch_contest_xp] + ) + + def index(conn, %{"course_reg_id" => _course_reg_id}) do + course_reg = conn.assigns.target_course_reg {:ok, assessments} = Assessments.all_assessments(course_reg) assessments = Assessments.format_all_assessments(assessments) render(conn, "index.json", assessments: assessments) end - def get_assessment(conn, %{"course_reg_id" => course_reg_id, "assessmentid" => assessment_id}) + def get_assessment(conn, %{"course_reg_id" => _course_reg_id, "assessmentid" => assessment_id}) when is_ecto_id(assessment_id) do - course_reg = Repo.get(CourseRegistration, course_reg_id) + course_reg = conn.assigns.target_course_reg case Assessments.assessment_with_questions_and_answers(assessment_id, course_reg) do {:ok, assessment} -> render(conn, "show.json", assessment: assessment) @@ -135,7 +146,7 @@ defmodule CadetWeb.AdminAssessmentsController do end end - def calculate_contest_score(conn, %{"assessmentid" => assessment_id, "course_id" => course_id}) do + def calculate_contest_score(conn, %{"assessmentid" => assessment_id, "course_id" => _course_id}) do voting_questions = Question |> where(type: :voting) @@ -150,7 +161,7 @@ defmodule CadetWeb.AdminAssessmentsController do end end - def dispatch_contest_xp(conn, %{"assessmentid" => assessment_id, "course_id" => course_id}) do + def dispatch_contest_xp(conn, %{"assessmentid" => assessment_id, "course_id" => _course_id}) do voting_questions = Question |> where(type: :voting) diff --git a/lib/cadet_web/admin_controllers/admin_goals_controller.ex b/lib/cadet_web/admin_controllers/admin_goals_controller.ex index 628540232..5ca9e6936 100644 --- a/lib/cadet_web/admin_controllers/admin_goals_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_goals_controller.ex @@ -4,16 +4,20 @@ defmodule CadetWeb.AdminGoalsController do use PhoenixSwagger alias Cadet.Incentives.Goals - alias Cadet.Accounts.CourseRegistration + + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :course_reg, param: "course_reg_id", assign: :target_course_reg] + when action in [:index_goals_with_progress, :update_progress] + ) def index(conn, _) do course_id = conn.assigns.course_reg.course_id render(conn, "index.json", goals: Goals.get(course_id)) end - def index_goals_with_progress(conn, %{"course_reg_id" => course_reg_id}) do - course_id = conn.assigns.course_reg.course_id - course_reg = %CourseRegistration{id: String.to_integer(course_reg_id), course_id: course_id} + def index_goals_with_progress(conn, %{"course_reg_id" => _course_reg_id}) do + course_reg = conn.assigns.target_course_reg render(conn, "index_goals_with_progress.json", goals: Goals.get_with_progress(course_reg)) end @@ -38,10 +42,10 @@ defmodule CadetWeb.AdminGoalsController do def update_progress(conn, %{ "uuid" => uuid, - "course_reg_id" => course_reg_id, + "course_reg_id" => _course_reg_id, "progress" => progress }) do - course_reg_id = String.to_integer(course_reg_id) + course_reg_id = conn.assigns.target_course_reg.id progress |> json_to_progress(uuid, course_reg_id) diff --git a/lib/cadet_web/admin_controllers/admin_grading_controller.ex b/lib/cadet_web/admin_controllers/admin_grading_controller.ex index 564988b9f..1c38859c2 100644 --- a/lib/cadet_web/admin_controllers/admin_grading_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_grading_controller.ex @@ -4,6 +4,32 @@ defmodule CadetWeb.AdminGradingController do alias Cadet.{Assessments, Courses} + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :submission, param: "submissionid", assign: :scoped_submission] + when action in [ + :show, + :update, + :unsubmit, + :unpublish_grades, + :publish_grades, + :autograde_submission, + :autograde_answer + ] + ) + + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :question, param: "questionid", assign: :scoped_question] + when action in [:update, :autograde_answer] + ) + + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :assessment, param: "assessmentid", assign: :scoped_assessment] + when action in [:publish_all_grades, :unpublish_all_grades] + ) + @doc """ # Query Parameters - `pageSize`: Integer. The number of submissions to return. Default 10. diff --git a/lib/cadet_web/admin_controllers/admin_teams_controller.ex b/lib/cadet_web/admin_controllers/admin_teams_controller.ex index 30349e772..c43389496 100644 --- a/lib/cadet_web/admin_controllers/admin_teams_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_teams_controller.ex @@ -5,6 +5,12 @@ defmodule CadetWeb.AdminTeamsController do alias Cadet.Accounts.{Teams, Team} + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :team, param: "teamid", assign: :scoped_team] + when action in [:update, :delete] + ) + def index(conn, %{"course_id" => course_id}) do teams = Teams.all_teams_for_course(course_id) @@ -47,14 +53,9 @@ defmodule CadetWeb.AdminTeamsController do end end - def update(conn, %{ - "teamId" => teamId, - "assessmentId" => assessmentId, - "student_ids" => student_ids - }) do + def update(conn, %{"assessmentId" => assessmentId, "student_ids" => student_ids}) do team = - Team - |> Repo.get!(teamId) + conn.assigns.scoped_team |> Repo.preload(assessment: [:config], team_members: [student: [:user]]) case Teams.update_team(team, assessmentId, student_ids) do @@ -70,8 +71,8 @@ defmodule CadetWeb.AdminTeamsController do end end - def delete(conn, %{"teamId" => team_id}) do - team = Repo.get(Team, team_id) + def delete(conn, %{"teamid" => _team_id}) do + team = conn.assigns.scoped_team if team do case Teams.delete_team(team) do @@ -90,10 +91,6 @@ defmodule CadetWeb.AdminTeamsController do end end - def delete(conn, %{"course_id" => course_id, "teamid" => team_id}) do - delete(conn, %{"teamId" => team_id}) - end - swagger_path :index do get("/admin/teams") diff --git a/lib/cadet_web/admin_controllers/admin_user_controller.ex b/lib/cadet_web/admin_controllers/admin_user_controller.ex index 53add9133..840d12820 100644 --- a/lib/cadet_web/admin_controllers/admin_user_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_user_controller.ex @@ -8,6 +8,12 @@ defmodule CadetWeb.AdminUserController do alias Cadet.{Accounts, Assessments, Courses} alias Cadet.Accounts.{CourseRegistrations, CourseRegistration, Role} + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :course_reg, param: "course_reg_id", assign: :target_course_reg] + when action in [:combined_total_xp] + ) + # This controller is used to find all users of a course def index(conn, filter) do @@ -17,8 +23,8 @@ defmodule CadetWeb.AdminUserController do render(conn, "users.json", users: users) end - def combined_total_xp(conn, %{"course_reg_id" => course_reg_id}) do - course_reg = Repo.get(CourseRegistration, course_reg_id) + def combined_total_xp(conn, %{"course_reg_id" => _course_reg_id}) do + course_reg = conn.assigns.target_course_reg course_id = course_reg.course_id user_id = course_reg.user_id diff --git a/lib/cadet_web/controllers/generate_ai_comments.ex b/lib/cadet_web/controllers/generate_ai_comments.ex index 24122dd4d..23d4b2f2f 100644 --- a/lib/cadet_web/controllers/generate_ai_comments.ex +++ b/lib/cadet_web/controllers/generate_ai_comments.ex @@ -7,6 +7,12 @@ defmodule CadetWeb.AICodeAnalysisController do alias Cadet.{Assessments, AIComments, Courses} alias CadetWeb.{AICodeAnalysisController, AICommentsHelpers} + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :answer, param: "answer_id", assign: :scoped_answer] + when action in [:generate_ai_comments, :save_final_comment] + ) + # For logging outputs to both database and file defp save_comment(answer_id, raw_prompt, answers_json, response, error \\ nil) do # Log to database @@ -82,8 +88,9 @@ defmodule CadetWeb.AICodeAnalysisController do "course_id" => course_id }) when is_ecto_id(answer_id) do - with {answer_id_parsed, ""} <- Integer.parse(answer_id), - {:ok, course} <- Courses.get_course_config(course_id), + answer = conn.assigns.scoped_answer + + with {:ok, course} <- Courses.get_course_config(course_id), {:ok} <- ensure_llm_enabled(course), {:ok, key} <- AICommentsHelpers.decrypt_llm_api_key(course.llm_api_key), {:ok} <- @@ -92,8 +99,7 @@ defmodule CadetWeb.AICodeAnalysisController do course.llm_model, course.llm_api_url, course.llm_course_level_prompt - ), - {:ok, answer} <- Assessments.get_answer(answer_id_parsed) do + ) do # Get head of answers (should only be one answer for given submission # and question since we filter to only 1 question) analyze_code( @@ -108,11 +114,6 @@ defmodule CadetWeb.AICodeAnalysisController do } ) else - :error -> - conn - |> put_status(:bad_request) - |> text("Invalid question ID format") - {:decrypt_error, err} -> conn |> put_status(:internal_server_error) @@ -123,11 +124,6 @@ defmodule CadetWeb.AICodeAnalysisController do conn |> put_status(:bad_request) |> text(error_msg) - - {:error, {status, message}} -> - conn - |> put_status(status) - |> text(message) end end diff --git a/lib/cadet_web/plug/ensure_resource_scope.ex b/lib/cadet_web/plug/ensure_resource_scope.ex new file mode 100644 index 000000000..82c234ba9 --- /dev/null +++ b/lib/cadet_web/plug/ensure_resource_scope.ex @@ -0,0 +1,110 @@ +defmodule CadetWeb.Plug.EnsureResourceScope do + @moduledoc """ + Plug that validates route resource IDs against the current course scope. + + Options: + - :resource (required) - one of :assessment, :question, :submission, :answer, :team, :course_reg + - :param (required) - request param name containing the resource ID + - :assign (optional) - conn.assigns key to store the loaded resource + """ + import Plug.Conn + + alias Cadet.Accounts.{CourseRegistrations, Teams} + alias Cadet.Assessments + + @type resource_type :: :assessment | :question | :submission | :answer | :team | :course_reg + + def init(opts), do: opts + + def call(conn, opts) do + resource = Keyword.fetch!(opts, :resource) + param = Keyword.fetch!(opts, :param) + assign_key = Keyword.get(opts, :assign, resource) + + with {:ok, course_id} <- fetch_current_course_id(conn), + {:ok, resource_id} <- fetch_resource_id(conn.params, param), + {:ok, record} <- resolve(resource, resource_id, course_id) do + assign(conn, assign_key, record) + else + {:error, :bad_request} -> + conn + |> put_status(:bad_request) + |> send_resp(:bad_request, "Missing or invalid parameter(s)") + |> halt() + + {:error, :forbidden} -> + conn + |> put_status(:forbidden) + |> send_resp(:forbidden, "Forbidden") + |> halt() + end + end + + defp fetch_current_course_id(conn) do + case conn.assigns[:course_reg] do + %{course_id: course_id} -> {:ok, course_id} + _ -> {:error, :forbidden} + end + end + + defp fetch_resource_id(params, param) do + case Map.get(params, param) do + nil -> + {:error, :bad_request} + + id when is_integer(id) and id > 0 -> + {:ok, id} + + id when is_binary(id) -> + case Integer.parse(id) do + {parsed_id, ""} when parsed_id > 0 -> {:ok, parsed_id} + _ -> {:error, :bad_request} + end + + _ -> + {:error, :bad_request} + end + end + + defp resolve(:assessment, resource_id, course_id) do + case Assessments.get_assessment_in_course(resource_id, course_id) do + nil -> {:error, :forbidden} + assessment -> {:ok, assessment} + end + end + + defp resolve(:question, resource_id, course_id) do + case Assessments.get_question_in_course(resource_id, course_id) do + nil -> {:error, :forbidden} + question -> {:ok, question} + end + end + + defp resolve(:submission, resource_id, course_id) do + case Assessments.get_submission_in_course(resource_id, course_id) do + nil -> {:error, :forbidden} + submission -> {:ok, submission} + end + end + + defp resolve(:answer, resource_id, course_id) do + case Assessments.get_answer_in_course(resource_id, course_id) do + {:ok, answer} -> {:ok, answer} + {:error, _} -> {:error, :forbidden} + end + end + + defp resolve(:team, resource_id, course_id) do + case Teams.get_team_in_course(resource_id, course_id) do + nil -> {:error, :forbidden} + team -> {:ok, team} + end + end + + defp resolve(:course_reg, resource_id, course_id) do + case CourseRegistrations.get_course_reg_in_course(resource_id, course_id) do + nil -> {:error, :forbidden} + course_reg -> {:ok, course_reg} + end + end +end From e8609291aa34098acf5c25a6986d55701dff6e36 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Wed, 25 Mar 2026 15:46:51 +0800 Subject: [PATCH 2/4] fix(ai): Improve implementation and add tests --- .../admin_user_controller.ex | 45 ++++++------------- .../controllers/answer_controller.ex | 26 +++++------ .../controllers/assessments_controller.ex | 18 +++++++- lib/cadet_web/controllers/team_controller.ex | 7 +++ .../controllers/answer_controller_test.exs | 16 ++++++- .../assessments_controller_test.exs | 16 ++++++- .../controllers/teams_controller_test.exs | 11 +++++ 7 files changed, 86 insertions(+), 53 deletions(-) diff --git a/lib/cadet_web/admin_controllers/admin_user_controller.ex b/lib/cadet_web/admin_controllers/admin_user_controller.ex index 840d12820..dc6ad16c5 100644 --- a/lib/cadet_web/admin_controllers/admin_user_controller.ex +++ b/lib/cadet_web/admin_controllers/admin_user_controller.ex @@ -2,16 +2,14 @@ defmodule CadetWeb.AdminUserController do use CadetWeb, :controller use PhoenixSwagger - import Ecto.Query - alias Cadet.Repo alias Cadet.{Accounts, Assessments, Courses} - alias Cadet.Accounts.{CourseRegistrations, CourseRegistration, Role} + alias Cadet.Accounts.{CourseRegistrations, Role} plug( CadetWeb.Plug.EnsureResourceScope, [resource: :course_reg, param: "course_reg_id", assign: :target_course_reg] - when action in [:combined_total_xp] + when action in [:combined_total_xp, :update_role, :delete_user] ) # This controller is used to find all users of a course @@ -113,18 +111,15 @@ defmodule CadetWeb.AdminUserController do end @update_role_roles ~w(admin)a - def update_role(conn, %{"role" => role, "course_reg_id" => course_reg_id}) do - course_reg_id = course_reg_id |> String.to_integer() + def update_role(conn, %{"role" => role, "course_reg_id" => _course_reg_id}) do + target_course_reg = conn.assigns.target_course_reg + course_reg_id = target_course_reg.id - %{id: admin_course_reg_id, role: admin_role, course_id: admin_course_id} = + %{id: admin_course_reg_id, role: admin_role} = conn.assigns.course_reg with {:validate_role, true} <- {:validate_role, admin_role in @update_role_roles}, - {:validate_not_self, true} <- {:validate_not_self, admin_course_reg_id != course_reg_id}, - {:get_cr, user_course_reg} when not is_nil(user_course_reg) <- - {:get_cr, CourseRegistration |> where(id: ^course_reg_id) |> Repo.one()}, - {:validate_same_course, true} <- - {:validate_same_course, user_course_reg.course_id == admin_course_id} do + {:validate_not_self, true} <- {:validate_not_self, admin_course_reg_id != course_reg_id} do case CourseRegistrations.update_role(role, course_reg_id) do {:ok, %{}} -> text(conn, "OK") @@ -140,29 +135,21 @@ defmodule CadetWeb.AdminUserController do {:validate_not_self, false} -> conn |> put_status(:bad_request) |> text("Admin not allowed to downgrade own role") - - {:get_cr, _} -> - conn |> put_status(:bad_request) |> text("User course registration does not exist") - - {:validate_same_course, false} -> - conn |> put_status(:forbidden) |> text("User is in a different course") end end @delete_user_roles ~w(admin)a - def delete_user(conn, %{"course_reg_id" => course_reg_id}) do - course_reg_id = course_reg_id |> String.to_integer() + def delete_user(conn, %{"course_reg_id" => _course_reg_id}) do + target_course_reg = conn.assigns.target_course_reg + course_reg_id = target_course_reg.id - %{id: admin_course_reg_id, role: admin_role, course_id: admin_course_id} = + %{id: admin_course_reg_id, role: admin_role} = conn.assigns.course_reg with {:validate_role, true} <- {:validate_role, admin_role in @delete_user_roles}, {:validate_not_self, true} <- {:validate_not_self, admin_course_reg_id != course_reg_id}, - {:get_cr, user_course_reg} when not is_nil(user_course_reg) <- - {:get_cr, CourseRegistration |> where(id: ^course_reg_id) |> Repo.one()}, - {:prevent_delete_admin, true} <- {:prevent_delete_admin, user_course_reg.role != :admin}, - {:validate_same_course, true} <- - {:validate_same_course, user_course_reg.course_id == admin_course_id} do + {:prevent_delete_admin, true} <- + {:prevent_delete_admin, target_course_reg.role != :admin} do case CourseRegistrations.delete_course_registration(course_reg_id) do {:ok, %{}} -> text(conn, "OK") @@ -181,14 +168,8 @@ defmodule CadetWeb.AdminUserController do |> put_status(:bad_request) |> text("Admin not allowed to delete ownself from course") - {:get_cr, _} -> - conn |> put_status(:bad_request) |> text("User course registration does not exist") - {:prevent_delete_admin, false} -> conn |> put_status(:bad_request) |> text("Admins cannot be deleted") - - {:validate_same_course, false} -> - conn |> put_status(:forbidden) |> text("User is in a different course") end end diff --git a/lib/cadet_web/controllers/answer_controller.ex b/lib/cadet_web/controllers/answer_controller.ex index c4c99f03f..6f26849d8 100644 --- a/lib/cadet_web/controllers/answer_controller.ex +++ b/lib/cadet_web/controllers/answer_controller.ex @@ -5,6 +5,12 @@ defmodule CadetWeb.AnswerController do alias Cadet.Assessments + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :question, param: "questionid", assign: :scoped_question] + when action in [:submit, :check_last_modified] + ) + # These roles can save and finalise answers for # closed assessments and submitted answers @bypass_closed_roles ~w(staff admin)a @@ -13,19 +19,13 @@ defmodule CadetWeb.AnswerController do when is_ecto_id(question_id) do course_reg = conn.assigns[:course_reg] can_bypass? = course_reg.role in @bypass_closed_roles + question = conn.assigns.scoped_question - with {:question, question} when not is_nil(question) <- - {:question, Assessments.get_question(question_id)}, - {:is_open?, true} <- + with {:is_open?, true} <- {:is_open?, can_bypass? or Assessments.is_open?(question.assessment)}, {:ok, _nil} <- Assessments.answer_question(question, course_reg, answer, can_bypass?) do text(conn, "OK") else - {:question, nil} -> - conn - |> put_status(:not_found) - |> text("Question not found") - {:is_open?, false} -> conn |> put_status(:forbidden) @@ -49,10 +49,9 @@ defmodule CadetWeb.AnswerController do when is_ecto_id(question_id) do course_reg = conn.assigns[:course_reg] can_bypass? = course_reg.role in @bypass_closed_roles + question = conn.assigns.scoped_question - with {:question, question} when not is_nil(question) <- - {:question, Assessments.get_question(question_id)}, - {:is_open?, true} <- + with {:is_open?, true} <- {:is_open?, can_bypass? or Assessments.is_open?(question.assessment)}, {:ok, last_modified} <- Assessments.has_last_modified_answer?( @@ -66,11 +65,6 @@ defmodule CadetWeb.AnswerController do |> put_resp_content_type("application/json") |> render("lastModified.json", lastModified: last_modified) else - {:question, nil} -> - conn - |> put_status(:not_found) - |> text("Question not found") - {:is_open?, false} -> conn |> put_status(:forbidden) diff --git a/lib/cadet_web/controllers/assessments_controller.ex b/lib/cadet_web/controllers/assessments_controller.ex index 76e746dfe..161e86a70 100644 --- a/lib/cadet_web/controllers/assessments_controller.ex +++ b/lib/cadet_web/controllers/assessments_controller.ex @@ -7,6 +7,18 @@ defmodule CadetWeb.AssessmentsController do alias Cadet.{Assessments, Repo} alias CadetWeb.AssessmentsHelpers + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :assessment, param: "assessmentid", assign: :scoped_assessment] + when action in [ + :show, + :unlock, + :submit, + :contest_popular_leaderboard, + :contest_score_leaderboard + ] + ) + # These roles can save and finalise answers for closed assessments and # submitted answers @bypass_closed_roles ~w(staff admin)a @@ -124,7 +136,8 @@ defmodule CadetWeb.AssessmentsController do "Fetching contest score leaderboard for assessment #{assessment_id} in course #{course_id}" ) - case {:voting_question, Assessments.get_contest_voting_question(assessment_id)} do + case {:voting_question, + Assessments.get_contest_voting_question(conn.assigns.scoped_assessment.id)} do {:voting_question, voting_question} when not is_nil(voting_question) -> question_id = Assessments.fetch_associated_contest_question_id(course_id, voting_question) @@ -165,7 +178,8 @@ defmodule CadetWeb.AssessmentsController do "Fetching contest popular leaderboard for assessment #{assessment_id} in course #{course_id}" ) - case {:voting_question, Assessments.get_contest_voting_question(assessment_id)} do + case {:voting_question, + Assessments.get_contest_voting_question(conn.assigns.scoped_assessment.id)} do {:voting_question, voting_question} when not is_nil(voting_question) -> question_id = Assessments.fetch_associated_contest_question_id(course_id, voting_question) diff --git a/lib/cadet_web/controllers/team_controller.ex b/lib/cadet_web/controllers/team_controller.ex index 476790055..33f7973c8 100644 --- a/lib/cadet_web/controllers/team_controller.ex +++ b/lib/cadet_web/controllers/team_controller.ex @@ -11,8 +11,15 @@ defmodule CadetWeb.TeamController do alias Cadet.Repo alias Cadet.Accounts.Team + plug( + CadetWeb.Plug.EnsureResourceScope, + [resource: :assessment, param: "assessmentid", assign: :scoped_assessment] + when action in [:index] + ) + def index(conn, %{"assessmentid" => assessment_id}) when is_ecto_id(assessment_id) do cr = conn.assigns.course_reg + assessment_id = conn.assigns.scoped_assessment.id query = from(t in Team, diff --git a/test/cadet_web/controllers/answer_controller_test.exs b/test/cadet_web/controllers/answer_controller_test.exs index 3117b230e..0a351b9c5 100644 --- a/test/cadet_web/controllers/answer_controller_test.exs +++ b/test/cadet_web/controllers/answer_controller_test.exs @@ -264,6 +264,18 @@ defmodule CadetWeb.AnswerControllerTest do end end + @tag authenticate: :student + test "forbidden when question belongs to another course", %{conn: conn} do + course_id = conn.assigns.course_id + other_course = insert(:course) + other_assessment = insert(:assessment, %{course: other_course}) + other_question = insert(:mcq_question, %{assessment: other_assessment}) + + conn = post(conn, build_url(course_id, other_question.id), %{answer: 5}) + + assert response(conn, 403) == "Forbidden" + end + @tag authenticate: :student test "invalid params missing question is unsuccessful", %{ conn: conn, @@ -275,7 +287,7 @@ defmodule CadetWeb.AnswerControllerTest do {:ok, _} = Repo.delete(mcq_question) conn = post(conn, build_url(course_id, mcq_question.id), %{answer: 5}) - assert response(conn, 404) == "Question not found" + assert response(conn, 403) == "Forbidden" assert is_nil(get_answer_value(mcq_question, assessment, course_reg)) end @@ -413,7 +425,7 @@ defmodule CadetWeb.AnswerControllerTest do } ) - assert response(check_last_modified_conn, 404) == "Question not found" + assert response(check_last_modified_conn, 403) == "Forbidden" end @tag authenticate: :student diff --git a/test/cadet_web/controllers/assessments_controller_test.exs b/test/cadet_web/controllers/assessments_controller_test.exs index 17e7cca72..8759665ac 100644 --- a/test/cadet_web/controllers/assessments_controller_test.exs +++ b/test/cadet_web/controllers/assessments_controller_test.exs @@ -1599,7 +1599,7 @@ defmodule CadetWeb.AssessmentsControllerTest do |> sign_in(student.user) |> post(build_url_submit(course2.id, assessment.id)) - assert response(conn, 404) == "Submission not found" + assert response(conn, 403) == "Forbidden" end test "forbidden if not in course", %{ @@ -1620,6 +1620,20 @@ defmodule CadetWeb.AssessmentsControllerTest do end end + describe "GET /:assessment_id, scoped access" do + @tag authenticate: :student + test "forbidden when assessment belongs to another course", %{conn: conn} do + course_id = conn.assigns.course_id + other_course = insert(:course) + other_config = insert(:assessment_config, %{course: other_course}) + other_assessment = insert(:assessment, %{course: other_course, config: other_config}) + + conn = get(conn, build_url(course_id, other_assessment.id)) + + assert response(conn, 403) == "Forbidden" + end + end + test "graded count is updated when assessment is graded", %{ conn: conn, courses: %{course1: course1}, diff --git a/test/cadet_web/controllers/teams_controller_test.exs b/test/cadet_web/controllers/teams_controller_test.exs index e67324adb..4d974e42d 100644 --- a/test/cadet_web/controllers/teams_controller_test.exs +++ b/test/cadet_web/controllers/teams_controller_test.exs @@ -52,6 +52,17 @@ defmodule CadetWeb.TeamsControllerTest do end describe "GET /v2/courses/:course_id/team/:assessment_id" do + @tag authenticate: :admin + test "forbidden when assessment belongs to another course", %{conn: conn} do + course_id = conn.assigns[:course_id] + other_course = insert(:course) + other_assessment = insert(:assessment, %{course: other_course}) + + conn = get(conn, build_url_get_by_assessment(course_id, other_assessment.id)) + + assert response(conn, 403) == "Forbidden" + end + @tag authenticate: :admin test "team not found", %{conn: conn} do course_id = conn.assigns[:course_id] From 47842a6caf5e7005e120b62c15e12b5d781b0514 Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Wed, 25 Mar 2026 15:54:44 +0800 Subject: [PATCH 3/4] fix(ai): Fix tests --- .../controllers/answer_controller_test.exs | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/test/cadet_web/controllers/answer_controller_test.exs b/test/cadet_web/controllers/answer_controller_test.exs index 0a351b9c5..812534dcb 100644 --- a/test/cadet_web/controllers/answer_controller_test.exs +++ b/test/cadet_web/controllers/answer_controller_test.exs @@ -4,6 +4,7 @@ defmodule CadetWeb.AnswerControllerTest do import Ecto.Query alias Cadet.Assessments.{Answer, Submission, SubmissionVotes} + alias Cadet.Courses.Course alias Cadet.Repo alias CadetWeb.AnswerController @@ -12,8 +13,13 @@ defmodule CadetWeb.AnswerControllerTest do AnswerController.swagger_path_submit(nil) end - setup do - course = insert(:course) + setup %{conn: conn} do + course = + case conn.assigns[:course_id] do + nil -> insert(:course) + course_id -> Repo.get(Course, course_id) + end + config = insert(:assessment_config, %{course: course}) assessment = insert(:assessment, %{is_published: true, course: course, config: config}) mcq_question = insert(:mcq_question, %{assessment: assessment}) @@ -295,11 +301,13 @@ defmodule CadetWeb.AnswerControllerTest do test "invalid params not open submission is unsuccessful", %{conn: conn} do course_reg = conn.assigns.test_cr course_id = conn.assigns.course_id + course = Repo.get(Course, course_id) before_open_at_assessment = insert(:assessment, %{ open_at: Timex.shift(Timex.now(), days: 5), - close_at: Timex.shift(Timex.now(), days: 10) + close_at: Timex.shift(Timex.now(), days: 10), + course: course }) before_open_at_question = insert(:mcq_question, %{assessment: before_open_at_assessment}) @@ -316,7 +324,8 @@ defmodule CadetWeb.AnswerControllerTest do after_close_at_assessment = insert(:assessment, %{ open_at: Timex.shift(Timex.now(), days: -10), - close_at: Timex.shift(Timex.now(), days: -5) + close_at: Timex.shift(Timex.now(), days: -5), + course: course }) after_close_at_question = insert(:mcq_question, %{assessment: after_close_at_assessment}) @@ -330,7 +339,7 @@ defmodule CadetWeb.AnswerControllerTest do get_answer_value(after_close_at_question, after_close_at_assessment, course_reg) ) - unpublished_assessment = insert(:assessment, %{is_published: false}) + unpublished_assessment = insert(:assessment, %{is_published: false, course: course}) unpublished_question = insert(:mcq_question, %{assessment: unpublished_assessment}) @@ -425,18 +434,20 @@ defmodule CadetWeb.AnswerControllerTest do } ) - assert response(check_last_modified_conn, 403) == "Forbidden" + assert response(check_last_modified_conn, 400) == "Missing or invalid parameter(s)" end @tag authenticate: :student test "check last modified, not open submission is unsuccessful", %{conn: conn} do course_id = conn.assigns.course_id + course = Repo.get(Course, course_id) last_modified_at = DateTime.to_iso8601(DateTime.utc_now()) before_open_at_assessment = insert(:assessment, %{ open_at: Timex.shift(Timex.now(), days: 5), - close_at: Timex.shift(Timex.now(), days: 10) + close_at: Timex.shift(Timex.now(), days: 10), + course: course }) before_open_at_question = From 0e30ba28c3839eb77a807df0ee5e96a0114aa70d Mon Sep 17 00:00:00 2001 From: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Date: Wed, 25 Mar 2026 16:09:33 +0800 Subject: [PATCH 4/4] fix(ai): Fix remaining tests --- .../admin_grading_controller_test.exs | 16 ++++++++-------- .../admin_teams_controller_test.exs | 6 ++++-- .../admin_user_controller_test.exs | 8 ++++---- .../ai_code_analysis_controller_test.exs | 3 ++- 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs index 6554b669b..6981409b7 100644 --- a/test/cadet_web/admin_controllers/admin_grading_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_grading_controller_test.exs @@ -452,7 +452,7 @@ defmodule CadetWeb.AdminGradingControllerTest do course_id = conn.assigns[:course_id] conn = get(conn, build_url(course_id, 1)) - assert response(conn, 400) == "Submission is not found." + assert response(conn, 403) == "Forbidden" end end @@ -499,7 +499,7 @@ defmodule CadetWeb.AdminGradingControllerTest do test "missing parameter", %{conn: conn} do course_id = conn.assigns.course_id conn = post(conn, build_url(course_id, 1, 3), %{}) - assert response(conn, 400) =~ "Missing parameter" + assert response(conn, 403) == "Forbidden" end @tag authenticate: :staff @@ -1486,7 +1486,7 @@ defmodule CadetWeb.AdminGradingControllerTest do course_id = conn.assigns[:course_id] conn = get(conn, build_url(course_id, 1)) - assert response(conn, 400) == "Submission is not found." + assert response(conn, 403) == "Forbidden" end end @@ -1510,7 +1510,7 @@ defmodule CadetWeb.AdminGradingControllerTest do test "missing parameter", %{conn: conn} do course_id = conn.assigns.course_id conn = post(conn, build_url(course_id, 1, 3), %{}) - assert response(conn, 400) =~ "Missing parameter" + assert response(conn, 403) == "Forbidden" end end @@ -1613,8 +1613,8 @@ defmodule CadetWeb.AdminGradingControllerTest do @tag authenticate: :staff test "fails if not found", %{conn: conn, course: course} do - assert conn |> post(build_url_autograde(course.id, 2_147_483_647)) |> response(404) == - "Submission not found" + assert conn |> post(build_url_autograde(course.id, 2_147_483_647)) |> response(403) == + "Forbidden" end end @@ -1661,8 +1661,8 @@ defmodule CadetWeb.AdminGradingControllerTest do @tag authenticate: :staff test "fails if not found", %{conn: conn, course: course} do - assert conn |> post(build_url_autograde(course.id, 2_147_483_647, 123_456)) |> response(404) == - "Answer not found" + assert conn |> post(build_url_autograde(course.id, 2_147_483_647, 123_456)) |> response(403) == + "Forbidden" end end diff --git a/test/cadet_web/admin_controllers/admin_teams_controller_test.exs b/test/cadet_web/admin_controllers/admin_teams_controller_test.exs index 55f68a8af..6c59a148c 100644 --- a/test/cadet_web/admin_controllers/admin_teams_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_teams_controller_test.exs @@ -266,7 +266,9 @@ defmodule CadetWeb.AdminTeamsControllerTest do @tag authenticate: :staff test "deletes a team", %{conn: conn} do course_id = conn.assigns.course_id - team = insert(:team) + course = Repo.get(Course, course_id) + assessment = insert(:assessment, %{course: course}) + team = insert(:team, %{assessment: assessment}) conn = delete(conn, build_url(course_id, team.id)) assert response(conn, 200) =~ "Team deleted successfully." end @@ -275,7 +277,7 @@ defmodule CadetWeb.AdminTeamsControllerTest do test "delete a team that does not exist", %{conn: conn} do course_id = conn.assigns.course_id conn = delete(conn, build_url(course_id, -1)) - assert response(conn, 404) =~ "Team not found!" + assert response(conn, 400) == "Missing or invalid parameter(s)" end @tag authenticate: :staff diff --git a/test/cadet_web/admin_controllers/admin_user_controller_test.exs b/test/cadet_web/admin_controllers/admin_user_controller_test.exs index 7b5a15393..41bfb4985 100644 --- a/test/cadet_web/admin_controllers/admin_user_controller_test.exs +++ b/test/cadet_web/admin_controllers/admin_user_controller_test.exs @@ -419,7 +419,7 @@ defmodule CadetWeb.AdminUserControllerTest do conn = put(conn, build_url_users_role(course_id, 10_000), params) - assert response(conn, 400) == "User course registration does not exist" + assert response(conn, 403) == "Forbidden" end @tag authenticate: :admin @@ -433,7 +433,7 @@ defmodule CadetWeb.AdminUserControllerTest do conn = put(conn, build_url_users_role(course_id, user_course_reg.id), params) - assert response(conn, 403) == "User is in a different course" + assert response(conn, 403) == "Forbidden" unchanged_course_reg = Repo.get(CourseRegistration, user_course_reg.id) assert unchanged_course_reg.role == :student end @@ -537,7 +537,7 @@ defmodule CadetWeb.AdminUserControllerTest do conn = delete(conn, build_url_users(course_id, 1)) - assert response(conn, 400) == "User course registration does not exist" + assert response(conn, 403) == "Forbidden" end @tag authenticate: :admin @@ -562,7 +562,7 @@ defmodule CadetWeb.AdminUserControllerTest do conn = delete(conn, build_url_users(course_id, user_course_reg.id)) - assert response(conn, 403) == "User is in a different course" + assert response(conn, 403) == "Forbidden" end end diff --git a/test/cadet_web/controllers/ai_code_analysis_controller_test.exs b/test/cadet_web/controllers/ai_code_analysis_controller_test.exs index c6e1ca3a4..009e42737 100644 --- a/test/cadet_web/controllers/ai_code_analysis_controller_test.exs +++ b/test/cadet_web/controllers/ai_code_analysis_controller_test.exs @@ -101,7 +101,8 @@ defmodule CadetWeb.AICodeAnalysisControllerTest do conn |> sign_in(admin_user.user) |> post(build_url_generate_ai_comments(course_with_llm.id, random_answer_id)) - |> text_response(400) + + assert response(response, 403) == "Forbidden" end end