diff --git a/CHANGES.rst b/CHANGES.rst index b8bf7e5fd..46f034fcd 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,9 +1,21 @@ SimPhoNy Remote CHANGELOG ========================= -What's new in SimPhoNy Remote 1.2.0 +What's new in SimPhoNy Remote 2.0.0 ----------------------------------- +- Renamed some entities to more appropriate names (#419) + - The term Accounting now refers exclusively to the relationship table + connecting users, applications and policies. + - The config file accounting_class and accounting_kwargs are now + database_class and database_kwargs. + - AppAccounting is now ORMDatabase. Similarly, CSVAccounting is now + CSVDatabase. + - The above two break compatibility with the old configuration files. + Old configuration files must be changed both in the options + and the class they refer to. + + What's new in SimPhoNy Remote 1.1.0 ----------------------------------- diff --git a/doc/source/configuration.rst b/doc/source/configuration.rst index 4f2b25de2..59801c90c 100644 --- a/doc/source/configuration.rst +++ b/doc/source/configuration.rst @@ -80,6 +80,6 @@ docker setup. For example, to use CSV as the database, `/path/to/config.py` would contain the followings:: - accounting_class = 'remoteappmanager.db.csv_db.CSVAccounting' - accounting_kwargs = {'url': '/path/to/csv_file'} + database_class = 'remoteappmanager.db.csv_db.CSVDatabase' + database_kwargs = {'url': '/path/to/csv_file'} diff --git a/jupyterhub/remoteappmanager_config.py b/jupyterhub/remoteappmanager_config.py index 4e612861a..d71feecc9 100644 --- a/jupyterhub/remoteappmanager_config.py +++ b/jupyterhub/remoteappmanager_config.py @@ -54,14 +54,14 @@ # # # CSV database support # -# accounting_class = "remoteappmanager.db.csv_db.CSVAccounting" -# accounting_kwargs = { +# database_class = "remoteappmanager.db.csv_db.CSVDatabase" +# database_kwargs = { # "csv_file_path": os.path.abspath("./remoteappmanager.csv")} # # # Sqlite database support # -# accounting_class = "remoteappmanager.db.orm.AppAccounting" -# accounting_kwargs = { +# database_class = "remoteappmanager.db.orm.ORMDatabase" +# database_kwargs = { # "url": "sqlite:///"+os.path.abspath('./remoteappmanager.db')} # # ---------------- diff --git a/remoteappmanager/base_application.py b/remoteappmanager/base_application.py index 11496e1a1..5989b3580 100644 --- a/remoteappmanager/base_application.py +++ b/remoteappmanager/base_application.py @@ -8,7 +8,7 @@ from tornadowebapi.registry import Registry -from remoteappmanager.db.interfaces import ABCAccounting +from remoteappmanager.db.interfaces import ABCDatabase from remoteappmanager.logging.logging_mixin import LoggingMixin from remoteappmanager.docker.container_manager import ContainerManager from remoteappmanager.jinja2_adapters import Jinja2LoaderAdapter, gravatar_id @@ -33,7 +33,7 @@ class BaseApplication(web.Application, LoggingMixin): #: An accounting system that knows the allowed users, what applications #: they can run etc. - db = Instance(ABCAccounting, allow_none=True) + db = Instance(ABCDatabase, allow_none=True) #: API access to the configurable-http-proxy reverse_proxy = Instance(ReverseProxy) @@ -119,11 +119,11 @@ def _hub_default(self): @default("db") def _db_default(self): """Initializes the database connection.""" - class_path = self.file_config.accounting_class + class_path = self.file_config.database_class module_path, _, cls_name = class_path.rpartition('.') cls = getattr(importlib.import_module(module_path), cls_name) try: - return cls(**self.file_config.accounting_kwargs) + return cls(**self.file_config.database_kwargs) except Exception: reason = 'The database is not initialised properly.' self.log.exception(reason) diff --git a/remoteappmanager/cli/remoteappdb/__main__.py b/remoteappmanager/cli/remoteappdb/__main__.py index 9750a55be..6f3bd8f01 100644 --- a/remoteappmanager/cli/remoteappdb/__main__.py +++ b/remoteappmanager/cli/remoteappdb/__main__.py @@ -233,14 +233,14 @@ def list(ctx, no_decoration, show_apps): cur = [user.id, user.name] table.append(cur) if show_apps: - apps = [[app.image, - policy.allow_home, - policy.allow_view, - policy.allow_common, - policy.volume_source, - policy.volume_target, - policy.volume_mode] - for _, app, policy in orm.apps_for_user(session, user)] + apps = [[entry.application.image, + entry.application_policy.allow_home, + entry.application_policy.allow_view, + entry.application_policy.allow_common, + entry.application_policy.volume_source, + entry.application_policy.volume_target, + entry.application_policy.volume_mode] + for entry in orm.accounting_for_user(session, user)] if len(apps) == 0: apps = [['']*7] diff --git a/remoteappmanager/db/csv_db.py b/remoteappmanager/db/csv_db.py index 42cbd4afc..9630a131f 100644 --- a/remoteappmanager/db/csv_db.py +++ b/remoteappmanager/db/csv_db.py @@ -47,7 +47,7 @@ import uuid from remoteappmanager.db.interfaces import ( - ABCAccounting, ABCApplication, ABCApplicationPolicy) + ABCDatabase, ABCApplication, ABCApplicationPolicy, ABCAccounting) from remoteappmanager.db.exceptions import UnsupportedOperation from remoteappmanager.utils import mergedocs, one @@ -60,6 +60,10 @@ class CSVApplicationPolicy(ABCApplicationPolicy): pass +class CSVAccounting(ABCAccounting): + pass + + # FIXME: The user object could simply be a str (the username) # We need this because HomeHandler._start_container takes an object with # the username in its `name` attribute @@ -80,9 +84,9 @@ def __init__(self, id, name): 'policy.volume_mode') -@mergedocs(ABCAccounting) -class CSVAccounting(ABCAccounting): - """ Accounting class that reads a CSV file and is used by the +@mergedocs(ABCDatabase) +class CSVDatabase(ABCDatabase): + """ Database class that reads a CSV file and is used by the remoteappmanager. Currently only accepts one csv file. """ @@ -167,10 +171,13 @@ def __init__(self, csv_file_path, **kwargs): # Save the configuration # Note that we don't filter existing duplicate entry - self.all_records.setdefault(user.name, []).append(( - uuid.uuid4().hex, - application, - application_policy)) + self.all_records.setdefault(user.name, []).append( + CSVAccounting( + id=uuid.uuid4().hex, + user=user, + application=application, + application_policy=application_policy) + ) def get_user(self, *, user_name=None, id=None): if not one([user_name, id]): @@ -183,11 +190,11 @@ def get_user(self, *, user_name=None, id=None): else: raise RuntimeError("Impossible condition") # pragma: no cover - def get_apps_for_user(self, user): + def get_accounting_for_user(self, user): if user is not None: - return tuple(self.all_records.get(user.name, ())) + return self.all_records.get(user.name, []) else: - return () + return [] def create_user(self, user_name): raise UnsupportedOperation() diff --git a/remoteappmanager/db/interfaces.py b/remoteappmanager/db/interfaces.py index 1cd93ad9d..5ee9b4dea 100644 --- a/remoteappmanager/db/interfaces.py +++ b/remoteappmanager/db/interfaces.py @@ -57,6 +57,14 @@ def __repr__(self): class ABCAccounting(metaclass=ABCMeta): + def __init__(self, id, user, application, application_policy): + self.id = id + self.user = user + self.application = application + self.application_policy = application_policy + + +class ABCDatabase(metaclass=ABCMeta): """ Main accounting interface required by the single user application. """ @@ -80,8 +88,8 @@ def get_user(self, *, user_name=None, id=None): """ @abstractmethod - def get_apps_for_user(self, user): - """ Return an iterable of ApplicationConfig for a given user + def get_accounting_for_user(self, user): + """ Returns the accounting information for a given user Parameters ---------- @@ -90,10 +98,9 @@ def get_apps_for_user(self, user): Returns ------- - tuple - each item of the tuple should be a tuple of - (id, ABCApplication, ABCApplicationPolicy) where id is a string - used for identifying (ABCApplication, ABCApplicationPolicy) + list + each item of the list should be an instance satisfying the + ABCAccounting format (duck typing) """ @abstractmethod diff --git a/remoteappmanager/db/orm.py b/remoteappmanager/db/orm.py index 649e5e298..1b568d718 100644 --- a/remoteappmanager/db/orm.py +++ b/remoteappmanager/db/orm.py @@ -8,11 +8,11 @@ from sqlalchemy.engine import Engine from sqlalchemy.exc import OperationalError, IntegrityError from sqlalchemy.ext.declarative import declarative_base -from sqlalchemy.orm import sessionmaker, relationship +from sqlalchemy.orm import sessionmaker, relationship, joinedload from sqlalchemy.orm.exc import DetachedInstanceError, NoResultFound from remoteappmanager.logging.logging_mixin import LoggingMixin -from remoteappmanager.db.interfaces import ABCAccounting +from remoteappmanager.db.interfaces import ABCDatabase from remoteappmanager.db import exceptions from remoteappmanager.utils import parse_volume_string, mergedocs, one @@ -163,8 +163,8 @@ def reset(self): Base.metadata.create_all(self.engine) -@mergedocs(ABCAccounting) -class AppAccounting(ABCAccounting): +@mergedocs(ABCDatabase) +class ORMDatabase(ABCDatabase): def __init__(self, url, **kwargs): ''' Initialiser @@ -226,11 +226,11 @@ def get_user(self, *, user_name=None, id=None): return user - def get_apps_for_user(self, user): + def get_accounting_for_user(self, user): # We create a session here to make sure it is only # used in one thread with contextlib.closing(self.db.create_session()) as session: - result = apps_for_user(session, user) + result = accounting_for_user(session, user) # Removing internal references to the session is # required such that the objects can be reused @@ -436,25 +436,28 @@ def transaction(session): session.commit() -def apps_for_user(session, user): - """Returns a tuple of tuples, each containing an application and the - associated policy that the specified orm user is allowed to run. +def accounting_for_user(session, user): + """Returns a list of Accounting objects, each containing + an application and the associated policy that the specified orm user is + allowed to run. If the user is None, the default is to return an empty list. - The mapping_id is a unique string identifying the combination of + The id is a unique string identifying the combination of application and policy. It is not unique per user. + Parameters ---------- session : Session The current session user : User or None the orm User, or None. + Returns ------- - A tuple of tuples (mapping_id, orm.Application, orm.ApplicationPolicy) + A list of Accounting objects """ if user is None: - return () + return [] try: user_name = user.name @@ -466,10 +469,10 @@ def apps_for_user(session, user): session.add(user) user_name = user.name - res = session.query(Accounting).join( - Accounting.user, aliased=True).filter_by( - name=user_name).all() + res = session.query(Accounting) \ + .join(Accounting.user, aliased=True) \ + .options(joinedload(Accounting.application)) \ + .options(joinedload(Accounting.application_policy)) \ + .filter_by(name=user_name).all() - return tuple(((acc.id, - acc.application, - acc.application_policy) for acc in res)) + return res diff --git a/remoteappmanager/db/tests/abc_test_interfaces.py b/remoteappmanager/db/tests/abc_test_interfaces.py index b2f7719ce..8dd7b5457 100644 --- a/remoteappmanager/db/tests/abc_test_interfaces.py +++ b/remoteappmanager/db/tests/abc_test_interfaces.py @@ -35,31 +35,30 @@ def create_expected_configs(self, user): """ @abstractmethod - def create_accounting(self): + def create_database(self): """ Create an object that complies with ABCAccounting """ @abstractmethod def test_get_user(self): """ Test ABCDatabase.get_user """ - def test_get_apps_for_user(self): - """ Test get_apps_for_user returns an iterable of ApplicationConfig + def test_get_accounting_for_user(self): + """ Test get_accounting_for_user returns an iterable of ApplicationConfig """ - accounting = self.create_accounting() + database = self.create_database() - self.assertEqual(accounting.get_apps_for_user(None), ()) + self.assertEqual(database.get_accounting_for_user(None), []) for user in self.create_expected_users(): expected_configs = self.create_expected_configs(user) - # should be ((mapping_id, Application, ApplicationPolicy), - # (mapping_id, Application, ApplicationPolicy) ... ) - actual_id_configs = accounting.get_apps_for_user(user) + actual_id_configs = database.get_accounting_for_user(user) # should be ( (Application, ApplicationPolicy), # (Application, ApplicationPolicy) ... ) - actual_configs = tuple((app, policy) - for _, app, policy in actual_id_configs) + actual_configs = tuple((accounting.application, + accounting.application_policy) + for accounting in actual_id_configs) # Compare the content of list of (Application, ApplicationPolicy) # Note that their order does not matter @@ -85,36 +84,36 @@ def test_get_apps_for_user(self): if temp: self.fail('These are not expected: {}'.format(temp)) - def test_get_apps_for_user_mapping_id_rest_compliant(self): + def test_get_accounting_for_user_mapping_id_rest_compliant(self): ''' Test if the mapping_id to be rest identifier complient ''' allowed_chars = set(string.ascii_letters+string.digits) - accounting = self.create_accounting() + database = self.create_database() for user in self.create_expected_users(): # should be ((mapping_id, Application, ApplicationPolicy), # (mapping_id, Application, ApplicationPolicy) ... ) - actual_id_configs = accounting.get_apps_for_user(user) + actual_id_configs = database.get_accounting_for_user(user) if not actual_id_configs: continue - for mapping_id, _, _ in actual_id_configs: + for entry in actual_id_configs: self.assertFalse( - set(mapping_id) - allowed_chars, + set(entry.id) - allowed_chars, "mapping id should contain these characters only: {} " - "Got : {}".format(allowed_chars, mapping_id)) + "Got : {}".format(allowed_chars, entry.id)) def test_list_users(self): - accounting = self.create_accounting() + database = self.create_database() expected_names = sorted([user.name for user in self.create_expected_users()]) obtained_names = sorted([user.name - for user in accounting.list_users()]) + for user in database.list_users()]) self.assertEqual(expected_names, obtained_names) def test_list_applications(self): - accounting = self.create_accounting() + database = self.create_database() expected_images = set() for user in self.create_expected_users(): @@ -124,13 +123,13 @@ def test_list_applications(self): ) obtained_images = set( - [app.image for app in accounting.list_applications()] + [app.image for app in database.list_applications()] ) self.assertEqual(expected_images, obtained_images) def test_unsupported_ops(self): - db = self.create_accounting() + db = self.create_database() for method in [db.create_user, db.create_application, diff --git a/remoteappmanager/db/tests/test_csv_db.py b/remoteappmanager/db/tests/test_csv_db.py index 3c79b6395..381ab929c 100644 --- a/remoteappmanager/db/tests/test_csv_db.py +++ b/remoteappmanager/db/tests/test_csv_db.py @@ -2,7 +2,7 @@ import unittest from remoteappmanager.db.csv_db import ( - CSVApplication, CSVApplicationPolicy, CSVUser, CSVAccounting) + CSVApplication, CSVApplicationPolicy, CSVUser, CSVDatabase) from remoteappmanager.db.tests.abc_test_interfaces import ( ABCTestDatabaseInterface) from remoteappmanager.tests.temp_mixin import TempMixin @@ -57,8 +57,8 @@ def write_csv_file(file_name, headers, records): print(*record, sep=',', file=csv_file) -class TestCSVAccounting(TempMixin, ABCTestDatabaseInterface, - unittest.TestCase): +class TestCSVDatabase(TempMixin, ABCTestDatabaseInterface, + unittest.TestCase): def setUp(self): # Setup temporary directory super().setUp() @@ -103,25 +103,25 @@ def create_expected_configs(self, user): )} return mappings[user.name] - def create_accounting(self): - return CSVAccounting(self.csv_file) + def create_database(self): + return CSVDatabase(self.csv_file) def test_get_user(self): - accounting = self.create_accounting() + database = self.create_database() - user = accounting.get_user(user_name='markdoe') + user = database.get_user(user_name='markdoe') self.assertIsInstance(user, CSVUser) self.assertEqual(user.name, 'markdoe') - user = accounting.get_user(id=0) + user = database.get_user(id=0) self.assertIsInstance(user, CSVUser) self.assertEqual(user.name, 'markdoe') - user = accounting.get_user(user_name='unknown') + user = database.get_user(user_name='unknown') self.assertIsNone(user) with self.assertRaises(ValueError): - accounting.get_user(user_name='unknown', id=123) + database.get_user(user_name='unknown', id=123) def test_error_with_missing_headers(self): write_csv_file(self.csv_file, @@ -129,11 +129,11 @@ def test_error_with_missing_headers(self): BadTableMissingHeaders.records) with self.assertRaisesRegex(ValueError, "Missing headers"): - self.create_accounting() + self.create_database() def test_load_csv_file_with_good_headers_different_order(self): """ Test loading a good table with headers of different orders """ write_csv_file(self.csv_file, GoodTableWithDifferentHeaders.headers, GoodTableWithDifferentHeaders.records) - self.create_accounting() + self.create_database() diff --git a/remoteappmanager/db/tests/test_interfaces.py b/remoteappmanager/db/tests/test_interfaces.py index 82669bc17..d6f7cc0fe 100644 --- a/remoteappmanager/db/tests/test_interfaces.py +++ b/remoteappmanager/db/tests/test_interfaces.py @@ -8,7 +8,7 @@ from collections import namedtuple from remoteappmanager.db.interfaces import ( - ABCApplication, ABCApplicationPolicy, ABCAccounting) + ABCApplication, ABCApplicationPolicy, ABCDatabase, ABCAccounting) from remoteappmanager.db import exceptions from remoteappmanager.db.tests.abc_test_interfaces import ( ABCTestDatabaseInterface) @@ -25,18 +25,30 @@ class ApplicationPolicy(ABCApplicationPolicy): class Accounting(ABCAccounting): + pass + + +class Database(ABCDatabase): def get_user(self, *, user_name=None, id=None): return User(id=0, name=user_name) - def get_apps_for_user(self, user): + def get_accounting_for_user(self, user): if user is None: - return () - - return (('abc1', - Application(id=0, image=user.name+'1'), ApplicationPolicy()), - ('abc2', - Application(id=1, image=user.name+'2'), ApplicationPolicy())) + return [] + + return [ + Accounting( + id='abc1', + user=user, + application=Application(id=0, image=user.name+'1'), + application_policy=ApplicationPolicy() + ), + Accounting( + id='abc2', + user=user, + application=Application(id=1, image=user.name+'2'), + application_policy=ApplicationPolicy())] def create_user(self, user_name): raise exceptions.UnsupportedOperation() @@ -86,9 +98,9 @@ def create_expected_configs(self, user): return [(Application(id=0, image=user.name+'1'), ApplicationPolicy()), (Application(id=1, image=user.name+'2'), ApplicationPolicy())] - def create_accounting(self): - return Accounting() + def create_database(self): + return Database() def test_get_user(self): - database = self.create_accounting() + database = self.create_database() self.assertEqual(database.get_user(user_name='foo'), User(0, 'foo')) diff --git a/remoteappmanager/db/tests/test_orm.py b/remoteappmanager/db/tests/test_orm.py index 56e60aaa7..581605be0 100644 --- a/remoteappmanager/db/tests/test_orm.py +++ b/remoteappmanager/db/tests/test_orm.py @@ -7,7 +7,7 @@ from remoteappmanager.db import orm from remoteappmanager.db import exceptions from remoteappmanager.db.orm import (Database, transaction, Accounting, - AppAccounting) + ORMDatabase) from remoteappmanager.db.tests.abc_test_interfaces import ( ABCTestDatabaseInterface) from remoteappmanager.tests import utils @@ -107,40 +107,40 @@ def test_orm_objects(self): self.assertIn("docker/image0", [acc.application.image for acc in res]) - def test_apps_for_user(self): + def test_accounting_for_user(self): db = Database(url="sqlite:///"+self.sqlite_file_path) session = db.create_session() fill_db(session) with transaction(session): # verify back population users = session.query(orm.User).all() - res = orm.apps_for_user(session, users[1]) + res = orm.accounting_for_user(session, users[1]) self.assertEqual(len(res), 2) self.assertIn("docker/image0", - [acc[1].image for acc in res]) + [acc.application.image for acc in res]) self.assertIn("docker/image2", - [acc[1].image for acc in res]) + [acc.application.image for acc in res]) - res = orm.apps_for_user(session, users[2]) + res = orm.accounting_for_user(session, users[2]) self.assertEqual(len(res), 0) # User 0 should have access to app 1 - res = orm.apps_for_user(session, users[0]) + res = orm.accounting_for_user(session, users[0]) self.assertEqual(len(res), 1) self.assertIn("docker/image1", - [acc[1].image for acc in res]) + [acc.application.image for acc in res]) - res = orm.apps_for_user(session, users[3]) + res = orm.accounting_for_user(session, users[3]) self.assertEqual(len(res), 1) self.assertIn("docker/image0", - [acc[1].image for acc in res]) + [acc.application.image for acc in res]) - res = orm.apps_for_user(session, None) + res = orm.accounting_for_user(session, None) self.assertEqual(len(res), 0) -class TestOrmAppAccounting(TempMixin, ABCTestDatabaseInterface, - LogTrapTestCase): +class TestOrmDatabase(TempMixin, ABCTestDatabaseInterface, + LogTrapTestCase): def setUp(self): # Setup temporary directory super().setUp() @@ -171,238 +171,238 @@ def create_expected_configs(self, user): 'user4': ((apps[0], policy),)} return mappings[user.name] - def create_accounting(self): - accounting = AppAccounting( + def create_database(self): + database = ORMDatabase( url="sqlite:///"+self.sqlite_file_path) # Fill the database - with contextlib.closing(accounting.db.create_session()) as session: + with contextlib.closing(database.db.create_session()) as session: fill_db(session) - return accounting + return database def test_get_user(self): - accounting = self.create_accounting() + database = self.create_database() - user = accounting.get_user(user_name='user1') + user = database.get_user(user_name='user1') self.assertIsInstance(user, orm.User) - user = accounting.get_user(id=1) + user = database.get_user(id=1) self.assertIsInstance(user, orm.User) # user not found, result should be None - user = accounting.get_user(user_name='foo') + user = database.get_user(user_name='foo') self.assertIsNone(user) - user = accounting.get_user(id=124) + user = database.get_user(id=124) self.assertIsNone(user) with self.assertRaises(ValueError): - accounting.get_user(id=1, user_name="foo") + database.get_user(id=1, user_name="foo") - def test_get_apps_for_user_across_sessions(self): - accounting = self.create_accounting() + def test_get_accounting_for_user_across_sessions(self): + database = self.create_database() # user is retrieved from one session - user = accounting.get_user(user_name='user1') + user = database.get_user(user_name='user1') # apps is retrieved from another sessions - actual_app, actual_policy = accounting.get_apps_for_user(user)[0][1:] + accounting = database.get_accounting_for_user(user)[0] expected_config = self.create_expected_configs(orm.User(name='user1')) - self.assertEqual(actual_app, expected_config[0][0]) - self.assertEqual(actual_policy, expected_config[0][1]) + self.assertEqual(accounting.application, expected_config[0][0]) + self.assertEqual(accounting.application_policy, expected_config[0][1]) def test_no_file_creation_if_sqlite_database_not_exist(self): temp_file_path = os.path.join(self.tempdir, 'some.db') with self.assertRaises(IOError): - AppAccounting( + ORMDatabase( url="sqlite:///"+temp_file_path) self.assertFalse(os.path.exists(temp_file_path)) def test_create_user(self): - accounting = self.create_accounting() - prev_length = len(accounting.list_users()) + database = self.create_database() + prev_length = len(database.list_users()) - id = accounting.create_user("ciccio") + id = database.create_user("ciccio") - user = accounting.get_user(user_name="ciccio") + user = database.get_user(user_name="ciccio") self.assertIsNotNone(user) self.assertIsNotNone(id) - self.assertEqual(len(accounting.list_users()), prev_length + 1) + self.assertEqual(len(database.list_users()), prev_length + 1) with self.assertRaises(exceptions.Exists): - accounting.create_user("ciccio") + database.create_user("ciccio") def test_remove_user_by_name(self): - accounting = self.create_accounting() - prev_length = len(accounting.list_users()) + database = self.create_database() + prev_length = len(database.list_users()) - accounting.remove_user(user_name="user1") + database.remove_user(user_name="user1") - self.assertIsNone(accounting.get_user(user_name="ciccio")) - self.assertEqual(len(accounting.list_users()), prev_length - 1) + self.assertIsNone(database.get_user(user_name="ciccio")) + self.assertEqual(len(database.list_users()), prev_length - 1) # This should be neutral - accounting.remove_user(user_name="user1") + database.remove_user(user_name="user1") def test_remove_user_by_id(self): - accounting = self.create_accounting() - user = accounting.get_user(user_name="user1") + database = self.create_database() + user = database.get_user(user_name="user1") id = user.id - prev_length = len(accounting.list_users()) + prev_length = len(database.list_users()) - accounting.remove_user(id=id) - self.assertIsNone(accounting.get_user(user_name="user1")) - self.assertEqual(len(accounting.list_users()), prev_length - 1) + database.remove_user(id=id) + self.assertIsNone(database.get_user(user_name="user1")) + self.assertEqual(len(database.list_users()), prev_length - 1) # This should be neutral - accounting.remove_user(id=id) + database.remove_user(id=id) def test_remove_user_one_arg(self): - accounting = self.create_accounting() + database = self.create_database() with self.assertRaises(ValueError): - accounting.remove_user(user_name="foo", id=3) + database.remove_user(user_name="foo", id=3) with self.assertRaises(ValueError): - accounting.remove_user() + database.remove_user() def test_create_application(self): - accounting = self.create_accounting() - prev_length = len(accounting.list_applications()) + database = self.create_database() + prev_length = len(database.list_applications()) - id = accounting.create_application("simphonyremote/amazing") + id = database.create_application("simphonyremote/amazing") self.assertIsNotNone(id) - app_list = accounting.list_applications() + app_list = database.list_applications() self.assertEqual(len(app_list), prev_length + 1) apps = [a for a in app_list if a.id == id] self.assertEqual(len(apps), 1) with self.assertRaises(exceptions.Exists): - accounting.create_application("simphonyremote/amazing") + database.create_application("simphonyremote/amazing") def test_remove_application(self): - accounting = self.create_accounting() - prev_length = len(accounting.list_applications()) + database = self.create_database() + prev_length = len(database.list_applications()) - accounting.remove_application(app_name="docker/image0") + database.remove_application(app_name="docker/image0") - self.assertEqual(len(accounting.list_applications()), prev_length - 1) + self.assertEqual(len(database.list_applications()), prev_length - 1) # This should be neutral - accounting.remove_application(app_name="docker/image0") + database.remove_application(app_name="docker/image0") def test_remove_application_by_id(self): - accounting = self.create_accounting() - app_list = accounting.list_applications() + database = self.create_database() + app_list = database.list_applications() id = app_list[0].id prev_length = len(app_list) - accounting.remove_application(id=id) + database.remove_application(id=id) - self.assertEqual(len(accounting.list_applications()), prev_length - 1) + self.assertEqual(len(database.list_applications()), prev_length - 1) # This should be neutral - accounting.remove_application(id=id) + database.remove_application(id=id) def test_remove_application_one_arg(self): - accounting = self.create_accounting() + database = self.create_database() with self.assertRaises(ValueError): - accounting.remove_application(app_name="foo", id=3) + database.remove_application(app_name="foo", id=3) with self.assertRaises(ValueError): - accounting.remove_application() + database.remove_application() def test_grant_revoke_access(self): - accounting = self.create_accounting() + database = self.create_database() with self.assertRaises(exceptions.NotFound): - accounting.grant_access("simphonyremote/amazing", "ciccio", - True, False, "/foo:/bar:ro") - accounting.create_user("ciccio") + database.grant_access("simphonyremote/amazing", "ciccio", + True, False, "/foo:/bar:ro") + database.create_user("ciccio") with self.assertRaises(exceptions.NotFound): - accounting.grant_access("simphonyremote/amazing", "ciccio", - True, False, "/foo:/bar:ro") + database.grant_access("simphonyremote/amazing", "ciccio", + True, False, "/foo:/bar:ro") - accounting.create_application("simphonyremote/amazing") + database.create_application("simphonyremote/amazing") - id = accounting.grant_access("simphonyremote/amazing", "ciccio", - True, False, "/foo:/bar:ro") + id = database.grant_access("simphonyremote/amazing", "ciccio", + True, False, "/foo:/bar:ro") self.assertIsNotNone(id) - user = accounting.get_user(user_name="ciccio") - apps = accounting.get_apps_for_user(user) - self.assertEqual(apps[0][0], id) - self.assertEqual(apps[0][1].image, "simphonyremote/amazing") - self.assertEqual(apps[0][2].allow_home, True) - self.assertEqual(apps[0][2].allow_view, False) - self.assertEqual(apps[0][2].volume_source, "/foo") - self.assertEqual(apps[0][2].volume_target, "/bar") - self.assertEqual(apps[0][2].volume_mode, "ro") + user = database.get_user(user_name="ciccio") + acc = database.get_accounting_for_user(user) + self.assertEqual(acc[0].id, id) + self.assertEqual(acc[0].application.image, "simphonyremote/amazing") + self.assertEqual(acc[0].application_policy.allow_home, True) + self.assertEqual(acc[0].application_policy.allow_view, False) + self.assertEqual(acc[0].application_policy.volume_source, "/foo") + self.assertEqual(acc[0].application_policy.volume_target, "/bar") + self.assertEqual(acc[0].application_policy.volume_mode, "ro") # Do it twice to check idempotency - id2 = accounting.grant_access("simphonyremote/amazing", "ciccio", - True, False, "/foo:/bar:ro") + id2 = database.grant_access("simphonyremote/amazing", "ciccio", + True, False, "/foo:/bar:ro") self.assertEqual(id, id2) - accounting.revoke_access("simphonyremote/amazing", "ciccio", - True, False, "/foo:/bar:ro") + database.revoke_access("simphonyremote/amazing", "ciccio", + True, False, "/foo:/bar:ro") - self.assertEqual(len(accounting.get_apps_for_user(user)), 0) + self.assertEqual(len(database.get_accounting_for_user(user)), 0) with self.assertRaises(exceptions.NotFound): - accounting.revoke_access("simphonyremote/amazing", "hello", - True, False, "/foo:/bar:ro") + database.revoke_access("simphonyremote/amazing", "hello", + True, False, "/foo:/bar:ro") def test_grant_revoke_access_volume(self): - accounting = self.create_accounting() + database = self.create_database() - accounting.create_user("ciccio") - accounting.create_application("simphonyremote/amazing") - accounting.grant_access("simphonyremote/amazing", "ciccio", - True, False, None) + database.create_user("ciccio") + database.create_application("simphonyremote/amazing") + database.grant_access("simphonyremote/amazing", "ciccio", + True, False, None) - user = accounting.get_user(user_name="ciccio") - apps = accounting.get_apps_for_user(user) - self.assertEqual(apps[0][1].image, "simphonyremote/amazing") - self.assertEqual(apps[0][2].allow_home, True) - self.assertEqual(apps[0][2].allow_view, False) - self.assertEqual(apps[0][2].volume_source, None) - self.assertEqual(apps[0][2].volume_target, None) - self.assertEqual(apps[0][2].volume_mode, None) + user = database.get_user(user_name="ciccio") + acc = database.get_accounting_for_user(user) + self.assertEqual(acc[0].application.image, "simphonyremote/amazing") + self.assertEqual(acc[0].application_policy.allow_home, True) + self.assertEqual(acc[0].application_policy.allow_view, False) + self.assertEqual(acc[0].application_policy.volume_source, None) + self.assertEqual(acc[0].application_policy.volume_target, None) + self.assertEqual(acc[0].application_policy.volume_mode, None) - accounting.revoke_access("simphonyremote/amazing", "ciccio", - True, False, None) + database.revoke_access("simphonyremote/amazing", "ciccio", + True, False, None) - self.assertEqual(len(accounting.get_apps_for_user(user)), 0) + self.assertEqual(len(database.get_accounting_for_user(user)), 0) def test_revoke_by_id(self): - accounting = self.create_accounting() + database = self.create_database() - accounting.create_user("ciccio") - accounting.create_application("simphonyremote/amazing") - id = accounting.grant_access("simphonyremote/amazing", "ciccio", - True, False, None) + database.create_user("ciccio") + database.create_application("simphonyremote/amazing") + id = database.grant_access("simphonyremote/amazing", "ciccio", + True, False, None) - user = accounting.get_user(user_name="ciccio") - apps = accounting.get_apps_for_user(user) + user = database.get_user(user_name="ciccio") + apps = database.get_accounting_for_user(user) self.assertEqual(len(apps), 1) self.assertIsNotNone(id) - accounting.revoke_access_by_id(id) - user = accounting.get_user(user_name="ciccio") - apps = accounting.get_apps_for_user(user) + database.revoke_access_by_id(id) + user = database.get_user(user_name="ciccio") + apps = database.get_accounting_for_user(user) self.assertEqual(len(apps), 0) # Id not present, should do nothing - accounting.revoke_access_by_id(3441) + database.revoke_access_by_id(3441) def test_unsupported_ops(self): """Override to silence the base class assumption that most of diff --git a/remoteappmanager/file_config.py b/remoteappmanager/file_config.py index 00c33c718..6f5c96aa9 100644 --- a/remoteappmanager/file_config.py +++ b/remoteappmanager/file_config.py @@ -41,13 +41,13 @@ class FileConfig(HasTraits): help="The docker realm. Identifies which containers belong to a " "specific instance of simphony-remote.") - accounting_class = Unicode( - default_value="remoteappmanager.db.orm.AppAccounting", - help="The import path to a subclass of ABCAccounting") + database_class = Unicode( + default_value="remoteappmanager.db.orm.ORMDatabase", + help="The import path to a subclass of ABCDatabase") - accounting_kwargs = Dict( + database_kwargs = Dict( default_value={'url': 'sqlite:///remoteappmanager.db'}, - help="The keyword arguments for initialising the Accounting instance") + help="The keyword arguments for initialising the Database instance") login_url = Unicode(default_value="/hub", help=("The url to be redirected to if the user is not " diff --git a/remoteappmanager/handlers/admin/accounting_handler.py b/remoteappmanager/handlers/admin/accounting_handler.py index 4e818b861..90b2d3f8b 100644 --- a/remoteappmanager/handlers/admin/accounting_handler.py +++ b/remoteappmanager/handlers/admin/accounting_handler.py @@ -16,12 +16,12 @@ def get(self, id): if acc_user is None: raise web.HTTPError(404) - apps = db.get_apps_for_user(acc_user) + accountings = db.get_accounting_for_user(acc_user) - info = [{"mapping_id": mapping_id, - "app": app, - "policy": policy} - for mapping_id, app, policy in apps] + info = [{"mapping_id": accounting.id, + "app": accounting.application, + "policy": accounting.application_policy} + for accounting in accountings] self.render('admin/accounting.html', info=info, diff --git a/remoteappmanager/handlers/tests/test_base_handler.py b/remoteappmanager/handlers/tests/test_base_handler.py index fcf2d02ec..63781c492 100644 --- a/remoteappmanager/handlers/tests/test_base_handler.py +++ b/remoteappmanager/handlers/tests/test_base_handler.py @@ -9,9 +9,9 @@ class TestBaseHandler(TempMixin, utils.AsyncHTTPTestCase, LogTrapTestCase): def get_file_config(self): file_config = FileConfig() - file_config.accounting_class = \ - 'remoteappmanager.tests.mocking.dummy.DummyDBAccounting' - file_config.accounting_kwargs = {} + file_config.database_class = \ + 'remoteappmanager.tests.mocking.dummy.DummyDB' + file_config.database_kwargs = {} return file_config def get_app(self): @@ -29,7 +29,7 @@ def get_app(self): class TestBaseHandlerInvalidAccounting(TestBaseHandler): def get_file_config(self): file_config = super().get_file_config() - file_config.accounting_class = 'this_should_fail' + file_config.database_class = 'this_should_fail' return file_config def test_home_internal_error(self): @@ -47,7 +47,7 @@ def test_home_internal_error(self): class TestBaseHandlerDatabaseError(TestBaseHandler): def get_file_config(self): file_config = super().get_file_config() - file_config.accounting_class = "remoteappmanager.db.orm.AppAccounting" + file_config.database_class = "remoteappmanager.db.orm.ORMDatabase" return file_config def test_home_internal_error(self): diff --git a/remoteappmanager/tests/mocking/dummy.py b/remoteappmanager/tests/mocking/dummy.py index f5cd6ead2..477f880fa 100644 --- a/remoteappmanager/tests/mocking/dummy.py +++ b/remoteappmanager/tests/mocking/dummy.py @@ -26,10 +26,14 @@ class DummyDBApplicationPolicy(interfaces.ABCApplicationPolicy): pass +class DummyDBAccounting(interfaces.ABCAccounting): + pass + + User = namedtuple('User', ('id', 'name')) -class DummyDBAccounting(interfaces.ABCAccounting): +class DummyDB(interfaces.ABCDatabase): def __init__(self): self.users = { 0: User(0, "johndoe") @@ -64,12 +68,16 @@ def get_user(self, *, user_name=None, id=None): # pragma: no cover user = [u for u in self.users.values() if u.name == user_name] return user[0] if len(user) else None - def get_apps_for_user(self, account): # pragma: no cover - return tuple( - [(mapping_id, application, policy) - for mapping_id, (user, application, policy) - in self.accounting.items() - if user == account]) + def get_accounting_for_user(self, user): # pragma: no cover + return [ + DummyDBAccounting( + id=id, + user=user, + application=application, + application_policy=policy) + for id, (tbl_user, application, policy) + in self.accounting.items() + if tbl_user == user] def create_user(self, user_name): # pragma: no cover if user_name in [u.name for u in self.list_users()]: @@ -294,9 +302,9 @@ def _create_application_from_class( if file_config is None: file_config = FileConfig() - file_config.accounting_class = \ - 'remoteappmanager.tests.mocking.dummy.DummyDBAccounting' - file_config.accounting_kwargs = {} + file_config.database_class = \ + 'remoteappmanager.tests.mocking.dummy.DummyDB' + file_config.database_kwargs = {} if command_line_config is None: command_line_config = basic_command_line_config() diff --git a/remoteappmanager/tests/test_application.py b/remoteappmanager/tests/test_application.py index 476d71176..98907aa77 100644 --- a/remoteappmanager/tests/test_application.py +++ b/remoteappmanager/tests/test_application.py @@ -10,7 +10,7 @@ from traitlets.traitlets import TraitError -class DummyAccounting: +class DummyDB: def __init__(self, *args, **kwargs): pass @@ -21,7 +21,7 @@ class TestApplication(TempMixin, def setUp(self): super().setUp() - # File config with orm.AppAccounting + # File config with orm.ORMDatabase self.file_config = utils.basic_file_config() self.command_line_config = utils.basic_command_line_config() self.environment_config = utils.basic_environment_config() @@ -31,9 +31,9 @@ def test_initialization_with_sqlite_db(self): sqlite_file_path = os.path.join(self.tempdir, "sqlite.db") utils.init_sqlite_db(sqlite_file_path) - self.file_config.accounting_class = ( - "remoteappmanager.db.orm.AppAccounting") - self.file_config.accounting_kwargs = { + self.file_config.database_class = ( + "remoteappmanager.db.orm.ORMDatabase") + self.file_config.database_kwargs = { "url": "sqlite:///"+sqlite_file_path} app = Application(self.command_line_config, @@ -53,7 +53,7 @@ def test_initialization_with_sqlite_db(self): self.assertEqual(app.user.account, None) def test_error_default_value_with_unimportable_accounting(self): - self.file_config.accounting_class = "not.importable.Class" + self.file_config.database_class = "not.importable.Class" app = Application(self.command_line_config, self.file_config, self.environment_config) @@ -62,8 +62,8 @@ def test_error_default_value_with_unimportable_accounting(self): app.db def test_db_default_value_with_accounting_wrong_subclass(self): - self.file_config.accounting_class = ( - "remoteappmanager.tests.test_application.DummyAccounting") + self.file_config.database_class = ( + "remoteappmanager.tests.test_application.DummyDB") app = Application(self.command_line_config, self.file_config, self.environment_config) @@ -72,11 +72,11 @@ def test_db_default_value_with_accounting_wrong_subclass(self): app.db def test_initialization(self): - self.file_config.accounting_class = ( - "remoteappmanager.db.csv_db.CSVAccounting") + self.file_config.database_class = ( + "remoteappmanager.db.csv_db.CSVDatabase") csv_file = os.path.join(self.tempdir, 'testing.csv') - self.file_config.accounting_kwargs = {"csv_file_path": csv_file} + self.file_config.database_kwargs = {"csv_file_path": csv_file} test_csv_db.write_csv_file(csv_file, test_csv_db.GoodTable.headers, diff --git a/remoteappmanager/tests/test_file_config.py b/remoteappmanager/tests/test_file_config.py index a2732d564..6660ab819 100644 --- a/remoteappmanager/tests/test_file_config.py +++ b/remoteappmanager/tests/test_file_config.py @@ -21,8 +21,8 @@ ''' GOOD_ACCOUNTING_CONFIG = ''' -accounting_class = "remoteappmanager.db.csv_db.CSVAccounting" -accounting_kwargs = {"csv_file_path": "file_path.csv"} +database_class = "remoteappmanager.db.csv_db.CSVDatabase" +database_kwargs = {"csv_file_path": "file_path.csv"} ''' GA_TRACKING = ''' @@ -102,9 +102,9 @@ def test_initialization_with_good_accounting(self): config = FileConfig() config.parse_config(self.config_file) - self.assertEqual(config.accounting_class, - "remoteappmanager.db.csv_db.CSVAccounting") - self.assertDictEqual(config.accounting_kwargs, + self.assertEqual(config.database_class, + "remoteappmanager.db.csv_db.CSVDatabase") + self.assertDictEqual(config.database_kwargs, {"csv_file_path": "file_path.csv"}) def test_initialization_on_nonlocal_docker_machine(self): diff --git a/remoteappmanager/webapi/admin/tests/test_accounting.py b/remoteappmanager/webapi/admin/tests/test_accounting.py index e3c2a97b9..0393ffb1c 100644 --- a/remoteappmanager/webapi/admin/tests/test_accounting.py +++ b/remoteappmanager/webapi/admin/tests/test_accounting.py @@ -26,7 +26,7 @@ def test_delete(self): def test_unable_to_delete(self): with mock.patch("remoteappmanager.tests.mocking." - "dummy.DummyDBAccounting.revoke_access_by_id" + "dummy.DummyDB.revoke_access_by_id" ) as mock_delete_user: mock_delete_user.side_effect = UnsupportedOperation() self.delete("/user/johndoe/api/v1/accounting/cbaee2e8ef414f9fb0f1c97416b8aa6c/", # noqa @@ -60,7 +60,7 @@ def test_create(self): def test_unable_to_create(self): with mock.patch("remoteappmanager.tests.mocking." - "dummy.DummyDBAccounting.grant_access" + "dummy.DummyDB.grant_access" ) as mock_grant_access: mock_grant_access.side_effect = UnsupportedOperation() self.post("/user/johndoe/api/v1/accounting/", diff --git a/remoteappmanager/webapi/admin/tests/test_application.py b/remoteappmanager/webapi/admin/tests/test_application.py index 09660ae7c..3704382e4 100644 --- a/remoteappmanager/webapi/admin/tests/test_application.py +++ b/remoteappmanager/webapi/admin/tests/test_application.py @@ -29,7 +29,7 @@ def test_delete(self): def test_unable_to_delete(self): with mock.patch("remoteappmanager.tests.mocking." - "dummy.DummyDBAccounting.remove_application" + "dummy.DummyDB.remove_application" ) as mock_delete_app: mock_delete_app.side_effect = UnsupportedOperation() self.delete("/user/johndoe/api/v1/applications/1/", @@ -46,7 +46,7 @@ def test_create(self): def test_unable_to_create(self): with mock.patch("remoteappmanager.tests.mocking." - "dummy.DummyDBAccounting.create_application" + "dummy.DummyDB.create_application" ) as mock_create_app: mock_create_app.side_effect = UnsupportedOperation() self.post("/user/johndoe/api/v1/applications/", diff --git a/remoteappmanager/webapi/admin/tests/test_user.py b/remoteappmanager/webapi/admin/tests/test_user.py index 3d2fab0ed..a29df780f 100644 --- a/remoteappmanager/webapi/admin/tests/test_user.py +++ b/remoteappmanager/webapi/admin/tests/test_user.py @@ -29,7 +29,7 @@ def test_delete(self): def test_unable_to_delete(self): with mock.patch("remoteappmanager.tests.mocking." - "dummy.DummyDBAccounting.remove_user" + "dummy.DummyDB.remove_user" ) as mock_delete_user: mock_delete_user.side_effect = UnsupportedOperation() self.delete("/user/johndoe/api/v1/users/1/", @@ -54,7 +54,7 @@ def test_create(self): def test_unable_to_create(self): with mock.patch("remoteappmanager.tests.mocking." - "dummy.DummyDBAccounting.create_user" + "dummy.DummyDB.create_user" ) as mock_create_user: mock_create_user.side_effect = UnsupportedOperation() self.post("/user/johndoe/api/v1/users/", diff --git a/remoteappmanager/webapi/application.py b/remoteappmanager/webapi/application.py index a006938d5..41ea75e56 100644 --- a/remoteappmanager/webapi/application.py +++ b/remoteappmanager/webapi/application.py @@ -42,19 +42,20 @@ class ApplicationHandler(ResourceHandler): @gen.coroutine @authenticated def retrieve(self, resource, **kwargs): - apps = self.application.db.get_apps_for_user( + accs = self.application.db.get_accounting_for_user( self.current_user.account ) identifier = resource.identifier # Convert the list of tuples in a dict - apps_dict = {mapping_id: (app, policy) - for mapping_id, app, policy in apps} + accs_dict = { + acc.id: (acc.application, acc.application_policy) + for acc in accs} - if identifier not in apps_dict: + if identifier not in accs_dict: raise NotFound() - app, policy = apps_dict[identifier] + app, policy = accs_dict[identifier] container_manager = self.application.container_manager image = yield container_manager.image(app.image) @@ -90,11 +91,12 @@ def retrieve(self, resource, **kwargs): def items(self, items_response, **kwargs): """Retrieves a dictionary containing the image and the associated container, if active, as values.""" - apps = self.application.db.get_apps_for_user(self.current_user.account) + accs = self.application.db.get_accounting_for_user( + self.current_user.account) result = [] - for mapping_id, _, _ in apps: - resource = self.resource_class(identifier=mapping_id) + for entry in accs: + resource = self.resource_class(identifier=entry.id) try: yield self.retrieve(resource) except NotFound: diff --git a/remoteappmanager/webapi/container.py b/remoteappmanager/webapi/container.py index 87e8b9220..d4d903b5a 100644 --- a/remoteappmanager/webapi/container.py +++ b/remoteappmanager/webapi/container.py @@ -31,12 +31,14 @@ def create(self, resource, **kwargs): webapp = self.application account = self.current_user.account - all_apps = webapp.db.get_apps_for_user(account) + accountings = webapp.db.get_accounting_for_user(account) container_manager = webapp.container_manager - choice = [(m_id, app, policy) - for m_id, app, policy in all_apps - if m_id == mapping_id] + choice = [(accounting.id, + accounting.application, + accounting.application_policy) + for accounting in accountings + if accounting.id == mapping_id] if not choice: self.log.warning("Could not find resource " @@ -144,13 +146,13 @@ def items(self, items_response, **kwargs): """"Return the list of containers we are currently running.""" container_manager = self.application.container_manager - apps = self.application.db.get_apps_for_user( + accountings = self.application.db.get_accounting_for_user( self.current_user.account) running_containers = [] - for mapping_id, app, policy in apps: - image = yield container_manager.image(app.image) + for accounting in accountings: + image = yield container_manager.image(accounting.application.image) if image is None: # The user has access to an application that is no longer @@ -159,7 +161,7 @@ def items(self, items_response, **kwargs): container = yield container_manager.find_container( user_name=self.current_user.name, - mapping_id=mapping_id) + mapping_id=accounting.id) rest_container = Container(identifier=container.url_id) rest_container.fill(container) diff --git a/remoteappmanager/webapi/tests/test_application.py b/remoteappmanager/webapi/tests/test_application.py index c6fedb086..56fb72a91 100644 --- a/remoteappmanager/webapi/tests/test_application.py +++ b/remoteappmanager/webapi/tests/test_application.py @@ -53,9 +53,14 @@ def get_app(self): volume_mode="ro", ) - app.db.get_apps_for_user = Mock(return_value=[ - ("one", application_mock_1, policy), - ("two", application_mock_2, policy), + app.db.get_accounting_for_user = Mock(return_value=[ + Mock(id="one", + application=application_mock_1, + application_policy=policy), + Mock( + id="two", + application=application_mock_2, + application_policy=policy), ]) return app