From f6bbbdfb78ae70dd0b0e66f5a75a7277d41d2907 Mon Sep 17 00:00:00 2001 From: Zeid Zabaneh Date: Tue, 29 Oct 2024 14:34:59 +1100 Subject: [PATCH 1/7] landing_jobs: fix job cancelling (bug 1915712) --- src/lando/api/legacy/api/landing_jobs.py | 49 +++++++++++---- .../legacy/js/components/Timeline.js | 62 ++++++++++--------- .../ui/jinja2/stack/partials/timeline.html | 2 +- src/lando/urls.py | 6 ++ 4 files changed, 77 insertions(+), 42 deletions(-) diff --git a/src/lando/api/legacy/api/landing_jobs.py b/src/lando/api/legacy/api/landing_jobs.py index 065eb118..dbcfae74 100644 --- a/src/lando/api/legacy/api/landing_jobs.py +++ b/src/lando/api/legacy/api/landing_jobs.py @@ -1,5 +1,7 @@ +import json import logging +from django import forms from django.http import Http404, HttpRequest, JsonResponse from lando.main.auth import require_authenticated_user @@ -8,8 +10,18 @@ logger = logging.getLogger(__name__) +class LandingJobForm(forms.Form): + """Simple form to clean API endpoint fields.""" + + # NOTE: this is here as a quick solution to safely check and clean user input, + # however it will likely be deprecated in favour of a more universal solution + # as part of bug 1870097. + landing_job_id = forms.IntegerField() + status = forms.CharField() + + @require_authenticated_user -def put(request: HttpRequest, landing_job_id: str, data: dict): +def put(request: HttpRequest, landing_job_id: str): """Update a landing job. Checks whether the logged in user is allowed to modify the landing job that is @@ -27,11 +39,27 @@ def put(request: HttpRequest, landing_job_id: str, data: dict): updated (for example, when trying to cancel a job that is already in progress). """ - with LandingJob.lock_table: - landing_job = LandingJob.objects.get(pk=landing_job_id) + data = json.loads(request.body) + data["landing_job_id"] = landing_job_id + form = LandingJobForm(data) - if not landing_job: - raise Http404(f"A landing job with ID {landing_job_id} was not found.") + if not form.is_valid(): + data = { + "errors": [ + f"{field}: {', '.join(field_errors)}" + for field, field_errors in form.errors.items() + ] + } + return JsonResponse(data, status=400) + + landing_job_id = form.cleaned_data["landing_job_id"] + status = form.cleaned_data["status"] + + with LandingJob.lock_table: + try: + landing_job = LandingJob.objects.get(pk=landing_job_id) + except LandingJob.DoesNotExist: + raise Http404(f"A landing job with ID {landing_job_id} was not found.") ldap_username = request.user.email if landing_job.requester_email != ldap_username: @@ -39,19 +67,18 @@ def put(request: HttpRequest, landing_job_id: str, data: dict): f"User not authorized to update landing job {landing_job_id}" ) - # TODO: fix this. See bug 1893455. - if data["status"] != "CANCELLED": - data = {"errors": [f"The provided status {data['status']} is not allowed."]} - return JsonResponse(data, status_code=400) + if status != "CANCELLED": + data = {"errors": [f"The provided status {status} is not allowed."]} + return JsonResponse(data, status=400) if landing_job.status in (LandingJobStatus.SUBMITTED, LandingJobStatus.DEFERRED): landing_job.transition_status(LandingJobAction.CANCEL) landing_job.save() - return {"id": landing_job.id}, 200 + return JsonResponse({"id": landing_job.id}) else: data = { "errors": [ f"Landing job status ({landing_job.status}) does not allow cancelling." ] } - return JsonResponse(data, status_code=400) + return JsonResponse(data, status=400) diff --git a/src/lando/static_src/legacy/js/components/Timeline.js b/src/lando/static_src/legacy/js/components/Timeline.js index ed111e90..f5d504d7 100644 --- a/src/lando/static_src/legacy/js/components/Timeline.js +++ b/src/lando/static_src/legacy/js/components/Timeline.js @@ -6,45 +6,47 @@ $.fn.timeline = function() { // Format timestamps $timeline.find('time[data-timestamp]').formatTime(); - }); -}; -$('button.cancel-landing-job').on('click', function(e) { - var button = $(this); - var landing_job_id = this.dataset.landing_job_id; + $('button.cancel-landing-job').on('click', function(e) { + var button = $(this); + var landing_job_id = this.dataset.landing_job_id; - button.addClass("is-loading"); - fetch(`/landing_jobs/${landing_job_id}`, { + button.addClass("is-loading"); + fetch(`/landing_jobs/${landing_job_id}`, { method: 'PUT', body: JSON.stringify({"status": "CANCELLED"}), headers: { - 'Accept': 'application/json', - 'Content-Type': 'application/json' + 'Accept': 'application/json', + 'Content-Type': 'application/json', + 'X-CSRFToken': button.data("csrf-token") }, - }).then(response => { + }).then(response => { if (response.status == 200) { - window.location.reload(); + window.location.reload(); } else if (response.status == 400) { - button.prop("disabled", true); - button.removeClass("is-danger").removeClass("is-loading").addClass("is-warning"); - button.html("Could not cancel landing request"); + button.prop("disabled", true); + button.removeClass("is-danger").removeClass("is-loading").addClass("is-warning"); + button.html("Could not cancel landing request"); } else { - button.prop("disabled", true); - button.removeClass("is-danger").removeClass("is-loading").addClass("is-warning"); - button.html("An unknown error occurred"); + button.prop("disabled", true); + button.removeClass("is-danger").removeClass("is-loading").addClass("is-warning"); + button.html("An unknown error occurred"); } + }); + }); + + $("a.toggle-content,button.toggle-content").on("click", function() { + /* A link with the `toggle-snippet` class will hide its parent, and show + * any of the parent's siblings. For example: + *
+ *
Some content toggle
+ *
Other content toggle
+ *
+ */ + var link = $(this); + link.parent().hide(); + link.parent().siblings().show() }); -}); -$("a.toggle-content,button.toggle-content").on("click", function() { - /* A link with the `toggle-snippet` class will hide its parent, and show - * any of the parent's siblings. For example: - *
- *
Some content toggle
- *
Other content toggle
- *
- */ - var link = $(this); - link.parent().hide(); - link.parent().siblings().show() -}); + }); +}; diff --git a/src/lando/ui/jinja2/stack/partials/timeline.html b/src/lando/ui/jinja2/stack/partials/timeline.html index 0f0be63c..acae3ef9 100644 --- a/src/lando/ui/jinja2/stack/partials/timeline.html +++ b/src/lando/ui/jinja2/stack/partials/timeline.html @@ -50,7 +50,7 @@ {% endif %} {% if transplant['status'] in ("SUBMITTED", "DEFERRED") %} - + {% endif %} diff --git a/src/lando/urls.py b/src/lando/urls.py index fc2c82c7..fc166dd6 100644 --- a/src/lando/urls.py +++ b/src/lando/urls.py @@ -18,6 +18,7 @@ from django.contrib import admin from django.urls import include, path +from lando.api.legacy.api import landing_jobs from lando.dockerflow import views as DockerflowViews from lando.ui.legacy import pages, revisions, user_settings @@ -42,3 +43,8 @@ path("D/", revisions.Revision.as_view(), name="revisions-page"), path("manage_api_key/", user_settings.manage_api_key, name="user-settings"), ] + +# "API" endpoints ported from legacy API app. +urlpatterns += [ + path("landing_jobs//", landing_jobs.put, name="landing-jobs"), +] From 8c07efe0f6de90a0c59c36c6b8f2f851cbc64ab1 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Wed, 30 Oct 2024 16:33:10 +1100 Subject: [PATCH 2/7] conftest.fake_request: add support for body --- src/lando/api/tests/conftest.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/lando/api/tests/conftest.py b/src/lando/api/tests/conftest.py index e31147ca..194690ff 100644 --- a/src/lando/api/tests/conftest.py +++ b/src/lando/api/tests/conftest.py @@ -481,6 +481,9 @@ def __init__(self, is_authenticated=True, has_email=True, permissions=None): class FakeRequest: def __init__(self, *args, **kwargs): + self.body = "{}" + if "body" in kwargs: + self.body = kwargs.pop("body") self.user = FakeUser(*args, **kwargs) return FakeRequest From a237bf55712b4632c6cc21586e319398e8d5e323 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Tue, 29 Oct 2024 16:37:51 +1100 Subject: [PATCH 3/7] tests: update transplants tests for updated landing_jobs.put (bug 1915712) --- src/lando/api/legacy/api/landing_jobs.py | 4 ++-- src/lando/api/tests/conftest.py | 14 +++++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/lando/api/legacy/api/landing_jobs.py b/src/lando/api/legacy/api/landing_jobs.py index dbcfae74..456f1963 100644 --- a/src/lando/api/legacy/api/landing_jobs.py +++ b/src/lando/api/legacy/api/landing_jobs.py @@ -21,7 +21,7 @@ class LandingJobForm(forms.Form): @require_authenticated_user -def put(request: HttpRequest, landing_job_id: str): +def put(request: HttpRequest, landing_job_id: int) -> JsonResponse: """Update a landing job. Checks whether the logged in user is allowed to modify the landing job that is @@ -29,7 +29,7 @@ def put(request: HttpRequest, landing_job_id: str): instance accordingly. Args: - landing_job_id (str): The unique ID of the LandingJob object. + landing_job_id (int): The unique ID of the LandingJob object. data (dict): A dictionary containing the cleaned data payload from the request. Raises: diff --git a/src/lando/api/tests/conftest.py b/src/lando/api/tests/conftest.py index 194690ff..a816aa09 100644 --- a/src/lando/api/tests/conftest.py +++ b/src/lando/api/tests/conftest.py @@ -579,10 +579,8 @@ def _handle__post__transplants(self, path, **kwargs): def _handle__put__landing_jobs__id(self, path, **kwargs): job_id = int(path.removeprefix("/landing_jobs/")) - json_response = legacy_api_landing_jobs.put( - self.request, job_id, kwargs["json"] - ) - return MockResponse(json=json.loads(json.dumps(json_response))) + response = legacy_api_landing_jobs.put(self.request, job_id) + return MockResponse(json=json.loads(response.content)) def get(self, path, *args, **kwargs): """Handle various get endpoints.""" @@ -605,8 +603,14 @@ def post(self, path, **kwargs): def put(self, path, **kwargs): """Handle put endpoint.""" + request_dict = {} if "permissions" in kwargs: - self.request = fake_request(permissions=kwargs["permissions"]) + request_dict["permissions"] = kwargs["permissions"] + + if "json" in kwargs: + request_dict["body"] = json.dumps(kwargs["json"]) + + self.request = fake_request(**request_dict) if path.startswith("/landing_jobs/"): return self._handle__put__landing_jobs__id(path, **kwargs) From c658b421a576c9306cfed7727745d2649b3c9e63 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Wed, 6 Nov 2024 06:42:51 +1100 Subject: [PATCH 4/7] test/api: add authenticated_user fixture (bug 1915712) --- src/lando/api/tests/conftest.py | 46 ++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/lando/api/tests/conftest.py b/src/lando/api/tests/conftest.py index a816aa09..d2c71637 100644 --- a/src/lando/api/tests/conftest.py +++ b/src/lando/api/tests/conftest.py @@ -10,6 +10,7 @@ import requests import requests_mock from django.conf import settings +from django.contrib.auth.models import User from django.core.cache import cache from django.http import HttpResponse from django.http import JsonResponse as JSONResponse @@ -26,7 +27,7 @@ ) from lando.api.legacy.transplants import CODE_FREEZE_OFFSET, tokens_are_equal from lando.api.tests.mocks import PhabricatorDouble, TreeStatusDouble -from lando.main.models import SCM_LEVEL_1, SCM_LEVEL_3, Repo +from lando.main.models import SCM_LEVEL_1, SCM_LEVEL_3, Profile, Repo from lando.main.support import LegacyAPIException from lando.utils.phabricator import PhabricatorClient @@ -616,3 +617,46 @@ def put(self, path, **kwargs): return self._handle__put__landing_jobs__id(path, **kwargs) return ProxyClient() + + +@pytest.fixture +def conduit_permissions(): + permissions = ( + "scm_level_1", + "scm_level_2", + "scm_level_3", + "scm_conduit", + ) + all_perms = Profile.get_all_scm_permissions() + + return [all_perms[p] for p in permissions] + + +@pytest.fixture +def user_plaintext_password(): + return "test_password" + + +@pytest.fixture +def user(user_plaintext_password, conduit_permissions): + user = User.objects.create_user( + username="test_user", + password=user_plaintext_password, + email="testuser@example.org", + ) + + user.profile = Profile(user=user, userinfo={"name": "test user"}) + + for permission in conduit_permissions: + user.user_permissions.add(permission) + + user.save() + user.profile.save() + + return user + + +@pytest.fixture +def authenticated_client(user, user_plaintext_password, client): + client.login(username=user.username, password=user_plaintext_password) + return client From c56c9faf90b1e4bb80dbd89b2365d20313f0e37b Mon Sep 17 00:00:00 2001 From: Zeid Zabaneh Date: Tue, 5 Nov 2024 14:40:43 +1100 Subject: [PATCH 5/7] test_landing_jobs: unskip job cancelling tests (bug 1915712) --- src/lando/api/tests/test_landing_job.py | 66 +++++++++++++------------ 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/src/lando/api/tests/test_landing_job.py b/src/lando/api/tests/test_landing_job.py index 0b13c28c..ecbf73b5 100644 --- a/src/lando/api/tests/test_landing_job.py +++ b/src/lando/api/tests/test_landing_job.py @@ -1,3 +1,5 @@ +import json + import pytest from lando.main.models import LandingJob, LandingJobStatus, Repo @@ -19,56 +21,56 @@ def _landing_job(status, requester_email="tuser@example.com"): return _landing_job -@pytest.mark.skip def test_cancel_landing_job_cancels_when_submitted( - db, client, landing_job, mock_permissions + db, authenticated_client, user, landing_job, mock_permissions ): """Test happy path; cancelling a job that has not started yet.""" - job = landing_job(LandingJobStatus.SUBMITTED) - response = client.put( - f"/landing_jobs/{job.id}", - json={"status": LandingJobStatus.CANCELLED.value}, - permissions=mock_permissions, + job = landing_job(LandingJobStatus.SUBMITTED, requester_email=user.email) + response = authenticated_client.put( + f"/landing_jobs/{job.id}/", + json.dumps({"status": LandingJobStatus.CANCELLED.value}), ) assert response.status_code == 200 - assert response.json["id"] == job.id + assert response.json()["id"] == job.id + job.refresh_from_db() assert job.status == LandingJobStatus.CANCELLED -@pytest.mark.skip def test_cancel_landing_job_cancels_when_deferred( - db, client, landing_job, mock_permissions + db, authenticated_client, user, landing_job, mock_permissions ): """Test happy path; cancelling a job that has been deferred.""" - job = landing_job(LandingJobStatus.DEFERRED) - response = client.put( - f"/landing_jobs/{job.id}", - json={"status": LandingJobStatus.CANCELLED.value}, + job = landing_job(LandingJobStatus.DEFERRED, requester_email=user.email) + response = authenticated_client.put( + f"/landing_jobs/{job.id}/", + json.dumps({"status": LandingJobStatus.CANCELLED.value}), permissions=mock_permissions, ) assert response.status_code == 200 - assert response.json["id"] == job.id + assert response.json()["id"] == job.id + job.refresh_from_db() assert job.status == LandingJobStatus.CANCELLED -@pytest.mark.skip def test_cancel_landing_job_fails_in_progress( - db, client, landing_job, mock_permissions + db, authenticated_client, user, landing_job, mock_permissions ): """Test trying to cancel a job that is in progress fails.""" - job = landing_job(LandingJobStatus.IN_PROGRESS) - response = client.put( - f"/landing_jobs/{job.id}", - json={"status": LandingJobStatus.CANCELLED.value}, + job = landing_job(LandingJobStatus.IN_PROGRESS, requester_email=user.email) + response = authenticated_client.put( + f"/landing_jobs/{job.id}/", + json.dumps({"status": LandingJobStatus.CANCELLED.value}), permissions=mock_permissions, ) assert response.status_code == 400 - assert response.json["detail"] == ( - "Landing job status (LandingJobStatus.IN_PROGRESS) does not allow cancelling." + assert ( + "Landing job status (IN_PROGRESS) does not allow cancelling." + in response.json()["errors"] ) + job.refresh_from_db() assert job.status == LandingJobStatus.IN_PROGRESS @@ -100,20 +102,22 @@ def test_cancel_landing_job_fails_not_found(db, client, landing_job, mock_permis assert response.json()["detail"] == ("A landing job with ID 1 was not found.") -@pytest.mark.skip -def test_cancel_landing_job_fails_bad_input(db, client, landing_job, mock_permissions): +def test_cancel_landing_job_fails_bad_input( + db, authenticated_client, user, landing_job, mock_permissions +): """Test trying to send an invalid status to the update endpoint.""" - job = landing_job(LandingJobStatus.SUBMITTED) - response = client.put( - f"/landing_jobs/{job.id}", - json={"status": LandingJobStatus.IN_PROGRESS.value}, + job = landing_job(LandingJobStatus.SUBMITTED, requester_email=user.email) + response = authenticated_client.put( + f"/landing_jobs/{job.id}/", + json.dumps({"status": LandingJobStatus.IN_PROGRESS.value}), permissions=mock_permissions, ) assert response.status_code == 400 - assert response.json["detail"] == ( - "'IN_PROGRESS' is not one of ['CANCELLED'] - 'status'" + assert ( + "The provided status IN_PROGRESS is not allowed." in response.json()["errors"] ) + job.refresh_from_db() assert job.status == LandingJobStatus.SUBMITTED From d41e43b16cf2976017214e8f4606eefb38cc29ae Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Wed, 6 Nov 2024 11:43:27 +1100 Subject: [PATCH 6/7] test_landing_jobs: unskip remaining tests (bug 1929466) --- src/lando/api/tests/test_landing_job.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/lando/api/tests/test_landing_job.py b/src/lando/api/tests/test_landing_job.py index ecbf73b5..f67db52a 100644 --- a/src/lando/api/tests/test_landing_job.py +++ b/src/lando/api/tests/test_landing_job.py @@ -74,22 +74,23 @@ def test_cancel_landing_job_fails_in_progress( assert job.status == LandingJobStatus.IN_PROGRESS -@pytest.mark.skip -def test_cancel_landing_job_fails_not_owner(db, client, landing_job, mock_permissions): +def test_cancel_landing_job_fails_not_owner( + db, authenticated_client, landing_job, mock_permissions +): """Test trying to cancel a job that is created by a different user.""" job = landing_job(LandingJobStatus.SUBMITTED, "anotheruser@example.org") - response = client.put( - f"/landing_jobs/{job.id}", + response = authenticated_client.put( + f"/landing_jobs/{job.id}/", json={"status": LandingJobStatus.CANCELLED.value}, permissions=mock_permissions, ) assert response.status_code == 403 - assert response.json["detail"] == ("User not authorized to update landing job 1") + assert "User not authorized to update landing job 1" in response.json()["errors"] + job.refresh_from_db() assert job.status == LandingJobStatus.SUBMITTED -@pytest.mark.skip def test_cancel_landing_job_fails_not_found(db, client, landing_job, mock_permissions): """Test trying to cancel a job that does not exist.""" response = client.put( From dada213827ddab7138f11c9c3ff8de1c0b64c7cc Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Wed, 6 Nov 2024 11:43:44 +1100 Subject: [PATCH 7/7] landing_jobs: return JSON responses for exceptions (bug 1929466) --- src/lando/api/legacy/api/landing_jobs.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lando/api/legacy/api/landing_jobs.py b/src/lando/api/legacy/api/landing_jobs.py index 456f1963..6c1081f2 100644 --- a/src/lando/api/legacy/api/landing_jobs.py +++ b/src/lando/api/legacy/api/landing_jobs.py @@ -2,6 +2,7 @@ import logging from django import forms +from django.core.exceptions import PermissionDenied from django.http import Http404, HttpRequest, JsonResponse from lando.main.auth import require_authenticated_user @@ -63,7 +64,7 @@ def put(request: HttpRequest, landing_job_id: int) -> JsonResponse: ldap_username = request.user.email if landing_job.requester_email != ldap_username: - raise PermissionError( + raise PermissionDenied( f"User not authorized to update landing job {landing_job_id}" )