SFR-1865_S3APIAuthentication #282
Conversation
There was a problem hiding this comment.
Hi Dmitri, thanks so much for your work on this PR. I talked with Sean and David about this PR, and since I have some security concerns about it (which aren't related directly to the code you wrote here), we are going to wait to discuss this further/merge this until after Limited Access is out.
I do think we should think carefully about what authentication method we use for this service and that's one of the things I think could be a larger discussion before we merge this. My other questions are mostly about the scope of objects that this PR would allow access to, since I'm not sure what the AWS credentials in QA/production would allow in scope (they're different from the credentials I have). I think we should make sure before merging this that it does not allow retrieving file objects managed by other portfolios within Digital, unless the developers who manage those buckets are aware of the possibility.
Over all I think this looks like a good contribution that could provide some utility in the future, but I think we should hold off merging it until we feel good about our security approach.
|
|
||
| if not user or APIUtils.validatePassword(password, user.password, user.salt) is False: | ||
| return APIUtils.formatResponseObject( | ||
| 401, 'authResponse', {'message': 'invalid user/password'} |
There was a problem hiding this comment.
I only learned this myself a few weeks ago, but as part of the HTTP standard, an endpoint returning a 401 response should also add a WWW-Authenticate header describing of what kind of authentication the endpoint accepts (in this case, basic auth).
I think it will become a little more important since we will potentially be accepting different kinds of auth at different endpoints.
When I needed to implement this, I extended the FormatResponseObject in this PR to take a header object, so that is a potential approach you could take in the future.
There was a problem hiding this comment.
I appreciate the additional info on improving this validateToken method. This is definitely something we should discuss more in the future when we finalize our plans on the security of this endpoint.
…SFR-1865_S3APIAuthentication
This branch adds a new API and endpoint for the AWS S3 objects in DRB. Right now, there's only endpoint and it returns a unique, presigned link to an S3 object with user authentication based on the psql database. I wonder if I should have the user and password be based on something else besides the database so any feedback on that point specifically would be appreciated.