Merge ~ltrager/maas:lp1870171_2.3 into maas:2.3

[Lee Trager]

Backport of d0653c9 LP: #1870171 - Remove the token column from the Node table.

The token column is only used by the API when allocating and machine and
listing allocated machines. When a User Token is deleted this association
causes the Node to be deleted as well. Allocated machines are already
filtered by owner and status so the token column isn't needed.

In the backport Node.token is no longer used but the column remains to
avoid creating a migration.

[Lee Trager (ltrager)]

Approved in https://code.launchpad.net/~ltrager/maas/+git/maas/+merge/381876

review: Approve

diff --git a/src/maasserver/api/account.py b/src/maasserver/api/account.py
index adc8722..ba6ed54 100644
--- a/src/maasserver/api/account.py
+++ b/src/maasserver/api/account.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """API handler: `Account`."""
diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py
index e8b671b..9482631 100644
--- a/src/maasserver/api/machines.py
+++ b/src/maasserver/api/machines.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
@@ -41,7 +41,6 @@ from maasserver.api.support import (
 )
 from maasserver.api.utils import (
     get_mandatory_param,
-    get_oauth_token,
     get_optional_list,
     get_optional_param,
 )
@@ -525,7 +524,7 @@ class MachineHandler(NodeHandler, OwnerDataMixin, PowerMixin):
                     "Request from user %s to acquire machine: %s (%s)",
                     request.user.username, machine.fqdn, machine.system_id)
                 machine.acquire(
-                    request.user, get_oauth_token(request),
+                    request.user,
                     agent_name=agent_name, comment=comment,
                     bridge_all=bridge_all, bridge_stp=bridge_stp,
                     bridge_fd=bridge_fd)
@@ -1314,11 +1313,24 @@ class MachinesHandler(NodesHandler, PowersMixin):
 
     @operation(idempotent=True)
     def list_allocated(self, request):
-        """Fetch Machines that were allocated to the User/oauth token."""
-        token = get_oauth_token(request)
-        match_ids = get_optional_list(request.GET, 'id')
-        machines = Machine.objects.get_allocated_visible_machines(
-            token, match_ids)
+        """@description-title List allocated
+        @description List machines that were allocated to the User.
+
+        @success (http-status-code) "200" 200
+        @success (json) "success-json" A JSON object containing a list of
+        allocated machines.
+        @success-example "success-json" [exkey=machines-placeholder]
+        placeholder text
+        """
+        # limit to machines that the user can view
+        machines = Machine.objects.get_nodes(
+            request.user, NODE_PERMISSION.VIEW)
+        machines = machines.filter(
+            owner=request.user, status=NODE_STATUS.ALLOCATED
+        )
+        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)
@@ -1606,7 +1618,7 @@ class MachinesHandler(NodesHandler, PowersMixin):
                 raise NodesNotAvailable(message)
             if not dry_run:
                 machine.acquire(
-                    request.user, get_oauth_token(request),
+                    request.user,
                     agent_name=agent_name, comment=comment,
                     bridge_all=bridge_all, bridge_stp=bridge_stp,
                     bridge_fd=bridge_fd)
diff --git a/src/maasserver/api/tests/test_api.py b/src/maasserver/api/tests/test_api.py
index 08892d9..4e8b9e0 100644
--- a/src/maasserver/api/tests/test_api.py
+++ b/src/maasserver/api/tests/test_api.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test maasserver API."""
@@ -45,7 +45,10 @@ from maasserver.testing.testclient import MAASSensibleOAuthClient
 from maasserver.utils.converters import json_load_bytes
 from maasserver.utils.django_urls import reverse
 from maasserver.utils.keys import ImportSSHKeysError
-from maasserver.utils.orm import get_one
+from maasserver.utils.orm import (
+    get_one,
+    reload_object,
+)
 from maastesting.matchers import MockCalledOnceWith
 from maastesting.testcase import MAASTestCase
 from piston3.doc import generate_doc
@@ -288,6 +291,16 @@ class AccountAPITest(APITestCase.ForUser):
 
         self.assertEqual(http.client.BAD_REQUEST, response.status_code)
 
+    def test_delete_authorisation_token_doesnt_delete_node(self):
+        token = self.user.tokens.first()
+        node = factory.make_Node(owner=self.user, token=token)
+        response = self.client.post(
+            reverse("account_handler"),
+            {"op": "delete_authorisation_token", "token_key": token.key},
+        )
+        self.assertEqual(http.client.NO_CONTENT, response.status_code)
+        self.assertIsNotNone(reload_object(node))
+
     def test_update_authorisation_token(self):
         token_name_orig = 'Test_Token'
         token_name_updated = 'Test_Token update'
diff --git a/src/maasserver/api/tests/test_machine.py b/src/maasserver/api/tests/test_machine.py
index 5b55eaf..e0b45ab 100644
--- a/src/maasserver/api/tests/test_machine.py
+++ b/src/maasserver/api/tests/test_machine.py
@@ -716,10 +716,16 @@ class TestMachineAPI(APITestCase.ForUser):
         }
         self.client.post(self.get_machine_uri(machine), request)
         self.assertThat(
-            machine_method, MockCalledOnceWith(
-                ANY, ANY, agent_name=ANY,
-                bridge_all=False, bridge_fd=False,
-                bridge_stp=False, comment=request['comment']))
+            machine_method,
+            MockCalledOnceWith(
+                ANY,
+                agent_name=ANY,
+                bridge_all=False,
+                bridge_fd=0,
+                bridge_stp=False,
+                comment=request["comment"],
+            ),
+        )
 
     def test_POST_deploy_passes_bridge_settings(self):
         self.patch(node_module.Node, "_start")
@@ -740,10 +746,16 @@ class TestMachineAPI(APITestCase.ForUser):
         }
         self.client.post(self.get_machine_uri(machine), request)
         self.assertThat(
-            machine_method, MockCalledOnceWith(
-                ANY, ANY, agent_name=ANY,
-                bridge_all=True, bridge_fd=7,
-                bridge_stp=True, comment=None))
+            machine_method,
+            MockCalledOnceWith(
+                ANY,
+                agent_name=ANY,
+                bridge_all=True,
+                bridge_fd=7,
+                bridge_stp=True,
+                comment=None,
+            ),
+        )
 
     def test_POST_release_releases_owned_machine(self):
         self.patch(node_module.Machine, '_stop')
@@ -898,10 +910,16 @@ class TestMachineAPI(APITestCase.ForUser):
             reverse('machines_handler'),
             {'op': 'allocate', 'comment': comment})
         self.assertThat(
-            machine_method, MockCalledOnceWith(
-                ANY, ANY, agent_name=ANY,
-                bridge_all=False, bridge_fd=False,
-                bridge_stp=False, comment=comment))
+            machine_method,
+            MockCalledOnceWith(
+                ANY,
+                agent_name=ANY,
+                bridge_all=False,
+                bridge_fd=False,
+                bridge_stp=False,
+                comment=comment,
+            ),
+        )
 
     def test_POST_allocate_handles_missing_comment(self):
         factory.make_Node(
@@ -911,10 +929,16 @@ class TestMachineAPI(APITestCase.ForUser):
         self.client.post(
             reverse('machines_handler'), {'op': 'allocate'})
         self.assertThat(
-            machine_method, MockCalledOnceWith(
-                ANY, ANY, agent_name=ANY,
-                bridge_all=False, bridge_fd=False,
-                bridge_stp=False, comment=None))
+            machine_method,
+            MockCalledOnceWith(
+                ANY,
+                agent_name=ANY,
+                bridge_all=False,
+                bridge_fd=0,
+                bridge_stp=False,
+                comment=None,
+            ),
+        )
 
     def test_POST_release_frees_hwe_kernel(self):
         self.patch(node_module.Machine, '_stop')
diff --git a/src/maasserver/api/tests/test_machines.py b/src/maasserver/api/tests/test_machines.py
index b58f737..474ecaf 100644
--- a/src/maasserver/api/tests/test_machines.py
+++ b/src/maasserver/api/tests/test_machines.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the machines API."""
@@ -33,10 +33,6 @@ from maasserver.models import (
     node as node_module,
 )
 from maasserver.models.node import RELEASABLE_STATUSES
-from maasserver.models.user import (
-    create_auth_token,
-    get_auth_tokens,
-)
 from maasserver.node_constraint_filter_forms import AcquireNodeForm
 from maasserver.rpc.testing.fixtures import MockLiveRegionToClusterRPCFixture
 from maasserver.testing.api import (
@@ -546,29 +542,11 @@ class TestMachinesAPI(APITestCase.ForUser):
             [machine.system_id for machine in machines],
             extract_system_ids(parsed_result))
 
-    def test_GET_list_allocated_returns_only_allocated_with_user_token(self):
-        # If the user's allocated machines have different session tokens,
-        # list_allocated should only return the machines that have the
-        # current request's token on them.
-        machine_1 = factory.make_Node(
-            status=NODE_STATUS.ALLOCATED, owner=self.user,
-            token=get_auth_tokens(self.user)[0])
-        second_token = create_auth_token(self.user)
-        factory.make_Node(
-            owner=self.user, status=NODE_STATUS.ALLOCATED,
-            token=second_token)
-
-        user_2 = factory.make_User()
-        create_auth_token(user_2)
-        factory.make_Node(
-            owner=self.user, status=NODE_STATUS.ALLOCATED,
-            token=second_token)
-
-        # At this point we have two machines owned by the same user but
-        # allocated with different tokens, and a third machine allocated to
-        # someone else entirely.  We expect list_allocated to
-        # return the machine with the same token as the one used in
-        # self.client, which is the one we set on machine_1 above.
+    def test_GET_list_allocated_returns_only_allocated_with_user(self):
+        machine = factory.make_Node(
+            status=NODE_STATUS.ALLOCATED, owner=self.user
+        )
+        factory.make_Node(status=NODE_STATUS.ALLOCATED)
 
         response = self.client.get(reverse('machines_handler'), {
             'op': 'list_allocated'})
@@ -576,17 +554,17 @@ class TestMachinesAPI(APITestCase.ForUser):
         parsed_result = json.loads(
             response.content.decode(settings.DEFAULT_CHARSET))
         self.assertItemsEqual(
-            [machine_1.system_id], extract_system_ids(parsed_result))
+            [machine.system_id], extract_system_ids(parsed_result)
+        )
 
     def test_GET_list_allocated_filters_by_id(self):
-        # list_allocated takes an optional list of 'id' parameters to
-        # filter returned results.
-        current_token = get_auth_tokens(self.user)[0]
         machines = []
         for _ in range(3):
-            machines.append(factory.make_Node(
-                status=NODE_STATUS.ALLOCATED,
-                owner=self.user, token=current_token))
+            machines.append(
+                factory.make_Node(
+                    status=NODE_STATUS.ALLOCATED, owner=self.user
+                )
+            )
 
         required_machine_ids = [machines[0].system_id, machines[1].system_id]
         response = self.client.get(reverse('machines_handler'), {
@@ -1450,19 +1428,6 @@ class TestMachinesAPI(APITestCase.ForUser):
             response.content.decode(settings.DEFAULT_CHARSET))['system_id']
         self.assertEqual(eligible_machine.system_id, system_id)
 
-    def test_POST_allocate_sets_a_token(self):
-        # "acquire" should set the Token being used in the request on
-        # the Machine that is allocated.
-        available_status = NODE_STATUS.READY
-        machine = factory.make_Node(
-            status=available_status, owner=None, with_boot_disk=True)
-        response = self.client.post(
-            reverse('machines_handler'), {'op': 'allocate'})
-        self.assertThat(response, HasStatusCode(http.client.OK))
-        machine = Machine.objects.get(system_id=machine.system_id)
-        oauth_key = self.client.token.key
-        self.assertEqual(oauth_key, machine.token.key)
-
     def test_POST_accept_gets_machine_out_of_declared_state(self):
         # This will change when we add provisioning.  Until then,
         # acceptance gets a machine straight to Ready state.
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 1eb14d9..cb49543 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -2551,11 +2551,16 @@ class Node(CleanSave, TimestampedModel):
                     "s" if len(missing_packages) > 1 else ""))
 
     def acquire(
-            self, user, token=None, agent_name='', comment=None,
-            bridge_all=False, bridge_stp=None, bridge_fd=None):
-        """Mark commissioned node as acquired by the given user and token."""
+        self,
+        user,
+        agent_name="",
+        comment=None,
+        bridge_all=False,
+        bridge_stp=None,
+        bridge_fd=None,
+    ):
+        """Mark commissioned node as acquired by the given user."""
         assert self.owner is None or self.owner == user
-        assert token is None or token.user == user
 
         self._create_acquired_filesystems()
         self._register_request_event(
@@ -2564,7 +2569,6 @@ class Node(CleanSave, TimestampedModel):
         self.status = NODE_STATUS.ALLOCATED
         self.owner = user
         self.agent_name = agent_name
-        self.token = token
         if bridge_all:
             self._create_acquired_bridges(
                 bridge_stp=bridge_stp, bridge_fd=bridge_fd)
@@ -2823,8 +2827,7 @@ class Node(CleanSave, TimestampedModel):
             finalize_release = True
 
         self.status = NODE_STATUS.RELEASING
-        self.token = None
-        self.agent_name = ''
+        self.agent_name = ""
         self.set_netboot()
         self.osystem = ''
         self.distro_series = ''
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index a47ee5d..193296e 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test maasserver models."""
@@ -108,7 +108,6 @@ from maasserver.models.node import (
 )
 from maasserver.models.signals import power as node_query
 from maasserver.models.timestampedmodel import now
-from maasserver.models.user import create_auth_token
 from maasserver.node_status import (
     COMMISSIONING_LIKE_STATUSES,
     NODE_FAILURE_MONITORED_STATUS_TRANSITIONS,
@@ -1113,12 +1112,6 @@ class TestNode(MAASServerTestCase):
         node = factory.make_Node(bios_boot_method=factory.make_name("boot"))
         self.assertEqual("pxe", node.get_bios_boot_method())
 
-    def test_add_node_with_token(self):
-        user = factory.make_User()
-        token = create_auth_token(user)
-        node = factory.make_Node(token=token)
-        self.assertEqual(token, node.token)
-
     def test_add_physical_interface(self):
         mac = factory.make_mac_address()
         node = factory.make_Node()
@@ -1560,9 +1553,8 @@ class TestNode(MAASServerTestCase):
     def test_acquire(self):
         node = factory.make_Node(status=NODE_STATUS.READY, with_boot_disk=True)
         user = factory.make_User()
-        token = create_auth_token(user)
-        agent_name = factory.make_name('agent-name')
-        node.acquire(user, token, agent_name)
+        agent_name = factory.make_name("agent-name")
+        node.acquire(user, agent_name)
         self.assertEqual(
             (user, NODE_STATUS.ALLOCATED, agent_name),
             (node.owner, node.status, node.agent_name))
@@ -1570,36 +1562,44 @@ class TestNode(MAASServerTestCase):
     def test_acquire_calls__create_acquired_filesystems(self):
         node = factory.make_Node(status=NODE_STATUS.READY, with_boot_disk=True)
         user = factory.make_User()
-        token = create_auth_token(user)
-        agent_name = factory.make_name('agent-name')
+        agent_name = factory.make_name("agent-name")
         mock_create_acquired_filesystems = self.patch_autospec(
-            node, "_create_acquired_filesystems")
-        node.acquire(user, token, agent_name)
+            node, "_create_acquired_filesystems"
+        )
+        node.acquire(user, agent_name)
         self.assertThat(mock_create_acquired_filesystems, MockCalledOnceWith())
 
     def test_acquire_logs_user_request(self):
         node = factory.make_Node(status=NODE_STATUS.READY, with_boot_disk=True)
         user = factory.make_User()
-        token = create_auth_token(user)
-        agent_name = factory.make_name('agent-name')
-        register_event = self.patch(node, '_register_request_event')
-        node.acquire(user, token, agent_name)
-        self.assertThat(register_event, MockCalledOnceWith(
-            user, EVENT_TYPES.REQUEST_NODE_ACQUIRE, action='acquire',
-            comment=None))
+        agent_name = factory.make_name("agent-name")
+        register_event = self.patch(node, "_register_request_event")
+        node.acquire(user, agent_name)
+        self.assertThat(
+            register_event,
+            MockCalledOnceWith(
+                user,
+                EVENT_TYPES.REQUEST_NODE_ACQUIRE,
+                action="acquire",
+                comment=None,
+            ),
+        )
 
     def test_acquire_calls__create_acquired_bridges(self):
         node = factory.make_Node(status=NODE_STATUS.READY, with_boot_disk=True)
         user = factory.make_User()
-        token = create_auth_token(user)
-        agent_name = factory.make_name('agent-name')
+        agent_name = factory.make_name("agent-name")
         mock_create_acquired_bridges = self.patch_autospec(
             node, "_create_acquired_bridges")
         bridge_stp = factory.pick_bool()
         bridge_fd = random.randint(0, 500)
         node.acquire(
-            user, token, agent_name,
-            bridge_all=True, bridge_stp=bridge_stp, bridge_fd=bridge_fd)
+            user,
+            agent_name,
+            bridge_all=True,
+            bridge_stp=bridge_stp,
+            bridge_fd=bridge_fd,
+        )
         self.assertThat(
             mock_create_acquired_bridges,
             MockCalledOnceWith(bridge_stp=bridge_stp, bridge_fd=bridge_fd))
@@ -2102,8 +2102,7 @@ class TestNode(MAASServerTestCase):
             MockCalledOnceWith(node.system_id, node.get_releasing_time()))
         self.expectThat(node.status, Equals(NODE_STATUS.RELEASING))
         self.expectThat(node.owner, Equals(owner))
-        self.expectThat(node.agent_name, Equals(''))
-        self.expectThat(node.token, Is(None))
+        self.expectThat(node.agent_name, Equals(""))
         self.expectThat(node.netboot, Is(True))
         self.expectThat(node.osystem, Equals(''))
         self.expectThat(node.distro_series, Equals(''))
@@ -2141,8 +2140,7 @@ class TestNode(MAASServerTestCase):
         self.expectThat(Node._set_status_expires, MockNotCalled())
         self.expectThat(node.status, Equals(NODE_STATUS.RELEASING))
         self.expectThat(node.owner, Equals(owner))
-        self.expectThat(node.agent_name, Equals(''))
-        self.expectThat(node.token, Is(None))
+        self.expectThat(node.agent_name, Equals(""))
         self.expectThat(node.netboot, Is(True))
         self.expectThat(node.osystem, Equals(''))
         self.expectThat(node.distro_series, Equals(''))
@@ -2169,8 +2167,7 @@ class TestNode(MAASServerTestCase):
         self.expectThat(Node._set_status_expires, MockNotCalled())
         self.expectThat(node.status, Equals(NODE_STATUS.READY))
         self.expectThat(node.owner, Equals(None))
-        self.expectThat(node.agent_name, Equals(''))
-        self.expectThat(node.token, Is(None))
+        self.expectThat(node.agent_name, Equals(""))
         self.expectThat(node.netboot, Is(True))
         self.expectThat(node.osystem, Equals(''))
         self.expectThat(node.distro_series, Equals(''))
diff --git a/src/maasserver/models/tests/test_userprofile.py b/src/maasserver/models/tests/test_userprofile.py
index 9fa2ba1..1a9bdc3 100644
--- a/src/maasserver/models/tests/test_userprofile.py
+++ b/src/maasserver/models/tests/test_userprofile.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the UserProfile model."""
@@ -85,6 +85,14 @@ class UserProfileTest(MAASServerTestCase):
         self.assertFalse(Consumer.objects.filter(id__in=consumer_ids).exists())
         self.assertFalse(Token.objects.filter(id__in=token_ids).exists())
 
+    def test_delete_doesnt_delete_node(self):
+        user = factory.make_User()
+        profile = user.userprofile
+        _, token = profile.create_authorisation_token()
+        node = factory.make_Node(owner=user, token=token)
+        profile.delete_authorisation_token(token.key)
+        self.assertIsNotNone(reload_object(node))
+
     def test_delete_deletes_related_filestorage_objects(self):
         # Deleting a profile deletes the related filestorage objects.
         profile = factory.make_User().userprofile
diff --git a/src/maasserver/models/userprofile.py b/src/maasserver/models/userprofile.py
index fcffb8d..aaa302f 100644
--- a/src/maasserver/models/userprofile.py
+++ b/src/maasserver/models/userprofile.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """UserProfile model."""
@@ -19,6 +19,7 @@ from django.db.models import (
 from django.shortcuts import get_object_or_404
 from maasserver import DefaultMeta
 from maasserver.exceptions import CannotDeleteUserException
+from maasserver.models import Node
 from maasserver.models.cleansave import CleanSave
 from piston3.models import Token
 
@@ -139,7 +140,11 @@ class UserProfile(CleanSave, Model):
 
         """
         token = get_object_or_404(
-            Token, user=self.user, token_type=Token.ACCESS, key=token_key)
+            Token, user=self.user, token_type=Token.ACCESS, key=token_key,
+        )
+        # LP:1870171 - Make sure the key being deleted isn't assoicated with
+        # any node. In MAAS 2.8+ Node.token has been removed.
+        Node.objects.filter(token=token).update(token=None)
         token.consumer.delete()
         token.delete()
 
diff --git a/src/maasserver/node_action.py b/src/maasserver/node_action.py
index 74c8df7..f034453 100644
--- a/src/maasserver/node_action.py
+++ b/src/maasserver/node_action.py
@@ -311,7 +311,10 @@ class Acquire(NodeAction):
     def execute(self):
         """See `NodeAction.execute`."""
         with locks.node_acquire:
-            self.node.acquire(self.user, token=None)
+            try:
+                self.node.acquire(self.user)
+            except ValidationError as e:
+                raise NodeActionError(e)
 
 
 class Deploy(NodeAction):
@@ -327,7 +330,10 @@ class Deploy(NodeAction):
         """See `NodeAction.execute`."""
         if self.node.owner is None:
             with locks.node_acquire:
-                self.node.acquire(self.user, token=None)
+                try:
+                    self.node.acquire(self.user)
+                except ValidationError as e:
+                    raise NodeActionError(e)
 
         if osystem and distro_series:
             try: