From 66f7b317d5f7ee99e843cd12f2674a7c240b4d2a Mon Sep 17 00:00:00 2001 From: mabw-rte <41002227+mabw-rte@users.noreply.github.com> Date: Thu, 29 Aug 2024 16:18:39 +0200 Subject: [PATCH] fix(launcher-api): remove orphan JobResults visibility permissions (#2128) Context: In the job listing of `AntaREST`, we call two endpoints: - `GET`, `v1/launcher/jobs` - `GET`, `v1/launcher/jobs/{job_id}/progress` It turns out that when user does not have a permission for some job, this job, though listed (using the first endpoint of `jobs`), spits out an error `403` when it is called for the second endpoint `progress`. Issue: For the endpoint `jobs`, the logic beyond the permissions includes a second option where when an orphan job is created recently enough would become visible to everyone. This logic was not included in the `progress` endpoint which leads to the `Exception` above. Solution: After consulting the application users, they think that this `orphan` visibility logic is not that much relevant and should be removed (at least temporarily). --------- Co-authored-by: Sylvain Leclerc --- antarest/launcher/service.py | 8 +--- .../launcher_blueprint/test_launcher_local.py | 41 +++++++++++++++++++ tests/launcher/test_service.py | 12 ++---- 3 files changed, 46 insertions(+), 15 deletions(-) diff --git a/antarest/launcher/service.py b/antarest/launcher/service.py index d3a439e17c..8c8e88905d 100644 --- a/antarest/launcher/service.py +++ b/antarest/launcher/service.py @@ -2,7 +2,7 @@ import logging import os import shutil -from datetime import datetime, timedelta +from datetime import datetime from http import HTTPStatus from pathlib import Path from typing import Dict, List, Optional, cast @@ -59,7 +59,6 @@ def __init__(self, engine: str): ) -ORPHAN_JOBS_VISIBILITY_THRESHOLD = 10 # days LAUNCHER_PARAM_NAME_SUFFIX = "output_suffix" EXECUTION_INFO_FILE = "execution_info.ini" @@ -305,7 +304,6 @@ def _filter_from_user_permission(self, job_results: List[JobResult], user: Optio if not user: return [] - orphan_visibility_threshold = datetime.utcnow() - timedelta(days=ORPHAN_JOBS_VISIBILITY_THRESHOLD) allowed_job_results = [] study_ids = [job_result.study_id for job_result in job_results] @@ -330,9 +328,7 @@ def _filter_from_user_permission(self, job_results: List[JobResult], user: Optio raising=False, ): allowed_job_results.append(job_result) - elif ( - user and (user.is_site_admin() or user.is_admin_token()) - ) or job_result.creation_date >= orphan_visibility_threshold: + elif user and (user.is_site_admin() or user.is_admin_token()): allowed_job_results.append(job_result) return allowed_job_results diff --git a/tests/integration/launcher_blueprint/test_launcher_local.py b/tests/integration/launcher_blueprint/test_launcher_local.py index 08d1175889..aeff69e315 100644 --- a/tests/integration/launcher_blueprint/test_launcher_local.py +++ b/tests/integration/launcher_blueprint/test_launcher_local.py @@ -122,3 +122,44 @@ def test_get_launcher_time_limit( "description": "Unknown solver configuration: 'unknown'", "exception": "UnknownSolverConfig", } + + def test_jobs_permissions( + self, + client: TestClient, + user_access_token: str, + admin_access_token: str, + ) -> None: + # create an admin study with no permissions + res = client.post( + "/v1/studies", + headers={"Authorization": f"Bearer {admin_access_token}"}, + params={"name": "study_admin"}, + ) + res.raise_for_status() + # get the study_id + study_id = res.json() + + # launch a job with the admin user + res = client.post( + f"/v1/launcher/run/{study_id}", + headers={"Authorization": f"Bearer {admin_access_token}"}, + json={"launcher": "local"}, + ) + res.raise_for_status() + job_id = res.json()["job_id"] + + # check that the user cannot see the job + res = client.get( + "/v1/launcher/jobs", + headers={"Authorization": f"Bearer {user_access_token}"}, + ) + res.raise_for_status() + assert job_id not in [job.get("id") for job in res.json()] + + # check that the admin can see the job + res = client.get( + "/v1/launcher/jobs", + headers={"Authorization": f"Bearer {admin_access_token}"}, + ) + res.raise_for_status() + assert job_id in [job.get("id") for job in res.json()] diff --git a/tests/launcher/test_service.py b/tests/launcher/test_service.py index 9095673070..6998df5667 100644 --- a/tests/launcher/test_service.py +++ b/tests/launcher/test_service.py @@ -40,13 +40,7 @@ LauncherParametersDTO, LogType, ) -from antarest.launcher.service import ( - EXECUTION_INFO_FILE, - LAUNCHER_PARAM_NAME_SUFFIX, - ORPHAN_JOBS_VISIBILITY_THRESHOLD, - JobNotFound, - LauncherService, -) +from antarest.launcher.service import EXECUTION_INFO_FILE, LAUNCHER_PARAM_NAME_SUFFIX, JobNotFound, LauncherService from antarest.login.auth import Auth from antarest.login.model import Identity from antarest.study.model import OwnerInfo, PublicMode, Study, StudyMetadataDTO @@ -257,7 +251,7 @@ def test_service_get_jobs_from_database(self, db_session) -> None: job_status=JobStatus.SUCCESS, msg="Hello, World!", exit_code=0, - creation_date=now - timedelta(days=ORPHAN_JOBS_VISIBILITY_THRESHOLD + 1), + creation_date=now - timedelta(days=11), owner=identity_instance, ) ] @@ -308,7 +302,7 @@ def test_service_get_jobs_from_database(self, db_session) -> None: ) ), ) - == returned_faked_execution_results + == [] ) with pytest.raises(UserHasNotPermissionError):