Add verdict administrator review. Fixes #6062. (#7339)

* Add verdict administrator review. Fixes #6062.

- Add new `admin.verdicts.review` endpoint
- Change layout of verdict list and detail view and add forms
- Change sort order of the MalwareChecks, and update the tests

* Code review changes.

- Rename MalwareVerdict field `administrator_verdict` to `reviewer_verdict`.
- Change verdict review permission from `admin` to `moderator`.

Author: xmunoz

tests/common/db/malware.py

@@ -55,7 +55,7 @@ class Meta:
     release = None
     project = None
     manually_reviewed = True
-    administrator_verdict = factory.fuzzy.FuzzyChoice(list(VerdictClassification))
+    reviewer_verdict = factory.fuzzy.FuzzyChoice(list(VerdictClassification))
     classification = factory.fuzzy.FuzzyChoice(list(VerdictClassification))
     confidence = factory.fuzzy.FuzzyChoice(list(VerdictConfidence))
     message = factory.fuzzy.FuzzyText(length=80)

tests/unit/admin/test_routes.py

@@ -141,4 +141,9 @@ def test_includeme():
         pretend.call(
             "admin.verdicts.detail", "/admin/verdicts/{verdict_id}", domain=warehouse
         ),
+        pretend.call(
+            "admin.verdicts.review",
+            "/admin/verdicts/{verdict_id}/review",
+            domain=warehouse,
+        ),
     ]

tests/unit/admin/views/test_checks.py

@@ -28,15 +28,21 @@ def test_get_checks_none(self, db_request):
 
     def test_get_checks(self, db_request):
         checks = [MalwareCheckFactory.create() for _ in range(10)]
-        assert views.get_checks(db_request) == {"checks": checks}
+        result = views.get_checks(db_request)["checks"]
+        assert len(result) == len(checks)
+        for r in result:
+            assert r in checks
 
     def test_get_checks_different_versions(self, db_request):
         checks = [MalwareCheckFactory.create() for _ in range(5)]
         checks_same = [
             MalwareCheckFactory.create(name="MyCheck", version=i) for i in range(1, 6)
         ]
         checks.append(checks_same[-1])
-        assert views.get_checks(db_request) == {"checks": checks}
+        result = views.get_checks(db_request)["checks"]
+        assert len(result) == len(checks)
+        for r in result:
+            assert r in checks
 
 
 class TestGetCheck:

tests/unit/admin/views/test_verdicts.py

@@ -14,6 +14,7 @@
 
 from random import randint
 
+import pretend
 import pytest
 
 from pyramid.httpexceptions import HTTPBadRequest, HTTPNotFound
@@ -193,10 +194,55 @@ def test_found(self, db_request):
         lookup_id = verdicts[index].id
         db_request.matchdict["verdict_id"] = lookup_id
 
-        assert views.get_verdict(db_request) == {"verdict": verdicts[index]}
+        assert views.get_verdict(db_request) == {
+            "verdict": verdicts[index],
+            "classifications": ["Benign", "Indeterminate", "Threat"],
+        }
 
     def test_not_found(self, db_request):
         db_request.matchdict["verdict_id"] = uuid.uuid4()
 
         with pytest.raises(HTTPNotFound):
             views.get_verdict(db_request)
+
+
+class TestReviewVerdict:
+    @pytest.mark.parametrize(
+        "manually_reviewed, reviewer_verdict",
+        [
+            (False, None),  # unreviewed verdict
+            (True, VerdictClassification.Threat),  # previously reviewed
+        ],
+    )
+    def test_set_classification(self, db_request, manually_reviewed, reviewer_verdict):
+        verdict = MalwareVerdictFactory.create(
+            manually_reviewed=manually_reviewed, reviewer_verdict=reviewer_verdict,
+        )
+
+        db_request.matchdict["verdict_id"] = verdict.id
+        db_request.POST = {"classification": "Benign"}
+        db_request.session = pretend.stub(
+            flash=pretend.call_recorder(lambda *a, **kw: None)
+        )
+
+        db_request.route_path = pretend.call_recorder(
+            lambda *a, **kw: "/admin/verdicts/%s/review" % verdict.id
+        )
+
+        views.review_verdict(db_request)
+
+        assert db_request.session.flash.calls == [
+            pretend.call("Verdict %s marked as reviewed." % verdict.id, queue="success")
+        ]
+
+        assert verdict.manually_reviewed
+        assert verdict.reviewer_verdict == VerdictClassification.Benign
+
+    @pytest.mark.parametrize("post_params", [{}, {"classification": "Nope"}])
+    def test_errors(self, db_request, post_params):
+        verdict = MalwareVerdictFactory.create()
+        db_request.matchdict["verdict_id"] = verdict.id
+        db_request.POST = post_params
+
+        with pytest.raises(HTTPBadRequest):
+            views.review_verdict(db_request)

warehouse/admin/routes.py

@@ -148,3 +148,6 @@ def includeme(config):
     config.add_route(
         "admin.verdicts.detail", "/admin/verdicts/{verdict_id}", domain=warehouse
     )
+    config.add_route(
+        "admin.verdicts.review", "/admin/verdicts/{verdict_id}/review", domain=warehouse
+    )

warehouse/admin/templates/admin/malware/verdicts/detail.html

@@ -13,7 +13,7 @@
 -#}
 {% extends "admin/base.html" %}
 
-{% block title %}Verdict {{ verdict.id }}{% endblock %}
+{% block title %}Verdict Details{% endblock %}
 
 {% block breadcrumb %}
   <li><a href="{{ request.route_path('admin.verdicts.list') }}">Verdicts</a></li>
@@ -44,24 +44,20 @@
           <th scope="row">Object</th>
           <td>{% include 'object_link.html' %}</td>
         </tr>
+        {% if verdict.manually_reviewed %}
         <tr>
-          <th scope="row">Verdict Classification</th>
-          <td>{{ verdict.classification.value }}</td>
-        </tr>
-        <tr>
-          <th scope="row">Verdict Confidence</th>
-          <td>{{ verdict.confidence.value }}</td>
+          <th scope="row">Reviewer Verdict</th>
+          <td>{{ verdict.reviewer_verdict.value }}</td>
         </tr>
+        {% endif %}
         <tr>
-          <th scope="row">Manually Reviewed</th>
-          <td>{{ verdict.manually_reviewed }}</td>
+          <th scope="row">Check Verdict</th>
+          <td>{{ verdict.classification.value }}</td>
         </tr>
-        {% if verdict.manually_reviewed %}
         <tr>
-          <th scope="row">Administrator Verdict</th>
-          <td>{{ verdict.administrator_verdict }}</td>
+          <th scope="row">Confidence</th>
+          <td>{{ verdict.confidence.value }}</td>
         </tr>
-        {% endif %}
         {% if verdict.full_report_link %}
         <tr>
           <th scope="row">Full Report Link</th>
@@ -70,10 +66,25 @@
         {% endif %}
         {% if verdict.details %}
         <tr>
-          <th scope="row">Details</th>
+          <th scope="row">Additional Details</th>
           <td><pre>{{ verdict.details|tojson(indent=4) }}</pre></td>
         </tr>
         {% endif %}
+        <tr>
+          <th scope="row">{{ "Change" if verdict.manually_reviewed else "Set"}} Verdict</th>
+          <td>
+            <form class="form-inline" method="POST" action="{{ request.route_path('admin.verdicts.review', verdict_id=verdict.id) }}">
+              <input name="csrf_token" type="hidden" value="{{ request.session.get_csrf_token() }}">
+              <select class="form-control" name="classification">
+                <option disabled selected></option>
+                {% for c in classifications %}
+                <option>{{ c }}</option>
+                {% endfor %}
+              </select>
+              <button type="submit" class="btn btn-primary">Save</button>
+            </form>
+          </td>
+        </tr>
       </table>
     </div>
   </div>

warehouse/admin/templates/admin/malware/verdicts/index.html

@@ -67,6 +67,7 @@
   <div class="box-body table-responsive no-padding">
     <table class="table table-hover">
       <tr>
+        <th>Investigate</th>
         <th>Object</th>
         <th>Check</th>
         <th>Classification</th>
@@ -75,6 +76,11 @@
       </tr>
       {% for verdict in verdicts %}
       <tr>
+        <td>
+          <a href="{{ request.route_path('admin.verdicts.detail', verdict_id=verdict.id) }}">
+            Detail
+          </a>
+        </td>
         <td>{% include 'object_link.html' %}</td>
         <td>
           <a href="{{ request.route_path('admin.checks.detail', check_name=verdict.check.name) }}">
@@ -104,9 +110,15 @@
           </span>
         </td>
         <td>
-          <a href="{{ request.route_path('admin.verdicts.detail', verdict_id=verdict.id) }}">
-            Detail
-          </a>
+          {% if verdict.manually_reviewed %}
+          Marked as {{ verdict.reviewer_verdict.value }}
+          {% else %}
+          <form method="POST" action="{{ request.route_path('admin.verdicts.review', verdict_id=verdict.id, _query=request.params) }}">
+            <input type="hidden" name="classification" value="Benign">
+            <input name="csrf_token" type="hidden" value="{{ request.session.get_csrf_token() }}">
+            <input type="submit" value="Mark Benign">
+          </form>
+	  {% endif %}
         </td>
       </tr>
       {% else %}

warehouse/admin/views/checks.py

@@ -34,6 +34,8 @@ def get_checks(request):
         if not check.is_stale:
             active_checks.append(check)
 
+    active_checks.sort(key=lambda check: check.created, reverse=True)
+
     return {"checks": active_checks}
 
 

warehouse/admin/views/verdicts.py

@@ -11,7 +11,7 @@
 # limitations under the License.
 
 from paginate_sqlalchemy import SqlalchemyOrmPage as SQLAlchemyORMPage
-from pyramid.httpexceptions import HTTPBadRequest, HTTPNotFound
+from pyramid.httpexceptions import HTTPBadRequest, HTTPNotFound, HTTPSeeOther
 from pyramid.view import view_config
 
 from warehouse.malware.models import (
@@ -61,11 +61,43 @@ def get_verdict(request):
     verdict = request.db.query(MalwareVerdict).get(request.matchdict["verdict_id"])
 
     if verdict:
-        return {"verdict": verdict}
+        return {
+            "verdict": verdict,
+            "classifications": list(VerdictClassification.__members__.keys()),
+        }
 
     raise HTTPNotFound
 
 
+@view_config(
+    route_name="admin.verdicts.review",
+    permission="moderator",
+    request_method="POST",
+    uses_session=True,
+    require_methods=False,
+    require_csrf=True,
+)
+def review_verdict(request):
+    verdict = request.db.query(MalwareVerdict).get(request.matchdict["verdict_id"])
+
+    try:
+        classification = getattr(VerdictClassification, request.POST["classification"])
+    except (KeyError, AttributeError):
+        raise HTTPBadRequest("Invalid verdict classification.") from None
+
+    verdict.manually_reviewed = True
+    verdict.reviewer_verdict = classification
+
+    request.session.flash(
+        "Verdict %s marked as reviewed." % verdict.id, queue="success"
+    )
+
+    # If no query params are provided (e.g. request originating from
+    # admins.verdicts.detail view), then route to the default list view
+    query = request.GET or {"classification": "threat", "manually_reviewed": "0"}
+    return HTTPSeeOther(request.route_path("admin.verdicts.list", _query=query))
+
+
 def validate_fields(request, validators):
     try:
         int(request.params.get("page", 1))

warehouse/malware/models.py

@@ -161,7 +161,7 @@ class MalwareVerdict(db.Model):
     message = Column(Text, nullable=True)
     details = Column(JSONB, nullable=True)
     manually_reviewed = Column(Boolean, nullable=False, server_default=sql.false())
-    administrator_verdict = Column(
+    reviewer_verdict = Column(
         Enum(VerdictClassification, values_callable=lambda x: [e.value for e in x]),
         nullable=True,
     )

warehouse/migrations/versions/061ff3d24c22_add_malware_detection_tables.py

@@ -91,7 +91,7 @@ def upgrade():
             server_default=sa.text("false"),
             nullable=False,
         ),
-        sa.Column("administrator_verdict", VerdictClassifications, nullable=True,),
+        sa.Column("reviewer_verdict", VerdictClassifications, nullable=True,),
         sa.Column("full_report_link", sa.String(), nullable=True),
         sa.ForeignKeyConstraint(
             ["check_id"], ["malware_checks.id"], onupdate="CASCADE", ondelete="CASCADE"