Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hma] Cleanup /m/lookup Hash + Match API #1703

Open
Dcallies opened this issue Dec 2, 2024 · 5 comments · May be fixed by #1729
Open

[hma] Cleanup /m/lookup Hash + Match API #1703

Dcallies opened this issue Dec 2, 2024 · 5 comments · May be fixed by #1729
Labels
hma Items related to the hasher-matcher-actioner system

Comments

@Dcallies
Copy link
Contributor

Dcallies commented Dec 2, 2024

Some users have asked for an API that will allow them to combine the hash+match API into one call. It turns out this already exists as /m/lookup, but this triggers hashing (high CPU cost) even if the hashing role is not enabled, which defeats the point of roles!

There's also a development/UI version of this already here: https://github.com/facebook/ThreatExchange/blob/main/hasher-matcher-actioner/src/OpenMediaMatch/blueprints/ui.py#L170 - this probably should be replaced with the new and improved method.

Your mission, should you choose to accept it:

  1. Refactor /lookup so that that the URL and POST versions are only enabled when the hashing role is enabled. At the very worst, you can just check for the role in the app config and throw if it's not.
  2. Delete the /ui version of lookup and replace it with your new fixed version (can be a separate PR)
  3. Make sure everything is tested
@Dcallies Dcallies added the hma Items related to the hasher-matcher-actioner system label Dec 2, 2024
@aryzle
Copy link
Collaborator

aryzle commented Dec 15, 2024

@Dcallies I can start working on this now

@Dcallies
Copy link
Contributor Author

Gopher it!

@aryzle
Copy link
Collaborator

aryzle commented Dec 16, 2024

Hey @Dcallies just want to a clarify a few things, maybe I'm missing some context

What type of refactoring did you have in mind? It's easy enough to check for the ROLE_HASHER role and abort if it's false, but the lookup function itself looks ok to me - no glaring errors.

In the ui endpoint /ui/query we could also check for the role, but do you mean have the match form POST to /m/lookup instead of /ui/query ? looks like they return diff things (list vs dict)

  • also the ui bp isn't registered if ROLE_HASHER=False so as it stands no need to check for the role in /ui/query

@Dcallies
Copy link
Contributor Author

Lookup should not allow lookup by URL or lookup by POST with content (which triggers hashing) unless the hashing role is enabled.

When you say you looked at the lookup API and it looked fine, did you check to see if you can trigger hashing without the hash role?

@aryzle
Copy link
Collaborator

aryzle commented Dec 17, 2024

so there's lookup_get, lookup_post and they both call lookup. lookup_get and lookup_post are where the hashing role can be checked, that's what I'm saying is easy enough. Do you want the lookup method refactored? and what sort of refactoring did you have in mind?

I already implemented the hashing role checks and tested them yup, can't do a GET with a URL or a POST with files unless the hashing role is enabled ✅

@aryzle aryzle linked a pull request Dec 22, 2024 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hma Items related to the hasher-matcher-actioner system
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants