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

fix: Check if AWS S3 bucket is public before generate signed url (M2-8020) #1640

Conversation

rcmerlo
Copy link
Contributor

@rcmerlo rcmerlo commented Oct 24, 2024

  • Tests for the changes have been added
  • Related documentation has been added / updated
  • For new features, QA automation engineers have been tagged
  • OSS packages added to MindLogger open source credit page

📝 Description

AWS S3 is returning http 403 error when signing a public S3 bucket, so this code add a check for publicity of the bucket before generated signed url

🔗 Jira Ticket M2-8020

🪤 Peer Testing

Requires pipenv sync --dev --system

Copy link

github-actions bot commented Oct 24, 2024

➡️ Preview environment failed to be destroyed

Copy link

❌ E2E tests failed

Copy link
Contributor

@AlejandroCoronadoN AlejandroCoronadoN left a comment

Choose a reason for hiding this comment

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

I couldn't validate cdn_client.py on the debugger but I review the code and aws bucket policies and the code seems correct. I also used test_presign_answer_url test to review the code but it requires to match legacy_file_url_pattern. After this requirement we need to pass a couple of validations that looked correct and then configure the client and _generate_public_url. After this I couldn't look further as I needed to setup an S3 bucket with the required settings and policies. I'm trying to setup S3 but as I said the code inside cdn_client.py it's accurate.

Copy link
Contributor

@mbanting mbanting left a comment

Choose a reason for hiding this comment

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

I tested this by pointing a local Admin Panel to a local backend (on this branch), pointing to UAT DB, UAT s3 and prod legacy S3 with VPN.

I couldn't find the right set of env vars to successfully fetch both UAT and legacy media files, but was able to get it configured to download the legacy media files successfully. I'm going to assume it's my local configuration vs this implementation if you were able to successfully test it and both UAT and legacy media files were exported @rcmerlo.

@rcmerlo rcmerlo merged commit 6b158cc into develop Oct 30, 2024
13 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants