sts-sponsors team mailing list archive
-
sts-sponsors team
-
Mailing list archive
-
Message #08264
[Merge] ~r00ta/maas:bug-1823153 into maas:master
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
Follow ups