Merge ~ack/maas:1812402-create-device-rbac into maas:master

[Alberto Donato]

LP: #1812402 - check if a user is allowed to create devices when RBAC is used

[Blake Rouse (blake-rouse)]

This really should be done where all other permissions are done. Inside the user.has_perm().

We could add a new NodePermission or a specific DevicePermission. But I think placing this in the form, seperate from the other permission checks is the wrong place.

The permission check logic is correct, I just don't like the location of the check itself. So could it be moved to to inside the user.has_perm(DevicePermission.create) or something like that.

review: Needs Fixing

[Alberto Donato (ack)]

do you mean MAASAuthorizationBackend.has_perm ? that's where other checks are, and that logic exists there (that's where I copied it from) but the form doesn't go through that flow it seems

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b 1812402-create-device-rbac lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/5785/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b 1812402-create-device-rbac lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/5789/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b 1812402-create-device-rbac lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/5790/consoleText

diff --git a/src/maasserver/api/devices.py b/src/maasserver/api/devices.py
index 89df68b..65a2423 100644
--- a/src/maasserver/api/devices.py
+++ b/src/maasserver/api/devices.py
@@ -6,6 +6,7 @@ __all__ = [
     "DevicesHandler",
     ]
 
+from django.core.exceptions import PermissionDenied
 from maasserver.api.interfaces import DISPLAYED_INTERFACE_FIELDS
 from maasserver.api.logger import maaslog
 from maasserver.api.nodes import (
@@ -234,6 +235,8 @@ class DevicesHandler(NodesHandler):
         parameters.
         """
         form = DeviceWithMACsForm(data=request.data, request=request)
+        if not form.has_perm(request.user):
+            raise PermissionDenied()
         if form.is_valid():
             device = form.save()
             parent = device.parent
diff --git a/src/maasserver/api/tests/test_devices.py b/src/maasserver/api/tests/test_devices.py
index 117a13a..9b34672 100644
--- a/src/maasserver/api/tests/test_devices.py
+++ b/src/maasserver/api/tests/test_devices.py
@@ -268,6 +268,15 @@ class TestDevicesAPI(APITestCase.ForUser):
             ],
             list(parsed_result[0]))
 
+    def test_create_no_permission(self):
+        self.patch(auth, 'validate_user_external_auth').return_value = True
+        self.useFixture(RBACEnabled())
+        self.become_non_local()
+        response = self.client.post(
+            reverse('devices_handler'),
+            {'mac_addresses': ['aa:bb:cc:dd:ee:ff']})
+        self.assertEqual(response.status_code, http.client.FORBIDDEN)
+
 
 def get_device_uri(device):
     """Return a device's URI on the API."""
diff --git a/src/maasserver/forms/__init__.py b/src/maasserver/forms/__init__.py
index 419fcfa..48cd078 100644
--- a/src/maasserver/forms/__init__.py
+++ b/src/maasserver/forms/__init__.py
@@ -944,6 +944,11 @@ class DeviceForm(NodeForm):
         if instance is not None:
             self.initial['zone'] = instance.zone.name
 
+    def has_perm(self, user):
+        # see MAASAuthorizationBackend.has_perm for the logic behind the
+        # permission check
+        return user.has_perm(NodePermission.view)
+
     def save(self, commit=True):
         device = super(DeviceForm, self).save(commit=False)
         device.node_type = NODE_TYPE.DEVICE
diff --git a/src/maasserver/forms/tests/test_device.py b/src/maasserver/forms/tests/test_device.py
index 071bc08..79e2409 100644
--- a/src/maasserver/forms/tests/test_device.py
+++ b/src/maasserver/forms/tests/test_device.py
@@ -16,6 +16,7 @@ from maasserver.models import (
     Interface,
 )
 from maasserver.testing.factory import factory
+from maasserver.testing.fixtures import RBACEnabled
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils.forms import get_QueryDict
 from maasserver.utils.orm import (
@@ -59,6 +60,37 @@ class TestDeviceForm(MAASServerTestCase):
 
         self.assertEqual(parent, device.parent)
 
+    def test_has_perm_no_rbac(self):
+        form = DeviceForm()
+        self.assertTrue(form.has_perm(factory.make_User()))
+
+    def test_has_perm_rbac_no_permision(self):
+        self.useFixture(RBACEnabled())
+        form = DeviceForm()
+        self.assertFalse(form.has_perm(factory.make_User()))
+
+    def test_has_perm_rbac_global_admin(self):
+        self.useFixture(RBACEnabled())
+        user = factory.make_admin()
+        form = DeviceForm()
+        self.assertTrue(form.has_perm(user))
+
+    def test_has_perm_rbac_permission_on_pool(self):
+        rbac = self.useFixture(RBACEnabled())
+        user = factory.make_User()
+        rbac.store.allow(
+            user.username, factory.make_ResourcePool(), 'admin-machines')
+        form = DeviceForm()
+        self.assertTrue(form.has_perm(user))
+
+    def test_has_perm_rbac_read_permission_on_pool(self):
+        rbac = self.useFixture(RBACEnabled())
+        user = factory.make_User()
+        rbac.store.allow(
+            user.username, factory.make_ResourcePool(), 'view')
+        form = DeviceForm()
+        self.assertFalse(form.has_perm(user))
+
 
 class TestDeviceWithMACsForm(MAASServerTestCase):
 
diff --git a/src/maasserver/websockets/base.py b/src/maasserver/websockets/base.py
index 1e8baab..6060a44 100644
--- a/src/maasserver/websockets/base.py
+++ b/src/maasserver/websockets/base.py
@@ -64,6 +64,9 @@ class HandlerDoesNotExistError(HandlerError):
 class HandlerPermissionError(HandlerError):
     """Raised when permission is denied for the user of a given action."""
 
+    def __init__(self):
+        super().__init__("Permission denied")
+
 
 class HandlerOptions(object):
     """Configuraton class for `Handler`.
diff --git a/src/maasserver/websockets/handlers/tests/test_device.py b/src/maasserver/websockets/handlers/tests/test_device.py
index 0ca33af..9f74a44 100644
--- a/src/maasserver/websockets/handlers/tests/test_device.py
+++ b/src/maasserver/websockets/handlers/tests/test_device.py
@@ -1025,8 +1025,10 @@ class TestDeviceHandler(MAASTransactionServerTestCase):
 
     @transactional
     def test_update_owned_with_rbac(self):
-        self.useFixture(RBACEnabled())
+        rbac = self.useFixture(RBACEnabled())
         user = factory.make_User(is_local=False)
+        rbac.store.allow(
+            user.username, factory.make_ResourcePool(), 'admin-machines')
         node = factory.make_Node(owner=user, node_type=NODE_TYPE.DEVICE)
         handler = DeviceHandler(user, {}, None)
         new_hostname = factory.make_name("hostname")