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

Add support for A-api-request-uuid query parameter #807

Merged
merged 15 commits into from
Apr 20, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Python
*.pyc
/venv
/.venv

# IDE
.idea
Expand Down
1 change: 1 addition & 0 deletions Makefile.config.example
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ LOG_TYPE := flat
# For NGAP Deployments #
########################
# In an NGAP environment you must uncomment the following line or the deployment will fail
# This has been deprecated in NGAP and is no longer necessary
# PERMISSION_BOUNDARY_NAME := NGAPShRoleBoundary

PRIVATE_VPC :=
Expand Down
1 change: 1 addition & 0 deletions README.MD
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ container path:
file:/var/deps/rain-api-core
```
*NOTE: This is a generated file and you should NOT be committing these changes*

3. Add any source files of that dependency to the `REQUIREMENTS_DEPS` in your `Makefile.config`:
```makefile
REQUIREMENTS_DEPS := $(shell find /host/path/to/rain-api-core/rain_api_core/ -name '*.py')
Expand Down
61 changes: 48 additions & 13 deletions tests/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@
client.get_bucket_location.return_value = {"LocationConstraint": "us-east-1"}
client.head_object.return_value = {"ContentLength": 2048}

response = app.try_download_from_bucket("somebucket", "somefile", user_profile, {})
response = app.try_download_from_bucket("somebucket", "somefile", user_profile, {}, api_request_uuid=None)

Check failure on line 592 in tests/test_app.py

View workflow job for this annotation

GitHub Actions / test

test_try_download_from_bucket TypeError: too many positional arguments
client.head_object.assert_called_once()
assert response.body == ""
assert response.status_code == 303
Expand All @@ -606,7 +606,7 @@
monkeypatch.setenv("AWS_DEFAULT_REGION", "us-west-2")
client.head_object.reset_mock()

response = app.try_download_from_bucket("somebucket", "somefile", user_profile, "not a dict")
response = app.try_download_from_bucket("somebucket", "somefile", user_profile, "not a dict", api_request_uuid=None)
client.head_object.assert_not_called()
assert response.body == ""
assert response.status_code == 303
Expand Down Expand Up @@ -635,7 +635,7 @@
mock_get_role_creds.return_value = (mock.Mock(), 1000)
mock_get_role_session().client.side_effect = ClientError({}, "bar")

app.try_download_from_bucket("somebucket", "somefile", user_profile, {})
app.try_download_from_bucket("somebucket", "somefile", user_profile, {}, None)
mock_make_html_response.assert_called_once_with(
{
"contentstring": "There was a problem accessing download data.",
Expand Down Expand Up @@ -672,7 +672,7 @@
"bar"
)

app.try_download_from_bucket("somebucket", "somefile", user_profile, {})
app.try_download_from_bucket("somebucket", "somefile", user_profile, {}, None)
mock_make_html_response.assert_called_once_with(
{
"contentstring": "Could not find requested data.",
Expand Down Expand Up @@ -710,7 +710,7 @@
"bar"
)

response = app.try_download_from_bucket("somebucket", "somefile", user_profile, {})
response = app.try_download_from_bucket("somebucket", "somefile", user_profile, {}, None)
assert response.body == "Invalid Range"
assert response.status_code == 416
assert response.headers == {}
Expand Down Expand Up @@ -1197,13 +1197,15 @@


@mock.patch(f"{MODULE}.get_yaml_file", autospec=True)
@mock.patch(f"{MODULE}.get_api_request_uuid", autospec=True)
@mock.patch(f"{MODULE}.try_download_from_bucket", autospec=True)
@mock.patch(f"{MODULE}.JwtManager.get_profile_from_headers", autospec=True)
@mock.patch(f"{MODULE}.JWT_COOKIE_NAME", "asf-cookie")
@mock.patch(f"{MODULE}.b_map", None)
def test_dynamic_url(
mock_get_profile,
mock_try_download_from_bucket,
mock_get_api_request_uuid,
mock_get_yaml_file,
data_path,
user_profile,
Expand All @@ -1215,6 +1217,7 @@
mock_get_yaml_file.return_value = yaml.full_load(f)

mock_get_profile.return_value = user_profile
mock_get_api_request_uuid.return_value = None
current_request.uri_params = {"proxy": "DATA-TYPE-1/PLATFORM-A/OBJECT_1"}

# Can't use the chalice test client here as it doesn't seem to understand the `{proxy+}` route
Expand All @@ -1224,19 +1227,22 @@
"gsfc-ngap-d-pa-dt1",
"OBJECT_1",
user_profile,
{}
{},
None
)
assert response is MOCK_RESPONSE


@mock.patch(f"{MODULE}.get_yaml_file", autospec=True)
@mock.patch(f"{MODULE}.get_api_request_uuid", autospec=True)
@mock.patch(f"{MODULE}.try_download_from_bucket", autospec=True)
@mock.patch(f"{MODULE}.JwtManager.get_profile_from_headers", autospec=True)
@mock.patch(f"{MODULE}.JWT_COOKIE_NAME", "asf-cookie")
@mock.patch(f"{MODULE}.b_map", None)
def test_dynamic_url_public_unauthenticated(
mock_get_profile,
mock_try_download_from_bucket,
mock_get_api_request_uuid,
mock_get_yaml_file,
data_path,
current_request
Expand All @@ -1247,23 +1253,26 @@
mock_get_yaml_file.return_value = yaml.full_load(f)

mock_get_profile.return_value = None
mock_get_api_request_uuid.return_value = None
current_request.uri_params = {"proxy": "BROWSE/PLATFORM-A/OBJECT_2"}

# Can't use the chalice test client here as it doesn't seem to understand the `{proxy+}` route
response = app.dynamic_url()

mock_try_download_from_bucket.assert_called_once_with("gsfc-ngap-d-pa-bro", "OBJECT_2", None, {})
mock_try_download_from_bucket.assert_called_once_with("gsfc-ngap-d-pa-bro", "OBJECT_2", None, {}, None)
assert response is MOCK_RESPONSE


@mock.patch(f"{MODULE}.get_yaml_file", autospec=True)
@mock.patch(f"{MODULE}.get_api_request_uuid", autospec=True)
@mock.patch(f"{MODULE}.try_download_from_bucket", autospec=True)
@mock.patch(f"{MODULE}.JwtManager.get_profile_from_headers", autospec=True)
@mock.patch(f"{MODULE}.JWT_COOKIE_NAME", "asf-cookie")
@mock.patch(f"{MODULE}.b_map", None)
def test_dynamic_url_public_authenticated(
mock_get_profile,
mock_try_download_from_bucket,
mock_get_api_request_uuid,
mock_get_yaml_file,
data_path,
user_profile,
Expand All @@ -1275,23 +1284,26 @@
mock_get_yaml_file.return_value = yaml.full_load(f)

mock_get_profile.return_value = user_profile
mock_get_api_request_uuid.return_value = None
current_request.uri_params = {"proxy": "BROWSE/PLATFORM-A/OBJECT_2"}

# Can't use the chalice test client here as it doesn't seem to understand the `{proxy+}` route
response = app.dynamic_url()

mock_try_download_from_bucket.assert_called_once_with("gsfc-ngap-d-pa-bro", "OBJECT_2", user_profile, {})
mock_try_download_from_bucket.assert_called_once_with("gsfc-ngap-d-pa-bro", "OBJECT_2", user_profile, {}, None)
assert response is MOCK_RESPONSE


@mock.patch(f"{MODULE}.get_yaml_file", autospec=True)
@mock.patch(f"{MODULE}.get_api_request_uuid", autospec=True)
@mock.patch(f"{MODULE}.try_download_from_bucket", autospec=True)
@mock.patch(f"{MODULE}.JwtManager.get_profile_from_headers", autospec=True)
@mock.patch(f"{MODULE}.JWT_COOKIE_NAME", "asf-cookie")
@mock.patch(f"{MODULE}.b_map", None)
def test_dynamic_url_public_custom_headers(
mock_get_profile,
mock_try_download_from_bucket,
mock_get_api_request_uuid,
mock_get_yaml_file,
data_path,
current_request
Expand All @@ -1302,6 +1314,7 @@
mock_get_yaml_file.return_value = yaml.full_load(f)

mock_get_profile.return_value = None
mock_get_api_request_uuid.return_value = None
current_request.uri_params = {"proxy": "HEADERS/BROWSE/OBJECT_1"}

# Can't use the chalice test client here as it doesn't seem to understand the `{proxy+}` route
Expand All @@ -1314,12 +1327,14 @@
{
"custom-header-1": "custom-header-1-value",
"custom-header-2": "custom-header-2-value"
}
},
None
)
assert response is MOCK_RESPONSE


@mock.patch(f"{MODULE}.get_yaml_file", autospec=True)
@mock.patch(f"{MODULE}.get_api_request_uuid", autospec=True)
@mock.patch(f"{MODULE}.try_download_from_bucket", autospec=True)
@mock.patch(f"{MODULE}.user_in_group", autospec=True)
@mock.patch(f"{MODULE}.JwtManager.get_profile_from_headers", autospec=True)
Expand All @@ -1331,6 +1346,7 @@
mock_get_profile,
mock_user_in_group,
mock_try_download_from_bucket,
mock_get_api_request_uuid,
mock_get_yaml_file,
data_path,
user_profile,
Expand All @@ -1344,6 +1360,7 @@
mock_get_yaml_file.return_value = yaml.full_load(f)

mock_get_profile.return_value = user_profile
mock_get_api_request_uuid.return_value = None
current_request.uri_params = {"proxy": "PRIVATE/PLATFORM-A/OBJECT_2"}

# Can't use the chalice test client here as it doesn't seem to understand the `{proxy+}` route
Expand All @@ -1353,12 +1370,14 @@
"gsfc-ngap-d-pa-priv",
"OBJECT_2",
user_profile,
{"SET-COOKIE": "cookie"}
{"SET-COOKIE": "cookie"},
None
)
assert response is MOCK_RESPONSE


@mock.patch(f"{MODULE}.get_yaml_file", autospec=True)
@mock.patch(f"{MODULE}.get_api_request_uuid", autospec=True)
@mock.patch(f"{MODULE}.try_download_from_bucket", autospec=True)
@mock.patch(f"{MODULE}.user_in_group", autospec=True)
@mock.patch(f"{MODULE}.JwtManager.get_profile_from_headers", autospec=True)
Expand All @@ -1370,6 +1389,7 @@
mock_get_profile,
mock_user_in_group,
mock_try_download_from_bucket,
mock_get_api_request_uuid,
mock_get_yaml_file,
data_path,
user_profile,
Expand All @@ -1384,6 +1404,7 @@
mock_get_yaml_file.return_value = yaml.full_load(f)

mock_get_profile.return_value = user_profile
mock_get_api_request_uuid.return_value = None
current_request.uri_params = {"proxy": "HEADERS/PRIVATE/OBJECT_1"}

# Can't use the chalice test client here as it doesn't seem to understand the `{proxy+}` route
Expand All @@ -1397,19 +1418,22 @@
"custom-header-3": "custom-header-3-value",
"custom-header-4": "custom-header-4-value",
"SET-COOKIE": "cookie"
}
},
None
)
assert response is MOCK_RESPONSE


@mock.patch(f"{MODULE}.get_yaml_file", autospec=True)
@mock.patch(f"{MODULE}.get_api_request_uuid", autospec=True)
@mock.patch(f"{MODULE}.try_download_from_bucket", autospec=True)
@mock.patch(f"{MODULE}.JwtManager.get_profile_from_headers", autospec=True)
@mock.patch(f"{MODULE}.JWT_COOKIE_NAME", "asf-cookie")
@mock.patch(f"{MODULE}.b_map", None)
def test_dynamic_url_public_within_private(
mock_get_profile_from_headers,
mock_try_download_from_bucket,
mock_get_api_request_uuid,
mock_get_yaml_file,
current_request
):
Expand All @@ -1427,12 +1451,13 @@
}

mock_get_profile_from_headers.return_value = None
mock_get_api_request_uuid.return_value = None
current_request.uri_params = {"proxy": "FOO/BROWSE/OBJECT_1"}

# Can't use the chalice test client here as it doesn't seem to understand the `{proxy+}` route
response = app.dynamic_url()

mock_try_download_from_bucket.assert_called_once_with("gsfc-ngap-d-bucket", "BROWSE/OBJECT_1", None, {})
mock_try_download_from_bucket.assert_called_once_with("gsfc-ngap-d-bucket", "BROWSE/OBJECT_1", None, {}, None)
assert response is MOCK_RESPONSE


Expand Down Expand Up @@ -1503,6 +1528,7 @@


@mock.patch(f"{MODULE}.get_yaml_file", autospec=True)
@mock.patch(f"{MODULE}.get_api_request_uuid", autospec=True)
@mock.patch(f"{MODULE}.try_download_from_bucket", autospec=True)
@mock.patch(f"{MODULE}.JwtManager.get_profile_from_headers", autospec=True)
@mock.patch(f"{MODULE}.RequestAuthorizer._handle_auth_bearer_header", autospec=True)
Expand All @@ -1514,6 +1540,7 @@
mock_handle_auth_bearer_header,
mock_get_profile,
mock_try_download_from_bucket,
mock_get_api_request_uuid,
mock_get_yaml_file,
data_path,
user_profile,
Expand All @@ -1526,6 +1553,7 @@
mock_get_yaml_file.return_value = yaml.full_load(f)

mock_get_profile.return_value = None
mock_get_api_request_uuid.return_value = None
current_request.uri_params = {"proxy": "DATA-TYPE-1/PLATFORM-A/OBJECT_1"}
current_request.headers = {"Authorization": "bearer b64token"}

Expand All @@ -1536,7 +1564,8 @@
"gsfc-ngap-d-pa-dt1",
"OBJECT_1",
user_profile,
{"SET-COOKIE": "cookie"}
{"SET-COOKIE": "cookie"},
None
)
assert response.body == "Mock response"
assert response.status_code == 200
Expand Down Expand Up @@ -1687,3 +1716,9 @@
response = client.http.get("/profile", headers={"x-origin-request-id": "x_origin_request_1234"})

assert response.headers["x-origin-request-id"] == "x_origin_request_1234"


def test_get_api_request_uuid():
# Can't use the chalice test client here as it doesn't seem to understand the `{proxy+}` route
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this comment applies because the function being tested is a helper function and not a request handler. You wouldn't be able to test it in isolation with the chalice test client anyway.

response = app.get_api_request_uuid({"A-api-request-uuid": "test-uuid"})
assert response == "test-uuid"
27 changes: 24 additions & 3 deletions thin_egress_app/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,16 @@ def get_user_from_token(token):
return None


@with_trace()
def get_api_request_uuid(query_params):
api_request_uuid = None
if query_params is not None:
api_request_uuid = query_params.get("A-api-request-uuid")
if api_request_uuid is not None:
log.info("A-api-request-uuid query param is " + api_request_uuid)
return api_request_uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

api_request_uuid can just be local to the if statement.

Suggested change
api_request_uuid = None
if query_params is not None:
api_request_uuid = query_params.get("A-api-request-uuid")
if api_request_uuid is not None:
log.info("A-api-request-uuid query param is " + api_request_uuid)
return api_request_uuid
if query_params is not None:
api_request_uuid = query_params.get("A-api-request-uuid")
if api_request_uuid is not None:
log.info("A-api-request-uuid query param is " + api_request_uuid)
return api_request_uuid
return None



@with_trace()
def cumulus_log_message(outcome: str, code: int, http_method: str, k_v: dict):
k_v.update({
Expand Down Expand Up @@ -583,7 +593,7 @@ def get_user_ip():


@with_trace()
def try_download_from_bucket(bucket, filename, user_profile, headers: dict):
def try_download_from_bucket(bucket, filename, user_profile, headers: dict, api_request_uuid):
timer = Timer()
timer.mark()
user_id = None
Expand Down Expand Up @@ -668,7 +678,16 @@ def try_download_from_bucket(bucket, filename, user_profile, headers: dict):
redirheaders.update(headers)

# Generate URL
presigned_url = get_presigned_url(creds, bucket, filename, bucket_region, expires_in, user_id)
presigned_url = get_presigned_url(
creds,
bucket,
filename,
bucket_region,
expires_in,
user_id,
"GET",
api_request_uuid
)
s3_host = urlparse(presigned_url).netloc
log.debug("Presigned URL host was %s", s3_host)

Expand Down Expand Up @@ -1126,7 +1145,9 @@ def dynamic_url():
log.debug("timing for dynamic_url()")
timer.log_all(log)

return try_download_from_bucket(entry.bucket, entry.object_key, user_profile, custom_headers)
api_request_uuid = get_api_request_uuid(app.current_request.query_params)

return try_download_from_bucket(entry.bucket, entry.object_key, user_profile, custom_headers, api_request_uuid)


@app.route("/s3credentials", methods=["GET"])
Expand Down
Loading