From 3caa089623f5114786a7cd946898903b03127574 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Thu, 16 Jan 2025 15:11:13 +0100 Subject: [PATCH 01/18] add job access polocies --- gateway/api/access_policies/jobs.py | 36 +++++++++++++++++++++++++++++ gateway/api/views/jobs.py | 36 +++++++++++------------------ 2 files changed, 50 insertions(+), 22 deletions(-) create mode 100644 gateway/api/access_policies/jobs.py diff --git a/gateway/api/access_policies/jobs.py b/gateway/api/access_policies/jobs.py new file mode 100644 index 000000000..552849f14 --- /dev/null +++ b/gateway/api/access_policies/jobs.py @@ -0,0 +1,36 @@ +""" +Access policies implementation for Job access +""" +import logging + +from api.models import Job + + +logger = logging.getLogger("gateway") + + +class JobAccessPolocies: # pylint: disable=too-few-public-methods + """ + The main objective of this class is to manage the access for the user + to the Job entities. + """ + + @staticmethod + def can_save_result(user, job: Job) -> bool: + """ + Checks if the user has access to a Provider: + + Args: + user: Django user from the request + job: Job instance against to check the access + + Returns: + bool: True or False in case the user has access + """ + + has_access = user.id == job.author + if not has_access: + logger.warning( + "User [%s] has no access to job [%s].", user.id, job.author + ) + return has_access diff --git a/gateway/api/views/jobs.py b/gateway/api/views/jobs.py index 45376e50d..ba5ec7d64 100644 --- a/gateway/api/views/jobs.py +++ b/gateway/api/views/jobs.py @@ -26,6 +26,8 @@ from api.models import Job, RuntimeJob from api.ray import get_job_handler from api.views.enums.type_filter import TypeFilter +from api.services.result_storage import ResultStorage +from api.access_policies.jobs import JobAccessPolocies # pylint: disable=duplicate-code logger = logging.getLogger("gateway") @@ -116,29 +118,19 @@ def result(self, request, pk=None): # pylint: disable=invalid-name,unused-argum tracer = trace.get_tracer("gateway.tracer") ctx = TraceContextTextMapPropagator().extract(carrier=request.headers) with tracer.start_as_current_span("gateway.job.result", context=ctx): - saved = False - attempts_left = 10 - while not saved: - if attempts_left <= 0: - return Response( - {"error": "All attempts to save results failed."}, status=500 - ) - - attempts_left -= 1 + author = self.request.user + job = self.get_object() + can_access = JobAccessPolocies.can_save_result(author, job) - try: - job = self.get_object() - job.result = json.dumps(request.data.get("result")) - job.save() - saved = True - except RecordModifiedError: - logger.warning( - "Job [%s] record has not been updated due to lock. Retrying. Attempts left %s", # pylint: disable=line-too-long - job.id, - attempts_left, - ) - continue - time.sleep(1) + if not can_access: + return Response( + {"message": f"User [{author}] does not have permissions to access to job [{job.id}]."}, + status=status.status.status.HTTP_403_FORBIDDEN, + ) + + result = json.dumps(request.data.get("result")) + result_storage = ResultStorage(author) + result_storage.save(job.id, result) serializer = self.get_serializer(job) return Response(serializer.data) From e7fda8227c3b9f1e5288cf33d9a40d0bf814b593 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Thu, 16 Jan 2025 15:20:17 +0100 Subject: [PATCH 02/18] fix lint --- gateway/api/access_policies/jobs.py | 4 +--- gateway/api/views/jobs.py | 11 ++++++----- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/gateway/api/access_policies/jobs.py b/gateway/api/access_policies/jobs.py index 552849f14..1633226c3 100644 --- a/gateway/api/access_policies/jobs.py +++ b/gateway/api/access_policies/jobs.py @@ -30,7 +30,5 @@ def can_save_result(user, job: Job) -> bool: has_access = user.id == job.author if not has_access: - logger.warning( - "User [%s] has no access to job [%s].", user.id, job.author - ) + logger.warning("User [%s] has no access to job [%s].", user.id, job.author) return has_access diff --git a/gateway/api/views/jobs.py b/gateway/api/views/jobs.py index ba5ec7d64..5428941d1 100644 --- a/gateway/api/views/jobs.py +++ b/gateway/api/views/jobs.py @@ -6,9 +6,7 @@ import json import logging import os -import time -from concurrency.exceptions import RecordModifiedError from django.db.models import Q # pylint: disable=duplicate-code @@ -124,10 +122,13 @@ def result(self, request, pk=None): # pylint: disable=invalid-name,unused-argum if not can_access: return Response( - {"message": f"User [{author}] does not have permissions to access to job [{job.id}]."}, - status=status.status.status.HTTP_403_FORBIDDEN, + { + "message": f"User [{author}] does not have permissions" + + " to access to job [{job.id}]." + }, + status=status.HTTP_403_FORBIDDEN, ) - + result = json.dumps(request.data.get("result")) result_storage = ResultStorage(author) result_storage.save(job.id, result) From cc945ccfc598c1a08c955fda2866f0e82953974b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Thu, 16 Jan 2025 17:42:54 +0100 Subject: [PATCH 03/18] apply review suggestions --- gateway/api/services/result_storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gateway/api/services/result_storage.py b/gateway/api/services/result_storage.py index 028dad187..d706e7d0d 100644 --- a/gateway/api/services/result_storage.py +++ b/gateway/api/services/result_storage.py @@ -24,7 +24,7 @@ def __init__(self, username: str): ) os.makedirs(self.user_results_directory, exist_ok=True) - def __build_result_path(self, job_id: str) -> str: + def __get_result_path(self, job_id: str) -> str: """Construct the full path for a result file.""" return os.path.join( self.user_results_directory, f"{job_id}{self.RESULT_FILE_EXTENSION}" From 3492ff94a25162cbe73f8dab149e1ea34ae05315 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Wed, 22 Jan 2025 11:13:01 +0100 Subject: [PATCH 04/18] fix: create job repository --- gateway/api/access_policies/jobs.py | 5 ++- gateway/api/repositories/jobs.py | 34 +++++++++++++++ gateway/api/services/result_storage.py | 4 +- gateway/api/views/jobs.py | 29 ++++++++----- gateway/tests/api/test_job.py | 58 ++++++++++++++++++++------ 5 files changed, 103 insertions(+), 27 deletions(-) create mode 100644 gateway/api/repositories/jobs.py diff --git a/gateway/api/access_policies/jobs.py b/gateway/api/access_policies/jobs.py index 1633226c3..aa0d1d0bb 100644 --- a/gateway/api/access_policies/jobs.py +++ b/gateway/api/access_policies/jobs.py @@ -28,7 +28,8 @@ def can_save_result(user, job: Job) -> bool: bool: True or False in case the user has access """ - has_access = user.id == job.author + has_access = user.id == job.author.id if not has_access: - logger.warning("User [%s] has no access to job [%s].", user.id, job.author) + logger.warning( + "User [%s] has no access to job [%s].", user.username, job.author) return has_access diff --git a/gateway/api/repositories/jobs.py b/gateway/api/repositories/jobs.py new file mode 100644 index 000000000..1f10de54e --- /dev/null +++ b/gateway/api/repositories/jobs.py @@ -0,0 +1,34 @@ +""" +Repository implementation for Job model +""" +import logging +from api.models import Job +from django.db.models import Q + +logger = logging.getLogger("gateway") + + +class JobsRepository: + """ + The main objective of this class is to manage the access to the Job model + """ + + def get_job_by_id(self, job_id: str) -> Job: + """ + Returns the job for the given id: + + Args: + id (str): id of the job + + Returns: + Job | None: job with the requested id + """ + + id_criteria = Q(id=job_id) + + result_queryset = Job.objects.filter(id_criteria).first() + + if result_queryset is None: + logger.warning("Job [%s] was not found", id) + + return result_queryset diff --git a/gateway/api/services/result_storage.py b/gateway/api/services/result_storage.py index d706e7d0d..4bd6dae68 100644 --- a/gateway/api/services/result_storage.py +++ b/gateway/api/services/result_storage.py @@ -40,7 +40,7 @@ def get(self, job_id: str) -> Optional[Tuple[FileWrapper, str, int]]: - File MIME type - File size in bytes """ - result_path = self.__build_result_path(job_id) + result_path = self.__get_result_path(job_id) if not os.path.exists(result_path): logger.warning( @@ -67,7 +67,7 @@ def save(self, job_id: str, result: str) -> None: name for the result file. result (str): The job result content to be saved in the file. """ - result_path = self.__build_result_path(job_id) + result_path = self.__get_result_path(job_id) with open(result_path, "w", encoding=self.ENCODING) as result_file: result_file.write(result) diff --git a/gateway/api/views/jobs.py b/gateway/api/views/jobs.py index 5428941d1..88fe9fad2 100644 --- a/gateway/api/views/jobs.py +++ b/gateway/api/views/jobs.py @@ -26,6 +26,7 @@ from api.views.enums.type_filter import TypeFilter from api.services.result_storage import ResultStorage from api.access_policies.jobs import JobAccessPolocies +from api.repositories.jobs import JobsRepository # pylint: disable=duplicate-code logger = logging.getLogger("gateway") @@ -36,12 +37,14 @@ endpoint=os.environ.get( "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", "http://otel-collector:4317" ), - insecure=bool(int(os.environ.get("OTEL_EXPORTER_OTLP_TRACES_INSECURE", "0"))), + insecure=bool( + int(os.environ.get("OTEL_EXPORTER_OTLP_TRACES_INSECURE", "0"))), ) ) provider.add_span_processor(otel_exporter) if bool(int(os.environ.get("OTEL_ENABLED", "0"))): - trace._set_tracer_provider(provider, log=False) # pylint: disable=protected-access + trace._set_tracer_provider( + provider, log=False) # pylint: disable=protected-access class JobViewSet(viewsets.GenericViewSet): @@ -51,6 +54,9 @@ class JobViewSet(viewsets.GenericViewSet): BASE_NAME = "jobs" + + jobs_repository = JobsRepository() + def get_serializer_class(self): return self.serializer_class @@ -87,7 +93,8 @@ def retrieve(self, request, pk=None): # pylint: disable=unused-argument if job.program and job.program.provider: provider_groups = job.program.provider.admin_groups.all() author_groups = author.groups.all() - has_access = any(group in provider_groups for group in author_groups) + has_access = any( + group in provider_groups for group in author_groups) if has_access: serializer = self.get_serializer(job) return Response(serializer.data) @@ -117,20 +124,18 @@ def result(self, request, pk=None): # pylint: disable=invalid-name,unused-argum ctx = TraceContextTextMapPropagator().extract(carrier=request.headers) with tracer.start_as_current_span("gateway.job.result", context=ctx): author = self.request.user - job = self.get_object() + job = self.jobs_repository.get_job_by_id(pk) can_access = JobAccessPolocies.can_save_result(author, job) - if not can_access: return Response( { - "message": f"User [{author}] does not have permissions" - + " to access to job [{job.id}]." + "message": f"Job [{job.id}] nor found for user [{author}]" }, - status=status.HTTP_403_FORBIDDEN, + status=status.HTTP_404_NOT_FOUND, ) result = json.dumps(request.data.get("result")) - result_storage = ResultStorage(author) + result_storage = ResultStorage(author.username) result_storage.save(job.id, result) serializer = self.get_serializer(job) @@ -152,7 +157,8 @@ def logs(self, request, pk=None): # pylint: disable=invalid-name,unused-argumen if job.program and job.program.provider: provider_groups = job.program.provider.admin_groups.all() author_groups = author.groups.all() - has_access = any(group in provider_groups for group in author_groups) + has_access = any( + group in provider_groups for group in author_groups) if has_access: return Response({"logs": logs}) return Response({"logs": "No available logs"}) @@ -185,7 +191,8 @@ def stop(self, request, pk=None): # pylint: disable=invalid-name,unused-argumen ] ) for runtime_job_entry in runtime_jobs: - jobinstance = service.job(runtime_job_entry.runtime_job) + jobinstance = service.job( + runtime_job_entry.runtime_job) if jobinstance: try: logger.info( diff --git a/gateway/tests/api/test_job.py b/gateway/tests/api/test_job.py index 6e1b3b414..7dcd68856 100644 --- a/gateway/tests/api/test_job.py +++ b/gateway/tests/api/test_job.py @@ -1,11 +1,13 @@ """Tests jobs APIs.""" +import os from django.urls import reverse from rest_framework import status from rest_framework.test import APITestCase from api.models import Job from django.contrib.auth import models +from django.conf import settings class TestJobApi(APITestCase): @@ -47,7 +49,8 @@ def test_job_catalog_list(self): ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) self.assertEqual(jobs_response.data.get("count"), 1) - self.assertEqual(jobs_response.data.get("results")[0].get("status"), "QUEUED") + self.assertEqual(jobs_response.data.get( + "results")[0].get("status"), "QUEUED") self.assertEqual( jobs_response.data.get("results")[0].get("result"), '{"somekey":1}' ) @@ -64,11 +67,13 @@ def test_job_serverless_list(self): job_status = jobs_response.data.get("results")[0].get("status") if job_status == "SUCCEEDED": self.assertEqual( - jobs_response.data.get("results")[0].get("result"), '{"somekey":1}' + jobs_response.data.get("results")[0].get( + "result"), '{"somekey":1}' ) elif job_status == "QUEUED": self.assertEqual( - jobs_response.data.get("results")[0].get("result"), '{"somekey":2}' + jobs_response.data.get("results")[0].get( + "result"), '{"somekey":2}' ) def test_job_detail(self): @@ -76,7 +81,8 @@ def test_job_detail(self): self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]), + reverse("v1:jobs-detail", + args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -89,7 +95,8 @@ def test_job_provider_detail(self): self.client.force_authenticate(user=user) jobs_response = self.client.get( - reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec86"]), + reverse("v1:jobs-detail", + args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec86"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -101,7 +108,8 @@ def test_not_authorized_job_detail(self): self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec84"]), + reverse("v1:jobs-detail", + args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec84"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_404_NOT_FOUND) @@ -110,14 +118,35 @@ def test_job_save_result(self): """Tests job results save.""" self._authorize() + job_id = "1a7947f9-6ae8-4e3d-ac1e-e7d608deec86" jobs_response = self.client.post( - reverse("v1:jobs-result", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]), + reverse("v1:jobs-result", + args=[job_id]), format="json", data={"result": {"ultimate": 42}}, ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) self.assertEqual(jobs_response.data.get("status"), "SUCCEEDED") self.assertEqual(jobs_response.data.get("result"), '{"ultimate": 42}') + result_path = os.path.join( + settings.MEDIA_ROOT, "test_user", "result", f"{job_id}.json" + ) + self.assertTrue(os.path.exists(result_path)) + + def test_not_authorized_job_save_result(self): + """Tests job results save.""" + self._authorize() + job_id = "1a7947f9-6ae8-4e3d-ac1e-e7d608deec84" + jobs_response = self.client.post( + reverse("v1:jobs-result", + args=[job_id]), + format="json", + data={"result": {"ultimate": 42}}, + ) + + self.assertEqual(jobs_response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(jobs_response.data.get("message"), + f"Job [{job_id}] nor found for user [test_user]") def test_stop_job(self): """Tests job stop.""" @@ -135,14 +164,16 @@ def test_stop_job(self): id__exact="1a7947f9-6ae8-4e3d-ac1e-e7d608deec83" ).first() self.assertEqual(job.status, Job.STOPPED) - self.assertEqual(job_stop_response.data.get("message"), "Job has been stopped.") + self.assertEqual(job_stop_response.data.get( + "message"), "Job has been stopped.") def test_job_logs_by_author_for_function_without_provider(self): """Tests job log by job author.""" self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-logs", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]), + reverse("v1:jobs-logs", + args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -153,7 +184,8 @@ def test_job_logs_by_author_for_function_with_provider(self): self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-logs", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), + reverse("v1:jobs-logs", + args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -165,7 +197,8 @@ def test_job_logs_by_function_provider(self): self.client.force_authenticate(user=user) jobs_response = self.client.get( - reverse("v1:jobs-logs", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), + reverse("v1:jobs-logs", + args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -177,7 +210,8 @@ def test_job_logs(self): self.client.force_authenticate(user=user) jobs_response = self.client.get( - reverse("v1:jobs-logs", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), + reverse("v1:jobs-logs", + args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) From 78cb7f1a1938663ed43d0af30a3f10713c5a53cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Wed, 22 Jan 2025 11:52:04 +0100 Subject: [PATCH 05/18] fix job id --- gateway/tests/api/test_job.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gateway/tests/api/test_job.py b/gateway/tests/api/test_job.py index 7dcd68856..3b45540b7 100644 --- a/gateway/tests/api/test_job.py +++ b/gateway/tests/api/test_job.py @@ -118,7 +118,7 @@ def test_job_save_result(self): """Tests job results save.""" self._authorize() - job_id = "1a7947f9-6ae8-4e3d-ac1e-e7d608deec86" + job_id = "1a7947f9-6ae8-4e3d-ac1e-e7d608deec82" jobs_response = self.client.post( reverse("v1:jobs-result", args=[job_id]), From 2030f0e2485ccf7832ae1f50bb68efe344aae1bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Wed, 22 Jan 2025 16:27:25 +0100 Subject: [PATCH 06/18] fix result asignment --- gateway/api/views/jobs.py | 5 +-- gateway/tests/api/test_job.py | 41 +++++++++++-------- .../1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json | 1 + 3 files changed, 28 insertions(+), 19 deletions(-) create mode 100644 gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json diff --git a/gateway/api/views/jobs.py b/gateway/api/views/jobs.py index 88fe9fad2..b8a3273a1 100644 --- a/gateway/api/views/jobs.py +++ b/gateway/api/views/jobs.py @@ -54,7 +54,6 @@ class JobViewSet(viewsets.GenericViewSet): BASE_NAME = "jobs" - jobs_repository = JobsRepository() def get_serializer_class(self): @@ -134,9 +133,9 @@ def result(self, request, pk=None): # pylint: disable=invalid-name,unused-argum status=status.HTTP_404_NOT_FOUND, ) - result = json.dumps(request.data.get("result")) + job.result = json.dumps(request.data.get("result")) result_storage = ResultStorage(author.username) - result_storage.save(job.id, result) + result_storage.save(job.id, job.result) serializer = self.get_serializer(job) return Response(serializer.data) diff --git a/gateway/tests/api/test_job.py b/gateway/tests/api/test_job.py index 3b45540b7..61d0e863c 100644 --- a/gateway/tests/api/test_job.py +++ b/gateway/tests/api/test_job.py @@ -116,22 +116,31 @@ def test_not_authorized_job_detail(self): def test_job_save_result(self): """Tests job results save.""" - self._authorize() - - job_id = "1a7947f9-6ae8-4e3d-ac1e-e7d608deec82" - jobs_response = self.client.post( - reverse("v1:jobs-result", - args=[job_id]), - format="json", - data={"result": {"ultimate": 42}}, - ) - self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) - self.assertEqual(jobs_response.data.get("status"), "SUCCEEDED") - self.assertEqual(jobs_response.data.get("result"), '{"ultimate": 42}') - result_path = os.path.join( - settings.MEDIA_ROOT, "test_user", "result", f"{job_id}.json" - ) - self.assertTrue(os.path.exists(result_path)) + media_root = os.path.join( + os.path.dirname(os.path.abspath(__file__)), + "..", + "resources", + "fake_media", + ) + media_root = os.path.normpath(os.path.join(os.getcwd(), media_root)) + + with self.settings(MEDIA_ROOT=media_root): + self._authorize() + + job_id = "1a7947f9-6ae8-4e3d-ac1e-e7d608deec82" + jobs_response = self.client.post( + reverse("v1:jobs-result", + args=[job_id]), + format="json", + data={"result": {"ultimate": 42}}, + ) + self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) + self.assertEqual(jobs_response.data.get( + "result"), '{"ultimate": 42}') + result_path = os.path.join( + settings.MEDIA_ROOT, "test_user", "result", f"{job_id}.json" + ) + self.assertTrue(os.path.exists(result_path)) def test_not_authorized_job_save_result(self): """Tests job results save.""" diff --git a/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json b/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json new file mode 100644 index 000000000..a132681f7 --- /dev/null +++ b/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json @@ -0,0 +1 @@ +{"ultimate": 42} \ No newline at end of file From 96d1788f3101f4647d9f01d1d5c3cda90bdf82d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Thu, 23 Jan 2025 10:24:17 +0100 Subject: [PATCH 07/18] fix test and lint --- gateway/api/access_policies/jobs.py | 3 +- gateway/api/repositories/jobs.py | 4 +-- gateway/api/views/jobs.py | 19 ++++------- gateway/tests/api/test_job.py | 50 +++++++++++------------------ 4 files changed, 29 insertions(+), 47 deletions(-) diff --git a/gateway/api/access_policies/jobs.py b/gateway/api/access_policies/jobs.py index aa0d1d0bb..ddb36bd80 100644 --- a/gateway/api/access_policies/jobs.py +++ b/gateway/api/access_policies/jobs.py @@ -31,5 +31,6 @@ def can_save_result(user, job: Job) -> bool: has_access = user.id == job.author.id if not has_access: logger.warning( - "User [%s] has no access to job [%s].", user.username, job.author) + "User [%s] has no access to job [%s].", user.username, job.author + ) return has_access diff --git a/gateway/api/repositories/jobs.py b/gateway/api/repositories/jobs.py index 1f10de54e..4f7fd3857 100644 --- a/gateway/api/repositories/jobs.py +++ b/gateway/api/repositories/jobs.py @@ -2,13 +2,13 @@ Repository implementation for Job model """ import logging -from api.models import Job from django.db.models import Q +from api.models import Job logger = logging.getLogger("gateway") -class JobsRepository: +class JobsRepository: # pylint: disable=too-few-public-methods """ The main objective of this class is to manage the access to the Job model """ diff --git a/gateway/api/views/jobs.py b/gateway/api/views/jobs.py index b8a3273a1..e00d60c15 100644 --- a/gateway/api/views/jobs.py +++ b/gateway/api/views/jobs.py @@ -37,14 +37,12 @@ endpoint=os.environ.get( "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", "http://otel-collector:4317" ), - insecure=bool( - int(os.environ.get("OTEL_EXPORTER_OTLP_TRACES_INSECURE", "0"))), + insecure=bool(int(os.environ.get("OTEL_EXPORTER_OTLP_TRACES_INSECURE", "0"))), ) ) provider.add_span_processor(otel_exporter) if bool(int(os.environ.get("OTEL_ENABLED", "0"))): - trace._set_tracer_provider( - provider, log=False) # pylint: disable=protected-access + trace._set_tracer_provider(provider, log=False) # pylint: disable=protected-access class JobViewSet(viewsets.GenericViewSet): @@ -92,8 +90,7 @@ def retrieve(self, request, pk=None): # pylint: disable=unused-argument if job.program and job.program.provider: provider_groups = job.program.provider.admin_groups.all() author_groups = author.groups.all() - has_access = any( - group in provider_groups for group in author_groups) + has_access = any(group in provider_groups for group in author_groups) if has_access: serializer = self.get_serializer(job) return Response(serializer.data) @@ -127,9 +124,7 @@ def result(self, request, pk=None): # pylint: disable=invalid-name,unused-argum can_access = JobAccessPolocies.can_save_result(author, job) if not can_access: return Response( - { - "message": f"Job [{job.id}] nor found for user [{author}]" - }, + {"message": f"Job [{job.id}] nor found for user [{author}]"}, status=status.HTTP_404_NOT_FOUND, ) @@ -156,8 +151,7 @@ def logs(self, request, pk=None): # pylint: disable=invalid-name,unused-argumen if job.program and job.program.provider: provider_groups = job.program.provider.admin_groups.all() author_groups = author.groups.all() - has_access = any( - group in provider_groups for group in author_groups) + has_access = any(group in provider_groups for group in author_groups) if has_access: return Response({"logs": logs}) return Response({"logs": "No available logs"}) @@ -190,8 +184,7 @@ def stop(self, request, pk=None): # pylint: disable=invalid-name,unused-argumen ] ) for runtime_job_entry in runtime_jobs: - jobinstance = service.job( - runtime_job_entry.runtime_job) + jobinstance = service.job(runtime_job_entry.runtime_job) if jobinstance: try: logger.info( diff --git a/gateway/tests/api/test_job.py b/gateway/tests/api/test_job.py index 61d0e863c..3df8db67a 100644 --- a/gateway/tests/api/test_job.py +++ b/gateway/tests/api/test_job.py @@ -49,8 +49,7 @@ def test_job_catalog_list(self): ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) self.assertEqual(jobs_response.data.get("count"), 1) - self.assertEqual(jobs_response.data.get( - "results")[0].get("status"), "QUEUED") + self.assertEqual(jobs_response.data.get("results")[0].get("status"), "QUEUED") self.assertEqual( jobs_response.data.get("results")[0].get("result"), '{"somekey":1}' ) @@ -67,13 +66,11 @@ def test_job_serverless_list(self): job_status = jobs_response.data.get("results")[0].get("status") if job_status == "SUCCEEDED": self.assertEqual( - jobs_response.data.get("results")[0].get( - "result"), '{"somekey":1}' + jobs_response.data.get("results")[0].get("result"), '{"somekey":1}' ) elif job_status == "QUEUED": self.assertEqual( - jobs_response.data.get("results")[0].get( - "result"), '{"somekey":2}' + jobs_response.data.get("results")[0].get("result"), '{"somekey":2}' ) def test_job_detail(self): @@ -81,8 +78,7 @@ def test_job_detail(self): self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-detail", - args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]), + reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -95,8 +91,7 @@ def test_job_provider_detail(self): self.client.force_authenticate(user=user) jobs_response = self.client.get( - reverse("v1:jobs-detail", - args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec86"]), + reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec86"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -108,8 +103,7 @@ def test_not_authorized_job_detail(self): self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-detail", - args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec84"]), + reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec84"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_404_NOT_FOUND) @@ -129,16 +123,14 @@ def test_job_save_result(self): job_id = "1a7947f9-6ae8-4e3d-ac1e-e7d608deec82" jobs_response = self.client.post( - reverse("v1:jobs-result", - args=[job_id]), + reverse("v1:jobs-result", args=[job_id]), format="json", data={"result": {"ultimate": 42}}, ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) - self.assertEqual(jobs_response.data.get( - "result"), '{"ultimate": 42}') + self.assertEqual(jobs_response.data.get("result"), '{"ultimate": 42}') result_path = os.path.join( - settings.MEDIA_ROOT, "test_user", "result", f"{job_id}.json" + settings.MEDIA_ROOT, "test_user", "results", f"{job_id}.json" ) self.assertTrue(os.path.exists(result_path)) @@ -147,15 +139,16 @@ def test_not_authorized_job_save_result(self): self._authorize() job_id = "1a7947f9-6ae8-4e3d-ac1e-e7d608deec84" jobs_response = self.client.post( - reverse("v1:jobs-result", - args=[job_id]), + reverse("v1:jobs-result", args=[job_id]), format="json", data={"result": {"ultimate": 42}}, ) self.assertEqual(jobs_response.status_code, status.HTTP_404_NOT_FOUND) - self.assertEqual(jobs_response.data.get("message"), - f"Job [{job_id}] nor found for user [test_user]") + self.assertEqual( + jobs_response.data.get("message"), + f"Job [{job_id}] nor found for user [test_user]", + ) def test_stop_job(self): """Tests job stop.""" @@ -173,16 +166,14 @@ def test_stop_job(self): id__exact="1a7947f9-6ae8-4e3d-ac1e-e7d608deec83" ).first() self.assertEqual(job.status, Job.STOPPED) - self.assertEqual(job_stop_response.data.get( - "message"), "Job has been stopped.") + self.assertEqual(job_stop_response.data.get("message"), "Job has been stopped.") def test_job_logs_by_author_for_function_without_provider(self): """Tests job log by job author.""" self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-logs", - args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]), + reverse("v1:jobs-logs", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -193,8 +184,7 @@ def test_job_logs_by_author_for_function_with_provider(self): self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-logs", - args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), + reverse("v1:jobs-logs", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -206,8 +196,7 @@ def test_job_logs_by_function_provider(self): self.client.force_authenticate(user=user) jobs_response = self.client.get( - reverse("v1:jobs-logs", - args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), + reverse("v1:jobs-logs", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -219,8 +208,7 @@ def test_job_logs(self): self.client.force_authenticate(user=user) jobs_response = self.client.get( - reverse("v1:jobs-logs", - args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), + reverse("v1:jobs-logs", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec85"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) From 6ecdcf1897ba9989f29e57e2891d1b1fcd35cbc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Thu, 23 Jan 2025 15:07:53 +0100 Subject: [PATCH 08/18] feat: retrieve job results from result storage --- gateway/api/access_policies/jobs.py | 34 ++++++++++++++++-- gateway/api/v1/serializers.py | 14 +++++++- gateway/api/views/jobs.py | 55 +++++++++++++++++++---------- 3 files changed, 81 insertions(+), 22 deletions(-) diff --git a/gateway/api/access_policies/jobs.py b/gateway/api/access_policies/jobs.py index ddb36bd80..9f99cec58 100644 --- a/gateway/api/access_policies/jobs.py +++ b/gateway/api/access_policies/jobs.py @@ -2,7 +2,7 @@ Access policies implementation for Job access """ import logging - +from django.contrib.auth.models import User from api.models import Job @@ -14,9 +14,8 @@ class JobAccessPolocies: # pylint: disable=too-few-public-methods The main objective of this class is to manage the access for the user to the Job entities. """ - @staticmethod - def can_save_result(user, job: Job) -> bool: + def can_access(user: User, job: Job) -> bool: """ Checks if the user has access to a Provider: @@ -28,6 +27,35 @@ def can_save_result(user, job: Job) -> bool: bool: True or False in case the user has access """ + print(type(user)) + is_provider_job = job.program and job.program.provider + if is_provider_job: + provider_groups = job.program.provider.admin_groups.all() + author_groups = user.groups.all() + has_access = any( + group in provider_groups for group in author_groups) + else: + has_access = user.id == job.author.id + + if not has_access: + logger.warning( + "User [%s] has no access to job [%s].", user.username, job.author + ) + return has_access + + @staticmethod + def can_save_result(user: User, job: Job) -> bool: + """ + Checks if the user has permissions to save the result of a job: + + Args: + user: Django user from the request + job: Job instance against to check the permission + + Returns: + bool: True or False in case the user has permissions + """ + has_access = user.id == job.author.id if not has_access: logger.warning( diff --git a/gateway/api/v1/serializers.py b/gateway/api/v1/serializers.py index 65ae25409..11832637e 100644 --- a/gateway/api/v1/serializers.py +++ b/gateway/api/v1/serializers.py @@ -67,7 +67,8 @@ def validate(self, attrs): # pylint: disable=too-many-branches title = attrs.get("title") provider = attrs.get("provider", None) if provider and "/" in title: - raise ValidationError("Provider defined in title and in provider fields.") + raise ValidationError( + "Provider defined in title and in provider fields.") title_split = title.split("/") if len(title_split) > 2: @@ -146,6 +147,17 @@ class Meta(serializers.JobSerializer.Meta): fields = ["id", "result", "status", "program", "created"] +class JobSerializerWithoutResult(serializers.JobSerializer): + """ + Job serializer first version. Include basic fields from the initial model. + """ + + program = ProgramSerializer(many=False) + + class Meta(serializers.JobSerializer.Meta): + fields = ["id", "status", "program", "created"] + + class RuntimeJobSerializer(serializers.RuntimeJobSerializer): """ Runtime job serializer first version. Serializer for the runtime job model. diff --git a/gateway/api/views/jobs.py b/gateway/api/views/jobs.py index e00d60c15..30b9b65d2 100644 --- a/gateway/api/views/jobs.py +++ b/gateway/api/views/jobs.py @@ -27,6 +27,7 @@ from api.services.result_storage import ResultStorage from api.access_policies.jobs import JobAccessPolocies from api.repositories.jobs import JobsRepository +from api.v1 import serializers as v1_serializers # pylint: disable=duplicate-code logger = logging.getLogger("gateway") @@ -37,12 +38,14 @@ endpoint=os.environ.get( "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", "http://otel-collector:4317" ), - insecure=bool(int(os.environ.get("OTEL_EXPORTER_OTLP_TRACES_INSECURE", "0"))), + insecure=bool( + int(os.environ.get("OTEL_EXPORTER_OTLP_TRACES_INSECURE", "0"))), ) ) provider.add_span_processor(otel_exporter) if bool(int(os.environ.get("OTEL_ENABLED", "0"))): - trace._set_tracer_provider(provider, log=False) # pylint: disable=protected-access + trace._set_tracer_provider( + provider, log=False) # pylint: disable=protected-access class JobViewSet(viewsets.GenericViewSet): @@ -57,6 +60,14 @@ class JobViewSet(viewsets.GenericViewSet): def get_serializer_class(self): return self.serializer_class + @staticmethod + def get_serializer_job(*args, **kwargs): + return v1_serializers.JobSerializer(*args, **kwargs) + + @staticmethod + def get_serializer_job_without_result(*args, **kwargs): + return v1_serializers.JobSerializerWithoutResult(*args, **kwargs) + def get_queryset(self): type_filter = self.request.query_params.get("filter") if type_filter: @@ -79,24 +90,29 @@ def retrieve(self, request, pk=None): # pylint: disable=unused-argument tracer = trace.get_tracer("gateway.tracer") ctx = TraceContextTextMapPropagator().extract(carrier=request.headers) with tracer.start_as_current_span("gateway.job.retrieve", context=ctx): - job = Job.objects.filter(pk=pk).first() - if job is None: + + author = self.request.user + job = self.jobs_repository.get_job_by_id(pk) + + if not JobAccessPolocies.can_access(author, job): logger.warning("Job [%s] not found", pk) return Response( {"message": f"Job [{pk}] was not found."}, status=status.HTTP_404_NOT_FOUND, ) - author = self.request.user - if job.program and job.program.provider: - provider_groups = job.program.provider.admin_groups.all() - author_groups = author.groups.all() - has_access = any(group in provider_groups for group in author_groups) - if has_access: - serializer = self.get_serializer(job) - return Response(serializer.data) - instance = self.get_object() - serializer = self.get_serializer(instance) - return Response(serializer.data) + + is_provider_job = job.program and job.program.provider + if is_provider_job: + serializer = self.get_serializer_job_without_result(job) + return Response(serializer.data) + + serializer = self.get_serializer_job(job) + result_store = ResultStorage(author.username) + results = result_store.get(job.id) + if results is not None: + serializer.results = results + + return Response(serializer.data) def list(self, request): """List jobs:""" @@ -124,7 +140,8 @@ def result(self, request, pk=None): # pylint: disable=invalid-name,unused-argum can_access = JobAccessPolocies.can_save_result(author, job) if not can_access: return Response( - {"message": f"Job [{job.id}] nor found for user [{author}]"}, + {"message": f"Job [{ + job.id}] nor found for user [{author}]"}, status=status.HTTP_404_NOT_FOUND, ) @@ -151,7 +168,8 @@ def logs(self, request, pk=None): # pylint: disable=invalid-name,unused-argumen if job.program and job.program.provider: provider_groups = job.program.provider.admin_groups.all() author_groups = author.groups.all() - has_access = any(group in provider_groups for group in author_groups) + has_access = any( + group in provider_groups for group in author_groups) if has_access: return Response({"logs": logs}) return Response({"logs": "No available logs"}) @@ -184,7 +202,8 @@ def stop(self, request, pk=None): # pylint: disable=invalid-name,unused-argumen ] ) for runtime_job_entry in runtime_jobs: - jobinstance = service.job(runtime_job_entry.runtime_job) + jobinstance = service.job( + runtime_job_entry.runtime_job) if jobinstance: try: logger.info( From e918eb4b7e1448ffeff3496b32877d4b8e767406 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Thu, 23 Jan 2025 15:08:35 +0100 Subject: [PATCH 09/18] remove result file --- gateway/tests/api/test_job.py | 1 + 1 file changed, 1 insertion(+) diff --git a/gateway/tests/api/test_job.py b/gateway/tests/api/test_job.py index 3df8db67a..3276033e7 100644 --- a/gateway/tests/api/test_job.py +++ b/gateway/tests/api/test_job.py @@ -133,6 +133,7 @@ def test_job_save_result(self): settings.MEDIA_ROOT, "test_user", "results", f"{job_id}.json" ) self.assertTrue(os.path.exists(result_path)) + os.remove(result_path) def test_not_authorized_job_save_result(self): """Tests job results save.""" From 16651685e80b8c7bcf78798beb241dab20d0b229 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Tue, 28 Jan 2025 17:26:57 +0100 Subject: [PATCH 10/18] retrieve job results --- gateway/api/access_policies/jobs.py | 1 - gateway/api/services/result_storage.py | 17 ++++++++++------- gateway/api/views/jobs.py | 12 ++++++------ gateway/tests/api/test_job.py | 15 +++++++++++++-- .../1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json | 1 - 5 files changed, 29 insertions(+), 17 deletions(-) delete mode 100644 gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json diff --git a/gateway/api/access_policies/jobs.py b/gateway/api/access_policies/jobs.py index 9f99cec58..41ffbe7b1 100644 --- a/gateway/api/access_policies/jobs.py +++ b/gateway/api/access_policies/jobs.py @@ -27,7 +27,6 @@ def can_access(user: User, job: Job) -> bool: bool: True or False in case the user has access """ - print(type(user)) is_provider_job = job.program and job.program.provider if is_provider_job: provider_groups = job.program.provider.admin_groups.all() diff --git a/gateway/api/services/result_storage.py b/gateway/api/services/result_storage.py index 4bd6dae68..2e465d2ea 100644 --- a/gateway/api/services/result_storage.py +++ b/gateway/api/services/result_storage.py @@ -30,7 +30,7 @@ def __get_result_path(self, job_id: str) -> str: self.user_results_directory, f"{job_id}{self.RESULT_FILE_EXTENSION}" ) - def get(self, job_id: str) -> Optional[Tuple[FileWrapper, str, int]]: + def get(self, job_id: str) -> Optional[str]: """ Retrieve a result file for the given job ID. @@ -50,13 +50,16 @@ def get(self, job_id: str) -> Optional[Tuple[FileWrapper, str, int]]: ) return None - with open(result_path, "rb") as result_file: - file_wrapper = FileWrapper(result_file) - file_type = ( - mimetypes.guess_type(result_path)[0] or "application/octet-stream" + try: + with open(result_path, "r", encoding="utf-8") as result_file: + return result_file.read() + except (UnicodeDecodeError, IOError) as e: + logger.error( + "Failed to read result file for job ID '%s': %s", + job_id, + str(e), ) - file_size = os.path.getsize(result_path) - return file_wrapper, file_type, file_size + return None def save(self, job_id: str, result: str) -> None: """ diff --git a/gateway/api/views/jobs.py b/gateway/api/views/jobs.py index 30b9b65d2..114a2472c 100644 --- a/gateway/api/views/jobs.py +++ b/gateway/api/views/jobs.py @@ -105,12 +105,13 @@ def retrieve(self, request, pk=None): # pylint: disable=unused-argument if is_provider_job: serializer = self.get_serializer_job_without_result(job) return Response(serializer.data) + + result_store = ResultStorage(author.username) + result = result_store.get(job.id) + if result is not None: + job.result = result serializer = self.get_serializer_job(job) - result_store = ResultStorage(author.username) - results = result_store.get(job.id) - if results is not None: - serializer.results = results return Response(serializer.data) @@ -140,8 +141,7 @@ def result(self, request, pk=None): # pylint: disable=invalid-name,unused-argum can_access = JobAccessPolocies.can_save_result(author, job) if not can_access: return Response( - {"message": f"Job [{ - job.id}] nor found for user [{author}]"}, + {"message": f"Job [{job.id}] nor found for user [{author}]"}, status=status.HTTP_404_NOT_FOUND, ) diff --git a/gateway/tests/api/test_job.py b/gateway/tests/api/test_job.py index 3df8db67a..fdd75128e 100644 --- a/gateway/tests/api/test_job.py +++ b/gateway/tests/api/test_job.py @@ -82,7 +82,17 @@ def test_job_detail(self): format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) - self.assertEqual(jobs_response.data.get("status"), "SUCCEEDED") + self.assertEqual(jobs_response.data.get("result"), '{"ultimate": 42}') + + def test_job_detail_without_result_file(self): + """Tests job detail authorized.""" + self._authorize() + + jobs_response = self.client.get( + reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec83"]), + format="json", + ) + self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) self.assertEqual(jobs_response.data.get("result"), '{"somekey":1}') def test_job_provider_detail(self): @@ -96,7 +106,7 @@ def test_job_provider_detail(self): ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) self.assertEqual(jobs_response.data.get("status"), "QUEUED") - self.assertEqual(jobs_response.data.get("result"), '{"somekey":1}') + self.assertEqual(jobs_response.data.get("result"), None) def test_not_authorized_job_detail(self): """Tests job detail fails trying to access to other user job.""" @@ -133,6 +143,7 @@ def test_job_save_result(self): settings.MEDIA_ROOT, "test_user", "results", f"{job_id}.json" ) self.assertTrue(os.path.exists(result_path)) + os.remove(result_path) def test_not_authorized_job_save_result(self): """Tests job results save.""" diff --git a/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json b/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json deleted file mode 100644 index a132681f7..000000000 --- a/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json +++ /dev/null @@ -1 +0,0 @@ -{"ultimate": 42} \ No newline at end of file From 46e4d532674d8be5b88648553d136f15229020ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Tue, 28 Jan 2025 17:28:35 +0100 Subject: [PATCH 11/18] apply review suggestions --- gateway/api/access_policies/jobs.py | 4 ++-- gateway/api/repositories/jobs.py | 4 +--- .../results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json | 1 - 3 files changed, 3 insertions(+), 6 deletions(-) delete mode 100644 gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json diff --git a/gateway/api/access_policies/jobs.py b/gateway/api/access_policies/jobs.py index ddb36bd80..95daf4482 100644 --- a/gateway/api/access_policies/jobs.py +++ b/gateway/api/access_policies/jobs.py @@ -18,7 +18,7 @@ class JobAccessPolocies: # pylint: disable=too-few-public-methods @staticmethod def can_save_result(user, job: Job) -> bool: """ - Checks if the user has access to a Provider: + Checks if the user has access to save the result of a Job: Args: user: Django user from the request @@ -31,6 +31,6 @@ def can_save_result(user, job: Job) -> bool: has_access = user.id == job.author.id if not has_access: logger.warning( - "User [%s] has no access to job [%s].", user.username, job.author + "User [%s] has no access to save the result of the job [%s].", user.username, job.author" ) return has_access diff --git a/gateway/api/repositories/jobs.py b/gateway/api/repositories/jobs.py index 4f7fd3857..93b9e9b4b 100644 --- a/gateway/api/repositories/jobs.py +++ b/gateway/api/repositories/jobs.py @@ -24,9 +24,7 @@ def get_job_by_id(self, job_id: str) -> Job: Job | None: job with the requested id """ - id_criteria = Q(id=job_id) - - result_queryset = Job.objects.filter(id_criteria).first() + result_queryset = Job.objects.filter(id=job_id).first() if result_queryset is None: logger.warning("Job [%s] was not found", id) diff --git a/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json b/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json deleted file mode 100644 index a132681f7..000000000 --- a/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec82.json +++ /dev/null @@ -1 +0,0 @@ -{"ultimate": 42} \ No newline at end of file From 989a2fbfc95d2e1833807035c126939e7ab7aa77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Tue, 28 Jan 2025 17:30:05 +0100 Subject: [PATCH 12/18] fix typo --- gateway/api/access_policies/jobs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gateway/api/access_policies/jobs.py b/gateway/api/access_policies/jobs.py index 95daf4482..3a146818c 100644 --- a/gateway/api/access_policies/jobs.py +++ b/gateway/api/access_policies/jobs.py @@ -31,6 +31,6 @@ def can_save_result(user, job: Job) -> bool: has_access = user.id == job.author.id if not has_access: logger.warning( - "User [%s] has no access to save the result of the job [%s].", user.username, job.author" + "User [%s] has no access to save the result of the job [%s].", user.username, job.author ) return has_access From 35ba5cd8efc443c988c77b794e7beb99b2f8ae8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Tue, 28 Jan 2025 17:43:23 +0100 Subject: [PATCH 13/18] add test fake file --- gateway/tests/api/test_job.py | 26 +++++++++++++------ .../1a7947f9-6ae8-4e3d-ac1e-e7d608deec83.json | 1 + 2 files changed, 19 insertions(+), 8 deletions(-) create mode 100644 gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec83.json diff --git a/gateway/tests/api/test_job.py b/gateway/tests/api/test_job.py index fdd75128e..926b44190 100644 --- a/gateway/tests/api/test_job.py +++ b/gateway/tests/api/test_job.py @@ -75,21 +75,31 @@ def test_job_serverless_list(self): def test_job_detail(self): """Tests job detail authorized.""" - self._authorize() - - jobs_response = self.client.get( - reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]), - format="json", + media_root = os.path.join( + os.path.dirname(os.path.abspath(__file__)), + "..", + "resources", + "fake_media", ) - self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) - self.assertEqual(jobs_response.data.get("result"), '{"ultimate": 42}') + media_root = os.path.normpath(os.path.join(os.getcwd(), media_root)) + + with self.settings(MEDIA_ROOT=media_root): + self._authorize() + + jobs_response = self.client.get( + reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec83"]), + format="json", + ) + self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) + self.assertEqual(jobs_response.data.get("result"), '{"ultimate": 42}') def test_job_detail_without_result_file(self): """Tests job detail authorized.""" + self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec83"]), + reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec86"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) diff --git a/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec83.json b/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec83.json new file mode 100644 index 000000000..a132681f7 --- /dev/null +++ b/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec83.json @@ -0,0 +1 @@ +{"ultimate": 42} \ No newline at end of file From a60fdee2a22236c73d5eb8e1383e4842aa13870d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Tue, 28 Jan 2025 18:19:17 +0100 Subject: [PATCH 14/18] fix test media path for retrieve result --- gateway/api/services/result_storage.py | 4 +-- gateway/api/views/jobs.py | 20 +++++++++-- gateway/tests/api/test_job.py | 34 ++++++++++++------- ...8317718f-5c0d-4fb6-9947-72e480b8a348.json} | 0 4 files changed, 41 insertions(+), 17 deletions(-) rename gateway/tests/resources/fake_media/test_user/results/{1a7947f9-6ae8-4e3d-ac1e-e7d608deec83.json => 8317718f-5c0d-4fb6-9947-72e480b8a348.json} (100%) diff --git a/gateway/api/services/result_storage.py b/gateway/api/services/result_storage.py index 2e465d2ea..68abcf9e4 100644 --- a/gateway/api/services/result_storage.py +++ b/gateway/api/services/result_storage.py @@ -27,7 +27,8 @@ def __init__(self, username: str): def __get_result_path(self, job_id: str) -> str: """Construct the full path for a result file.""" return os.path.join( - self.user_results_directory, f"{job_id}{self.RESULT_FILE_EXTENSION}" + self.user_results_directory, f"{ + job_id}{self.RESULT_FILE_EXTENSION}" ) def get(self, job_id: str) -> Optional[str]: @@ -41,7 +42,6 @@ def get(self, job_id: str) -> Optional[str]: - File size in bytes """ result_path = self.__get_result_path(job_id) - if not os.path.exists(result_path): logger.warning( "Result file for job ID '%s' not found in directory '%s'.", diff --git a/gateway/api/views/jobs.py b/gateway/api/views/jobs.py index 114a2472c..ab753247f 100644 --- a/gateway/api/views/jobs.py +++ b/gateway/api/views/jobs.py @@ -93,6 +93,12 @@ def retrieve(self, request, pk=None): # pylint: disable=unused-argument author = self.request.user job = self.jobs_repository.get_job_by_id(pk) + if job is None: + logger.info("Job [%s] nor found", pk) + return Response( + {"message": f"Job [{pk}] nor found"}, + status=status.HTTP_404_NOT_FOUND, + ) if not JobAccessPolocies.can_access(author, job): logger.warning("Job [%s] not found", pk) @@ -105,9 +111,9 @@ def retrieve(self, request, pk=None): # pylint: disable=unused-argument if is_provider_job: serializer = self.get_serializer_job_without_result(job) return Response(serializer.data) - + result_store = ResultStorage(author.username) - result = result_store.get(job.id) + result = result_store.get(str(job.id)) if result is not None: job.result = result @@ -138,10 +144,18 @@ def result(self, request, pk=None): # pylint: disable=invalid-name,unused-argum with tracer.start_as_current_span("gateway.job.result", context=ctx): author = self.request.user job = self.jobs_repository.get_job_by_id(pk) + if job is None: + logger.info("Job [%s] nor found", pk) + return Response( + {"message": f"Job [{pk}] nor found"}, + status=status.HTTP_404_NOT_FOUND, + ) + can_access = JobAccessPolocies.can_save_result(author, job) if not can_access: + logger.info("Job [%s] nor found for author %s", pk, author.username) return Response( - {"message": f"Job [{job.id}] nor found for user [{author}]"}, + {"message": f"Job [{job.id}] nor found"}, status=status.HTTP_404_NOT_FOUND, ) diff --git a/gateway/tests/api/test_job.py b/gateway/tests/api/test_job.py index 1f95d2248..871e1a5ea 100644 --- a/gateway/tests/api/test_job.py +++ b/gateway/tests/api/test_job.py @@ -87,7 +87,7 @@ def test_job_detail(self): self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec83"]), + reverse("v1:jobs-detail", args=["8317718f-5c0d-4fb6-9947-72e480b8a348"]), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -95,15 +95,25 @@ def test_job_detail(self): def test_job_detail_without_result_file(self): """Tests job detail authorized.""" - - self._authorize() - - jobs_response = self.client.get( - reverse("v1:jobs-detail", args=["57fc2e4d-267f-40c6-91a3-38153272e764"]), - format="json", + media_root = os.path.join( + os.path.dirname(os.path.abspath(__file__)), + "..", + "resources", + "fake_media", ) - self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) - self.assertEqual(jobs_response.data.get("result"), '{"somekey":1}') + media_root = os.path.normpath(os.path.join(os.getcwd(), media_root)) + + with self.settings(MEDIA_ROOT=media_root): + self._authorize() + + jobs_response = self.client.get( + reverse("v1:jobs-detail", args=["57fc2e4d-267f-40c6-91a3-38153272e764"]), + format="json", + ) + print("AQUI") + print(jobs_response.data) + self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) + self.assertEqual(jobs_response.data.get("result"), '{"somekey":1}') def test_job_provider_detail(self): """Tests job detail authorized.""" @@ -141,7 +151,7 @@ def test_job_save_result(self): with self.settings(MEDIA_ROOT=media_root): self._authorize() - job_id = "1a7947f9-6ae8-4e3d-ac1e-e7d608deec82" + job_id = "57fc2e4d-267f-40c6-91a3-38153272e764" jobs_response = self.client.post( reverse("v1:jobs-result", args=[job_id]), format="json", @@ -160,7 +170,7 @@ def test_not_authorized_job_save_result(self): self._authorize() job_id = "1a7947f9-6ae8-4e3d-ac1e-e7d608deec84" jobs_response = self.client.post( - reverse("v1:jobs-result", args=["57fc2e4d-267f-40c6-91a3-38153272e764"]), + reverse("v1:jobs-result", args=[job_id]), format="json", data={"result": {"ultimate": 42}}, ) @@ -168,7 +178,7 @@ def test_not_authorized_job_save_result(self): self.assertEqual(jobs_response.status_code, status.HTTP_404_NOT_FOUND) self.assertEqual( jobs_response.data.get("message"), - f"Job [{job_id}] nor found for user [test_user]", + f"Job [{job_id}] nor found", ) def test_stop_job(self): diff --git a/gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec83.json b/gateway/tests/resources/fake_media/test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json similarity index 100% rename from gateway/tests/resources/fake_media/test_user/results/1a7947f9-6ae8-4e3d-ac1e-e7d608deec83.json rename to gateway/tests/resources/fake_media/test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json From df4df179d1bad137ad242ae9294ba1cffefff243 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Tue, 28 Jan 2025 18:20:41 +0100 Subject: [PATCH 15/18] remove prints --- gateway/api/services/result_storage.py | 3 +-- gateway/tests/api/test_job.py | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/gateway/api/services/result_storage.py b/gateway/api/services/result_storage.py index 68abcf9e4..846fcb899 100644 --- a/gateway/api/services/result_storage.py +++ b/gateway/api/services/result_storage.py @@ -27,8 +27,7 @@ def __init__(self, username: str): def __get_result_path(self, job_id: str) -> str: """Construct the full path for a result file.""" return os.path.join( - self.user_results_directory, f"{ - job_id}{self.RESULT_FILE_EXTENSION}" + self.user_results_directory, f"{job_id}{self.RESULT_FILE_EXTENSION}" ) def get(self, job_id: str) -> Optional[str]: diff --git a/gateway/tests/api/test_job.py b/gateway/tests/api/test_job.py index 871e1a5ea..cba8925f3 100644 --- a/gateway/tests/api/test_job.py +++ b/gateway/tests/api/test_job.py @@ -110,8 +110,6 @@ def test_job_detail_without_result_file(self): reverse("v1:jobs-detail", args=["57fc2e4d-267f-40c6-91a3-38153272e764"]), format="json", ) - print("AQUI") - print(jobs_response.data) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) self.assertEqual(jobs_response.data.get("result"), '{"somekey":1}') From 1cd0302ee12a8d8a5436f2f51a8ec74a82efb04d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Wed, 29 Jan 2025 11:24:35 +0100 Subject: [PATCH 16/18] fix lint --- gateway/api/access_policies/jobs.py | 12 ++++++---- gateway/api/repositories/jobs.py | 1 - gateway/api/services/result_storage.py | 4 +--- gateway/api/v1/serializers.py | 3 +-- gateway/api/views/jobs.py | 31 +++++++++++++++++++------- gateway/tests/api/test_job.py | 8 +++++-- 6 files changed, 39 insertions(+), 20 deletions(-) diff --git a/gateway/api/access_policies/jobs.py b/gateway/api/access_policies/jobs.py index 26981aa1b..499ff1d26 100644 --- a/gateway/api/access_policies/jobs.py +++ b/gateway/api/access_policies/jobs.py @@ -2,9 +2,11 @@ Access policies implementation for Job access """ import logging -from django.contrib.auth.models import User +from django.contrib.auth import get_user_model from api.models import Job +User = get_user_model() + logger = logging.getLogger("gateway") @@ -14,6 +16,7 @@ class JobAccessPolocies: # pylint: disable=too-few-public-methods The main objective of this class is to manage the access for the user to the Job entities. """ + @staticmethod def can_access(user: User, job: Job) -> bool: """ @@ -31,8 +34,7 @@ def can_access(user: User, job: Job) -> bool: if is_provider_job: provider_groups = job.program.provider.admin_groups.all() author_groups = user.groups.all() - has_access = any( - group in provider_groups for group in author_groups) + has_access = any(group in provider_groups for group in author_groups) else: has_access = user.id == job.author.id @@ -58,6 +60,8 @@ def can_save_result(user: User, job: Job) -> bool: has_access = user.id == job.author.id if not has_access: logger.warning( - "User [%s] has no access to save the result of the job [%s].", user.username, job.author + "User [%s] has no access to save the result of the job [%s].", + user.username, + job.author, ) return has_access diff --git a/gateway/api/repositories/jobs.py b/gateway/api/repositories/jobs.py index 93b9e9b4b..39ba5ada9 100644 --- a/gateway/api/repositories/jobs.py +++ b/gateway/api/repositories/jobs.py @@ -2,7 +2,6 @@ Repository implementation for Job model """ import logging -from django.db.models import Q from api.models import Job logger = logging.getLogger("gateway") diff --git a/gateway/api/services/result_storage.py b/gateway/api/services/result_storage.py index 846fcb899..ce740d881 100644 --- a/gateway/api/services/result_storage.py +++ b/gateway/api/services/result_storage.py @@ -3,9 +3,7 @@ """ import os import logging -import mimetypes -from typing import Optional, Tuple -from wsgiref.util import FileWrapper +from typing import Optional from django.conf import settings logger = logging.getLogger("gateway") diff --git a/gateway/api/v1/serializers.py b/gateway/api/v1/serializers.py index 11832637e..d9d64fc4f 100644 --- a/gateway/api/v1/serializers.py +++ b/gateway/api/v1/serializers.py @@ -67,8 +67,7 @@ def validate(self, attrs): # pylint: disable=too-many-branches title = attrs.get("title") provider = attrs.get("provider", None) if provider and "/" in title: - raise ValidationError( - "Provider defined in title and in provider fields.") + raise ValidationError("Provider defined in title and in provider fields.") title_split = title.split("/") if len(title_split) > 2: diff --git a/gateway/api/views/jobs.py b/gateway/api/views/jobs.py index ab753247f..a9bf743bc 100644 --- a/gateway/api/views/jobs.py +++ b/gateway/api/views/jobs.py @@ -38,14 +38,12 @@ endpoint=os.environ.get( "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", "http://otel-collector:4317" ), - insecure=bool( - int(os.environ.get("OTEL_EXPORTER_OTLP_TRACES_INSECURE", "0"))), + insecure=bool(int(os.environ.get("OTEL_EXPORTER_OTLP_TRACES_INSECURE", "0"))), ) ) provider.add_span_processor(otel_exporter) if bool(int(os.environ.get("OTEL_ENABLED", "0"))): - trace._set_tracer_provider( - provider, log=False) # pylint: disable=protected-access + trace._set_tracer_provider(provider, log=False) # pylint: disable=protected-access class JobViewSet(viewsets.GenericViewSet): @@ -58,17 +56,36 @@ class JobViewSet(viewsets.GenericViewSet): jobs_repository = JobsRepository() def get_serializer_class(self): + """ + Returns the default serializer class for the view. + """ return self.serializer_class @staticmethod def get_serializer_job(*args, **kwargs): + """ + Returns a `JobSerializer` instance + """ return v1_serializers.JobSerializer(*args, **kwargs) @staticmethod def get_serializer_job_without_result(*args, **kwargs): + """ + Returns a `JobSerializerWithoutResult` instance + """ return v1_serializers.JobSerializerWithoutResult(*args, **kwargs) def get_queryset(self): + """ + Returns a filtered queryset of `Job` objects based on the `filter` query parameter. + + - If `filter=catalog`, returns jobs authored by the user with an existing provider. + - If `filter=serverless`, returns jobs authored by the user without a provider. + - Otherwise, returns all jobs authored by the user. + + Returns: + QuerySet: A filtered queryset of `Job` objects ordered by creation date (descending). + """ type_filter = self.request.query_params.get("filter") if type_filter: if type_filter == TypeFilter.CATALOG: @@ -182,8 +199,7 @@ def logs(self, request, pk=None): # pylint: disable=invalid-name,unused-argumen if job.program and job.program.provider: provider_groups = job.program.provider.admin_groups.all() author_groups = author.groups.all() - has_access = any( - group in provider_groups for group in author_groups) + has_access = any(group in provider_groups for group in author_groups) if has_access: return Response({"logs": logs}) return Response({"logs": "No available logs"}) @@ -216,8 +232,7 @@ def stop(self, request, pk=None): # pylint: disable=invalid-name,unused-argumen ] ) for runtime_job_entry in runtime_jobs: - jobinstance = service.job( - runtime_job_entry.runtime_job) + jobinstance = service.job(runtime_job_entry.runtime_job) if jobinstance: try: logger.info( diff --git a/gateway/tests/api/test_job.py b/gateway/tests/api/test_job.py index cba8925f3..8a004eeca 100644 --- a/gateway/tests/api/test_job.py +++ b/gateway/tests/api/test_job.py @@ -87,7 +87,9 @@ def test_job_detail(self): self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-detail", args=["8317718f-5c0d-4fb6-9947-72e480b8a348"]), + reverse( + "v1:jobs-detail", args=["8317718f-5c0d-4fb6-9947-72e480b8a348"] + ), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) @@ -107,7 +109,9 @@ def test_job_detail_without_result_file(self): self._authorize() jobs_response = self.client.get( - reverse("v1:jobs-detail", args=["57fc2e4d-267f-40c6-91a3-38153272e764"]), + reverse( + "v1:jobs-detail", args=["57fc2e4d-267f-40c6-91a3-38153272e764"] + ), format="json", ) self.assertEqual(jobs_response.status_code, status.HTTP_200_OK) From d5d7019f4df6a9f84b761823ca0322555db91d26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Wed, 29 Jan 2025 16:04:25 +0100 Subject: [PATCH 17/18] remove unnecesary file --- .../test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json | 1 - 1 file changed, 1 deletion(-) delete mode 100644 gateway/tests/resources/fake_media/test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json diff --git a/gateway/tests/resources/fake_media/test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json b/gateway/tests/resources/fake_media/test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json deleted file mode 100644 index a132681f7..000000000 --- a/gateway/tests/resources/fake_media/test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json +++ /dev/null @@ -1 +0,0 @@ -{"ultimate": 42} \ No newline at end of file From da5c05b863ff4548efcbd8d645ef6cd7489236e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Arag=C3=B3n?= Date: Wed, 29 Jan 2025 17:53:55 +0100 Subject: [PATCH 18/18] revert file deletion --- .../test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json | 1 + 1 file changed, 1 insertion(+) create mode 100644 gateway/tests/resources/fake_media/test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json diff --git a/gateway/tests/resources/fake_media/test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json b/gateway/tests/resources/fake_media/test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json new file mode 100644 index 000000000..a132681f7 --- /dev/null +++ b/gateway/tests/resources/fake_media/test_user/results/8317718f-5c0d-4fb6-9947-72e480b8a348.json @@ -0,0 +1 @@ +{"ultimate": 42} \ No newline at end of file