From 43e0c625e39f826b578e3e5c25519606d9b72808 Mon Sep 17 00:00:00 2001 From: Adrian Turjak Date: Wed, 6 May 2020 13:17:46 +1200 Subject: [PATCH] Allow authenticated token requirement for tasks Tasks when defined can now set if they require the user submitting a token to be authenticated. keystone_user is now passed to actions when a token is submitted. This requires all actions to update their submit function, but a suitable fallthrough will exist for a cycle to allow time. Also fixes a minor issue around where error handling for renamed or deprecated tasks is handled that cropped up while testing this patch. Change-Id: I4b51201872cb5a14f299f90e22a8b010d11a71cb --- adjutant/actions/v1/base.py | 13 ++- adjutant/actions/v1/misc.py | 2 +- adjutant/actions/v1/projects.py | 6 +- adjutant/actions/v1/resources.py | 6 +- adjutant/actions/v1/users.py | 8 +- adjutant/api/v1/tests/test_api_admin.py | 83 +++++++++++++++++++ adjutant/api/v1/views.py | 11 ++- adjutant/tasks/models.py | 11 ++- adjutant/tasks/v1/base.py | 5 +- adjutant/tasks/v1/manager.py | 14 +--- doc/source/feature-sets.rst | 2 +- .../notes/authed_token-6d29688676e7ee32.yaml | 14 ++++ 12 files changed, 145 insertions(+), 30 deletions(-) create mode 100644 releasenotes/notes/authed_token-6d29688676e7ee32.yaml diff --git a/adjutant/actions/v1/base.py b/adjutant/actions/v1/base.py index b69ae9f..261e00e 100644 --- a/adjutant/actions/v1/base.py +++ b/adjutant/actions/v1/base.py @@ -186,8 +186,15 @@ class BaseAction(object): ) return self._post_approve() - def submit(self, token_data): - return self._submit(token_data) + def submit(self, token_data, keystone_user=None): + try: + return self._submit(token_data, keystone_user) + except TypeError: + self.logger.warning( + "DEPRECATED: Action '_submit' must accept a second parameter " + "'keystone_user=None' along with the required 'token_data'." + ) + return self._submit(token_data) def _prepare(self): raise NotImplementedError @@ -195,7 +202,7 @@ class BaseAction(object): def _approve(self): raise NotImplementedError - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): raise NotImplementedError def __str__(self): diff --git a/adjutant/actions/v1/misc.py b/adjutant/actions/v1/misc.py index b634cba..244f4ab 100644 --- a/adjutant/actions/v1/misc.py +++ b/adjutant/actions/v1/misc.py @@ -165,7 +165,7 @@ class SendAdditionalEmailAction(BaseAction): def _approve(self): self.perform_action("approve") - def _submit(self, data): + def _submit(self, token_data, keystone_user=None): self.perform_action("submit") def perform_action(self, stage): diff --git a/adjutant/actions/v1/projects.py b/adjutant/actions/v1/projects.py index bdee05e..b0754bb 100644 --- a/adjutant/actions/v1/projects.py +++ b/adjutant/actions/v1/projects.py @@ -135,7 +135,7 @@ class NewProjectAction(BaseAction, ProjectMixin, UserMixin): % (user.name, project_id, default_roles) ) - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): """ Nothing to do here. Everything is done at the approve step. """ @@ -406,7 +406,7 @@ class NewProjectWithUserAction(UserNameAction, ProjectMixin, UserMixin): % (self.username, project_id, default_roles) ) - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): """ The submit action is performed when a token is submitted. This is done to set a user password only, and so should now only @@ -537,5 +537,5 @@ class AddDefaultUsersToProjectAction(BaseAction, ProjectMixin, UserMixin): self.action.save() self.add_note("All users added.") - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): pass diff --git a/adjutant/actions/v1/resources.py b/adjutant/actions/v1/resources.py index a5c0f97..4652cbb 100644 --- a/adjutant/actions/v1/resources.py +++ b/adjutant/actions/v1/resources.py @@ -219,7 +219,7 @@ class NewDefaultNetworkAction(BaseAction, ProjectMixin): if self.setup_network and self.valid: self._create_network() - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): pass @@ -437,7 +437,7 @@ class UpdateProjectQuotasAction(BaseAction, QuotaMixin): self.action.save() - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): """ Nothing to do here. Everything is done at approve. """ @@ -494,5 +494,5 @@ class SetProjectQuotaAction(UpdateProjectQuotasAction): self.action.state = "completed" self.action.save() - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): pass diff --git a/adjutant/actions/v1/users.py b/adjutant/actions/v1/users.py index 1db4406..4e44310 100644 --- a/adjutant/actions/v1/users.py +++ b/adjutant/actions/v1/users.py @@ -135,7 +135,7 @@ class NewUserAction(UserNameAction, ProjectMixin, UserMixin): def _approve(self): self._validate() - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): self._validate() if not self.valid: @@ -258,7 +258,7 @@ class ResetUserPasswordAction(UserNameAction, UserMixin): def _approve(self): self._validate() - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): self._validate() if not self.valid: @@ -366,7 +366,7 @@ class EditUserRolesAction(UserIdAction, ProjectMixin, UserMixin): def _approve(self): self._validate() - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): self._validate() if not self.valid: @@ -470,7 +470,7 @@ class UpdateUserEmailAction(UserIdAction, UserMixin): self.action.need_token = True self.set_token_fields(["confirm"]) - def _submit(self, token_data): + def _submit(self, token_data, keystone_user=None): self._validate() if not self.valid: diff --git a/adjutant/api/v1/tests/test_api_admin.py b/adjutant/api/v1/tests/test_api_admin.py index 1fba538..b02baaa 100644 --- a/adjutant/api/v1/tests/test_api_admin.py +++ b/adjutant/api/v1/tests/test_api_admin.py @@ -27,6 +27,8 @@ from adjutant.api.models import Task, Token, Notification from adjutant.common.tests import fake_clients from adjutant.common.tests.fake_clients import FakeManager, setup_identity_cache from adjutant.config import CONF +from adjutant.tasks.v1.users import InviteUser +from adjutant.tasks.v1.manager import TaskManager @mock.patch("adjutant.common.user_store.IdentityManager", FakeManager) @@ -218,6 +220,7 @@ class AdminAPITests(APITestCase): "actions": ["ResetUserPasswordAction"], "required_fields": ["password"], "task_type": "reset_user_password", + "requires_authentication": False, }, ) self.assertEqual(1, Token.objects.count()) @@ -1577,3 +1580,83 @@ class AdminAPITests(APITestCase): }, ) self.assertEqual(new_notification.task, new_task) + + @mock.patch.object(InviteUser, "token_requires_authentication", True) + def test_token_require_authenticated(self): + """ + test for reissue of tokens + """ + project = mock.Mock() + project.id = "test_project_id" + project.name = "test_project" + project.domain = "default" + project.roles = {} + + user = fake_clients.FakeUser( + name="test@example.com", password="123", email="test@example.com" + ) + + setup_identity_cache(projects=[project], users=[user]) + + url = "/v1/actions/InviteUser" + headers = { + "project_name": project.name, + "project_id": project.id, + "roles": "project_admin,member,project_mod", + "username": "owner@example.com", + "user_id": "test_user_id", + "authenticated": True, + } + data = { + "email": "test@example.com", + "roles": ["member"], + "project_id": "test_project_id", + } + response = self.client.post(url, data, format="json", headers=headers) + self.assertEqual(response.status_code, status.HTTP_202_ACCEPTED) + + new_token = Token.objects.all()[0] + + url = "/v1/tokens/" + new_token.token + data = {"confirm": True} + response = self.client.post(url, data, format="json", headers={}) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) + self.assertEqual( + response.json(), + {"errors": ["This token requires authentication to submit."]}, + ) + + headers = { + "project_name": project.name, + "project_id": project.id, + "roles": "project_admin,member,project_mod", + "username": "owner@example.com", + "user_id": "test_user_id", + "authenticated": True, + } + + submitted_keystone_user = {} + + def mocked_submit(self, task, token_data, keystone_user): + submitted_keystone_user.update(keystone_user) + + with mock.patch.object(TaskManager, "submit", mocked_submit): + response = self.client.post(url, data, format="json", headers=headers) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual( + response.json(), + {"notes": ["Token submitted successfully."]}, + ) + self.assertEqual( + submitted_keystone_user, + { + "authenticated": True, + "project_domain_id": "default", + "project_id": "test_project_id", + "project_name": "test_project", + "roles": ["project_admin", "member", "project_mod"], + "user_domain_id": "default", + "user_id": "test_user_id", + "username": "owner@example.com", + }, + ) diff --git a/adjutant/api/v1/views.py b/adjutant/api/v1/views.py index e8d0952..974a190 100644 --- a/adjutant/api/v1/views.py +++ b/adjutant/api/v1/views.py @@ -409,6 +409,7 @@ class TokenDetail(APIViewWithLogger): "actions": [str(act) for act in actions], "required_fields": required_fields, "task_type": token.task.task_type, + "requires_authentication": token.task.get_task().token_requires_authentication, } ) @@ -428,6 +429,14 @@ class TokenDetail(APIViewWithLogger): {"errors": ["This token does not exist or has expired."]}, status=404 ) - self.task_manager.submit(token.task, request.data) + task = self.task_manager.get(token.task) + if task.token_requires_authentication and not request.keystone_user.get( + "authenticated", False + ): + return Response( + {"errors": ["This token requires authentication to submit."]}, 401 + ) + + self.task_manager.submit(task, request.data, request.keystone_user) return Response({"notes": ["Token submitted successfully."]}, status=200) diff --git a/adjutant/tasks/models.py b/adjutant/tasks/models.py index f4ca74f..de80738 100644 --- a/adjutant/tasks/models.py +++ b/adjutant/tasks/models.py @@ -18,6 +18,7 @@ from django.utils import timezone from jsonfield import JSONField from adjutant.config import CONF +from adjutant import exceptions from adjutant import tasks @@ -76,7 +77,15 @@ class Task(models.Model): def get_task(self): """Returns self as the appropriate task wrapper type.""" - return tasks.TASK_CLASSES[self.task_type](task_model=self) + try: + return tasks.TASK_CLASSES[self.task_type](task_model=self) + except KeyError: + # TODO(adriant): Maybe we should handle this better + # for older deprecated tasks: + raise exceptions.TaskNotRegistered( + "Task type '%s' not registered, " + "and used for existing task." % self.task_type + ) @property def config(self): diff --git a/adjutant/tasks/v1/base.py b/adjutant/tasks/v1/base.py index 98ff360..1385c91 100644 --- a/adjutant/tasks/v1/base.py +++ b/adjutant/tasks/v1/base.py @@ -129,6 +129,7 @@ class BaseTask(object): deprecated_task_types = None duplicate_policy = "cancel" send_approval_notification = True + token_requires_authentication = False # config defaults for the task (used to generate default config): allow_auto_approve = True @@ -466,7 +467,7 @@ class BaseTask(object): for token in self.task.tokens: token.delete() - def submit(self, token_data=None): + def submit(self, token_data=None, keystone_user=None): self.confirm_state(approved=True, completed=False, cancelled=False) @@ -502,7 +503,7 @@ class BaseTask(object): for action in actions: try: - action.submit(data) + action.submit(data, keystone_user) except Exception as e: handle_task_error(e, self.task, "while submiting task") diff --git a/adjutant/tasks/v1/manager.py b/adjutant/tasks/v1/manager.py index 70fb669..85a94f4 100644 --- a/adjutant/tasks/v1/manager.py +++ b/adjutant/tasks/v1/manager.py @@ -66,15 +66,7 @@ class TaskManager(object): "Task not found with uuid of: '%s'" % task ) if isinstance(task, Task): - try: - return tasks.TASK_CLASSES[task.task_type](task) - except KeyError: - # TODO(adriant): Maybe we should handle this better - # for older deprecated tasks: - raise exceptions.TaskNotRegistered( - "Task type '%s' not registered, " - "and used for existing task." % task.task_type - ) + return task.get_task() raise exceptions.TaskNotFound("Task not found for value of: '%s'" % task) def update(self, task, action_data): @@ -87,9 +79,9 @@ class TaskManager(object): task.approve(approved_by) return task - def submit(self, task, token_data): + def submit(self, task, token_data, keystone_user=None): task = self.get(task) - task.submit(token_data) + task.submit(token_data, keystone_user) return task def cancel(self, task): diff --git a/doc/source/feature-sets.rst b/doc/source/feature-sets.rst index 7664e9d..995c2a1 100644 --- a/doc/source/feature-sets.rst +++ b/doc/source/feature-sets.rst @@ -185,7 +185,7 @@ functions:: # Do some logic here self.action.task.cache['value'] = self.value1 - def _submit(self, data): + def _submit(self, token_data, keystone_user=None): # Do some logic here self.add_note("Submit action performed") diff --git a/releasenotes/notes/authed_token-6d29688676e7ee32.yaml b/releasenotes/notes/authed_token-6d29688676e7ee32.yaml new file mode 100644 index 0000000..71a9415 --- /dev/null +++ b/releasenotes/notes/authed_token-6d29688676e7ee32.yaml @@ -0,0 +1,14 @@ +--- +features: + - | + Tasks can now be configured to required a user to be authenticated when an + Adjutant token is submitted for the final phase of a task. Actions will now + be passed the ``keystone_user`` who submitted the token to do any processing + on that as needed for the final step. +deprecations: + - | + All actions now need to have ``keystone_user`` as a second optional + paramater in the ``submit``function. It should have a default of ``None``, + set as ``keystone_user=None``. Any existing actions without this will continue + to work with a fallback, but that fallback will be removed in the W release + cycle. \ No newline at end of file