Merge ~newell-jensen/maas:lp1786193 into maas:master

[Newell Jensen]

LP: #1786193 -- Add ability to create ssh key during admin creation in the API. Add ability to specify user during ssh key creation in the API.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1786193 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 34b99fe81a153dce212c7840c15466a8f5301c13

review: Approve

[Lee Trager (ltrager)]

Allowing administrators to specify a users SSH keys at account creation makes sense. However once the account is created what keys are in use should be in control of that user. Allowing any user to create keys for another user creates a security risk. With this patch I could upload my SSH key to your account which will give me access to any machine you deploy or run ephemerally.

review: Needs Fixing

[Andres Rodriguez (andreserl)]

*only* administrators should be allowed to add keys for a given user.

0260d0c... by Newell Jensen on 2018-10-17

Review fixes.

[Lee Trager (ltrager)]

I don't think we want to allow administrators to add keys to another account once its created.

In a datacenter environment you'll often have 3 different teams accessing hardware. IT administrators who will handle administrating MAAS and have root access to the boxes running MAAS, data techs which handle setting up new machines and fixing existing hardware, and dev groups which actually deploy OS's to run applications which have customer data.

When a dev group deploys a machine it shouldn't be possible for anyone but that dev group to access that machine as it has customer data on it. The way MAAS works a data tech will need an administrative account to add machines and fix existing ones. A data tech should not be able to access any deployed machine but because they are an administrator they will be able to access any machine in the data center.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1786193 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 0260d0c4981ec35f6a932a1db2a26c5a62386a56

review: Approve

0bcf417... by Newell Jensen on 2018-10-17

Raise error if non admin tries supplies a username when creating an SSH key.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1786193 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 0bcf41785d8db79b17af419b02eccc83c715a0b2

review: Approve

[Lee Trager (ltrager)]

This looks much better but I think the audit log needs to be improved. Right now if I create a key for you the audit log will only show I created an SSH key. It should show I created an SSH key for you.

8e24359... by Newell Jensen on 2018-10-18

Update audit log description for SSH key creation when the requesting user is different than who the SSH key is created for.

[Lee Trager (ltrager)]

LGTM!

review: Approve

diff --git a/src/maasserver/api/ssh_keys.py b/src/maasserver/api/ssh_keys.py
index a911407..b26e157 100644
--- a/src/maasserver/api/ssh_keys.py
+++ b/src/maasserver/api/ssh_keys.py
@@ -11,6 +11,7 @@ __all__ = [
 import http.client
 
 from django.conf import settings
+from django.contrib.auth.models import User
 from django.http import (
     HttpResponse,
     HttpResponseForbidden,
@@ -20,6 +21,7 @@ from maasserver.api.support import (
     operation,
     OperationsHandler,
 )
+from maasserver.api.utils import get_optional_param
 from maasserver.audit import create_audit_event
 from maasserver.enum import (
     ENDPOINT,
@@ -35,6 +37,7 @@ from maasserver.models import (
     SSHKey,
 )
 from maasserver.utils.keys import ImportSSHKeysError
+from maasserver.utils.orm import get_one
 from piston3.emitters import JSONEmitter
 from piston3.handler import typemapper
 from piston3.utils import rc
@@ -56,12 +59,28 @@ class SSHKeysHandler(OperationsHandler):
         return SSHKey.objects.filter(user=request.user)
 
     def create(self, request):
-        """Add a new SSH key to the requesting user's account.
+        """Add a new SSH key to the requesting or supplied user's account.
 
         The request payload should contain the public SSH key data in form
         data whose name is "key".
         """
-        form = SSHKeyForm(user=request.user, data=request.data)
+        user = request.user
+        username = get_optional_param(request.POST, 'user')
+        if username is not None and request.user.is_superuser:
+            supplied_user = get_one(User.objects.filter(username=username))
+            if supplied_user is not None:
+                user = supplied_user
+            else:
+                # Raise an error so that the user can know that their
+                # attempt at specifying a user did not work.
+                raise MAASAPIValidationError(
+                    "Supplied username does not match any current users.")
+        elif username is not None and not request.user.is_superuser:
+            raise MAASAPIValidationError(
+                "Only administrators can specify a user"
+                " when creating an SSH key.")
+
+        form = SSHKeyForm(user=user, data=request.data)
         if form.is_valid():
             sshkey = form.save(ENDPOINT.API, request)
             emitter = JSONEmitter(
diff --git a/src/maasserver/api/tests/test_api.py b/src/maasserver/api/tests/test_api.py
index ed0f926..b36cfc9 100644
--- a/src/maasserver/api/tests/test_api.py
+++ b/src/maasserver/api/tests/test_api.py
@@ -529,6 +529,42 @@ class TestSSHKeyHandlers(APITestCase.ForUser):
         added_key = get_one(SSHKey.objects.filter(user=self.user))
         self.assertEqual(key_string, added_key.key)
 
+    def test_adding_works_with_user(self):
+        self.become_admin()
+        username = factory.make_name('user')
+        user = factory.make_User(username=username)
+        key_string = get_data('data/test_rsa0.pub')
+        response = self.client.post(
+            reverse('sshkeys_handler'), data=dict(
+                key=key_string, user=username))
+        self.assertEqual(http.client.CREATED, response.status_code)
+        parsed_response = json_load_bytes(response.content)
+        self.assertEqual(key_string, parsed_response["key"])
+        added_key = get_one(SSHKey.objects.filter(user=user))
+        self.assertEqual(key_string, added_key.key)
+
+    def test_adding_does_not_work_with_unknown_user(self):
+        self.become_admin()
+        username = factory.make_name('user')
+        key_string = get_data('data/test_rsa0.pub')
+        response = self.client.post(
+            reverse('sshkeys_handler'), data=dict(
+                key=key_string, user=username))
+        self.assertEqual(
+            http.client.BAD_REQUEST, response.status_code, response)
+        self.assertIn(b"Supplied username", response.content)
+
+    def test_adding_does_not_work_with_user_for_non_admin(self):
+        username = factory.make_name('user')
+        factory.make_User(username=username)
+        key_string = get_data('data/test_rsa0.pub')
+        response = self.client.post(
+            reverse('sshkeys_handler'), data=dict(
+                key=key_string, user=username))
+        self.assertEqual(
+            http.client.BAD_REQUEST, response.status_code, response)
+        self.assertIn(b"Only administrators", response.content)
+
     def test_adding_catches_key_validation_errors(self):
         key_string = factory.make_string()
         response = self.client.post(
diff --git a/src/maasserver/api/tests/test_users.py b/src/maasserver/api/tests/test_users.py
index 068ddd1..373a68b 100644
--- a/src/maasserver/api/tests/test_users.py
+++ b/src/maasserver/api/tests/test_users.py
@@ -23,9 +23,11 @@ from maasserver.models import (
     StaticIPAddress,
 )
 from maasserver.models.event import Event
+from maasserver.testing import get_data
 from maasserver.testing.api import APITestCase
 from maasserver.testing.factory import factory
 from maasserver.utils.django_urls import reverse
+from maasserver.utils.orm import get_one
 from provisioningserver.events import AUDIT
 from testtools.matchers import (
     ContainsAll,
@@ -119,6 +121,34 @@ class TestUsers(APITestCase.ForUser):
             (email, True),
             (created_user.email, created_user.is_superuser))
 
+    def test_POST_creates_admin_with_ssh_key(self):
+        self.become_admin()
+        username = factory.make_name('user')
+        email = factory.make_email_address()
+        password = factory.make_string()
+        key_string = get_data('data/test_rsa0.pub')
+        response = self.client.post(
+            reverse('users_handler'),
+            {
+                'username': username,
+                'email': email,
+                'password': password,
+                'is_superuser': '1',
+                'key': key_string,
+            })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+
+        self.assertEqual(
+            username, json.loads(
+                response.content.decode(settings.DEFAULT_CHARSET))['username'])
+        created_user = User.objects.get(username=username)
+        self.assertEqual(
+            (email, True),
+            (created_user.email, created_user.is_superuser))
+        added_key = get_one(SSHKey.objects.filter(user=created_user))
+        self.assertEqual(key_string, added_key.key)
+
     def test_POST_requires_admin(self):
         response = self.client.post(
             reverse('users_handler'),
diff --git a/src/maasserver/api/users.py b/src/maasserver/api/users.py
index e4a4a8d..10dfdf0 100644
--- a/src/maasserver/api/users.py
+++ b/src/maasserver/api/users.py
@@ -11,6 +11,7 @@ __all__ = [
 
 from django.core.exceptions import ValidationError
 from django.shortcuts import get_object_or_404
+from maasserver.api.ssh_keys import SSHKeysHandler
 from maasserver.api.support import (
     admin_method,
     operation,
@@ -90,8 +91,13 @@ class UsersHandler(OperationsHandler):
             description=("Created %s '%s'." % (
                 'admin' if is_superuser else 'user', username)))
         if is_superuser:
-            return User.objects.create_superuser(
+            user = User.objects.create_superuser(
                 username=username, password=password, email=email)
+            if request.data.get('key') is not None:
+                request.user = user
+                sshkeys_handler = SSHKeysHandler()
+                sshkeys_handler.create(request)
+            return user
         else:
             return User.objects.create_user(
                 username=username, password=password, email=email)
diff --git a/src/maasserver/forms/__init__.py b/src/maasserver/forms/__init__.py
index fd66dfd..414e949 100644
--- a/src/maasserver/forms/__init__.py
+++ b/src/maasserver/forms/__init__.py
@@ -1125,9 +1125,13 @@ class SSHKeyForm(MAASModelForm):
 
     def save(self, endpoint, request):
         sshkey = super(SSHKeyForm, self).save()
+        if self.instance.user is request.user:
+            description = "Created SSH key."
+        else:
+            description = "Created SSH key for %s." % self.instance.user
         create_audit_event(
             EVENT_TYPES.AUTHORISATION, endpoint, request,
-            None, description="Created SSH key.")
+            None, description=description)
         return sshkey
 
 
diff --git a/src/maasserver/forms/tests/test_sshkey.py b/src/maasserver/forms/tests/test_sshkey.py
index 7f6e950..9d946e5 100644
--- a/src/maasserver/forms/tests/test_sshkey.py
+++ b/src/maasserver/forms/tests/test_sshkey.py
@@ -30,3 +30,18 @@ class TestSSHKeyForm(MAASServerTestCase):
         event = Event.objects.get(type__level=AUDIT)
         self.assertIsNotNone(event)
         self.assertEqual(event.description, "Created SSH key.")
+
+    def test_creates_audit_event_for_specified_user_on_save(self):
+        specified_user = factory.make_User()
+        request_user = factory.make_User()
+        key_string = get_data('data/test_rsa0.pub')
+        form = SSHKeyForm(
+            user=specified_user,
+            data={'key': key_string})
+        request = HttpRequest()
+        request.user = request_user
+        form.save(factory.pick_choice(ENDPOINT_CHOICES), request)
+        event = Event.objects.get(type__level=AUDIT)
+        self.assertIsNotNone(event)
+        self.assertEqual(
+            event.description, "Created SSH key for %s." % specified_user)