Merge pull request #211 from opendatacube/users

User creation improvements

datacube/index/_api.py

@@ -58,9 +58,7 @@ class Index(object):
     :type metadata_types: datacube.index._datasets.MetadataTypeResource
     """
     def __init__(self, db):
-        """
-        :type db: datacube.index.postgres._api.PostgresDb
-        """
+        # type: (PostgresDb) -> None
         self._db = db
 
         self.users = UserResource(db)
@@ -105,6 +103,7 @@ def __repr__(self):
 
 class UserResource(object):
     def __init__(self, db):
+        # type: (PostgresDb) -> None
         self._db = db
 
     def grant_role(self, role, *usernames):
@@ -114,12 +113,12 @@ def grant_role(self, role, *usernames):
         with self._db.connect() as connection:
             connection.grant_role(role, usernames)
 
-    def create_user(self, username, password, role):
+    def create_user(self, username, password, role, description=None):
         """
         Create a new user.
         """
         with self._db.connect() as connection:
-            connection.create_user(username, password, role)
+            connection.create_user(username, password, role, description=description)
 
     def delete_user(self, *usernames):
         """

datacube/index/postgres/_api.py

@@ -840,9 +840,9 @@ def list_users(self):
         for row in result:
             yield tables.from_pg_role(row['role_name']), row['user_name'], row['description']
 
-    def create_user(self, username, password, role):
+    def create_user(self, username, password, role, description=None):
         pg_role = tables.to_pg_role(role)
-        tables.create_user(self._connection, username, password, pg_role)
+        tables.create_user(self._connection, username, password, pg_role, description=description)
 
     def drop_users(self, users):
         # type: (Iterable[str]) -> None

datacube/index/postgres/tables/_core.py

@@ -211,25 +211,37 @@ def _ensure_role(engine, name, inherits_from=None, add_user=False, create_db=Fal
     engine.execute(' '.join(sql))
 
 
-def create_user(conn, username, key, role):
+def _escape_pg_identifier(engine, name):
+    # New (2.7+) versions of psycopg2 have function: extensions.quote_ident()
+    # But it's too bleeding edge right now. We'll ask the server to escape instead, as
+    # these are not performance sensitive.
+    return engine.execute("select quote_ident(%s)", name).scalar()
+
+
+def create_user(conn, username, key, role, description=None):
     if role not in USER_ROLES:
         raise ValueError('Unknown role %r. Expected one of %r' % (role, USER_ROLES))
-
+    username = _escape_pg_identifier(conn, username)
     conn.execute(
         'create user {username} password %s in role {role}'.format(username=username, role=role),
         key
     )
+    if description:
+        conn.execute(
+            'comment on role {username} is %s'.format(username=username), description
+        )
 
 
 def drop_user(engine, *usernames):
     for username in usernames:
-        engine.execute('drop role {username}'.format(username=username))
+        engine.execute('drop role {username}'.format(username=_escape_pg_identifier(engine, username)))
 
 
 def grant_role(engine, role, users):
     if role not in USER_ROLES:
         raise ValueError('Unknown role %r. Expected one of %r' % (role, USER_ROLES))
 
+    users = [_escape_pg_identifier(engine, user) for user in users]
     with engine.begin():
         engine.execute('revoke {roles} from {users}'.format(users=', '.join(users), roles=', '.join(USER_ROLES)))
         engine.execute('grant {role} to {users}'.format(users=', '.join(users), role=role))

datacube/scripts/user.py

@@ -5,6 +5,8 @@
 import logging
 import click
 
+from datacube.config import LocalConfig
+from datacube.index._api import Index
 from datacube.ui import click as ui
 from datacube.ui.click import cli
 
@@ -45,14 +47,16 @@ def grant(index, role, users):
 @click.argument('role',
                 type=click.Choice(USER_ROLES), nargs=1)
 @click.argument('user', nargs=1)
+@click.option('--description')
 @ui.pass_index()
 @ui.pass_config
-def create_user(config, index, role, user):
+def create_user(config, index, role, user, description):
+    # type: (LocalConfig, Index, str, str, str) -> None
     """
     Create a User
     """
     password = base64.urlsafe_b64encode(os.urandom(12)).decode('utf-8')
-    index.users.create_user(user, password, role)
+    index.users.create_user(user, password, role, description=description)
 
     click.echo('{host}:{port}:*:{username}:{password}'.format(
         host=config.db_hostname or 'localhost',

integration_tests/test_config_tool.py

@@ -4,15 +4,16 @@
 """
 from __future__ import absolute_import, print_function
 
+import logging
 import random
+from pathlib import Path
 
-import datacube.scripts.cli_app
-import logging
+import pytest
 from click.testing import CliRunner
 
+import datacube.scripts.cli_app
 from datacube.index.postgres import _dynamic
 from datacube.index.postgres.tables._core import drop_db, has_schema, SCHEMA_NAME
-from pathlib import Path
 
 _LOG = logging.getLogger(__name__)
 
@@ -153,7 +154,6 @@ def test_db_init_noop(global_integration_cli_args, local_config, ls5_telem_type)
 
 
 def test_db_init_rebuild(global_integration_cli_args, local_config, ls5_telem_type):
-
     # We set the field creation logging to debug, as we assert its logging output below.
     _dynamic._LOG.setLevel(logging.DEBUG)
 
@@ -196,7 +196,14 @@ def test_db_init(global_integration_cli_args, db, local_config):
         assert has_schema(db._engine, connection._connection)
 
 
-def test_user_creation(global_integration_cli_args, db, default_metadata_type):
+@pytest.mark.parametrize("username, user_description", [
+    ('test_"user"_{n}', None),
+    ('test_"user"_{n}', 'Test user description'),
+    # Test that names are escaped
+    ('user_"invalid+_chars_{n}', None),
+    ('user_invalid_desc_{n}', 'Invalid "\' chars in description'),
+])
+def test_user_creation(global_integration_cli_args, db, username, user_description, default_metadata_type):
     """
     Add a user, grant them, delete them.
 
@@ -207,41 +214,41 @@ def test_user_creation(global_integration_cli_args, db, default_metadata_type):
 
     print('{} mappings'.format(existing_mappings))
 
-    test_number = random.randint(111111, 999999)
-    user_name = 'test_user_{}'.format(test_number)
+    username = username.format(n=random.randint(111111, 999999))
 
     # No user exists.
-    assert_no_user(global_integration_cli_args, user_name)
+    assert_no_user(global_integration_cli_args, username)
 
     # Create them
+    args = ['-v', 'user', 'create', 'ingest', username]
+    if user_description:
+        args.extend(['--description', user_description])
     _run_cli(
         global_integration_cli_args,
         datacube.scripts.cli_app.cli,
-        [
-            '-v', 'user', 'create', 'ingest', user_name
-        ]
+        args
     )
-    assert_user_with_role(global_integration_cli_args, 'ingest', user_name)
+    assert_user_with_role(global_integration_cli_args, 'ingest', username)
 
     # Grant them 'manage' permission
     _run_cli(
         global_integration_cli_args,
         datacube.scripts.cli_app.cli,
         [
-            '-v', 'user', 'grant', 'manage', user_name
+            '-v', 'user', 'grant', 'manage', username
         ]
     )
-    assert_user_with_role(global_integration_cli_args, 'manage', user_name)
+    assert_user_with_role(global_integration_cli_args, 'manage', username)
 
     # Delete them
     _run_cli(
         global_integration_cli_args,
         datacube.scripts.cli_app.cli,
         [
-            '-v', 'user', 'delete', user_name
+            '-v', 'user', 'delete', username
         ]
     )
-    assert_no_user(global_integration_cli_args, user_name)
+    assert_no_user(global_integration_cli_args, username)
 
 
 def assert_user_with_role(global_integration_cli_args, role, user_name):