Merge ~ltrager/maas:lp1904398_2.8 into maas:2.8

[Lee Trager]

LP: #1904398 - Only set machines to commissioning in form when anonymous

Backport of 9f03422

[Lee Trager (ltrager)]

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

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b lp1904398_2.8 lp:~ltrager/maas/+git/maas into -b 2.8 lp:~maas-committers/maas

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

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1904398_2.8 lp:~ltrager/maas/+git/maas into -b 2.8 lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 21ab297d560c9cb8056c47fdda69bfcaffd36969

review: Approve

diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py
index 72281b4..9a18d34 100644
--- a/src/maasserver/api/machines.py
+++ b/src/maasserver/api/machines.py
@@ -1923,7 +1923,8 @@ class MachinesHandler(NodesHandler, PowersMixin):
         @param (boolean) "commission" [required=false,formatting=true] Request
         the newly created machine to be created with status set to
         COMMISSIONING. Machines will wait for COMMISSIONING results and not
-        time out.
+        time out. Machines created by administrators will be commissioned
+        unless set to false.
 
         @param (int) "enable_ssh" [required=false]  Whether to enable SSH for
         the commissioning environment using the user's SSH key(s). '1' == True,
@@ -1959,8 +1960,13 @@ class MachinesHandler(NodesHandler, PowersMixin):
         @success-example "success-json" [exkey=machines-create]
         placeholder text
         """
+        # Only admins are allowed to commission, store the value but remove it
+        # from the form.
+        commission = get_optional_param(
+            request.data, "commission", default=True, validator=StringBool
+        )
         machine = create_machine(request)
-        if request.user.is_superuser:
+        if request.user.is_superuser and commission:
             form = CommissionForm(
                 instance=machine, user=request.user, data=request.data
             )
diff --git a/src/maasserver/api/tests/test_machines.py b/src/maasserver/api/tests/test_machines.py
index 98c4485..070bb68 100644
--- a/src/maasserver/api/tests/test_machines.py
+++ b/src/maasserver/api/tests/test_machines.py
@@ -334,6 +334,39 @@ class TestMachinesAPI(APITestCase.ForUser):
             [script_result.name for script_result in script_set],
         )
 
+    def test_POST_commission_true(self):
+        # Regression test for LP:1904398
+        self.become_admin()
+        self.patch(Machine, "_start").return_value = None
+        make_usable_osystem(self)
+        response = self.client.post(
+            reverse("machines_handler"),
+            {
+                "architecture": make_usable_architecture(self),
+                "mac_addresses": ["aa:bb:cc:dd:ee:ff"],
+                "power_type": "manual",
+                "commission": True,
+            },
+        )
+        parsed_result = json.loads(response.content.decode())
+        self.assertEquals(NODE_STATUS.COMMISSIONING, parsed_result["status"])
+
+    def test_POST_commission_false(self):
+        # Regression test for LP:1904398
+        self.become_admin()
+        make_usable_osystem(self)
+        response = self.client.post(
+            reverse("machines_handler"),
+            {
+                "architecture": make_usable_architecture(self),
+                "mac_addresses": ["aa:bb:cc:dd:ee:ff"],
+                "power_type": "manual",
+                "commission": False,
+            },
+        )
+        parsed_result = json.loads(response.content.decode())
+        self.assertEquals(NODE_STATUS.NEW, parsed_result["status"])
+
     def test_GET_lists_machines(self):
         # The api allows for fetching the list of Machines.
         machine1 = factory.make_Node()
diff --git a/src/maasserver/forms/__init__.py b/src/maasserver/forms/__init__.py
index 62e1ecd..40be585 100644
--- a/src/maasserver/forms/__init__.py
+++ b/src/maasserver/forms/__init__.py
@@ -713,13 +713,8 @@ class MachineForm(NodeForm):
     def __init__(self, request=None, requires_arch=False, *args, **kwargs):
         super(MachineForm, self).__init__(*args, **kwargs)
 
-        # Even though it doesn't need it and doesn't use it, this form accepts
-        # a parameter named 'request' because it is used interchangingly
-        # with AdminMachineForm which actually uses this parameter.
-        request = kwargs.get("request")
-        if request is not None:
-            self.data = request.data
         instance = kwargs.get("instance")
+        self.request = request
 
         self.set_up_architecture_field(requires_arch=requires_arch)
         # We only want the license key field to render in the UI if the `OS`
@@ -919,7 +914,16 @@ class MachineForm(NodeForm):
 
         # LP:1807991 - If requested when creating a new Machine, set the status
         # to COMMISSIONING when the object is created.
-        commission = not self.instance.id and self.cleaned_data["commission"]
+        is_anonymous = (
+            self.request
+            and self.request.user
+            and self.request.user.is_anonymous
+        )
+        commission = (
+            is_anonymous
+            and not self.instance.id
+            and self.cleaned_data["commission"]
+        )
         if commission:
             self.instance.status = NODE_STATUS.COMMISSIONING
         machine = super(MachineForm, self).save(*args, **kwargs)
@@ -1144,10 +1148,8 @@ class AdminMachineForm(MachineForm, AdminNodeForm, WithPowerTypeMixin):
             "memory",
         )
 
-    def __init__(self, data=None, instance=None, request=None, **kwargs):
-        super(AdminMachineForm, self).__init__(
-            data=data, instance=instance, **kwargs
-        )
+    def __init__(self, data=None, instance=None, **kwargs):
+        super().__init__(data=data, instance=instance, **kwargs)
         WithPowerTypeMixin.set_up_power_fields(self, data, instance)
 
     def clean(self):
diff --git a/src/maasserver/forms/tests/test_machinewithmacaddresses.py b/src/maasserver/forms/tests/test_machinewithmacaddresses.py
index 6e78321..6afcd94 100644
--- a/src/maasserver/forms/tests/test_machinewithmacaddresses.py
+++ b/src/maasserver/forms/tests/test_machinewithmacaddresses.py
@@ -5,6 +5,8 @@
 
 __all__ = []
 
+from unittest.mock import MagicMock
+
 from django.http import QueryDict
 from testtools.matchers import Contains
 
@@ -218,8 +220,10 @@ class MachineWithMACAddressesFormTest(MAASServerTestCase):
         self.assertEqual("192-168-12-10-extra", node.hostname)
 
     def test_form_with_commissioning(self):
+        request = MagicMock()
+        request.user.is_anonymous = True
         form = MachineWithMACAddressesForm(
-            data={"commission": True, **self.make_params()}
+            request, data={"commission": True, **self.make_params()}
         )
         self.assertTrue(form.is_valid())
         machine = form.save()