sts-sponsors team mailing list archive

[Jacopo Rota <mp+442597@xxxxxxxxxxxxxxxxxx>]

Jacopo Rota has proposed merging ~r00ta/maas:bug-1823153 into maas:master.

Commit message:
createadmin cli command improvements: check if user already exists and fail if ssh keys are invalid

Requested reviews:
  Jacopo Rota (r00ta)
Related bugs:
  Bug #1823153 in MAAS: "maas init doesn't check if the user or email already exists"
  https://bugs.launchpad.net/maas/+bug/1823153

For more details, see:
https://code.launchpad.net/~r00ta/maas/+git/maas/+merge/442597

This merge proposal aims to fix https://bugs.launchpad.net/maas/+bug/1823153 . 

It introduces two small improvements: 

1) the user is not created if there is a failure in the ssh import
2) if the user already exist the error message is not a stacktrace but a clear message. 

here's the new interaction 

```
ubuntu@maas-edge:/work$ sudo maas createadmin
Username: aa
Password: 
Again: 
Email: aa
Import SSH keys [] (lp:user-id or gh:user-id): lp:fds
SSHKeysError: Importing SSH keys failed.
Unable to import SSH keys. There are no SSH keys for launchpad user fds.

ubuntu@maas-edge:/work$ sudo maas createadmin
Username: aa
Password: 
Again: 
Email: aa
Import SSH keys [] (lp:user-id or gh:user-id): 

ubuntu@maas-edge:/work$ sudo maas createadmin
Username: aa
Password: 
Again: 
Email: aa
Import SSH keys [] (lp:user-id or gh:user-id): 
AlreadyExistingUser: A user with the email aa already exists.
```

On the implementation side the fix was simply to wrap the creation of the user and the ssh keys in a transaction and raise the exceptions properly. 

The merge proposal includes some small refactorings of the impacted classes to introduce type hints. 
-- 
Your team MAAS Committers is subscribed to branch maas:master.

diff --git a/src/maasserver/management/commands/createadmin.py b/src/maasserver/management/commands/createadmin.py
index 0465bf1..1ad863d 100644
--- a/src/maasserver/management/commands/createadmin.py
+++ b/src/maasserver/management/commands/createadmin.py
@@ -3,13 +3,12 @@
 
 """Django command: create an administrator account."""
 
-
 from getpass import getpass
 import re
 
 from django.contrib.auth.models import User
 from django.core.management.base import BaseCommand, CommandError
-from django.db import DEFAULT_DB_ALIAS
+from django.db import DEFAULT_DB_ALIAS, IntegrityError, transaction
 import requests
 
 from maascli.init import read_input
@@ -31,11 +30,15 @@ class EmptyEmail(CommandError):
     """User did not provide an email."""
 
 
+class AlreadyExistingUser(CommandError):
+    """A user with the given email already exists."""
+
+
 class SSHKeysError(CommandError):
     """Error during SSH keys import."""
 
 
-def read_password(prompt):
+def read_password(prompt: str):
     while True:
         try:
             data = getpass(prompt)
@@ -53,7 +56,7 @@ def read_password(prompt):
             return data
 
 
-def prompt_for_username():
+def prompt_for_username() -> str:
     username = read_input("Username: ")
     if not username:
         raise EmptyUsername(
@@ -62,7 +65,7 @@ def prompt_for_username():
     return username
 
 
-def prompt_for_password():
+def prompt_for_password() -> str:
     """Prompt user for a choice of password, and confirm."""
     password = read_password("Password: ")
     confirm = read_password("Again: ")
@@ -73,7 +76,7 @@ def prompt_for_password():
     return password
 
 
-def prompt_for_email():
+def prompt_for_email() -> str:
     """Prompt user for an email."""
     email = read_input("Email: ")
     if not email:
@@ -81,12 +84,12 @@ def prompt_for_email():
     return email
 
 
-def prompt_for_ssh_import():
+def prompt_for_ssh_import() -> str:
     """Prompt user for protocal and user-id to import SSH keys."""
     return read_input("Import SSH keys [] (lp:user-id or gh:user-id): ")
 
 
-def validate_ssh_import(ssh_import):
+def validate_ssh_import(ssh_import: str):
     """Validate user's SSH import input."""
     if ssh_import.startswith(("lp", "gh")):
         import_regex = re.compile(r"^(?:lp|gh):[\w-]*$")
@@ -156,20 +159,27 @@ class Command(BaseCommand):
         if prompt_ssh_import:
             ssh_import = prompt_for_ssh_import()
 
-        User.objects.db_manager(DEFAULT_DB_ALIAS).create_superuser(
-            username, email=email, password=password
-        )
-
-        if ssh_import:  # User entered input
-            protocol, auth_id = validate_ssh_import(ssh_import)
-            user = User.objects.get(username=username)
+        with transaction.atomic():
             try:
-                KeySource.objects.save_keys_for_user(
-                    user=user, protocol=protocol, auth_id=auth_id
+                User.objects.db_manager(DEFAULT_DB_ALIAS).create_superuser(
+                    username, email=email, password=password
                 )
-            except ImportSSHKeysError as e:
-                return e.args[0]
-            except requests.exceptions.RequestException as e:
-                raise SSHKeysError(
-                    "Importing SSH keys failed.\n%s" % e.args[0]
+            except IntegrityError as e:
+                raise AlreadyExistingUser(
+                    "A user with the email %s already exists." % email
                 )
+
+            if ssh_import:  # User entered input
+                protocol, auth_id = validate_ssh_import(ssh_import)
+                user = User.objects.get(username=username)
+                try:
+                    KeySource.objects.save_keys_for_user(
+                        user=user, protocol=protocol, auth_id=auth_id
+                    )
+                except (
+                    ImportSSHKeysError,
+                    requests.exceptions.RequestException,
+                ) as e:
+                    raise SSHKeysError(
+                        "Importing SSH keys failed.\n%s" % e.args[0]
+                    )
diff --git a/src/maasserver/models/keysource.py b/src/maasserver/models/keysource.py
index 4fb130c..60ade44 100644
--- a/src/maasserver/models/keysource.py
+++ b/src/maasserver/models/keysource.py
@@ -4,6 +4,7 @@
 """:class:`KeySource` and friends."""
 
 
+from django.contrib.auth.models import User
 from django.db.models import BooleanField, CharField, Manager
 
 from maasserver.enum import KEYS_PROTOCOL_TYPE_CHOICES
@@ -15,7 +16,7 @@ from maasserver.utils.keys import get_protocol_keys
 class KeySourceManager(Manager):
     """A utility to manage the colletion of `KeySource`s."""
 
-    def save_keys_for_user(self, user, protocol, auth_id):
+    def save_keys_for_user(self, user: User, protocol: str, auth_id: str):
         """Save SSH Keys for user's protocol and auth_id.
 
         :param user: The user to save the SSH keys for.
@@ -60,7 +61,7 @@ class KeySource(CleanSave, TimestampedModel):
     def __str__(self):
         return f"{self.protocol}:{self.auth_id}"
 
-    def import_keys(self, user):
+    def import_keys(self, user: User):
         """Save SSH keys."""
         # Avoid circular imports.
         from maasserver.models.sshkey import SSHKey
diff --git a/src/maasserver/tests/test_commands.py b/src/maasserver/tests/test_commands.py
index 4e2cfef..8043c7d 100644
--- a/src/maasserver/tests/test_commands.py
+++ b/src/maasserver/tests/test_commands.py
@@ -281,6 +281,34 @@ class TestCommands(MAASServerTestCase):
             stderr=stderr,
             stdout=stdout,
         )
+        self.assertEqual(len(User.objects.filter(username=username)), 0)
+
+    def test_createadmin_user_already_exists(self):
+        stderr = StringIO()
+        stdout = StringIO()
+        username = factory.make_string()
+        password = factory.make_string()
+        email = "%s@xxxxxxxxxxx" % factory.make_string()
+        call_command(
+            "createadmin",
+            username=username,
+            password=password,
+            email=email,
+            stderr=stderr,
+            stdout=stdout,
+        )
+        self.assertEqual(len(User.objects.filter(username=username)), 1)
+
+        self.assertRaises(
+            createadmin.AlreadyExistingUser,
+            call_command,
+            "createadmin",
+            username=username,
+            password=password,
+            email=email,
+            stderr=stderr,
+            stdout=stdout,
+        )
 
     def test_prompt_for_password_returns_selected_password(self):
         password = factory.make_string()
diff --git a/src/maasserver/utils/keys.py b/src/maasserver/utils/keys.py
index ab39057..50d0d18 100644
--- a/src/maasserver/utils/keys.py
+++ b/src/maasserver/utils/keys.py
@@ -6,6 +6,7 @@
 
 import http
 import logging
+from typing import List
 
 import requests
 
@@ -29,7 +30,7 @@ def get_proxies():
     return proxies
 
 
-def get_protocol_keys(protocol, auth_id):
+def get_protocol_keys(protocol: str, auth_id: str) -> List[str]:
     """Retrieve SSH Keys for auth_id using protocol."""
     if protocol == KEYS_PROTOCOL_TYPE.LP:
         keys = get_launchpad_ssh_keys(auth_id)
@@ -44,7 +45,7 @@ def get_protocol_keys(protocol, auth_id):
     return keys
 
 
-def get_launchpad_ssh_keys(auth_id):
+def get_launchpad_ssh_keys(auth_id: str) -> List[str]:
     """Retrieve SSH Keys from launchpad."""
     url = "https://launchpad.net/~%s/+sshkeys"; % auth_id
     response = requests.get(url, proxies=get_proxies())
@@ -63,8 +64,8 @@ def get_launchpad_ssh_keys(auth_id):
     return [key for key in response.text.splitlines() if key]
 
 
-def get_github_ssh_keys(auth_id):
-    """Retrieve SSH Keys from github."""
+def get_github_ssh_keys(auth_id: str) -> List[str]:
+    """Retrieve SSH Keys from GitHub."""
     url = "https://api.github.com/users/%s/keys"; % auth_id
     response = requests.get(url, proxies=get_proxies())
     # Check for 404 error which happens for an unknown user