[#12048] Abstract Access Controls to BaseActionTest

[InfinityTwo]

Part of #12048
Part of #13304
Surfaced from #13245

Outline of Solution
Abstract various access controls for testing to BaseActionTest, like the previous way, to simplify test code.

To see which ones have been refactored, see https://docs.google.com/spreadsheets/d/1IcB-viJMrIOknYoBYasBuRBK9qnwY0-doEK4_Eb24IA/edit?usp=sharing

Current plan is to implement about half of the refactoring. The other half will be left open in a new issue for new contributors to contribute as a good first issue as the changes are rather straightforward for most of them.

PS: I'm not sure why the first 10 commits are there even though I started from a clean master but they are not relevant to the PR.

[jasonqiu212]

thanks for this huge refactoring!! 🤩

High-level structural changes:

• Change private methods that are possible to be called by test cases to public. To avoid duplicate code and extensibility
• For code clarity, we can still preserve the high-level and mid-level groups, as per the old BaseActionTest, but within the groups we should maintain order from admin -> maintainer -> instructor -> student -> unregistered -> without login. Let's also add comments saying: we prefer this ordering for new access control methods + we prefer using high-level access control methods over mid-level ones

Nits:

• Pluralization should be standardized (plural for everything, except for AnyXXX)
• Change guest to WithoutLogin, since guest can still imply guest account
• verifyInstructorsCanAccess: Should clarify to verifyAnyInstructorCanAccess
• instructorCanAccess: change to just log in as instructor + can access -> pass!

[InfinityTwo]

@jasonqiu212 Thanks for taking your time to review the PR yesterday with me! I've pushed the requested changes based on our discussion, let me know if I missed anything out.

As for the NoMasquarade part that I couldn't answer yesterday, if I'm not wrong, it's because I wanted to use verifyAnyInstructorsCanAccess() originally. However, that method in BaseActionTest.java calls the method verifyAccessibleForAdminsToMasqueradeAsInstructor(), which fails the test case as from my knowledge, you can't masquerade as an instructor from admin, if admins cannot access it. Then because of that, I had to make a special method for verifyAnyInstructorsCanAccess() which checks the opposite case of masquerade check.

[Copilot]

Pull Request Overview

This PR abstracts access control tests to the BaseActionTest, consolidating multiple role‐specific test cases into single, unified test methods. Key changes include:

• Removing redundant, role‐specific access control tests in favor of generic helper method calls.
• Updating access control verifications across various web API action tests to use a consistent testing approach.
• Minor refactoring and bugfix improvements in the checkAccessControl method in Action.java.

Reviewed Changes

Copilot reviewed 39 out of 39 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
FeedbackSessionClosedRemindersActionTest.java	Replaced multiple role-specific tests with a single testAccessControl using verifyOnlyAdminsCanAccess.
EnrollStudentsActionTest.java	Added a consolidated testAccessControl verifying same-course privileges.
DeleteStudentActionTest.java	Consolidated various access control tests into testAccessControl checking both admin and instructor privileges.
DeleteNotificationActionTest.java	Simplified access control tests to use only verifyOnlyAdminsCanAccess with parameters.
DeleteInstructorActionTest.java	Merged multiple tests into one testAccessControl method combining same-course instructor and admin access checks.
DeleteFeedbackSessionActionTest.java	Consolidated session access control into a single test verifying modify session privileges.
DeleteFeedbackQuestionActionTest.java	Combined access control tests to verify instructor modification privileges.
DeleteAccountRequestActionTest.java	Replaced separate admin/non-admin tests with a unified access control test.
DeleteAccountActionTest.java	Added an access control test ensuring only admins can access.
CreateNotificationActionTest.java	Consolidated access control testing to focus on admin-only access.
CreateInstructorActionTest.java	Merged various role tests into one verifying correct course privilege for instructors.
CreateFeedbackSessionLogActionTest.java	Centralized access control to a single test verifying universal access.
CreateFeedbackQuestionActionTest.java	Updated access control tests to use modify session privilege helper methods.
CreateAccountRequestActionTest.java	Added a disabled access control test for admin-only access.
CompileLogsActionTest.java	Consolidated access control tests to a single admin-only access check.
CalculateUsageStatisticsActionTest.java	Unified access control tests for multiple roles with helper assertions.
BinFeedbackSessionActionTest.java	Updated instructor access control tests with new helper methods and parameter verifications.
BaseActionTest.java	Added extensive helper methods for access control across various user types and privileges.
MockUserProvision.java	Introduced setters for user role booleans to support new access control tests.
Action.java	Fixed the masquerading check by swapping the order in the equals call to improve null safety.

[jasonqiu212]

I directly modified the branch, since it's way too much work to do this via reviews..

Big changes made:

• Shifted order of tests to high-level -> mid-level -> helper methods
  • Rationale: From reader's perspective, high-level tests should be the first thing they read, since we prefer for them to use these tests
• Re-classified and re-ordered some tests
• verifyInstructorsOfTheSameCourseCanAccess: Removed check that instructors from a different course cannot access
  • Rationale: The method name does not imply this
  • Resulting changes:
    • Moved logic of private instructorsOfTheSameCourseCanAccess into verifyInstructorsOfTheSameCourseCanAccess
    • Added check in verifyOnlyInstructorsOfTheSameCourseCanAccess that instructors from a different course cannot access
• verifyInstructorsCanAccess: Abstracted out logic of loginAsInstructorOfTheSameCourse
• verifyInstructorsCanAccess: Added additional check that instructors from different course can access as well
  • Note: The 2 checks here are looser versions of verifyInstructorsOfTheSameCourseCanAccess and verifyInstructorsOfOtherCoursesCanAccess
• verifyInstructorsOfOtherCoursesCanAccess: Added 2x verifyCannotMasquerade checks, as per old version
  • Requires that verifyInstructorsOfOtherCoursesCanAccess takes in argument of currentCourse
• verifyStudentsOfOtherCoursesCannotAccess: Changed loginAsStudentOfTheSameCourse to loginAsStudentOfOtherCourse

Nits:

• Renamed verifyAnyInstructorsCanAccess to verifyAnyInstructorCanAccess
• Renamed verifyNoOneCanAccess to verifyNoUsersCanAccess
• Renamed loginStudentsOfSameCourseSetup to loginAsStudentOfTheSameCourse
• Renamed loginInstructorsOfOtherCourseSetup to loginAsInstructorOfOtherCourse
• Renamed verifyInstructorsCanAccessNoMasquerade to verifyAnyNonMasqueradingInstructorCanAccess
• Changed comments explaining high-level and mid-level tests + best practices

[jasonqiu212]

lgtm!

@InfinityTwo Thank you so much for this huge refactoring!! 🤩🔥

do check through the changes that I did above 1 last time, and ensure all the tests pass, and we can merge this in! afterwards, please proceed with opening #13304 to external contributors