-
Notifications
You must be signed in to change notification settings - Fork 4
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
Issue 486: Signed Signature Endpoint #499
Conversation
Codecov Report
@@ Coverage Diff @@
## epics/398/backend/485/get-files #499 +/- ##
==================================================================
Coverage ? 99.87%
==================================================================
Files ? 55
Lines ? 819
Branches ? 65
==================================================================
Hits ? 818
Misses ? 0
Partials ? 1
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
89d8e60
to
6d2d682
Compare
39b74d9
to
fbe4196
Compare
I'm getting an error trying to get to the backend after deploying to dev site. Checked the logs in cloud.gov and noticed this:
which is pointing to the walrus operator. this operator is introduced in python version 3.8. we noticed that dockerfile.dev referenced python version 3.7. is it possible that this is source of the trouble getting to the backend site? cc: @carltonsmith |
This seems very likely. We can either factor out the walrus operator, or update the version in the dockerfile. |
I'd recommend that we rollback the usage of that operator and opened a separate ticket to track upgrading Python: #782 |
this sounds good to me! @riatzukiza @jtwillis92 |
Since I'm picking up some of the QASP reviewing from @ADPennington this week, I met up with @riatzukiza and we looked at the PR together. We decided we want to make a couple of changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(See comment above.)
Opened this issue #793 to get us moved over to localstack for dev and CI instead of using the real AWS. We can either address this later or as part of this PR. |
Thanks for tracking #793 as a separate ticket @jtwillis92! One thing I'm wondering about @jtwillis92 @riatzukiza: If we choose to address simulating AWS later instead of addressing as part of this PR, what would be the best way to test the permissions logic around the Pre-Signed URLs? Test with live AWS S3 instances for now? Shift to more of a unit testing strategy? |
Opened #805 to address the need for a simulated AWS for local and CI environments. |
s3_client = client( | ||
's3', | ||
aws_access_key_id=settings.AWS_S3_ACCESS_KEY_ID, | ||
aws_secret_access_key=settings.AWS_S3_SECRET_ACCESS_KEY, | ||
region_name=settings.AWS_REGION_NAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s3_client = client( | |
's3', | |
aws_access_key_id=settings.AWS_S3_ACCESS_KEY_ID, | |
aws_secret_access_key=settings.AWS_S3_SECRET_ACCESS_KEY, | |
region_name=settings.AWS_REGION_NAME) | |
s3_client = get_s3_client() |
@@ -1,27 +1,73 @@ | |||
"""Check if user is authorized.""" | |||
import logging | |||
from boto3 import client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from boto3 import client |
from .serializers import ReportFileSerializer | ||
|
||
from rest_framework.decorators import action | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from tdpservice.clients import get_s3_client |
from .models import User | ||
from .serializers import ReportFileSerializer | ||
|
||
from rest_framework.decorators import action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: import organization
From PEP8:
Imports should be grouped in the following order:
Standard library imports.
Related third party imports.
Local application/library specific imports.
You should put a blank line between each group of imports.
Absolute imports are recommended, as they are usually more readable and tend to be better behaved (or at least give better error messages) if the import system is incorrectly configured
Additionally, alphabetization by module name is a bonus :D
Summary of Changes
Add an endpoint for the frontend to perform
get_object
andput_object
operations on s3 bucketsIn order for the frontend to get permission to upload or download files from S3, the backend must create a presigned url. These presigned URLs are valid to anyone who has them for a limited amount of time. This is done so the frontend does not need access to secret information for s3 (API key and secret).
Addresses issue #457
How to Test
The test.html should have a result similar to this:
Documentations
Documentation can be seen at
http://<BACKEND_DOMAIN>/swagger/
Deliverable 1: Accepted Features
As facilitator/product manager, @kniz-raft will decide if ACs are met from Raft's perspective.
Deliverable 2: Tested Code
Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
No security issues detected