settings: catch IntegrityError and render error in output (Bug 2037991)

src/lando/api/tests/conftest.py

@@ -23,7 +23,6 @@
 from lando.api.legacy.workers.uplift_worker import (
     UpliftWorker,
 )
-from lando.api.tests.mocks import PhabricatorDouble
 from lando.main.models import JobStatus, Repo, Revision
 from lando.main.models.uplift import (
     RevisionUpliftJob,
@@ -135,22 +134,6 @@ def request_mocker():
         yield m
 
 
-@pytest.fixture
-def phabdouble(monkeypatch):
-    """Mock the Phabricator service and build fake response objects."""
-    phabdouble = PhabricatorDouble(monkeypatch)
-
-    # Create required projects.
-    phabdouble.project(SEC_PROJ_SLUG)
-    phabdouble.project(CHECKIN_PROJ_SLUG)
-    phabdouble.project(SEC_APPROVAL_PROJECT_SLUG)
-    phabdouble.project(
-        RELMAN_PROJECT_SLUG,
-        attachments={"members": {"members": [{"phid": "PHID-USER-1"}]}},
-    )
-    yield phabdouble
-
-
 @pytest.fixture
 def mock_uplift_email_tasks(monkeypatch):
     success_task = mock.MagicMock()

src/lando/conftest.py

@@ -17,13 +17,19 @@
 from django.contrib.contenttypes.models import ContentType
 from requests.models import HTTPError
 
+from lando.api.legacy.projects import (
+    CHECKIN_PROJ_SLUG,
+    RELMAN_PROJECT_SLUG,
+    SEC_APPROVAL_PROJECT_SLUG,
+    SEC_PROJ_SLUG,
+)
 from lando.api.legacy.stacks import (
     RevisionStack,
     build_stack_graph,
     request_extended_revision_data,
 )
 from lando.api.legacy.transplants import build_stack_assessment_state
-from lando.api.tests.mocks import TreeStatusDouble
+from lando.api.tests.mocks import PhabricatorDouble, TreeStatusDouble
 from lando.headless_api.models.automation_job import AutomationJob
 from lando.headless_api.models.tokens import ApiToken
 from lando.main.models import (
@@ -1186,6 +1192,22 @@ def treestatusdouble(monkeypatch, treestatus_url):
     yield TreeStatusDouble(monkeypatch, treestatus_url)
 
 
+@pytest.fixture
+def phabdouble(monkeypatch):
+    """Mock the Phabricator service and build fake response objects."""
+    phabdouble = PhabricatorDouble(monkeypatch)
+
+    # Create required projects.
+    phabdouble.project(SEC_PROJ_SLUG)
+    phabdouble.project(CHECKIN_PROJ_SLUG)
+    phabdouble.project(SEC_APPROVAL_PROJECT_SLUG)
+    phabdouble.project(
+        RELMAN_PROJECT_SLUG,
+        attachments={"members": {"members": [{"phid": "PHID-USER-1"}]}},
+    )
+    yield phabdouble
+
+
 #
 # PushLog fixtures
 #

src/lando/ui/legacy/user_settings.py

@@ -1,15 +1,35 @@
 import logging
 
 from django.core.handlers.wsgi import WSGIRequest
+from django.db import IntegrityError
 from django.http import HttpResponseNotAllowed, JsonResponse
 
 from lando.main.auth import require_authenticated_user
+from lando.main.models.profile import Profile
 from lando.ui.legacy.forms import UserSettingsForm
 from lando.utils.phabricator import get_phabricator_client
 
 logger = logging.getLogger(__name__)
 
 
+def phid_conflict_response(phid: str) -> JsonResponse:
+    """Return a 400 response indicating the PHID is owned by another account."""
+    logger.info(
+        "Phabricator PHID `%s` is already linked to another Lando account.",
+        phid,
+    )
+    return JsonResponse(
+        {
+            "errors": {
+                "phabricator_api_key": [
+                    "This Phabricator account is already linked to another Lando user."
+                ]
+            }
+        },
+        status=400,
+    )
+
+
 @require_authenticated_user
 def manage_api_key(request: WSGIRequest) -> JsonResponse:
     """Sets `phabricator-api-token` cookie from the UserSettingsForm.
@@ -43,6 +63,16 @@ def manage_api_key(request: WSGIRequest) -> JsonResponse:
         phid = whoami["phid"]
         logger.debug("Phabricator API key verified for PHID `%s`.", phid)
 
-        profile.save_phabricator_api_key(api_key, phid=phid)
+        if (
+            Profile.objects.filter(phabricator_phid=phid)
+            .exclude(pk=profile.pk)
+            .exists()
+        ):
+            return phid_conflict_response(phid)
+
+        try:
+            profile.save_phabricator_api_key(api_key, phid=phid)
+        except IntegrityError:
+            return phid_conflict_response(phid)
 
     return JsonResponse({"success": True}, status=200)

src/lando/ui/tests/test_user_settings.py

@@ -0,0 +1,71 @@
+import pytest
+from django.contrib.auth.models import User
+from django.test import Client
+
+from lando.main.models.profile import Profile
+
+VALID_API_KEY = "api-aaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+
+
+def post_api_key(client: Client, api_key: str = VALID_API_KEY):
+    """Submit an API key to the `manage_api_key` view."""
+    return client.post(
+        "/manage_api_key/",
+        data={"phabricator_api_key": api_key, "reset_key": False},
+    )
+
+
+@pytest.mark.django_db(transaction=True)
+def test_manage_api_key_happy_path(
+    client: Client,
+    user,
+    phabdouble,
+):
+    """A successful API key update stores the new key and PHID on the profile."""
+    phab_user = phabdouble.user(username="phab_user", api_key=VALID_API_KEY)
+
+    client.force_login(user)
+    response = post_api_key(client)
+
+    assert response.status_code == 200, (
+        "A valid API key with a unique PHID should be accepted."
+    )
+    assert response.json() == {"success": True}, "Response body should report success."
+
+    user.profile.refresh_from_db()
+    assert user.profile.phabricator_phid == phab_user["phid"], (
+        "Profile should store the PHID reported by `user.whoami`."
+    )
+    assert user.profile.phabricator_api_key == VALID_API_KEY, (
+        "Profile should store the submitted API key."
+    )
+
+
+@pytest.mark.django_db(transaction=True)
+def test_manage_api_key_phid_conflict_returns_error(
+    client: Client,
+    user,
+    phabdouble,
+):
+    """Linking an API key whose PHID already belongs to another user returns a 400."""
+    phab_user = phabdouble.user(username="phab_user", api_key=VALID_API_KEY)
+
+    # Another Lando user already owns this Phabricator PHID.
+    other_user = User.objects.create_user(
+        username="other_user", email="other@example.org"
+    )
+    Profile.objects.create(user=other_user, phabricator_phid=phab_user["phid"])
+
+    client.force_login(user)
+    response = post_api_key(client)
+
+    assert response.status_code == 400, (
+        "Linking an API key whose PHID is owned by another user should return 400."
+    )
+    assert response.json() == {
+        "errors": {
+            "phabricator_api_key": [
+                "This Phabricator account is already linked to another Lando user."
+            ]
+        }
+    }, "Response body should describe the PHID conflict."