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.

Sign up for GitHub

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

PR-3244 S3 credential fix #644

Merged
merged 2 commits into from
Oct 24, 2022
Merged

PR-3244 S3 credential fix #644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85b7e8b
Use compact JSON for IAM policy
reweeden Oct 21, 2022
becb60d
Update policy generator and return errors on empty policies
reweeden Oct 21, 2022
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
18 changes: 17 additions & 1 deletion lambda/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -976,6 +976,19 @@ def s3credentials():
policy = b_map.to_iam_policy(groups)
log.debug("policy: %s", policy)

if policy is None:
template_vars = {
"contentstring": "You do not have permission to access any data.",
"title": "Could not access data",
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 really matters but it seems weird to have contentstring before title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, unfortunately this is the order it's in everywhere. I'd love to refactor this whole file and fix all sorts of stuff like this, but last time I tried to do that I got blocked because people didn't like that refactoring would cause additional commits to be created in the history...

"requestid": get_request_id()
}
return make_html_response(
template_vars,
authorizer.get_success_response_headers(),
403,
"error.html"
)

app_name = app.current_request.headers.get("app-name", "")
role_session_name = get_role_session_name(user_profile.user_id, app_name)

Expand Down Expand Up @@ -1011,7 +1024,10 @@ def get_s3_credentials(user_id: str, role_session_name: str, policy: dict):
RoleSessionName=role_session_name,
ExternalId=user_id,
DurationSeconds=3600,
Policy=json.dumps(policy)
# NOTE: Policy max size is 2048 characters which is quite limiting.
# TODO(reweeden): We'll need to figure out how to accomodate large
# bucket maps that will push us over this limit.
Policy=json.dumps(policy, separators=(",", ":"))
)
return response["Credentials"]

Expand Down
2 changes: 1 addition & 1 deletion requirements/requirements.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@ cachetools
cfnresponse
chalice
flatdict
git+https://github.com/asfadmin/rain-api-core.git@6acd2cb943cb552c525bc5320297f62b812a33ba
git+https://github.com/asfadmin/rain-api-core.git@e1638eec08353651ed42105840ba6a80a7141ac2
netaddr
2 changes: 1 addition & 1 deletion requirements/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pyyaml==6.0
# via
# chalice
# rain-api-core
rain-api-core @ git+https://github.com/asfadmin/rain-api-core.git@6acd2cb943cb552c525bc5320297f62b812a33ba
rain-api-core @ git+https://github.com/asfadmin/rain-api-core.git@e1638eec08353651ed42105840ba6a80a7141ac2
# via -r requirements/requirements.in
readchar==4.0.3
# via inquirer
Expand Down
28 changes: 28 additions & 0 deletions tests/test_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,6 +1441,34 @@ def test_s3credentials_unauthenticated(
assert response.status_code == 301


@mock.patch(f"{MODULE}.get_yaml_file", autospec=True)
@mock.patch(f"{MODULE}.JwtManager.get_profile_from_headers", autospec=True)
@mock.patch(f"{MODULE}.b_map", None)
def test_s3credentials_no_permissions(
mock_get_profile,
mock_get_yaml_file,
mock_get_urs_creds,
mock_make_html_response,
user_profile,
client
):
mock_get_yaml_file.return_value = {}
mock_get_profile.return_value = user_profile

client.http.get("/s3credentials")

mock_make_html_response.assert_called_once_with(
{
"contentstring": "You do not have permission to access any data.",
"title": "Could not access data",
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, a little weird

"requestid": app.app.lambda_context.aws_request_id
},
{},
403,
"error.html"
)


@mock.patch(f"{MODULE}.boto3")
def test_get_s3_credentials(mock_boto3, monkeypatch):
monkeypatch.setenv("EGRESS_APP_DOWNLOAD_ROLE_INREGION_ARN", "aws:role:arn")
Expand Down