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

Check for hasher role in /lookup endpoint #1729

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aryzle
Copy link
Collaborator

@aryzle aryzle commented Dec 22, 2024

Summary

fixes #1703

TODO:

  • unit test

Test Plan

  1. set ROLE_HASHER = False in development_omm_config.py
  2. start server
  3. try GET/POST to /lookup endpoint, should error now with 403 "Hashing is disabled, missing role"

Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: I think all that's missing is a test that shows the function returns a 200 when it's running normally - it looks uncovered at the moment.

@@ -221,6 +223,26 @@ def test_banks_add_hash_index(app: Flask, client: FlaskClient):
assert post_response.json == {"matches": [2]}


def test_lookup_add_hash_without_role(app: Flask, client: FlaskClient):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't think we're "adding" a hash in this unittest. In other tests, this is referring to banking functionality.


# test GET
image_url = "https://github.com/facebook/ThreatExchange/blob/main/pdq/data/bridge-mods/aaa-orig.jpg?raw=true"
get_resp = client.get(f"/m/lookup?url={image_url}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: I don't see any existing unittests in this file that check against this endpoint at all (the others are using "raw_lookup". Can you also test a lookup is accepted?

This file is getting big enough that it may make sense to break out unittests for the different APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[hma] Cleanup /m/lookup Hash + Match API
3 participants