Merge ~ltrager/maas:lp1870171 into maas:master

[Lee Trager]

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 assoication
causes the Node to be deleted as well. Allocated machines are already
filtered by owner and status so the token column isn't needed.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1870171 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 432c4c8f40c7b559c538fdb6432352f63dd7b07d

review: Approve

[Alberto Donato (ack)]

Have we confirmed this doesn't break juju?

This is a change in behavior of the list-allocated call since it now returns all allocated machines for the user, where before it only returned the ones allocated with the specific key.

I think that's fine (and in fact saner) for users, but I wonder if juju relies on only seeing machines for the specific model (the UI used to mention that you need to create an API key for each juju environment/model).

review: Needs Information

[Lee Trager (ltrager)]

Spoke with Rick on IRC and it seems like this should be okay.

<ltrager> when Juju lists allocated machines in MAAS does it expect that list to contain all the allocated assoicated with that user's OAUTH token or just that user?
<rick_h> ltrager: you mean when you juju status?
<rick_h> ltrager: and since the oauth token is per user I'm not sure the difference?
<ltrager> rick_h: A user can have multiple OAUTH keys in MAAS. On the API only, MAAS assoicated an allocated machine with the OAUTH token being used. When listing allocated machines the API only lists machines allocated with that key.
<rick_h> ltrager: right, so that oauth token is how Juju communicates with MAAS and so won't see anything that the API key can't see
<ltrager> rick_h: this is the only place in MAAS that associates the API OAUTH key with a Node and we'd like to remove that association as its causing a bug
<ltrager> rick_h: right now if you have two OAUTH keys for a user and OAUTH1 allocates a node OAUTH2 won't see it even if they are the same user
<ltrager> rick_h: that seems like a bug which I'm trying to fix but it was brought up Juju might be expecting that behavior
<rick_h> ltrager: hmmm, so in juju land if you wanted to update tokens I'd expect you to be able to juju upgrade-credential on the contrller, supply the new token, and as long as the new token has access to the same hardware it'd be ok?
<ltrager> rick_h: currently that isn't the case
<rick_h> ltrager: on which end? MAAS or Juju?
<ltrager> rick_h: the bug I'm trying to fix is that if you allocate a node with an OAUTH key and delete it it also deletes any node assoicated with it
<ltrager> rick_h: MAAS
<rick_h> ltrager: ok, I don't believe we're relying on that in any way as we don't manage the oauth keys in Juju. We just use them to make calls to MAAS.
<ltrager> rick_h: ok thanks!

[Alberto Donato (ack)]

LGTM, +1

review: Approve

diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py
index 7a983c2..72281b4 100644
--- a/src/maasserver/api/machines.py
+++ b/src/maasserver/api/machines.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 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__ = [
@@ -40,7 +40,6 @@ from maasserver.api.nodes import (
 from maasserver.api.support import admin_method, operation
 from maasserver.api.utils import (
     get_mandatory_param,
-    get_oauth_token,
     get_optional_list,
     get_optional_param,
 )
@@ -746,7 +745,6 @@ class MachineHandler(NodeHandler, OwnerDataMixin, PowerMixin):
                 )
                 machine.acquire(
                     request.user,
-                    get_oauth_token(request),
                     agent_name=options.agent_name,
                     comment=options.comment,
                     bridge_all=options.bridge_all,
@@ -2139,7 +2137,7 @@ class MachinesHandler(NodesHandler, PowersMixin):
     @operation(idempotent=True)
     def list_allocated(self, request):
         """@description-title List allocated
-        @description List machines that were allocated to the User/oauth token.
+        @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
@@ -2149,7 +2147,9 @@ class MachinesHandler(NodesHandler, PowersMixin):
         """
         # 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))
+        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)
@@ -2472,7 +2472,6 @@ class MachinesHandler(NodesHandler, PowersMixin):
             if not dry_run:
                 machine.acquire(
                     request.user,
-                    get_oauth_token(request),
                     agent_name=options.agent_name,
                     comment=options.comment,
                     bridge_all=options.bridge_all,
diff --git a/src/maasserver/api/tests/test_machine.py b/src/maasserver/api/tests/test_machine.py
index 8b62ea3..d4e4370 100644
--- a/src/maasserver/api/tests/test_machine.py
+++ b/src/maasserver/api/tests/test_machine.py
@@ -911,7 +911,6 @@ class TestMachineAPI(APITestCase.ForUser):
             machine_method,
             MockCalledOnceWith(
                 ANY,
-                ANY,
                 agent_name=ANY,
                 bridge_all=False,
                 bridge_type=BRIDGE_TYPE.STANDARD,
@@ -947,7 +946,6 @@ class TestMachineAPI(APITestCase.ForUser):
             machine_method,
             MockCalledOnceWith(
                 ANY,
-                ANY,
                 agent_name=ANY,
                 bridge_all=True,
                 bridge_type=BRIDGE_TYPE.OVS,
@@ -1386,7 +1384,6 @@ class TestMachineAPI(APITestCase.ForUser):
             machine_method,
             MockCalledOnceWith(
                 ANY,
-                ANY,
                 agent_name=ANY,
                 bridge_all=False,
                 bridge_type=BRIDGE_TYPE.STANDARD,
@@ -1409,7 +1406,6 @@ class TestMachineAPI(APITestCase.ForUser):
             machine_method,
             MockCalledOnceWith(
                 ANY,
-                ANY,
                 agent_name=ANY,
                 bridge_all=False,
                 bridge_type=BRIDGE_TYPE.STANDARD,
diff --git a/src/maasserver/api/tests/test_machines.py b/src/maasserver/api/tests/test_machines.py
index b6a417c..d293824 100644
--- a/src/maasserver/api/tests/test_machines.py
+++ b/src/maasserver/api/tests/test_machines.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 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."""
@@ -30,7 +30,6 @@ from maasserver.forms.pods import ComposeMachineForm, ComposeMachineForPodsForm
 from maasserver.models import Config, Domain, Machine, Node
 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 APITestCase, APITransactionTestCase
@@ -694,31 +693,11 @@ class TestMachinesAPI(APITestCase.ForUser):
             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
+    def test_GET_list_allocated_returns_only_allocated_with_user(self):
+        machine = factory.make_Node(
+            status=NODE_STATUS.ALLOCATED, owner=self.user
         )
-
-        # 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.
+        factory.make_Node(status=NODE_STATUS.ALLOCATED)
 
         response = self.client.get(
             reverse("machines_handler"), {"op": "list_allocated"}
@@ -728,20 +707,15 @@ class TestMachinesAPI(APITestCase.ForUser):
             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,
+                    status=NODE_STATUS.ALLOCATED, owner=self.user
                 )
             )
 
@@ -771,20 +745,17 @@ class TestMachinesAPI(APITestCase.ForUser):
         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
+        # a machine with the same user 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
@@ -2260,21 +2231,6 @@ class TestMachinesAPI(APITestCase.ForUser):
         )["system_id"]
         self.assertEqual(node2.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/migrations/maasserver/0206_remove_node_token.py b/src/maasserver/migrations/maasserver/0206_remove_node_token.py
new file mode 100644
index 0000000..3a09b11
--- /dev/null
+++ b/src/maasserver/migrations/maasserver/0206_remove_node_token.py
@@ -0,0 +1,12 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.11 on 2020-04-08 03:28
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("maasserver", "0205_pod_nodes")]
+
+    operations = [migrations.RemoveField(model_name="node", name="token")]
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 3f0b1bb..ef8085c 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -61,7 +61,6 @@ from django.db.models.query import QuerySet
 from django.shortcuts import get_object_or_404
 from netaddr import IPAddress, IPNetwork
 import petname
-from piston3.models import Token
 from twisted.internet import reactor
 from twisted.internet.defer import (
     Deferred,
@@ -1092,15 +1091,6 @@ class Node(CleanSave, TimestampedModel):
         null=True, blank=False, default=None, editable=False
     )
 
-    token = ForeignKey(
-        Token,
-        db_index=True,
-        null=True,
-        editable=False,
-        unique=False,
-        on_delete=CASCADE,
-    )
-
     error = CharField(max_length=255, blank=True, default="")
 
     netboot = BooleanField(default=True)
@@ -3203,7 +3193,6 @@ class Node(CleanSave, TimestampedModel):
     def acquire(
         self,
         user,
-        token=None,
         agent_name="",
         comment=None,
         bridge_all=False,
@@ -3211,9 +3200,8 @@ class Node(CleanSave, TimestampedModel):
         bridge_stp=None,
         bridge_fd=None,
     ):
-        """Mark commissioned node as acquired by the given user and token."""
+        """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(
@@ -3225,7 +3213,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_type=bridge_type,
@@ -3559,7 +3546,6 @@ class Node(CleanSave, TimestampedModel):
             finalize_release = True
 
         self.status = NODE_STATUS.RELEASING
-        self.token = None
         self.agent_name = ""
         self.set_netboot()
         self.set_ephemeral_deploy()
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 26ccf9e..bf7cb93 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2019 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."""
@@ -125,7 +125,6 @@ from maasserver.models.partitiontable import PARTITION_TABLE_EXTRA_SPACE
 from maasserver.models.resourcepool import ResourcePool
 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,
     get_node_timeout,
@@ -1358,12 +1357,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()
@@ -1885,9 +1878,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)
+        node.acquire(user, agent_name)
         self.assertEqual(
             (user, NODE_STATUS.ALLOCATED, agent_name),
             (node.owner, node.status, node.agent_name),
@@ -1896,21 +1888,19 @@ 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")
         mock_create_acquired_filesystems = self.patch_autospec(
             node, "_create_acquired_filesystems"
         )
-        node.acquire(user, token, agent_name)
+        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)
+        node.acquire(user, agent_name)
         self.assertThat(
             register_event,
             MockCalledOnceWith(
@@ -1924,7 +1914,6 @@ class TestNode(MAASServerTestCase):
     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")
         mock_create_acquired_bridges = self.patch_autospec(
             node, "_create_acquired_bridges"
@@ -1934,7 +1923,6 @@ class TestNode(MAASServerTestCase):
         bridge_fd = random.randint(0, 500)
         node.acquire(
             user,
-            token,
             agent_name,
             bridge_all=True,
             bridge_type=bridge_type,
@@ -2546,7 +2534,6 @@ class TestNode(MAASServerTestCase):
         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.netboot, Is(True))
         self.expectThat(node.ephemeral_deploy, Is(False))
         self.expectThat(node.osystem, Equals(""))
@@ -2592,7 +2579,6 @@ class TestNode(MAASServerTestCase):
         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.netboot, Is(True))
         self.expectThat(node.ephemeral_deploy, Is(False))
         self.expectThat(node.osystem, Equals(""))
@@ -2625,7 +2611,6 @@ class TestNode(MAASServerTestCase):
         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.netboot, Is(True))
         self.expectThat(node.ephemeral_deploy, Is(False))
         self.expectThat(node.osystem, Equals(""))
diff --git a/src/maasserver/node_action.py b/src/maasserver/node_action.py
index 831585c..0e5af52 100644
--- a/src/maasserver/node_action.py
+++ b/src/maasserver/node_action.py
@@ -448,7 +448,7 @@ class Acquire(NodeAction):
         """See `NodeAction.execute`."""
         with locks.node_acquire:
             try:
-                self.node.acquire(self.user, token=None)
+                self.node.acquire(self.user)
             except ValidationError as e:
                 raise NodeActionError(e)
 
@@ -486,7 +486,7 @@ class Deploy(NodeAction):
         if self.node.owner is None:
             with locks.node_acquire:
                 try:
-                    self.node.acquire(self.user, token=None)
+                    self.node.acquire(self.user)
                 except ValidationError as e:
                     raise NodeActionError(e)
         if install_kvm:
diff --git a/src/maasserver/tests/test_node_action.py b/src/maasserver/tests/test_node_action.py
index b7278b7..6d438f0 100644
--- a/src/maasserver/tests/test_node_action.py
+++ b/src/maasserver/tests/test_node_action.py
@@ -792,9 +792,7 @@ class TestDeployAction(MAASServerTestCase):
         Deploy(node, user, request).execute(**extra)
         self.expectThat(mock_get_curtin_config, MockCalledOnceWith(ANY, node))
         # Make sure we set bridge_all when we acquire the Node.
-        self.expectThat(
-            mock_node_acquire, MockCalledOnceWith(user, token=None)
-        )
+        self.expectThat(mock_node_acquire, MockCalledOnceWith(user))
         self.expectThat(mock_node_start, MockCalledOnceWith(user))
         # Make sure ubuntu/bionic is selected if KVM pod deployment has been
         # selected.