Implement NCMEC Upvote/Downvote in reference API #1624

aryzle · 2024-09-18T15:57:52Z

Summary

Implement NCMEC Upvote/Downvote in reference API
docs: https://report.cybertip.org/hashsharing/v2/documentation/#working-with-feedback

fixes #1614

Test Plan

unit test for success flow
David to test on NCMEC API

aryzle · 2024-09-18T15:59:52Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/tests/data.py

+UPDATE_FEEDBACK_RESULT_XML = """
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<submissionResult xmlns="https://hashsharing.ncmec.org/hashsharing/v2">
+  <!-- What this returns is not documented -->


@Dcallies feedback submission has no response documented here

Dcallies

Looks like a good start. Once the CI is passing, let me know and I'll test it with some real credentials to confirm it works as expected.

Dcallies · 2024-09-20T12:16:19Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

 from threatexchange.exchanges.clients.utils.common import TimeoutHTTPAdapter


 _DATE_FORMAT_STR = "%Y-%m-%dT%H:%M:%SZ"
 _DEFAULT_ELE = ET.Element("")

 T = t.TypeVar("T")
+FEEDBACK = t.List[t.Dict[str, str]]


blocking: ALL_CAPS usually denotes a constant. In this case, your type is simple enough that you should just inline it.

Dcallies · 2024-09-20T12:17:26Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

@@ -131,6 +132,7 @@ class NCMECEntryUpdate:
    deleted: bool
    classification: t.Optional[str]
    fingerprints: t.Dict[str, str]
+    feedback: t.Optional[FEEDBACK]


blocking: We should not use optional here, because empty dictionary expresses the same thing

Dcallies · 2024-09-20T12:17:39Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

@@ -148,6 +150,18 @@ def from_xml(cls, xml: _XMLWrapper) -> "NCMECEntryUpdate":
            fingerprints={
                x.tag: x.text for x in xml.maybe("fingerprints") if x.has_text
            },
+            feedback=[


blocking; Your type says dict, your code says list!

type is t.List[t.Dict[str, str]]

Dcallies · 2024-09-20T12:19:27Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

@@ -401,6 +432,14 @@ def get_entries_iter(
            has_more = bool(next_)
            yield result

+    # TODO: split into 2, submit upvote and downvote
+    def submit_feedback(self, entry_id: str, is_good: bool) -> GetEntriesResponse:


unsure: This API also needs the ESP ID, no?

Dcallies · 2024-09-20T12:19:58Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

@@ -401,6 +432,14 @@ def get_entries_iter(
            has_more = bool(next_)
            yield result

+    # TODO: split into 2, submit upvote and downvote


blocking; Yup

send_upvote(...):

send_downvote(...):

python-threatexchange/threatexchange/exchanges/clients/ncmec/tests/data.py

Dcallies · 2024-09-20T12:23:38Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/tests/test_hash_api.py

-NEXT_UNESCAPED2 = (
-    "/v2/entries?from=2017-10-20T00%3A00%3A00.000Z"
-    "&to=2017-10-30T00%3A00%3A00.000Z&start=3001&size=1000&max=4000"
+from threatexchange.exchanges.clients.ncmec.tests.data import (


ignorable: Consider importing by module when you are importing many functions.

aryzle · 2024-09-28T20:11:27Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

+                    ),
+                    "reasons": x.maybe(
+                        "reasons"
+                    ),  # "reason" with "guid", "name", "type" | "members"


working on expanding members and reasons here

aryzle · 2024-09-30T13:26:13Z

@Dcallies this should be testable against the NCMEC API now

Dcallies

Thanks for all the hard work @aryzle ! We'll take it from here and test it against the real API and finish it off! You can consider this one done!

Dcallies · 2024-10-01T08:28:52Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

@@ -261,15 +343,19 @@ def __init__(
        username: str,
        password: str,
        environment: NCMECEnvironment,
+        member_id: t.Optional[str] = None,
+        reasons_map: t.Dict[str, t.List[t.Dict[str, str]]] = {},


blocking: We won't know this at client construction time, and can poll it afterwards.

Dcallies · 2024-10-01T08:31:18Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

    ) -> None:
        assert is_valid_user_pass(username, password)
        self.username = username
        self.password = password
        self._base_url = environment.value
+        self.member_id = member_id
+        self.reasons_map = reasons_map or {}


Hmm this or {} is fairly subtle. I'm assuming we are doing it because the default is {} which is a shared instance across any object, and the or {} will replace it with a new instance per call, but having it be optional and default to None is more easy to prove doesn't have a bug.

Dcallies · 2024-10-01T08:32:25Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

+        if not affirmative and not reason_id:
+            raise ValueError("Negative feedback must have a reason_id")


(note to self): If reason map only has one item in it, choose that item. Also test NCMEC API to make sure it won't accept empty reason, which should be our pref otherwise.

Dcallies · 2024-10-01T08:34:55Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

@@ -123,6 +123,19 @@ class NCMECEntryType(Enum):
    video = "video"


+@unique
+class NCMECFeedbackType(Enum):


(note to self): Add note that these are also the entry types

Dcallies · 2024-10-01T08:35:42Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

@@ -131,6 +144,7 @@ class NCMECEntryUpdate:
    deleted: bool
    classification: t.Optional[str]
    fingerprints: t.Dict[str, str]
+    feedback: t.List[t.Dict[str, t.Any]]


(note to self): Consider whether it makes sense to use the enum here, or whether to leave as string in all cases

Dcallies · 2024-10-01T08:36:54Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

+                        "members": [
+                            {"id": m.str("id"), "name": m.text}
+                            for m in x.maybe("members")
+                            if m.has_text
+                        ],


(note to self): Should this be dict instead of synthesizing a a dict? What about a dedicated class?

Dcallies · 2024-10-01T08:38:53Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

+                {
+                    "guid": reason.str("guid"),
+                    "name": reason.str("name"),
+                    "type": reason.str("type"),
+                }


(note to self): Use a dedicated class here instead of dict

Dcallies · 2024-10-01T08:40:57Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

+        entry_id: str,
+        feedback_type: NCMECFeedbackType,
+        affirmative: bool,
+        reason_id: t.Optional[str] = None,


(note to self): Is a reason string better to use here than the guid?

Dcallies · 2024-10-03T13:23:16Z

Continuing in #1648 - there's probably a way to have re-used this number, but I don't know it :/

wip

965cfa8

facebook-github-bot added the CLA Signed label Sep 18, 2024

aryzle commented Sep 18, 2024

View reviewed changes

Dcallies requested changes Sep 20, 2024

View reviewed changes

aryzle added 4 commits September 25, 2024 21:33

add NCMECFeedbackType, member_id, build xml payload

90aa0ed

get feedback_reasons

1da801d

update test check status code

be4cea2

type fixes

a82dd7b

aryzle commented Sep 28, 2024

View reviewed changes

parse feedback better

6dee071

aryzle marked this pull request as ready for review September 30, 2024 13:53

Dcallies reviewed Oct 1, 2024

View reviewed changes

Dcallies mentioned this pull request Oct 3, 2024

[py-tx] Implement NCMEC Upvote/Downvote in reference API #1648

Merged

Dcallies closed this Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement NCMEC Upvote/Downvote in reference API #1624

Implement NCMEC Upvote/Downvote in reference API #1624

aryzle commented Sep 18, 2024 •

edited

Loading

aryzle Sep 18, 2024

Dcallies left a comment •

edited

Loading

Dcallies Sep 20, 2024

Dcallies Sep 20, 2024

Dcallies Sep 20, 2024

aryzle Sep 26, 2024

Dcallies Sep 20, 2024

Dcallies Sep 20, 2024

Dcallies Sep 20, 2024

aryzle Sep 28, 2024

aryzle commented Sep 30, 2024 •

edited

Loading

Dcallies left a comment

Dcallies Oct 1, 2024

Dcallies Oct 1, 2024

Dcallies Oct 1, 2024

Dcallies Oct 1, 2024

Dcallies Oct 1, 2024

Dcallies Oct 1, 2024

Dcallies Oct 1, 2024

Dcallies Oct 1, 2024

Dcallies commented Oct 3, 2024

		if not affirmative and not reason_id:
		raise ValueError("Negative feedback must have a reason_id")

Implement NCMEC Upvote/Downvote in reference API #1624

Implement NCMEC Upvote/Downvote in reference API #1624

Conversation

aryzle commented Sep 18, 2024 • edited Loading

Summary

Test Plan

Choose a reason for hiding this comment

Dcallies left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aryzle commented Sep 30, 2024 • edited Loading

Dcallies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dcallies commented Oct 3, 2024

aryzle commented Sep 18, 2024 •

edited

Loading

Dcallies left a comment •

edited

Loading

aryzle commented Sep 30, 2024 •

edited

Loading