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

Generic post method to actually allow uploading files #333

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@codyseibert
Copy link

commented Oct 30, 2018

This PR adds support for uploading a file directly to the bucket via a multipart form post.

Overview

The createPresignedPost method is an AWS s3 method which creates a presigned url and fields which can be used for posting an s3 object directly to a private bucket

An example of calling this method:

s3.createPresignedPost({
  Bucket: 'my_bucket',
  Conditions: [
    ['starts-with', '$key', ''],
  ],
}, (err, data) => {
  if (err) return reject(err);
  resolve(data);
});

which returns something like this:

{
  "url": "http://localhost:9000/my_bucket",
  "fields": {
    "bucket": "my_bucket",
    "X-Amz-Algorithm": "AWS4-HMAC-SHA256",
    "X-Amz-Credential": "XXX/us-east-1/s3/aws4_request",
    "X-Amz-Date": "20181030T043058Z",
    "X-Amz-Security-Token": "XXX",
    "Policy": "XXX",
    "X-Amz-Signature": "XXX"
  }
}

Normally, you can just use the URL returned in the response in a multipart form and upload the file to the bucket. Currently, s3rver has no support for this upload method. This PR adds that support.

@leontastic

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2018

@codyseibert Thank you for taking the time to contribute a PR! Can you add some test cases for this functionality? I'm happy to review this PR, but we need tests proving your change provides the expected results you described.

adding necessary code to support uploading files directly to the buck…
…et using the policy which was generated using the createPresignedPost method
@codyseibert

This comment has been minimized.

Copy link
Author

commented Nov 13, 2018

@leontastic is this PR good now sir?

@leontastic
Copy link
Collaborator

left a comment

@codyseibert Thanks for taking the time to add some tests! I have reviewed your PR, please take a look at my comments.

My biggest concern is the possibility of the actual file contents being uploaded in parts, overwriting each other at the same bucket + key. Write a test to prove that a large file upload doesn't get corrupted – if it passes then all is good, if it fails then you will need to rewrite this logic to concatenate file parts together.

Also, please run prettier-fix npm script before committing and pushing, this will resolve style inconsistencies with the rest of the repo.

bucket: req.params.bucket
};
const form = new multiparty.Form();
let chain = Promise.resolve();
form.on("part", part => {
if (!part.filename) {
chain = chain.then(() => {
return new Promise(resolve => {

This comment has been minimized.

Copy link
@leontastic

leontastic Nov 14, 2018

Collaborator

This is an anti-pattern:

Promise.resolve()
  .then(() => new Promise(resolve => {
    /* do something */
    resolve()
  }))

Since you don't return or use this promise, you should remove it entirely:

if (!part.filename) {
  let string = "";
  part
    .on("data", buf => { string += buf.toString() })
    .on("end", () => { fields[part.name] = string; })
}
} else {
// 'file' must be the last field in the post (this is how aws requires it)
fileWasFound = true;
chain = chain.then(() => {

This comment has been minimized.

Copy link
@leontastic

leontastic Nov 14, 2018

Collaborator

Please remove this

},
content: part
},
(err, md5) => {

This comment has been minimized.

Copy link
@leontastic

leontastic Nov 14, 2018

Collaborator

Please handle this error

});
});
} else {
// 'file' must be the last field in the post (this is how aws requires it)

This comment has been minimized.

Copy link
@leontastic

leontastic Nov 14, 2018

Collaborator

Can you elaborate on this comment? I don't see any reference to a file field in the code

});

form.on("part", part => {
if (!part.filename) {

This comment has been minimized.

Copy link
@leontastic

leontastic Nov 14, 2018

Collaborator

It took me a while to realized this condition filters out the parts containing the actual file upload. Maybe add comments like // parse request fields in this block and // parse file upload parts in the else block?

fileWasFound = true;
chain = chain.then(() => {
if (!fields.bucket) {
return res.status(405).send(`

This comment has been minimized.

Copy link
@leontastic

leontastic Nov 14, 2018

Collaborator

👍early return

const data = yield s3Client
.getObject({ Bucket: buckets[0], Key: key })
.promise();
expect(data.ContentType).to.equal("binary/octet-stream");

This comment has been minimized.

Copy link
@leontastic

leontastic Nov 14, 2018

Collaborator

Can you add another expectation checking that the contents match? It would provide an important guarantee that the file is not corrupted during upload – no worries if it's too difficult, just reply in a comment.

error = err;
expect(err.response.statusCode).to.equal(400);
expect(err.response.body).to.contain(
"POST must contain a field named 'key'"

This comment has been minimized.

Copy link
@leontastic

leontastic Nov 14, 2018

Collaborator

Can you check for the response strings exactly instead of using contain?

error = err;
expect(err.response.statusCode).to.equal(400);
expect(err.response.body).to.contain(
"POST requires exactly one file upload per request."

This comment has been minimized.

Copy link
@leontastic

leontastic Nov 14, 2018

Collaborator

Can you check for the response strings exactly instead of using contain?

codyseibert added some commits Nov 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.