Add option to generate a presigned url with a expiration time

[danielsanfr]

Hello people.

This PR solves the issue: https://github.com/parse-community/parse-server-s3-adapter/issues/78

Let me know if everything is right or I need to do something else to accept this PR.

Unfortunately I can't test at DigitalOcean Spaces.

[ghost]

Danger run resulted in 1 warning; to find out more, see the checks page.

Generated by :no_entry_sign: dangerJS

[danielsanfr]

@mtrezza I did 2 tests :wink: .

[danielsanfr]

Hello people.

Let me know if I need to do anything else so that you can merge this PR.

[davimacedo]

I think we can merge. @mtrezza do you think it is good now?

[mtrezza]

Just my comment on the parameters table thread.

[mtrezza]

I have added the parameter to the table and and improved the descriptions.

Now I wonder if we should add an additional test that ensures that the presigned URL only presigns for a getObject operation. Given the implications of presigned URLs, if there is a change in the code the presigned URL could give wide access to the S3 bucket, as described in the docs.

What's your opinion on that?

[danielsanfr]

This can really be a problem.

I tried to generate a URL without passing a key in the hope of gaining access to the entire bucket, but the AWS SDK throws an exception. I also tried to put only one prefix as a key to try to access an entire "folder" from within the bucket, but when I open the URL the answer is that the key does not exist.

So, in case you read the bucket or folder, I don't see it as a problem.

Now for the case of writing to a file (or overwriting, via putObject operation), according to the documentation it can be a problem, but I don't know if I did the tests correctly locally, but the generated URL has no "visible" information that references the operation it allows.

That said, I may be mistaken, but I only see 2 "options" (that can be done together):

1. Put a comment in the code saying that this operation should not be changed within this method;
2. Create a variable or constant and create a test to validate that it has the value (getObject);

They are just suggestions, let me know if something really needs to be done or if we should make my suggestions or if you have any other ideas?

[mtrezza]

My two concerns are:

• the Parse Server S3 Adapter code is changed and getObject is not set as allowed operation by mistake
• the AWS SDK has a breaking change in the future and after upgrade the operations parameter is ignored and needs to be supplied in a different way

To address this I suggest to:

• add a comment in our code
• add a test case to spy on a method deep enough in the AWS SDK, preferably after some validation if there is any, ensuring that it receives getObject as allowed operation

[danielsanfr]

Hello.

Could you add the comment to the code?

Regarding the test, let me know if this test is enough for you: https://github.com/danielsanfr/parse-server-s3-adapter/commit/8266a66a0de25367e5ef7f303701d655f75ef10d . If so, I'll make the git cherry-pick.

[mtrezza]

The operation check should be a separate test case with a description that explains specifically what it is testing, to give this more significance.

Also, can you spy on a method within the S3 adapter and just call through or mock it?

The comment can just be a simple one that explains why the operation is specified and removing it can provide broad access to the S3 bucket, maybe with a like to the AWS S3 docs security section where this is explained.

[danielsanfr]

Hello guys. I hope everyone is okay.

@mtrezza, I separated the test for the operation used and added the comment about security concerns in the code. I don't know how to "spy" on a method inside the S3 library, and I sincerely believe that it is not necessary.

I wonder if you can approve this PR or we still need to make more changes?

[mtrezza]

@danielsanfr Apologies for the long wait time, I have overlooked this in my notifications.

[dblythy]

@danielsanfr I have been using this branch on my project, and it's working well. Thanks for your efforts!!

[mtrezza]

@danielsanfr Could you address the open questions so we can merge this? I think there is just some minor refactoring left.

[mtrezza]

@danielsanfr @dblythy Could anyone do this minor refactor? Then we can merge this feature and make is available officially. And the PR needs a rebase I think.