diff --git a/tests/common/db/malware.py b/tests/common/db/malware.py index 4e41a0c23865..32b57576862c 100644 --- a/tests/common/db/malware.py +++ b/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) diff --git a/tests/unit/admin/test_routes.py b/tests/unit/admin/test_routes.py index 451b6ad5a9e0..c03b772e45a7 100644 --- a/tests/unit/admin/test_routes.py +++ b/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, + ), ] diff --git a/tests/unit/admin/views/test_checks.py b/tests/unit/admin/views/test_checks.py index 0c3baeb03369..3edf31232e91 100644 --- a/tests/unit/admin/views/test_checks.py +++ b/tests/unit/admin/views/test_checks.py @@ -28,7 +28,10 @@ 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)] @@ -36,7 +39,10 @@ def test_get_checks_different_versions(self, db_request): 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: diff --git a/tests/unit/admin/views/test_verdicts.py b/tests/unit/admin/views/test_verdicts.py index 1492ce853f6e..51ee3587e496 100644 --- a/tests/unit/admin/views/test_verdicts.py +++ b/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) diff --git a/warehouse/admin/routes.py b/warehouse/admin/routes.py index 2b8ca93a3541..adaaf393afee 100644 --- a/warehouse/admin/routes.py +++ b/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 + ) diff --git a/warehouse/admin/templates/admin/malware/verdicts/detail.html b/warehouse/admin/templates/admin/malware/verdicts/detail.html index 7702943e8692..b31abf3e7221 100644 --- a/warehouse/admin/templates/admin/malware/verdicts/detail.html +++ b/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 %}
  • Verdicts
  • @@ -44,24 +44,20 @@ Object {% include 'object_link.html' %} + {% if verdict.manually_reviewed %} - Verdict Classification - {{ verdict.classification.value }} - - - Verdict Confidence - {{ verdict.confidence.value }} + Reviewer Verdict + {{ verdict.reviewer_verdict.value }} + {% endif %} - Manually Reviewed - {{ verdict.manually_reviewed }} + Check Verdict + {{ verdict.classification.value }} - {% if verdict.manually_reviewed %} - Administrator Verdict - {{ verdict.administrator_verdict }} + Confidence + {{ verdict.confidence.value }} - {% endif %} {% if verdict.full_report_link %} Full Report Link @@ -70,10 +66,25 @@ {% endif %} {% if verdict.details %} - Details + Additional Details
    {{ verdict.details|tojson(indent=4) }}
    {% endif %} + + {{ "Change" if verdict.manually_reviewed else "Set"}} Verdict + +
    + + + +
    + + diff --git a/warehouse/admin/templates/admin/malware/verdicts/index.html b/warehouse/admin/templates/admin/malware/verdicts/index.html index 443dd295aa15..f37073ee1c36 100644 --- a/warehouse/admin/templates/admin/malware/verdicts/index.html +++ b/warehouse/admin/templates/admin/malware/verdicts/index.html @@ -67,6 +67,7 @@
    + @@ -75,6 +76,11 @@ {% for verdict in verdicts %} + {% else %} diff --git a/warehouse/admin/views/checks.py b/warehouse/admin/views/checks.py index 0cd1761c8456..6b203d88a20a 100644 --- a/warehouse/admin/views/checks.py +++ b/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} diff --git a/warehouse/admin/views/verdicts.py b/warehouse/admin/views/verdicts.py index ab8dec6dd514..e55bc1322b36 100644 --- a/warehouse/admin/views/verdicts.py +++ b/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)) diff --git a/warehouse/malware/models.py b/warehouse/malware/models.py index a1ab490fc45e..e4f56fcf851a 100644 --- a/warehouse/malware/models.py +++ b/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, ) diff --git a/warehouse/migrations/versions/061ff3d24c22_add_malware_detection_tables.py b/warehouse/migrations/versions/061ff3d24c22_add_malware_detection_tables.py index 622660fd042f..1eb2da246e58 100644 --- a/warehouse/migrations/versions/061ff3d24c22_add_malware_detection_tables.py +++ b/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"
    Investigate Object Check Classification
    + + Detail + + {% include 'object_link.html' %} @@ -104,9 +110,15 @@ - - Detail - + {% if verdict.manually_reviewed %} + Marked as {{ verdict.reviewer_verdict.value }} + {% else %} +
    + + + +
    + {% endif %}