Merge lp:~ahasenack/landscape-charm/create-account-first-admin into lp:~landscape/landscape-charm/trunk

[Andreas Hasenack]

New charm config options for the creation of the first landscape administrator.

Three new config options that allow for the creation of the first landscape administrator. They can also be set after a deployment is done, but unless the database is empty, it's a noop.

I used ./schema instead of the BootstrapLDS API call because this call is not available in LDS 13.09, which is our currently released version.

I kept with the tradition of the existing code and used mocker a lot in the tests. I didn't like to have to use the juju log messages to determine why a method exited. I was tempted to set specific return values, or raise exceptions, but they would only be used by the tests, so I didn't do it.

[Adam Collard (adam-collard)]

Needs Fixing because of the horrible bit twiddling. Would otherwise be Needs Information as to why we use ./schema and not the API.

review: Needs Fixing

[Andreas Hasenack (ahasenack)]

Bits are so nice! Machine language! :)

[Andreas Hasenack (ahasenack)]

I used ./schema because 13.09, the currently released LDS, does not have that new api call.

[Adam Collard (adam-collard)]

> I used ./schema because 13.09, the currently released LDS, does not have that
> new api call.

Ah ok. Fair enough

[Chad Smith (chad.smith)]

Initial set of comments. Looks good so far. I want to try a deploy first and then I'll approve. I'm going to look over those unit tests again, it seems like it's sort of painful how we set it up. Not sure if we care about tackling that in this branch but it's something we need to refine at some point.

[Chad Smith (chad.smith)]

Looks good. works fine.

One thing we might want to consider at some point, since you are using schema directly. We aren't doing any validation on email address format specified in sample/sample.py. So it might be good to safeguard that configuration value with an email format validation so we can throw a config hook error exit(1) if that value is not correct so we don't configure the DB with an invalid email. Just a thought

review: Approve

[Adam Collard (adam-collard)]

Sorry for blocking this for so long, I don't have time to review it properly, so Abstaining.

review: Abstain

[Björn Tillenius (bjornt)]

I think this can be merged. I have some minor comments, but nothing blocking, some are just suggestions, and I'm not expecting them to be fixed here.

I also don't like the mocking in the tests, but I think it's quite a lot of work getting rid of it.

review: Approve

[lp:~ahasenack/landscape-charm/create-account-first-admin]

179. By Andreas Hasenack on 2014-10-01

Encode admin details at the last possible moment.

180. By Andreas Hasenack on 2014-10-01

Simplify test_first_admin_not_created_without_name_email_password() a bit

181. By Andreas Hasenack on 2014-10-01

create_landscape_admin() and _create_first_admin() now return True or
False.

=== modified file 'config.yaml'
--- config.yaml	2014-04-24 20:25:07 +0000
+++ config.yaml	2014-10-01 20:17:48 +0000
@@ -50,7 +50,24 @@
             schema (instead of just validating that it is correct).  This
             should only typically be enabled as part of a landscape maintenance
             cycle.
-        
-    
-
-
+    admin-name:
+        description: |
+            Name of the account administrator. If this, admin-email and
+            admin-password are specified, then the standalone account will be
+            created as part of the deployment, and this person will be its
+            administrator. If not specified, Landscape will prompt for these
+            details when it is first accessed with a web browser. Has no
+            effect if changed after a deployment.
+        type: string
+    admin-email:
+        description: |
+            Email of the account administrator. To have any effect, needs to
+            specified in conjunction with admin-name and admin-password. Has
+            no effect if changed after a deployment.
+        type: string
+    admin-password:
+        description: |
+            Password of the account administrator. To have any effect, needs
+            to be specified in conjunction with admin-name and admin-email.
+            Has no effect if changed after a deployment.
+        type: string

=== modified file 'config/landscape-deployments.yaml'
--- config/landscape-deployments.yaml	2014-09-15 20:26:59 +0000
+++ config/landscape-deployments.yaml	2014-10-01 20:17:48 +0000
@@ -39,6 +39,10 @@
                 services: static appserver pingserver combo-loader async-frontend apiserver package-upload jobhandler package-search cron juju-sync
                 repository: include-file://repo-file
                 license-file: include-file://license-file
+                # Optionally create the first admin:
+                #admin-name: Stan Peters
+                #admin-email: [e-mail]
+                #admin-password: pwd
         landscape-msg:
             branch: lp:~landscape/landscape-charm/stable
             charm: landscape

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-07-09 21:11:25 +0000
+++ hooks/hooks.py	2014-10-01 20:17:48 +0000
@@ -193,6 +193,52 @@
         juju.juju_log(str(e), level="DEBUG")
 
 
+def _get_db_access_details():
+    """
+    Returns the main database access details as they are set in the landscape
+    service configuration file.
+    """
+    config_obj = _get_config_obj(LANDSCAPE_SERVICE_CONF)
+    try:
+        section = config_obj["stores"]
+        database = section["main"]
+        db_host = section["host"]
+        db_user = section["user"]
+        db_password = section["password"]
+    except KeyError:
+        return None
+    return (database, db_host, db_user, db_password)
+
+
+def _create_first_admin():
+    """
+    If so requested by the presence of the right configuration keys,
+    tries to create the landscape first administrator and, as a consequence,
+    the standalone account too.
+    """
+    first_admin_email = juju.config_get("admin-email")
+    first_admin_name = juju.config_get("admin-name")
+    first_admin_password = juju.config_get("admin-password")
+    if not all((first_admin_email, first_admin_name, first_admin_password)):
+        juju.juju_log("Not creating a Landscape administrator: need "
+                      "admin-email, admin-name and admin-password.")
+        return False
+    juju.juju_log("First admin creation requested")
+    access_details = _get_db_access_details()
+    if not access_details:
+        juju.juju_log("No DB configuration yet, bailing.")
+        return False
+    database, db_host, db_user, db_password = access_details
+    if util.is_db_up(database, db_host, db_user, db_password):
+        with closing(util.connect_exclusive(db_host, db_user, db_password)):
+            return util.create_landscape_admin(
+                db_user, db_password, db_host, first_admin_name,
+                first_admin_email, first_admin_password)
+    else:
+        juju.juju_log("Can't talk to the DB yet, bailing.")
+        return False
+
+
 def amqp_relation_joined():
     juju.relation_set("username=landscape")
     juju.relation_set("vhost=landscape")
@@ -350,16 +396,11 @@
 
     notify_vhost_config_relation(os.environ.get("JUJU_RELATION_ID", None))
 
-    config_obj = _get_config_obj(LANDSCAPE_SERVICE_CONF)
-    try:
-        section = config_obj["stores"]
-        database = section["main"]
-        host = section["host"]
-        user = section["user"]
-        password = section["password"]
-    except KeyError:
+    access_details = _get_db_access_details()
+    if not access_details:
         juju.juju_log("Database not ready yet, deferring call")
         sys.exit(0)
+    database, host, user, password = access_details
 
     relids = juju.relation_ids("vhost-config")
     if relids:
@@ -413,6 +454,7 @@
     _set_maintenance()
     _enable_services()
     _set_upgrade_schema()
+    _create_first_admin()
 
     if _is_db_up() and _is_amqp_up():
         _lsctl("start")
@@ -726,17 +768,11 @@
     """
     Return True if the database is accessible and read/write, False otherwise.
     """
-    config_obj = _get_config_obj(LANDSCAPE_SERVICE_CONF)
-    try:
-        section = config_obj["stores"]
-        database = section["main"]
-        host = section["host"]
-        user = section["user"]
-        password = section["password"]
-    except KeyError:
+    access_details = _get_db_access_details()
+    if not access_details:
         return False
-    else:
-        return util.is_db_up(database, host, user, password)
+    database, host, user, password = access_details
+    return util.is_db_up(database, host, user, password)
 
 
 ERROR_PATH = "/opt/canonical/landscape/canonical/landscape/static/offline/"

=== modified file 'hooks/lib/util.py'
--- hooks/lib/util.py	2014-06-14 03:49:47 +0000
+++ hooks/lib/util.py	2014-10-01 20:17:48 +0000
@@ -5,10 +5,26 @@
 from psycopg2 import connect, Error as psycopg2Error
 from juju import Juju
 from contextlib import closing
+from subprocess import check_output
+
+import re
+import os
 
 juju = Juju()
 
 
+def is_email_valid(email):
+    """
+    Returns true if the given email is safe to use and has no "funny"
+    characters. We don't go overboard and look for an RFC compliant email
+    here.
+    
+    @param email: string containing the email to be validated
+    """
+    valid_email_re = r"^[\w.+-]+@[\w-]+\.[\w.]+$"
+    return re.search(valid_email_re, email) is not None
+
+
 def connect_exclusive(host, admin_user, admin_password):
     """
     database user creation and setup-landscape-server need to be done in a
@@ -49,6 +65,51 @@
         conn.close()
 
 
+def account_is_empty(db_user, db_password, db_host):
+    """
+    Returns true if the person and account tables from the
+    landscape-standalone-main database are empty.
+    """
+    with closing(connect(database="landscape-standalone-main", host=db_host,
+                         user=db_user, password=db_password)) as conn:
+        cur = conn.cursor()
+        cur.execute("SELECT COUNT(person.id),COUNT(account.id) FROM "
+                    "person,account")
+        result = cur.fetchall()[0]
+        return int(result[0]) == 0 and int(result[1]) == 0
+
+
+def create_landscape_admin(db_user, db_password, db_host, admin_name,
+                           admin_email, admin_password):
+    """
+    Create the first Landscape administrator with the given credentials.
+    Returns True if the administrator was created, False otherwise.
+    """
+    if account_is_empty(db_user, db_password, db_host):
+        if not is_email_valid(admin_email):
+            raise ValueError("Invalid administrator email %s" % admin_email)
+        juju.juju_log("Creating first administrator")
+        env = os.environ.copy()
+        env["LANDSCAPE_CONFIG"] = "standalone"
+        admin_name = admin_name.encode("utf-8")
+        admin_email = admin_email.encode("utf-8")
+        admin_password = admin_password.encode("utf-8")
+        cmd = ["./schema", "--create-lds-account-only", "--admin-name",
+               admin_name, "--admin-email", admin_email,
+               "--admin-password", admin_password]
+        # Throw stdout away, bceause when the call works, stdout will have API
+        # credentials, which we don't want in the juju logs. When the call
+        # fails, however, we want stderr because it will say why it failed, so
+        # let the exception be raised and stderr go through as usual.
+        check_output(cmd, cwd="/opt/canonical/landscape", env=env)
+        juju.juju_log("Administrator called %s with email %s created" %
+            (admin_name, admin_email))
+        return True
+    else:
+        juju.juju_log("DB not empty, skipping first admin creation")
+        return False
+
+
 def change_root_url(database, user, password, host, url):
     """Change the root url in the database."""
     url = "u%s:%s" % (len(url), url)

=== modified file 'hooks/test_hooks.py'
--- hooks/test_hooks.py	2014-07-08 19:21:33 +0000
+++ hooks/test_hooks.py	2014-10-01 20:17:48 +0000
@@ -1,5 +1,6 @@
+from configobj import ConfigObj
+from itertools import product
 import base64
-from configobj import ConfigObj
 import hooks
 import mocker
 import os
@@ -8,6 +9,7 @@
 import stat
 import subprocess
 import tempfile
+import unittest
 import yaml
 
 
@@ -28,7 +30,10 @@
             "license-file": "LICENSE_FILE_TEXT",
             "service-count": "msgserver:2 pingserver:1",
             "upgrade-schema": False,
-            "maintenance": False}
+            "maintenance": False,
+            "admin-name": None,
+            "admin-email": None,
+            "admin-password": None}
 
     def relation_set(self, *args, **kwargs):
         """
@@ -92,6 +97,7 @@
     def setUp(self):
         hooks._lsctl = lambda x: True
         hooks.juju = TestJuju()
+        hooks.util.juju = hooks.juju
         hooks.LANDSCAPE_LICENSE_DEST = self.makeFile()
         hooks.LANDSCAPE_DEFAULT_FILE = self.makeFile()
         self._default_file = open(hooks.LANDSCAPE_DEFAULT_FILE, "w")
@@ -137,6 +143,264 @@
 
 class TestHooksService(TestHooks):
 
+    def test_first_admin_not_created_without_name_email_password(self):
+        """
+        The first admin is not created when only one or two of name, email and
+        password are given.
+        """
+        admins = [("Foo Bar", "[e-mail]", None),
+                  ("Foo Bar", None, "secret"),
+                  (None, "[e-mail]", "secret")]
+        for name, email, password in admins:
+            hooks.juju.config["admin-name"] = name
+            hooks.juju.config["admin-email"] = email
+            hooks.juju.config["admin-password"] = password
+            self.assertFalse(hooks._create_first_admin())
+            hooks.juju._logs = ()
+
+    def test_first_admin_not_created_if_no_db_config(self):
+        """
+        The first administrator is not created if there is no database
+        configuration in the service.conf file.
+        """
+        messages = ("First admin creation requested",
+                    "No DB configuration yet, bailing.")
+        hooks.juju.config["admin-name"] = "Foo Bar"
+        hooks.juju.config["admin-email"] = "[e-mail]"
+        hooks.juju.config["admin-password"] = "secret"
+        config_obj = ConfigObj(hooks.LANDSCAPE_SERVICE_CONF)
+        config_obj["stores"] = {}
+        config_obj.write()
+        self.assertFalse(hooks._create_first_admin())
+        self.assertEqual(messages, hooks.juju._logs)
+
+    def test_email_syntax_check(self):
+        """
+        Invalid email addresses are flagged as such.
+        """
+        # some invalid choices
+        emails = ["invalidemail", "invalid@", "a@b", "a@b.",
+                  "`cat /etc/password`@example.com", "#[e-mail]",
+                  "(foo)@example.com", "foo@(example).com",
+                  "'[e-mail]", "\"[e-mail]"]
+        for email in emails:
+            self.assertIs(False, hooks.util.is_email_valid(email))
+
+    def test_create_landscape_admin_checks_email_syntax(self):
+        """
+        The util.create_landscape_admin() method verifies if the email is
+        valid before attempting to create the admin.
+        """
+        db_user = "user"
+        db_password = "password"
+        db_host = "example.com"
+        admin_name = "Foo Bar"
+        admin_email = "foo'@bar"
+        admin_password = "secret"
+
+        account_is_empty = self.mocker.replace(hooks.util.account_is_empty)
+        account_is_empty(db_user, db_password, db_host)
+        self.mocker.result(True)
+        self.mocker.replay()
+        with unittest.TestCase.assertRaises(self, ValueError) as invalid_email:
+            hooks.util.create_landscape_admin(db_user, db_password, db_host,
+                admin_name, admin_email, admin_password)
+        self.assertEqual("Invalid administrator email %s" % admin_email,
+            invalid_email.exception.message)
+
+    def test_first_admin_not_created_if_account_not_empty(self):
+        """
+        The first administrator is not created if the account is not
+        empty.
+        """
+        db_user = "user"
+        db_password = "password"
+        db_host = "example.com"
+        admin_name = "Foo Bar"
+        admin_email = "[e-mail]"
+        admin_password = "secret"
+        message = "DB not empty, skipping first admin creation"
+
+        account_is_empty = self.mocker.replace(hooks.util.account_is_empty)
+        account_is_empty(db_user, db_password, db_host)
+        self.mocker.result(False)
+        self.mocker.replay()
+        admin_created = hooks.util.create_landscape_admin(
+            db_user, db_password, db_host, admin_name, admin_email,
+            admin_password)
+        self.assertFalse(admin_created)
+        self.assertEqual((message,), hooks.juju._logs)
+
+    def test__create_first_admin_calls_create_landscape_admin(self):
+        """
+        When all conditions are met, _create_first_admin() calls
+        create_landscape_admin.
+        """
+        # juju log message we expect
+        message = "First admin creation requested"
+
+        # we have an admin user defined
+        admin_name = "Foo Bar"
+        admin_email = "[e-mail]"
+        admin_password = "secret"
+        hooks.juju.config["admin-name"] = admin_name
+        hooks.juju.config["admin-email"] = admin_email
+        hooks.juju.config["admin-password"] = admin_password
+
+        # we have the database access details
+        config_obj = ConfigObj(hooks.LANDSCAPE_SERVICE_CONF)
+        database = "mydb"
+        db_host = "myhost"
+        db_user = "myuser"
+        db_password = "mypassword"
+        stores = {"main": database, "host": db_host, "user": db_user,
+                  "password": db_password}
+        config_obj["stores"] = stores
+        config_obj.write()
+
+        # the db is up
+        is_db_up = self.mocker.replace(hooks.util.is_db_up)
+        is_db_up(database, db_host, db_user, db_password)
+        self.mocker.result(True)
+
+        # we can connect
+        connect_exclusive = self.mocker.replace(hooks.util.connect_exclusive)
+        connect_exclusive(db_host, db_user, db_password)
+        connection = self.mocker.mock()
+        self.mocker.result(connection)
+
+        # util.create_landscape_admin is called
+        create_landscape_admin = self.mocker.replace(
+            hooks.util.create_landscape_admin)
+        create_landscape_admin(
+            db_user, db_password, db_host, admin_name, admin_email,
+            admin_password)
+        connection.close()
+        self.mocker.replay()
+
+        hooks._create_first_admin()
+        self.assertEqual((message,), hooks.juju._logs)
+
+    def test__create_first_admin_bails_if_db_is_not_up(self):
+        """
+        The _create_first_admin() method gives up if the DB is not
+        accessible.
+        """
+        # juju log messages we expect
+        messages = ("First admin creation requested",
+                    "Can't talk to the DB yet, bailing.")
+        # we have an admin user defined
+        admin_name = "Foo Bar"
+        admin_email = "[e-mail]"
+        admin_password = "secret"
+        hooks.juju.config["admin-name"] = admin_name
+        hooks.juju.config["admin-email"] = admin_email
+        hooks.juju.config["admin-password"] = admin_password
+
+        # we have the database access details
+        config_obj = ConfigObj(hooks.LANDSCAPE_SERVICE_CONF)
+        database = "mydb"
+        db_host = "myhost"
+        db_user = "myuser"
+        db_password = "mypassword"
+        stores = {"main": database, "host": db_host, "user": db_user,
+                  "password": db_password}
+        config_obj["stores"] = stores
+        config_obj.write()
+
+        # the db is down, though
+        is_db_up = self.mocker.replace(hooks.util.is_db_up)
+        is_db_up(database, db_host, db_user, db_password)
+        self.mocker.result(False)
+
+        self.mocker.replay()
+        admin_created = hooks._create_first_admin()
+        self.assertFalse(admin_created)
+        self.assertEqual(messages, hooks.juju._logs)
+
+    def test__get_db_access_details(self):
+        """
+        The _get_db_access_details() function returns the database name and
+        access details when all these keys exist in the landscape service
+        configuration file.
+        """
+        config_obj = ConfigObj(hooks.LANDSCAPE_SERVICE_CONF)
+        stores = {"main": "mydb", "host": "myhost", "user": "myuser",
+                  "password": "mypassword"}
+        config_obj["stores"] = stores
+        config_obj.write()
+
+        result = hooks._get_db_access_details()
+        database, db_host, db_user, db_password = result
+        self.assertEqual(database, stores["main"])
+        self.assertEqual(db_host, stores["host"])
+        self.assertEqual(db_user, stores["user"])
+        self.assertEqual(db_password, stores["password"])
+
+    def test_no_db_access_details_if_missing_config_key(self):
+        """
+        The _get_db_access_details() function returns None if any of
+        the needed configuration keys is missing.
+        """
+        full_config = {"main": "mydb", "host": "myhost", "user": "myuser",
+                       "password": "mypassword"}
+        config_obj = ConfigObj(hooks.LANDSCAPE_SERVICE_CONF)
+        # remove one key each time
+        for key in full_config:
+            data = full_config.copy()
+            data.pop(key)
+            config_obj["stores"] = data
+            config_obj.write()
+            result = hooks._get_db_access_details()
+            self.assertIsNone(result)
+        # last try, with no data at all
+        config_obj.clear()
+        config_obj.write()
+        self.assertIsNone(hooks._get_db_access_details())
+
+    def test_create_landscape_admin_calls_schema_script(self):
+        """
+        The create_landscape_admin() method calls the landscape schema
+        script with the right parameters.
+        """
+        # we have an admin user defined
+        admin_name = "Foo Bar"
+        admin_email = "[e-mail]"
+        admin_password = "secret"
+
+        # juju log messages we expect
+        messages = ("Creating first administrator",
+                    "Administrator called %s with email %s created" %
+                    (admin_name, admin_email))
+
+        # we have the database access details
+        db_host = "myhost"
+        db_user = "myuser"
+        db_password = "mypassword"
+
+        # account is empty
+        account_is_empty = self.mocker.replace(hooks.util.account_is_empty)
+        account_is_empty(db_user, db_password, db_host)
+        self.mocker.result(True)
+
+        # schema script is called with the right parameters
+        self.addCleanup(setattr, hooks.util.os, "environ",
+                        hooks.util.os.environ)
+        hooks.util.os.environ = {}
+        env = {"LANDSCAPE_CONFIG": "standalone"}
+        schema_call = self.mocker.replace(hooks.util.check_output)
+        cmd = ["./schema", "--create-lds-account-only", "--admin-name",
+               admin_name, "--admin-email", admin_email, "--admin-password",
+               admin_password]
+        schema_call(cmd, cwd="/opt/canonical/landscape", env=env)
+
+        self.mocker.replay()
+
+        hooks.util.create_landscape_admin(
+            db_user, db_password, db_host, admin_name, admin_email,
+            admin_password)
+        self.assertEqual(messages, hooks.juju._logs)
+
     def test_get_services_non_proxied(self):
         """
         helper method should not break if non-proxied services are called for