feat: working POST API for Project Observations (#15228)

dev/environment

@@ -78,3 +78,8 @@ HCAPTCHA_SECRET_KEY=0x0000000000000000000000000000000000000000
 
 # Example of Captcha backend configuration
 # CAPTCHA_BACKEND=warehouse.captcha.hcaptcha.Service
+
+# Example of HelpScout configuration
+# HELPSCOUT_WAREHOUSE_APP_ID="an insecure helpscout app id"
+# HELPSCOUT_WAREHOUSE_APP_SECRET="an insecure helpscout app secret"
+# HELPSCOUT_WAREHOUSE_MAILBOX_ID=123456789

tests/unit/accounts/test_models.py

@@ -194,7 +194,12 @@ def test_acl(self, db_session):
                 True,
                 False,
                 False,
-                ["group:admins", "group:moderators", "group:psf_staff"],
+                [
+                    "group:admins",
+                    "group:moderators",
+                    "group:observers",
+                    "group:psf_staff",
+                ],
             ),
             (
                 False,
@@ -206,7 +211,12 @@ def test_acl(self, db_session):
                 True,
                 True,
                 False,
-                ["group:admins", "group:moderators", "group:psf_staff"],
+                [
+                    "group:admins",
+                    "group:moderators",
+                    "group:observers",
+                    "group:psf_staff",
+                ],
             ),
             (
                 False,

tests/unit/accounts/test_security_policy.py

@@ -215,7 +215,14 @@ def test_identity_missing_route(self, monkeypatch):
         assert add_vary_cb.calls == [pretend.call("Cookie")]
         assert request.add_response_callback.calls == [pretend.call(vary_cb)]
 
-    def test_identity_invalid_route(self, monkeypatch):
+    @pytest.mark.parametrize(
+        "route_name",
+        [
+            "forklift.legacy.file_upload",
+            "api.echo",
+        ],
+    )
+    def test_identity_invalid_route(self, route_name, monkeypatch):
         session_helper_obj = pretend.stub()
         session_helper_cls = pretend.call_recorder(lambda: session_helper_obj)
         monkeypatch.setattr(
@@ -230,7 +237,7 @@ def test_identity_invalid_route(self, monkeypatch):
 
         request = pretend.stub(
             add_response_callback=pretend.call_recorder(lambda cb: None),
-            matched_route=pretend.stub(name="forklift.legacy.file_upload"),
+            matched_route=pretend.stub(name=route_name),
             banned=pretend.stub(by_ip=lambda ip_address: False),
             remote_addr="1.2.3.4",
         )

tests/unit/api/test_echo.py

@@ -0,0 +1,111 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import pytest
+
+from pyramid.httpexceptions import HTTPAccepted, HTTPBadRequest
+
+from tests.common.db.packaging import ProjectFactory
+from warehouse.api.echo import api_echo, api_projects_observations
+
+
+class TestAPI:
+    def test_echo(self, pyramid_request, pyramid_user):
+        assert api_echo(pyramid_request) == {"username": pyramid_user.username}
+
+
+class TestAPIProjectObservations:
+    def test_missing_fields(self, pyramid_request):
+        project = ProjectFactory.create()
+        pyramid_request.json_body = {}
+
+        with pytest.raises(HTTPBadRequest) as exc:
+            api_projects_observations(project, pyramid_request)
+
+        assert exc.value.json == {
+            "error": "missing required fields",
+            "missing": ["kind", "summary"],
+        }
+
+    def test_invalid_kind(self, pyramid_request):
+        project = ProjectFactory.create()
+        pyramid_request.json_body = {"kind": "invalid", "summary": "test"}
+
+        with pytest.raises(HTTPBadRequest) as exc:
+            api_projects_observations(project, pyramid_request)
+
+        assert exc.value.json == {
+            "error": "invalid kind",
+            "kind": "invalid",
+            "project": project.name,
+        }
+
+    def test_malware_missing_inspector_url(self, pyramid_request):
+        project = ProjectFactory.create()
+        pyramid_request.json_body = {"kind": "is_malware", "summary": "test"}
+
+        with pytest.raises(HTTPBadRequest) as exc:
+            api_projects_observations(project, pyramid_request)
+
+        assert exc.value.json == {
+            "error": "missing required fields",
+            "missing": ["inspector_url"],
+            "project": project.name,
+        }
+
+    def test_malware_invalid_inspector_url(self, pyramid_request):
+        project = ProjectFactory.create()
+        pyramid_request.json_body = {
+            "kind": "is_malware",
+            "summary": "test",
+            "inspector_url": "invalid",
+        }
+
+        with pytest.raises(HTTPBadRequest) as exc:
+            api_projects_observations(project, pyramid_request)
+
+        assert exc.value.json == {
+            "error": "invalid inspector_url",
+            "inspector_url": "invalid",
+            "project": project.name,
+        }
+
+    def test_valid_malware_observation(self, db_request, pyramid_user):
+        project = ProjectFactory.create()
+        db_request.json_body = {
+            "kind": "is_malware",
+            "summary": "test",
+            "inspector_url": "https://inspector.pypi.io/...",
+        }
+
+        response = api_projects_observations(project, db_request)
+
+        assert isinstance(response, HTTPAccepted)
+        assert response.json_body == {
+            "project": project.name,
+            "thanks": "for the observation",
+        }
+
+    def test_valid_spam_observation(self, db_request, pyramid_user):
+        project = ProjectFactory.create()
+        db_request.json_body = {
+            "kind": "is_spam",
+            "summary": "test",
+        }
+
+        response = api_projects_observations(project, db_request)
+
+        assert isinstance(response, HTTPAccepted)
+        assert response.json_body == {
+            "project": project.name,
+            "thanks": "for the observation",
+        }

tests/unit/observations/test_tasks.py

@@ -0,0 +1,79 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import pretend
+import pytest
+import requests
+import responses
+
+from warehouse.observations.models import ObservationKind
+from warehouse.observations.tasks import (
+    execute_observation_report,
+    report_observation_to_helpscout,
+)
+
+from ...common.db.accounts import UserFactory
+from ...common.db.packaging import ProjectFactory
+
+
+def test_execute_observation_report(app_config):
+    _delay = pretend.call_recorder(lambda x: None)
+    app_config.task = lambda x: pretend.stub(delay=_delay)
+    observation = pretend.stub(id=pretend.stub())
+    session = pretend.stub(info={"warehouse.observations.new": {observation}})
+
+    execute_observation_report(app_config, session)
+
+    assert _delay.calls == [pretend.call(observation.id)]
+
+
+@responses.activate
+@pytest.mark.parametrize(
+    "kind", [ObservationKind.IsMalware, ObservationKind.SomethingElse]
+)
+@pytest.mark.parametrize("payload", [{}, {"foo": "bar"}])
+def test_report_observation_to_helpscout(kind, payload, db_request, monkeypatch):
+    db_request.registry.settings = {"helpscout.app_secret": "fake-sekret"}
+    # Mock out the authentication to HelpScout
+    monkeypatch.setattr(
+        "warehouse.observations.tasks._authenticate_helpscout",
+        lambda x: "SOME_TOKEN",
+    )
+
+    # Create an Observation
+    user = UserFactory.create()
+    db_request.user = user
+    project = ProjectFactory.create()
+    observation = project.record_observation(
+        request=db_request,
+        kind=kind,
+        summary="Project Observation",
+        payload=payload,
+        actor=user,
+    )
+    # Need to flush the session to ensure the Observation has an ID
+    db_request.db.flush()
+
+    db_request.http = requests.Session()
+
+    responses.add(
+        responses.POST,
+        "https://api.helpscout.net/v2/conversations",
+        json={"id": 123},
+    )
+
+    report_observation_to_helpscout(None, db_request, observation.id)
+
+    assert len(responses.calls) == 1
+    assert (
+        responses.calls[0].request.url == "https://api.helpscout.net/v2/conversations"
+    )

tests/unit/packaging/test_models.py

@@ -188,6 +188,11 @@ def test_acl(self, db_session):
                     Permissions.AdminRoleDelete,
                 ),
             ),
+            (
+                Allow,
+                "group:observers",
+                Permissions.APIObservationsAdd,
+            ),
         ] + sorted(
             [(Allow, f"oidc:{publisher.id}", ["upload"])], key=lambda x: x[1]
         ) + sorted(
@@ -472,6 +477,11 @@ def test_acl(self, db_session):
                     Permissions.AdminRoleDelete,
                 ),
             ),
+            (
+                Allow,
+                "group:observers",
+                Permissions.APIObservationsAdd,
+            ),
         ] + sorted(
             [
                 (Allow, f"user:{owner1.user.id}", ["manage:project", "upload"]),

tests/unit/test_config.py

@@ -520,5 +520,13 @@ def test_root_factory_access_control_list():
                 Permissions.AdminSponsorsWrite,
             ),
         ),
+        (
+            Allow,
+            "group:observers",
+            (
+                Permissions.APIEcho,
+                Permissions.APIObservationsAdd,
+            ),
+        ),
         (Allow, Authenticated, "manage:user"),
     ]

tests/unit/test_routes.py

@@ -533,6 +533,19 @@ def add_policy(name, filename):
             traverse="/{name}/",
             domain=warehouse,
         ),
+        # API URLs
+        pretend.call(
+            "api.echo",
+            "/danger-api/echo",
+            domain=warehouse,
+        ),
+        pretend.call(
+            "api.projects.observations",
+            "/danger-api/projects/{name}/observations",
+            factory="warehouse.packaging.models:ProjectFactory",
+            traverse="/{name}",
+            domain=warehouse,
+        ),
         # Mock URLs
         pretend.call(
             "mock.billing.checkout-session",

tests/unit/test_views.py

@@ -35,6 +35,7 @@
     current_user_indicator,
     flash_messages,
     forbidden,
+    forbidden_api,
     forbidden_include,
     force_status,
     health,
@@ -318,6 +319,18 @@ def test_forbidden_include(self):
         assert resp.content_length == 0
 
 
+class TestForbiddenAPIView:
+    def test_forbidden_api(self):
+        exc = pretend.stub()
+        request = pretend.stub()
+
+        resp = forbidden_api(exc, request)
+
+        assert resp.status_code == 403
+        assert resp.content_type == "application/json"
+        assert resp.json_body == {"message": "Access was denied to this resource."}
+
+
 class TestServiceUnavailableView:
     def test_renders_503(self, pyramid_config, pyramid_request):
         renderer = pyramid_config.testing_add_renderer("503.html")

warehouse/accounts/models.py

@@ -93,6 +93,9 @@ class User(SitemapMixin, HasObserversMixin, HasEvents, db.Model):
     is_superuser: Mapped[bool_false]
     is_moderator: Mapped[bool_false]
     is_psf_staff: Mapped[bool_false]
+    is_observer: Mapped[bool_false] = mapped_column(
+        comment="Is this user allowed to add Observations?"
+    )
     prohibit_password_reset: Mapped[bool_false]
     hide_avatar: Mapped[bool_false]
     date_joined: Mapped[datetime_now | None]
@@ -250,6 +253,8 @@ def __principals__(self) -> list[str]:
             principals.append("group:moderators")
         if self.is_psf_staff or self.is_superuser:
             principals.append("group:psf_staff")
+        if self.is_observer or self.is_superuser:
+            principals.append("group:observers")
 
         return principals
 

warehouse/accounts/security_policy.py

@@ -56,6 +56,17 @@ def identity(self, request):
         if request.matched_route.name == "forklift.legacy.file_upload":
             return None
 
+        # TODO: This feels wrong - special casing for paths and
+        #  prefixes isn't sustainable.
+        #  May need to revisit https://github.com/pypi/warehouse/pull/13854
+        #  Without this guard, we raise a RuntimeError related to `uses_session`,
+        #  because the `SessionAuthenticationHelper()` is called with no session.
+        #  Alternately, we could wrap the call to `authenticated_userid` in a
+        #  try/except RuntimeError block, but that feels like a band-aid.
+        # Session authentication cannot be used for /api routes
+        if request.matched_route.name.startswith("api."):
+            return None
+
         userid = self._session_helper.authenticated_userid(request)
         request._unauthenticated_userid = userid
 

warehouse/admin/templates/admin/users/detail.html

@@ -264,6 +264,7 @@ <h3 class="card-title">Permissions</h3>
                       {{ render_checkbox("Superuser", form.is_superuser, "is-superuser")}}
                       {{ render_checkbox("Moderator", form.is_moderator, "is-moderator")}}
                       {{ render_checkbox("PSF Staff", form.is_psf_staff, "is-psf-staff")}}
+                      {{ render_checkbox("Observer", form.is_observer, "is-observer")}}
                     </div>
                   </div>
                   <div class="col">

warehouse/admin/views/users.py

@@ -91,6 +91,7 @@ class UserForm(forms.Form):
     is_superuser = wtforms.fields.BooleanField()
     is_moderator = wtforms.fields.BooleanField()
     is_psf_staff = wtforms.fields.BooleanField()
+    is_observer = wtforms.fields.BooleanField()
 
     prohibit_password_reset = wtforms.fields.BooleanField()
     hide_avatar = wtforms.fields.BooleanField()