Merge ~ack/maas:1812201-list-allocated-rbac into maas:master

[Alberto Donato]

LP: #1812201 - filter allocated machines based on pools with RBAC

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1812201-list-allocated-rbac lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/4940/console
COMMIT: d0bdf2658f42eb458a58ebec0a5b740db199aa5a

review: Needs Fixing

[Björn Tillenius (bjornt)]

The fix looks good, and thanks for adding some more tests. But you forgot to add a test that confirms that the fix is good. Even after reverting the fix, all the tests you added still pass.

review: Needs Fixing

[Alberto Donato (ack)]

I do get a test failure on the RBAC test without the change, don't you?

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1812201-list-allocated-rbac lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: c0ee3c591339a1a1c51bf7295bd4bb6fa44ba0f5

review: Approve

[Björn Tillenius (bjornt)]

Ah, sorry. I ran the wrong tests. Although that said, it took me a while to understand what the test actually tested.

I'd said ideally you would split the test to test one thing, or at least add comments for what the different machines are needed for.

That said, I wonder if this fix is really in the right place. The get_allocated_visible_machines() is quite general. You would expect that if an admin called that method for someone else's token, they would get the full list.

I think it makes more sense to fix this in the API method itself, and use ids to limit which machines to consider. Otherwise it will be really hard to understand how get_allocated_visible_machines work. Maybe it would be more clear if you added a for_user parameter, though.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1812201-list-allocated-rbac lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 4468e28c3593e115e69e967e5bfe2f0b72f12d6e

review: Approve

[Björn Tillenius (bjornt)]

+1 with some suggestions inline.

review: Approve

[Alberto Donato (ack)]

addressed all points, thanks

[MAAS Lander (maas-lander)]

LANDING
-b 1812201-list-allocated-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/4949/consoleText

diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py
index aec4a29..9c4513a 100644
--- a/src/maasserver/api/machines.py
+++ b/src/maasserver/api/machines.py
@@ -1939,10 +1939,12 @@ class MachinesHandler(NodesHandler, PowersMixin):
         @success-example "success-json" [exkey=machines-placeholder]
         placeholder text
         """
-        token = get_oauth_token(request)
-        match_ids = get_optional_list(request.GET, 'id')
-        machines = Machine.objects.get_allocated_visible_machines(
-            token, match_ids)
+        # limit to machines that the user can view
+        machines = Machine.objects.get_nodes(request.user, NodePermission.view)
+        machines = machines.filter(token=get_oauth_token(request))
+        system_ids = get_optional_list(request.GET, 'id')
+        if system_ids:
+            machines = machines.filter(system_id__in=system_ids)
         return machines.order_by('id')
 
     @operation(idempotent=False)
diff --git a/src/maasserver/api/tests/test_machines.py b/src/maasserver/api/tests/test_machines.py
index 282c6bb..5eac395 100644
--- a/src/maasserver/api/tests/test_machines.py
+++ b/src/maasserver/api/tests/test_machines.py
@@ -629,6 +629,41 @@ class TestMachinesAPI(APITestCase.ForUser):
         self.assertItemsEqual(
             required_machine_ids, extract_system_ids(parsed_result))
 
+    def test_GET_list_allocated_with_rbac(self):
+        self.patch(auth, 'validate_user_external_auth').return_value = True
+        rbac = self.useFixture(RBACEnabled())
+        self.become_non_local()
+
+        user = factory.make_User()
+        pool = factory.make_ResourcePool()
+        rbac.store.allow(user.username, pool, 'view')
+
+        pool = factory.make_ResourcePool()
+        rbac.store.add_pool(pool)
+        rbac.store.allow(self.user.username, pool, 'view')
+
+        token = get_auth_tokens(self.user)[0]
+        factory.make_Node(
+            hostname='viewable', owner=self.user, token=token, pool=pool,
+            status=NODE_STATUS.ALLOCATED)
+        # a machine with the same token but not accesssible to the user (not in
+        # the allowed pool)
+        factory.make_Node(
+            hostname='not-accessible', owner=self.user, token=token,
+            status=NODE_STATUS.ALLOCATED)
+        # a machine owned by another user in the accessible pool
+        factory.make_Node(
+            hostname='other-user', owner=factory.make_User(),
+            status=NODE_STATUS.ALLOCATED, pool=pool)
+
+        response = self.client.get(
+            reverse('machines_handler'), {'op': 'list_allocated'})
+        self.assertEqual(http.client.OK, response.status_code)
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        hostnames = [machine['hostname'] for machine in parsed_result]
+        self.assertEqual(['viewable'], hostnames)
+
     def test_POST_allocate_returns_available_machine(self):
         # The "allocate" operation returns an available machine.
         available_status = NODE_STATUS.READY
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 8f744d9..998e02c 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -604,27 +604,6 @@ class MachineManager(BaseNodeManager):
 
     extra_filters = {'node_type': NODE_TYPE.MACHINE}
 
-    def get_allocated_visible_machines(self, token, ids):
-        """Fetch Machines that were allocated to the User_/oauth token.
-
-        :param user: The user whose machines to fetch
-        :type user: User_
-        :param token: The OAuth token associated with the Machines.
-        :type token: piston3.models.Token.
-        :param ids: Optional set of IDs to filter by. If given, machines whose
-            system_ids are not in `ids` will be ignored.
-        :type param_ids: Sequence
-
-        .. _User: https://
-           docs.djangoproject.com/en/dev/topics/auth/
-           #django.contrib.auth.models.User
-        """
-        if ids is None:
-            machines = self.filter(token=token)
-        else:
-            machines = self.filter(token=token, system_id__in=ids)
-        return machines
-
     def get_available_machines_for_acquisition(self, for_user):
         """Find the machines that can be acquired by the given user.
 
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index c4ebd6e..361b6f8 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -432,6 +432,7 @@ class TestNodeManager(MAASServerTestCase):
 
 
 class TestMachineManager(MAASServerTestCase):
+
     def make_machine(self, user=None, **kwargs):
         """Create a machine, allocated to `user` if given."""
         if user is None: