Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 31 commits into from
Nov 16, 2024

Conversation

josh1248
Copy link
Contributor

@josh1248 josh1248 commented Sep 8, 2024

Accompanies the frontend pull request at source-academy/frontend#3043

Previously, "admin" routes were available to both staff and admins to access.

Now, a new "staff" route has been created that accepts most of the previous functions. The remaining functions in "admin" scope have the ability to cause massive damage, such as the deletion of entire assessments. Hence, they are now inaccessible to avengers even if they bypass the frontend route checking.

Aside: I have also shifted admin panel actions into this admin scope.

@coveralls
Copy link

coveralls commented Sep 8, 2024

Coverage Status

coverage: 93.661% (-0.1%) from 93.78%
when pulling fe8c762 on GabrielCWT:No-GC-For-Staff
into 71192c3 on source-academy:master.

@josh1248
Copy link
Contributor Author

The new scope has caused the test error codes to change from 400 to 403 - will update the tests for staffs and admins.

josh1248 and others added 3 commits September 30, 2024 00:26
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.
@josh1248
Copy link
Contributor Author

josh1248 commented Oct 7, 2024

image

Error messages are no longer being generated at the controller level but rather at the router level due to the changes, which causes the tailored error messages to be hidden. I was wondering if this was something I should revert? Alternatively, it could be possible to make the admin check a per-route check, allowing for different error messages rather than a generic "403 Forbidden". Would take some time, though (should still be manageable to bulk-change).

Create test cases to indicate that non-admin staff can only read assets,
but not create, modify, or delete them.
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 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
@josh1248
Copy link
Contributor Author

Ready for review!

@josh1248 josh1248 requested a review from RichDom2185 October 27, 2024 03:33
Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert all the unnecessary changes? Refer to the diff for the (I think mostly autogenerated) changes. Thanks!

@josh1248
Copy link
Contributor Author

Apologies for missing this out - I have reverted the accidental formatting changes!

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@RichDom2185 RichDom2185 enabled auto-merge (squash) November 16, 2024 11:35
@RichDom2185 RichDom2185 merged commit 5203125 into source-academy:master Nov 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants