[pytx] Add Tech Against Terrorism Hash API #1617

Bruce-Pouncey-TAT · 2024-09-03T16:10:56Z

Summary

Hi

Bruce here from Tech Against Terrorism 👋

This PR contains Step 1 from this issue integrating the Tech Against Terrorism Hash List API into python-threatexchange

I used StopNSCII & NCMEC inside /client/exchanges/ as initial guidance.

Caveat
Our API returns a pre-signed URL to a JSON file for our hash list, instead of paginated results. I'm wondering how we want to handle this moving forward into part 2 (Once we're happy with part 1) ?

Test Plan

Manual
For manual testing you can use python shell to simply instantiate the TATHashListAPI with your TCAP username and password.

tat_api = TATHashListAPI(username=USER_NAME, password=PASSWORD)
response = tat_api.get_hash_list()

Automated
I have written unit tests techagainstterrorism/tests so pytest will do it.

facebook-github-bot · 2024-09-03T16:11:02Z

Hi @Bruce-Pouncey-TAT!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

Dcallies · 2024-09-04T13:34:23Z

Hey Bruce! Great to be working with you! I will help you get this landed and review it.

As for the details of the API, I did read your dos and though you picked a bit of a challenging implementation, but I think we can make it work! I'll give you some tips on the PR itself.

Bruce-Pouncey-TAT · 2024-09-04T15:05:16Z

Hey David, likewise!
I will get the pipeline failures resolved in the coming days, I'm also waiting on the CLA confirmation to come through on my side.

…atting

Dcallies

Overall looks pretty good!

Thank you for including your manual test steps in the PR summary!

The one blocking thing I have is that I think the interface will become easier if you standardize on throwing exceptions instead of catching them and return a new object.

Let me know if you are short on time or energy, I'm willing to do the labor to bring this in the rest of the way if you are short on either of those.

Dcallies · 2024-09-04T19:19:41Z

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

+# Maybe move to a common library someday
+from threatexchange.exchanges.clients.fb_threatexchange.api import TimeoutHTTPAdapter


ignorable: If you want to move this to a common helper library under clients, now seems like a reasonable time

Dcallies · 2024-09-04T19:20:14Z

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

+
+@dataclass
+class TATUser:
+  user: Any


Is it possible to narrow this anymore?

Our Actual User object can be quite large (possibly hundreds of permissions) and will vary in size based on who is using the interface. I can narrow this down to Dict[str, Any]

Dcallies · 2024-09-04T19:21:50Z

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

+
+  This API delivers a JSON file containing a comprehensive list of all hashed terrorist content within the TAT system.
+
+  The list is refreshed daily.


blocking: If you have public documentation, can you also add a link to it here?

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

Dcallies · 2024-09-05T11:48:22Z

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

+
+        except Exception as e:
+            logging.error("Error authenticating with TCAP: %s", e)
+            return None


blocking question: There's no recovery from this, right? Why not let the exception leak out?

Dcallies · 2024-09-05T11:48:53Z

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

+            response.raise_for_status()
+            return response.json()
+
+    def authenticate(self, username: str, password: str) -> t.Optional[str]:


nit: would personally rename this get_auth_token

Dcallies · 2024-09-05T11:49:09Z

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

+
+            endpoint = f"{TATEndpoint.hash_list.value}/{ideology}"
+
+            print("ENDPOINT", endpoint)


blocking: No stdout in libraries please

Dcallies · 2024-09-05T11:50:02Z

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

+        except requests.exceptions.HTTPError as http_err:
+            return TATAPIErrorResponse(error=str(http_err))
+
+        except Exception as e:


blocking q: Why not let exceptions leak out? This is exceptional circumstances, and I think the other clients in this library except on error. Some libraries are very anti-exception, but not this one :P

Thanks for the feedback!

Forgive me it's my first time creating/contributing to a package I have mostly worked on web apps where we catch and handle everything!

Today I have learned it's better practice in libraries to let the exception leak up the stack 🙇 awesome!

Dcallies · 2024-09-05T11:58:14Z

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

+
+    def get_hash_list(
+        self, ideology: str = TATIdeology._all.value
+    ) -> t.Union[TATHashListResponse, TATAPIErrorResponse]:


blocking: This is a challenging type to deal with on the caller side! The simplest solution is to just throw exceptions. Exceptions for for exceptional circumstances, and failing to fetch seems to match that.

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/tests/test_api.py

Bruce-Pouncey-TAT · 2024-09-05T13:50:13Z

Overall looks pretty good!

Thank you for including your manual test steps in the PR summary!

The one blocking thing I have is that I think the interface will become easier if you standardize on throwing exceptions instead of catching them and return a new object.

Let me know if you are short on time or energy, I'm willing to do the labor to bring this in the rest of the way if you are short on either of those.

Thanks!
I have resolved the comments with my best understanding of what you have requested. Also I have the time and energy to continue with anymore changes you suggest.

I have moved the TimeoutHTTPAdapter into a common utils file and updated where it has been used.

Idea 🤔
I have noticed across all clients we're using _get_session within a content manager
Maybe we could lift this up into a parent class ? PyTxClientAPIBase for the lack of a better definition or maybe introduce an Abstract Base Class for the places where client are using _get _post. Just a thought 😄

Dcallies

Idea 🤔
I have noticed across all clients we're using _get_session within a content manager
Maybe we could lift this up into a parent class ? PyTxClientAPIBase for the lack of a better definition or maybe introduce an Abstract Base Class for the places where client are using _get _post. Just a thought 😄

It's an interesting idea! Let's first get your SignalExchangeAPI implementation working, and we can come back to it.

You only have ONE thing left to fix - the lint error on running black!

Dcallies · 2024-09-06T11:41:41Z

python-threatexchange/threatexchange/exchanges/clients/utils/common.py

@@ -0,0 +1,17 @@
+from requests.adapters import HTTPAdapter


nit: I git an internal nagger that forces me to add

# Copyright (c) Meta Platforms, Inc. and affiliates. to the tops of all files, though I can do it as a followup

Dcallies · 2024-09-06T11:42:00Z

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/tests/test_api.py

@@ -0,0 +1,94 @@
+import typing as t


nit: I git an internal nagger that forces me to add

# Copyright (c) Meta Platforms, Inc. and affiliates. to the tops of all files, though I can do it as a followup

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

Dcallies · 2024-09-06T11:44:36Z

python-threatexchange/threatexchange/exchanges/clients/techagainstterrorism/api.py

+
+        except Exception as exception:
+            logging.error("Failed to get hash list: %s", exception)
+            raise Exception("Failed to fetch Hash List: %s", exception)


blocking (though I'm fine fixing it on a followup): Instead of wrapping this exception, rethrow the one on stack by just doing

raise

Ref: https://docs.python.org/3/tutorial/errors.html#enriching-exceptions-with-notes

I was in the code so I made the change. Thanks for the reference!

Dcallies

Looks good!

Bruce-Pouncey-TAT added 3 commits September 2, 2024 16:46

issue_1610: Initial code setup

d7102c5

issue_1610: hash list unit tests

b703541

issue_1610: hash list unit tests

8be962a

Bruce-Pouncey-TAT requested a review from Dcallies as a code owner September 3, 2024 16:10

Added logging

0104358

Dcallies changed the title ~~Issue 1610~~ [pytx] Add Tech Against Terrorism Hash API Sep 4, 2024

Bruce-Pouncey-TAT added 3 commits September 4, 2024 17:41

issue_1610: Fixed some type hints in api.py

44609fc

issue_1610: Attempted backwards compatability type hints | black form…

3481619

…atting

issue_1610: hash list request endpoint

ea8bfef

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

Dcallies requested changes Sep 5, 2024

View reviewed changes

Bruce-Pouncey-TAT added 4 commits September 5, 2024 13:15

issue_1610: Added links to TAT documentation

2ee9628

issue_1617: Attempt to resolve PR comments

cd65322

issue_1610: Ran black

cf11bc8

issue_1610: Updated import of TimeoutHTTPAdapter in fb_threatexchange

abe9003

Dcallies requested changes Sep 6, 2024

View reviewed changes

issue_1610: ran black

5d7151f

Dcallies approved these changes Sep 8, 2024

View reviewed changes

Dcallies merged commit 1a9c554 into facebook:main Sep 8, 2024
6 checks passed

Bruce-Pouncey-TAT added a commit to Bruce-Pouncey-TAT/ThreatExchange that referenced this pull request Sep 13, 2024

[pytx] Add Tech Against Terrorism Hash API (facebook#1617)

afa1996

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pytx] Add Tech Against Terrorism Hash API #1617

[pytx] Add Tech Against Terrorism Hash API #1617

Bruce-Pouncey-TAT commented Sep 3, 2024

facebook-github-bot commented Sep 3, 2024

Dcallies commented Sep 4, 2024

Bruce-Pouncey-TAT commented Sep 4, 2024

Dcallies left a comment

Dcallies Sep 4, 2024

Bruce-Pouncey-TAT Sep 5, 2024

Dcallies Sep 4, 2024

Bruce-Pouncey-TAT Sep 5, 2024 •

edited

Loading

Dcallies Sep 4, 2024

Dcallies Sep 5, 2024

Dcallies Sep 5, 2024

Dcallies Sep 5, 2024

Dcallies Sep 5, 2024

Bruce-Pouncey-TAT Sep 5, 2024

Dcallies Sep 5, 2024

Bruce-Pouncey-TAT commented Sep 5, 2024 •

edited

Loading

Dcallies left a comment

Dcallies Sep 6, 2024

Dcallies Sep 6, 2024

Dcallies Sep 6, 2024

Bruce-Pouncey-TAT Sep 6, 2024

Dcallies left a comment

		# Maybe move to a common library someday
		from threatexchange.exchanges.clients.fb_threatexchange.api import TimeoutHTTPAdapter


		This API delivers a JSON file containing a comprehensive list of all hashed terrorist content within the TAT system.

		The list is refreshed daily.


		endpoint = f"{TATEndpoint.hash_list.value}/{ideology}"

		print("ENDPOINT", endpoint)

[pytx] Add Tech Against Terrorism Hash API #1617

[pytx] Add Tech Against Terrorism Hash API #1617

Conversation

Bruce-Pouncey-TAT commented Sep 3, 2024

Summary

Test Plan

facebook-github-bot commented Sep 3, 2024

Action Required

Process

Dcallies commented Sep 4, 2024

Bruce-Pouncey-TAT commented Sep 4, 2024

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

Bruce-Pouncey-TAT Sep 5, 2024 • 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

Bruce-Pouncey-TAT commented Sep 5, 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

Dcallies left a comment

Choose a reason for hiding this comment

Bruce-Pouncey-TAT Sep 5, 2024 •

edited

Loading

Bruce-Pouncey-TAT commented Sep 5, 2024 •

edited

Loading