landing_jobs: return JSON responses for exceptions (bug 1929466)

src/lando/api/legacy/api/landing_jobs.py

@@ -1,5 +1,8 @@
+import json
 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
@@ -8,16 +11,26 @@
 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: int) -> JsonResponse:
     """Update a landing job.
 
     Checks whether the logged in user is allowed to modify the landing job that is
     passed, does some basic validation on the data passed, and updates the landing job
     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:
@@ -27,31 +40,46 @@ 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:
-        raise PermissionError(
+        raise PermissionDenied(
             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)

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
 
@@ -481,6 +482,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
@@ -576,10 +580,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."""
@@ -602,10 +604,59 @@ 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)
 
     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="[email protected]",
+    )
+
+    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

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,75 +21,76 @@ def _landing_job(status, requester_email="[email protected]"):
     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
 
 
-@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, "[email protected]")
-    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(
@@ -100,20 +103,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