Update /grading endpoint
#727
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qreoct
commented
Feb 21, 2021
| user = conn.assigns[:current_user] | ||
|
|
||
| case Assessments.get_group_grading_summary(user) do | ||
| case Assessments.get_group_grading_summary() do |
There was a problem hiding this comment.
Example where user role validation is moved out of the Model to the router.
We lose a descriptive error message, and get a 403 Forbidden when unauthorized users access these endpoints.
qreoct
commented
Feb 21, 2021
| @@ -249,6 +235,20 @@ defmodule CadetWeb.GradingController do | |||
| response(401, "Unauthorised") | |||
| end | |||
|
|
|||
| swagger_path :grading_summary do | |||
There was a problem hiding this comment.
Added Swagger documentation for the /grading/summary endpoint, which was previously undocumented.
qreoct
commented
Feb 21, 2021
| end | ||
|
|
||
| @tag authenticate: :student | ||
| test "missing parameter", %{conn: conn} do | ||
| conn = post(conn, build_url(1, 3), %{}) | ||
| assert response(conn, 400) =~ "Missing parameter" |
There was a problem hiding this comment.
Now we give a 403 Forbidden since a student would never be authorized to make this API call anyway.
bnjmnt4n
approved these changes
Feb 23, 2021
angelsl
pushed a commit
that referenced
this pull request
Apr 17, 2021
* Update `/assets` endpoint (#713) * Assets: remove role validation in model * AssetsController: update API endpoint and validation The endpoint is updated to `/v2/admin/assets`, and validation is performed within the router instead. * Shift to `AdminAssetsController` * Rename `AdminAssetsController` to `.exs` * Assets: switch to `case` instead of `with` * (1) Clearer Swagger documentation in Grading (2) Code documentation cleaup in auth_controller * Notifications changed to plural throughout * Settings swagger docs cleanup * User docs typo fix * CI fix formatting * Update `/settings` endpoint (#722) * `/auth`: shift `/auth` to `/auth/login` * `/devices`: fix typo and shift to v2 * Update `/grading` endpoint (#727) * shift role validation for Settings * shift Grading controller to AdminGrading * fix formatting and add details to docs Co-authored-by: Benjamin Tan <benjamin@dev.ofcr.se> * Rename `assets_controller_test` to `admin_assets_controller_test` * update swagger docs for /grading * Update `/assessments` endpoint (#729) * `/assessments`: split out separate `/admin` endpoint * Consistently use `/assessments/:assessmentid` * Merge `/admin/assessments/:assessmentid/{update,publish}` endpoints * Quick swagger docs fix for `AdminAssessmentsController` * Format code via `mix format` * Formatting fix * `/assessments/{assessmentid}/submit`: shift role validation to controller * Add more guards * `/assessments/question/{questionid}/submit`: shift role validation to AnswerController * clean up some docs * Modify `AnswerController#submit` path * Assessments: remove unused definitions * Remove leftover unused route * Split `AssesmentsController#unlock` out of `AssessmentsController#show` `AssesmentsController#unlock` unlocks password-protected assessments and creates a Submission for them, following which the assessments do not require requests with the password. * Fix formatting issues * `/assessments`: split out separate `/admin` endpoint * Consistently use `/assessments/:assessmentid` * Merge `/admin/assessments/:assessmentid/{update,publish}` endpoints * Quick swagger docs fix for `AdminAssessmentsController` * Format code via `mix format` * `/assessments/{assessmentid}/submit`: shift role validation to controller * Add more guards * `/assessments/question/{questionid}/submit`: shift role validation to AnswerController * clean up some docs * Modify `AnswerController#submit` path * Assessments: remove unused definitions * Remove leftover unused route * Split `AssesmentsController#unlock` out of `AssessmentsController#show` `AssesmentsController#unlock` unlocks password-protected assessments and creates a Submission for them, following which the assessments do not require requests with the password. * Fix formatting issues * formatting * Update per PR feedback * Remove unused variable Co-authored-by: qreoct <9080974+qreoct@users.noreply.github.com> * Change to v2 API throughout * Fix Swagger references * AdminGradingControllerTest: Add swagger path definition * Remove duplicated route * Rename file as per convention * Swagger documentation: Switch to enums for string values * /auth/login: shift parameters to query string in docs * UserController: add an empty UserGameStates map to the schema * Add comments to clarify invalid OpenAPI 2.0 types * Swagger: shift request parameters to separate schemas Request parameters were previously declared as separate body parameters in the Swagger schema, which was technically invalid. For endpoints which require more than 1 parameter, which is sent as JSON, I've created separated schemas which represent the parameters, to make the generated `swagger.json` more compliant. * Story: Fix inaccurate Swagger documentation * Shift all Swagger enums into their own schemas This allows `swagger-typescript-api` in `cadet-frontend` to generate TypeScript `enum`s for the corresponding type. * Fix inaccurate types * Update based on code review feedback * clean up docs * clean up docs * Endpoints: update to mark as required * More changes to the Swagger types * Even more type updates * more swagger docs consistency * /admin/assessments: update swagger to FormData parameters Co-authored-by: qreoct <9080974+qreoct@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move the Grading Controller to behind /admin endpoint for v2 API consistency.
Some (not all) of role validation was moved out of the
AssessmentsModel to the router. The methods that are required by the new AdminGrading Controller have all been fixed, but there are some test cases relating to Assessments Controller which are not rectified in this PR. @bnjmnt4n, let me know if the changes I've made aligns with your Assessments changes.The specific 3 tests that don't pass are involved with Students making API calls in AssessmentController, where the Students are directly authorized (with 200 response) when they shouldn't be. The tests fail since I did not move the /assessments endpoints to be role-validated in the router in this PR.