From 0dd8b8f81a1657521f593daeee798d8b650a58ef Mon Sep 17 00:00:00 2001 From: Joao Eduardo Luis Date: Thu, 20 Sep 2018 15:20:50 +0100 Subject: [PATCH] mgr/dashboard: no side-effects on failed user creation Read operations go first, operations with side-effects go last. Otherwise, something like 1) create user A with non-existent role R1; fails due to R1 DNE 2) create user B with existent role R2; success will result in both A and B being created; B will have a R2 assigned, A will have no roles. Signed-off-by: Joao Eduardo Luis --- .../mgr/dashboard/services/access_control.py | 8 ++- .../dashboard/tests/test_access_control.py | 60 +++++++++++++++++++ 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/pybind/mgr/dashboard/services/access_control.py b/src/pybind/mgr/dashboard/services/access_control.py index 10eb705706b79..1364e1900f834 100644 --- a/src/pybind/mgr/dashboard/services/access_control.py +++ b/src/pybind/mgr/dashboard/services/access_control.py @@ -550,15 +550,17 @@ Username and password updated''', '' name = cmd['name'] if 'name' in cmd else None email = cmd['email'] if 'email' in cmd else None try: - user = ACCESS_CTRL_DB.create_user(username, password, name, email) role = ACCESS_CTRL_DB.get_role(rolename) if rolename else None - except UserAlreadyExists as ex: - return -errno.EEXIST, '', str(ex) except RoleDoesNotExist as ex: if rolename not in SYSTEM_ROLES: return -errno.ENOENT, '', str(ex) role = SYSTEM_ROLES[rolename] + try: + user = ACCESS_CTRL_DB.create_user(username, password, name, email) + except UserAlreadyExists as ex: + return -errno.EEXIST, '', str(ex) + if role: user.set_roles([role]) ACCESS_CTRL_DB.save() diff --git a/src/pybind/mgr/dashboard/tests/test_access_control.py b/src/pybind/mgr/dashboard/tests/test_access_control.py index d5c98904939f9..ed9abc225b003 100644 --- a/src/pybind/mgr/dashboard/tests/test_access_control.py +++ b/src/pybind/mgr/dashboard/tests/test_access_control.py @@ -61,6 +61,27 @@ class AccessControlTest(unittest.TestCase): db = json.loads(db_json) return db + # The DB is written to persistent storage the first time it is saved. + # However, should an operation fail due to , we may end up in + # a state where we have a completely empty CONFIG_KEY_DICT (our mock + # equivalent to the persistent state). While this works for most of the + # tests in this class, that would prevent us from testing things like + # "run a command that is expected to fail, and then ensure nothing + # happened", because we'd be asserting in `load_persistent_db()` due to + # the map being empty. + # + # This function will therefore force state to be written to our mock + # persistent state. We could have added this extra step to + # `load_persistent_db()` directly, but that would conflict with the + # upgrade tests. This way, we can selectively enforce this requirement + # where we believe it to be necessary; generically speaking, this should + # not be needed unless we're testing very specific behaviors. + # + def setup_and_load_persistent_db(self): + from ..services.access_control import ACCESS_CTRL_DB + ACCESS_CTRL_DB.save() + self.load_persistent_db() + def validate_persistent_role(self, rolename, scopes_permissions, description=None): db = self.load_persistent_db() @@ -317,6 +338,45 @@ class AccessControlTest(unittest.TestCase): self.assertEqual(ctx.exception.retcode, -errno.EEXIST) self.assertEqual(str(ctx.exception), "User 'admin' already exists") + def test_create_users_with_dne_role(self): + # one time call to setup our persistent db + self.setup_and_load_persistent_db() + + # create a user with a role that does not exist; expect a failure + try: + self.exec_cmd('ac-user-create', username='foo', + rolename='dne_role', password='foopass', + name='foo User', email='foo@user.com') + except CmdException as e: + self.assertEqual(e.retcode, -errno.ENOENT) + + db = self.load_persistent_db() + if 'users' in db: + self.assertNotIn('foo', db['users']) + + # We could just finish our test here, given we ensured that the user + # with a non-existent role is not in persistent storage. However, + # we're going to test the database's consistency, making sure that + # side-effects are not written to persistent storage once we commit + # an unrelated operation. To ensure this, we'll issue another + # operation that is sharing the same code path, and will check whether + # the next operation commits dirty state. + + # create a role (this will be 'test_role') + self.test_create_role() + self.exec_cmd('ac-user-create', username='bar', + rolename='test_role', password='barpass', + name='bar User', email='bar@user.com') + + # validate db: + # user 'foo' should not exist + # user 'bar' should exist and have role 'test_role' + self.validate_persistent_user('bar', ['test_role']) + + db = self.load_persistent_db() + self.assertIn('users', db) + self.assertNotIn('foo', db['users']) + def test_delete_nonexistent_user(self): with self.assertRaises(CmdException) as ctx: self.exec_cmd('ac-user-delete', username='admin') -- 2.39.5