launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/forbidden-vs-unauthorized into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/forbidden-vs-unauthorized/+merge/92821

This one's for Doris.  Where do I start?

The part where I blame Piston, I suppose.  Our PermissionDenied exception referred to piston.utils.rc.FORBIDDEN.  Which seems appropriate.  Status code 403: Forbidden means “yes you're logged in, but despite that fact, you're not allowed to do what you tried to do.  Please do not press this button again.”

So far so good, but the piston error thingies were messy (you'd do piston.utils.rc.FORBIDDEN.write() to get your error message out!) and incomplete (some other errors we needed weren't represented at all).  So Raphaël replaced them with httplib error codes.

Great, right?  Except as it turns out, rc.FORBIDDEN referred not to the status code 403: Forbidden but to 401: Unauthorized!  Which means “you need to be logged in if you want to do this, so please authenticate at this point.”  And thus the migration faithfully replaced rc.FORBIDDEN with the wrong error code, httplib.UNAUTHORIZED.  Some of our tests even documented this as the appropriate error code, which it does not appear to be.

And so, in this yak-shaving branch I change the status code for PermissionDenied from 401: Unauthorized to 403: Forbidden — and make the associated changes in tests and documentation.

Doris will be pleased.
-- 
https://code.launchpad.net/~jtv/maas/forbidden-vs-unauthorized/+merge/92821
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/forbidden-vs-unauthorized into lp:maas.

=== modified file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py	2012-02-13 12:31:07 +0000
+++ src/maasserver/exceptions.py	2012-02-13 18:38:26 +0000
@@ -34,7 +34,13 @@
 
 
 class PermissionDenied(MaasAPIException):
-    api_error = httplib.UNAUTHORIZED
+    """HTTP error 403: Forbidden.  User is logged in, but lacks permission.
+
+    Do not confuse this with 401: Unauthorized ("you need to be logged in
+    for this, so please authenticate").  The Piston error codes do confuse
+    the two.
+    """
+    api_error = httplib.FORBIDDEN
 
 
 class NodesNotAvailable(MaasAPIException):

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-02-13 13:07:48 +0000
+++ src/maasserver/tests/test_api.py	2012-02-13 18:38:26 +0000
@@ -139,7 +139,7 @@
 
         response = self.client.get('/api/nodes/%s/' % other_node.system_id)
 
-        self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
     def test_GET_refuses_to_access_nonexistent_node(self):
         # When fetching a Node, the api returns a 'Not Found' (404) error
@@ -196,7 +196,7 @@
 
         response = self.client.put('/api/nodes/%s/' % other_node.system_id)
 
-        self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
     def test_PUT_refuses_to_update_nonexistent_node(self):
         # When updating a Node, the api returns a 'Not Found' (404) error
@@ -223,7 +223,7 @@
 
         response = self.client.delete('/api/nodes/%s/' % other_node.system_id)
 
-        self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
     def test_DELETE_refuses_to_delete_nonexistent_node(self):
         # When deleting a Node, the api returns a 'Not Found' (404) error
@@ -430,14 +430,14 @@
             self.mac2.mac_address, parsed_result[1]['mac_address'])
 
     def test_macs_GET_forbidden(self):
-        # When fetching MAC Addresses, the api returns a 'Unauthorized' (401)
+        # When fetching MAC Addresses, the api returns a 'Forbidden' (403)
         # error if the node is not visible to the logged-in user.
         other_node = factory.make_node(
             status=NODE_STATUS.DEPLOYED, owner=factory.make_user())
         response = self.client.get(
             '/api/nodes/%s/macs/' % other_node.system_id)
 
-        self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
     def test_macs_GET_not_found(self):
         # When fetching MAC Addresses, the api returns a 'Not Found' (404)
@@ -455,14 +455,14 @@
         self.assertEqual(httplib.NOT_FOUND, response.status_code)
 
     def test_macs_GET_node_forbidden(self):
-        # When fetching a MAC Address, the api returns a 'Unauthorized' (401)
+        # When fetching a MAC Address, the api returns a 'Forbidden' (403)
         # error if the node is not visible to the logged-in user.
         other_node = factory.make_node(
             status=NODE_STATUS.DEPLOYED, owner=factory.make_user())
         response = self.client.get(
             '/api/nodes/%s/macs/0-aa-22-cc-44-dd/' % other_node.system_id)
 
-        self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
     def test_macs_GET_node_bad_request(self):
         # When fetching a MAC Address, the api returns a 'Bad Request' (400)
@@ -513,7 +513,7 @@
             self.node.macaddress_set.count())
 
     def test_macs_DELETE_mac_forbidden(self):
-        # When deleting a MAC Address, the api returns a 'Unauthorized' (401)
+        # When deleting a MAC Address, the api returns a 'Forbidden' (403)
         # error if the node is not visible to the logged-in user.
         other_node = factory.make_node(
             status=NODE_STATUS.DEPLOYED, owner=factory.make_user())
@@ -521,7 +521,7 @@
             '/api/nodes/%s/macs/%s/' % (
                 other_node.system_id, self.mac1.mac_address))
 
-        self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
     def test_macs_DELETE_not_found(self):
         # When deleting a MAC Address, the api returns a 'Not Found' (404)