Add raise_exception arg to @group_required #10658 #10659
Conversation
Allows raising a 403 PermissionDenied for API-style requests, instead of a 302 redirect to nowhere.
jacobtylerwalls
changed the title
| self.client.force_login(self.anonymous) | ||
| response = self.client.post(reverse("workflow_history", kwargs={"workflowid": str(self.history.workflowid)})) |
There was a problem hiding this comment.
Noticed this wasn't testing what it I had intended (because the user wasn't even a resource editor), so I added a comment and adjusted the request so that it's an editor user and actually has some data.
|
I borrowed the idea from @permission_required. |
| @@ -60,8 +58,8 @@ def post(self, request, workflowid): | |||
| workflowid = workflowid, | |||
| ) | |||
| else: | |||
| return JSONErrorResponse(_("Request Failed"), _("Permission Denied"), status=403) | |||
|
|
|||
| raise PermissionDenied | |||
There was a problem hiding this comment.
Will the UI be able to handle the fact that we used to return a JSON object and are now returning a python exception?
There was a problem hiding this comment.
Good question. I just retested. No change on the frontend because the frontend doesn't even check the response for success:
Notice no try/catch here:
This is probably because you shouldn't be able to end up in this scenario (editing someone else's workflow) through the UI. You'd need to be trying to hack the endpoint with your own POST requests to reach this. I tested by leaving the UI open and then changing the owner of the workflow in the database.
In terms of the motivation for this change, I figured it was better to have all the permission failure cases return the same shape of a response so that we don't expose any information about what workflows exist.
There was a problem hiding this comment.
There was a problem hiding this comment.
Retested this one also. Same. Both 7.6 and this PR show the same in the UI:
The fact that the response changes from:
dev/7.6.x
{
"message": "Permission Denied",
"status": "false",
"success": false,
"title": "Request Failed"
}
to
<!doctype html>
<html lang="en">
<head>
<title>403 Forbidden</title>
</head>
<body>
<h1>403 Forbidden</h1><p></p>
</body>
</html>
doesn't seem to matter.
There was a problem hiding this comment.
All we're using is the statusText which remains the same.
Types of changes
Description of Change
Allows raising a 403 Forbidden for API-style requests, instead of a 302 redirect to nowhere.
More info about the use case in the linked issue.
Issues Solved
Closes #10658
Checklist
Ticket Background