From fef06515c93807b3857177bb23e4efdcb7b1d51c Mon Sep 17 00:00:00 2001 From: Adrian Turjak Date: Thu, 17 Jan 2019 14:36:25 +1300 Subject: [PATCH] Fix issues around LDAP backed Keystone Invite user workflow now defaults to domain_id from the project. Create project workflow now default to getting domain and parent id from config. Identity manager now has setting to flag the inability to edit/create users, which some actions now support. Fix an issue with email comparison when username_is_email was true. Change-Id: I8548914e3d2283b17f3015595ea72c4c8084d7f5 --- adjutant/actions/v1/projects.py | 31 +++++-- adjutant/actions/v1/users.py | 27 ++++-- adjutant/api/v1/openstack.py | 4 + adjutant/api/v1/tasks.py | 10 +++ adjutant/api/v1/tests/test_api_taskview.py | 99 ++++++++++++++++++++++ adjutant/common/tests/fake_clients.py | 7 +- adjutant/common/user_store.py | 4 + conf/conf.yaml | 3 +- 8 files changed, 168 insertions(+), 17 deletions(-) diff --git a/adjutant/actions/v1/projects.py b/adjutant/actions/v1/projects.py index a767154..1daeba3 100644 --- a/adjutant/actions/v1/projects.py +++ b/adjutant/actions/v1/projects.py @@ -14,6 +14,7 @@ from uuid import uuid4 +from django.conf import settings from django.utils import timezone from adjutant.common import user_store @@ -43,7 +44,7 @@ class NewProjectAction(BaseAction, ProjectMixin, UserMixin): def _validate(self): self.action.valid = validate_steps([ self._validate_domain_id, - self._validate_parent_project, + self._validate_keystone_user_parent_project, self._validate_project_absent, ]) self.action.save() @@ -57,7 +58,7 @@ class NewProjectAction(BaseAction, ProjectMixin, UserMixin): return super(NewProjectAction, self)._validate_domain_id() - def _validate_parent_project(self): + def _validate_keystone_user_parent_project(self): if self.parent_id: keystone_user = self.action.task.keystone_user @@ -65,7 +66,7 @@ class NewProjectAction(BaseAction, ProjectMixin, UserMixin): self.add_note( 'Parent id does not match keystone user project.') return False - return super(NewProjectAction, self)._validate_parent_project() + return self._validate_parent_project() return True def _pre_approve(self): @@ -151,29 +152,41 @@ class NewProjectWithUserAction(UserNameAction, ProjectMixin, UserMixin): user = id_manager.find_user(self.username, self.domain_id) if not user: + self.add_note( + "No user present with username '%s'. " + "Need to create new user." % self.username) + if not id_manager.can_edit_users: + self.add_note( + 'Identity backend does not support user editing, ' + 'cannot create new user.') + return False # add to cache to use in template self.action.task.cache['user_state'] = "default" self.action.need_token = True self.set_token_fields(["password"]) - self.add_note("No user present with username '%s'." % - self.username) return True - if user.email != self.email: + if (not settings.USERNAME_IS_EMAIL + and getattr(user, 'email', None) != self.email): self.add_note("Existing user '%s' with non-matching email." % self.username) return False if not user.enabled: + self.add_note( + "Existing disabled user '%s' with matching email." % + self.email) + if not id_manager.can_edit_users: + self.add_note( + 'Identity backend does not support user editing, ' + 'cannot renable user.') + return False self.action.state = "disabled" # add to cache to use in template self.action.task.cache['user_state'] = "disabled" self.action.need_token = True # as they are disabled we'll reset their password self.set_token_fields(["password"]) - self.add_note( - "Existing disabled user '%s' with matching email." % - self.email) return True else: self.action.state = "existing" diff --git a/adjutant/actions/v1/users.py b/adjutant/actions/v1/users.py index 5b8a34d..e20b853 100644 --- a/adjutant/actions/v1/users.py +++ b/adjutant/actions/v1/users.py @@ -45,28 +45,41 @@ class NewUserAction(UserNameAction, ProjectMixin, UserMixin): # this may mean we need a token. user = self._get_target_user() if not user: + self.add_note( + "No user present with username '%s'. " + "Need to create new user." % self.username) + if not id_manager.can_edit_users: + self.add_note( + 'Identity backend does not support user editing, ' + 'cannot create new user.') + return False self.action.need_token = True # add to cache to use in template self.action.task.cache['user_state'] = "default" self.set_token_fields(["password"]) - self.add_note( - 'No user present with username. Need to create new user.') return True - if user.email != self.email: + if (not settings.USERNAME_IS_EMAIL + and getattr(user, 'email', None) != self.email): self.add_note( 'Found matching username, but email did not match. ' 'Reporting as invalid.') return False if not user.enabled: + self.add_note( + "Existing disabled user '%s' with matching email." % + self.email) + if not id_manager.can_edit_users: + self.add_note( + 'Identity backend does not support user editing, ' + 'cannot renable user.') + return False self.action.need_token = True self.action.state = "disabled" # add to cache to use in template self.action.task.cache['user_state'] = "disabled" # as they are disabled we'll reset their password self.set_token_fields(["password"]) - self.add_note( - 'Existing disabled user with matching email.') return True # role_validation @@ -192,7 +205,9 @@ class ResetUserPasswordAction(UserNameAction, UserMixin): # NOTE(adriant): We only need to check the USERNAME_IS_EMAIL=False # case since '_validate_username_exists' will ensure the True case if not settings.USERNAME_IS_EMAIL: - if self.user and self.user.email.lower() != self.email.lower(): + if (self.user and ( + getattr(self.user, 'email', None).lower() + != self.email.lower())): self.add_note('Existing user with non-matching email.') return False diff --git a/adjutant/api/v1/openstack.py b/adjutant/api/v1/openstack.py index bd8d018..6517440 100644 --- a/adjutant/api/v1/openstack.py +++ b/adjutant/api/v1/openstack.py @@ -119,6 +119,10 @@ class UserList(tasks.InviteUser): if notification.error: status = "Failed" + for action in task.actions: + if not action.valid: + status = "Invalid" + task_data = {} for action in task.actions: task_data.update(action.action_data) diff --git a/adjutant/api/v1/tasks.py b/adjutant/api/v1/tasks.py index 23718ef..6c21e73 100644 --- a/adjutant/api/v1/tasks.py +++ b/adjutant/api/v1/tasks.py @@ -321,6 +321,10 @@ class CreateProject(TaskView): # we need to set the region the resources will be created in: request.data['region'] = class_conf.get('default_region') + # domain + request.data['domain_id'] = class_conf.get( + 'default_domain_id', 'default') + # parent_id for new project, if null defaults to domain: request.data['parent_id'] = class_conf.get('default_parent_id') @@ -372,6 +376,12 @@ class InviteUser(TaskView): or request.data['project_id'] is None): request.data['project_id'] = request.keystone_user['project_id'] + # Default domain_id to the keystone user's project + if ('domain_id' not in request.data + or request.data['domain_id'] is None): + request.data['domain_id'] = \ + request.keystone_user['project_domain_id'] + processed, status = self.process_actions(request) errors = processed.get('errors', None) diff --git a/adjutant/api/v1/tests/test_api_taskview.py b/adjutant/api/v1/tests/test_api_taskview.py index b4cb39a..81e8fe6 100644 --- a/adjutant/api/v1/tests/test_api_taskview.py +++ b/adjutant/api/v1/tests/test_api_taskview.py @@ -1433,3 +1433,102 @@ class TaskViewTests(AdjutantAPITestCase): "Error: KeyError('Forced key error.') while setting up " "task. See task itself for details."]}) self.assertEqual(new_notification.task, new_task) + + @override_settings(KEYSTONE={'can_edit_users': False}) + def test_user_invite_cant_edit_users(self): + """ + When can_edit_users is false, and a new user is invited, + the task should be marked as invalid if the user doesn't + already exist. + """ + project = fake_clients.FakeProject(name="test_project") + + setup_identity_cache(projects=[project]) + + url = "/v1/actions/InviteUser" + headers = { + 'project_name': "test_project", + 'project_id': project.id, + 'roles': "project_admin,_member_,project_mod", + 'username': "user", + 'user_id': "test_user_id", + 'authenticated': True + } + data = {'username': 'new_user', 'email': "new@example.com", + 'roles': ["_member_"], 'project_id': project.id} + response = self.client.post(url, data, format='json', headers=headers) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json(), {'errors': ['actions invalid']}) + + @override_settings(KEYSTONE={'can_edit_users': False}) + def test_user_invite_cant_edit_users_existing_user(self): + """ + When can_edit_users is false, and a new user is invited, + the task should be marked as valid if the user exists. + """ + project = fake_clients.FakeProject(name="test_project") + + user = fake_clients.FakeUser(name="test@example.com") + + setup_identity_cache(projects=[project], users=[user]) + + url = "/v1/actions/InviteUser" + headers = { + 'project_name': "test_project", + 'project_id': project.id, + 'roles': "project_admin,_member_,project_mod", + 'username': "user", + 'user_id': "test_user_id", + 'authenticated': True + } + data = {'username': 'new_user', 'email': "test@example.com", + 'roles': ["_member_"], 'project_id': project.id} + response = self.client.post(url, data, format='json', headers=headers) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json(), {'notes': ['created token']}) + + @override_settings(KEYSTONE={'can_edit_users': False}) + def test_project_create_cant_edit_users(self): + """ + When can_edit_users is false, and a new signup comes in, + the task should be marked as invalid if it needs to + create a new user. + + Will return OK (as task doesn't auto_approve), but task will + actually be invalid. + """ + setup_identity_cache() + + url = "/v1/actions/CreateProject" + data = {'project_name': "test_project", 'email': "test@example.com"} + response = self.client.post(url, data, format='json') + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json(), {'notes': ['task created']}) + task = Task.objects.all()[0] + action_models = task.actions + actions = [act.get_action() for act in action_models] + self.assertFalse(all([act.valid for act in actions])) + + @override_settings(KEYSTONE={'can_edit_users': False}) + def test_project_create_cant_edit_users_existing_user(self): + """ + When can_edit_users is false, and a new signup comes in, + the task should be marked as valid if the user already + exists. + + Will return OK (as task doesn't auto_approve), but task will + actually be valid. + """ + user = fake_clients.FakeUser(name="test@example.com") + + setup_identity_cache(users=[user]) + + url = "/v1/actions/CreateProject" + data = {'project_name': "test_project", 'email': "test@example.com"} + response = self.client.post(url, data, format='json') + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json(), {'notes': ['task created']}) + task = Task.objects.all()[0] + action_models = task.actions + actions = [act.get_action() for act in action_models] + self.assertTrue(all([act.valid for act in actions])) diff --git a/adjutant/common/tests/fake_clients.py b/adjutant/common/tests/fake_clients.py index faace53..c6f2bd6 100644 --- a/adjutant/common/tests/fake_clients.py +++ b/adjutant/common/tests/fake_clients.py @@ -46,7 +46,7 @@ class FakeProject(object): class FakeUser(object): - def __init__(self, name, password, domain_id='default', + def __init__(self, name, password="123", domain_id='default', enabled=True, default_project_id=None, **kwargs): self.id = uuid4().hex @@ -158,6 +158,11 @@ def setup_identity_cache(projects=None, users=None, role_assignments=None, class FakeManager(object): + def __init__(self): + # TODO(adriant): decide if we want to have some function calls + # throw errors if this is false. + self.can_edit_users = settings.KEYSTONE.get('can_edit_users', True) + def _project_from_id(self, project): if isinstance(project, FakeProject): return project diff --git a/adjutant/common/user_store.py b/adjutant/common/user_store.py index a4421d9..dbfc5cb 100644 --- a/adjutant/common/user_store.py +++ b/adjutant/common/user_store.py @@ -59,6 +59,10 @@ class IdentityManager(object): # pragma: no cover def __init__(self): self.ks_client = get_keystoneclient() + # TODO(adriant): decide if we want to have some function calls + # throw errors if this is false. + self.can_edit_users = settings.KEYSTONE.get('can_edit_users', True) + def find_user(self, name, domain): try: users = self.ks_client.users.list(name=name, domain=domain) diff --git a/conf/conf.yaml b/conf/conf.yaml index 4938c4e..f7bef16 100644 --- a/conf/conf.yaml +++ b/conf/conf.yaml @@ -47,7 +47,7 @@ EMAIL_SETTINGS: # to have different values. USERNAME_IS_EMAIL: True -# Keystone admin credentials: +# Keystone config KEYSTONE: username: admin password: openstack @@ -55,6 +55,7 @@ KEYSTONE: # MUST BE V3 API: auth_url: http://localhost/identity/v3 domain_id: default + can_edit_users: True HORIZON_URL: http://localhost:8080/