Handle Errors & Object Key Naming

[GetContented]

• Makes the s3-pipe put a response map of interpreted data from S3 onto its report-chan, instead of just the file object or location.
• Makes so that the s3-pipe can have either a map or a file put onto its upload-file chan: in the case of the map, it will have a :file key that contains the file, and will later be adjusted to allow an "object key" key to be passed through as well. Add an identifier key to this map for later passing back on the response channel - for identification by the user code that handles the response - also pass the identifier back thru the upload-file function, too.
• Make so that the sign-file handles errors and reports the errors back, and so that the upload-file function passes these errors through
• Make so that upload-file function handles errors of its own - such as http errors, and response codes from AWS, and reports these back on the response map
• Make so that s3-pipe can have a key-fn passed to it which will be used to generate file keys before signing
• Make it so that the upload-file chan’s map can have an object key which will be used for the file key before signing which is preferred to the key-fn

[martinklepsch]

Thanks for working on this, looks like you made some good progress :)

After thinking more about it I think the original reason to have the key-fn on the server was security. I.e. if you have that function in the client people can theoretically upload to any location they want inside your bucket. With that in mind I think key-fn should be part of the server.

I'll add a few more comments inline.

[martinklepsch]

@GetContented What do you think about the security concern I described in the first comment here?

[GetContented]

Oh sorry I missed the security concern. I think a proper addressing of this would be to use the policy... that is... specifically, the starts-with portion.

The server should authorise this as part of the policy.

[GetContented]

@martinklepsch Do you have any other further comments / adjustments? AFAICS I've done all the things you've commented on.

[martinklepsch]

@GetContented using the policy to enforce security seems like the right way, thanks.

We'll need to update the readme and the changelog and probably we should add some section about how to configure the policy correctly.

Besides that there are still a few things I want to think about plus ideally get some feedback from others.

[GetContented]

Sure no worries. I'm using this in my app.

Configuring the AWS backend stuff is SUCH a pain - took me like 3 hours yesterday. Hate it. Ended up finding the answers I needed in an external toolkit rather than with the AWS website. (I'm using Rails on the backend, not Clojure, unfortunately).

I'd still like to do the upload progress, too, but no word from @swannodette on getting cljs updated. I'll take that discussion to https://github.com/martinklepsch/s3-beam/issues/15 and reopen it.

[bensu]

Two of the proposed changes covers the reasons I had to diverge from master but I never found the time to clean up and merge back. I'm all for them.

What I didn't like:

• I agree that the key-fn should be in the server, I'm currently using (rand-uuid) but I wouldn't want a client to decide where to write and potentially overwrite another file.
• I don't see why edn-ize is a required parameter. It might be better to have a sensible default and let the user specify otherwise.

[GetContented]

@bensu I think I adressed the issue of key-fn being not on the server. The S3 policy can enforce this constraint.

I don't know why you can't just override the filename at the server if that's an issue for you. Having it on the client provides the most flexibility.

edn-ize is only a required parameter to the sign-file function - it's optional on the s3-pipe function opts, which merges the sensible default in. I'm just using the code that was in place originally there.

This should be documented.

[bensu]

@GetContented my bad then :) I agree that having key-fn in the client provides most flexibility but it seems like a foot-gun since it is by default unsafe. I will not suffer it, but somebody else might.

In any case, if those concerns were addressed, I'm all for the change, since it will effectively cover all my needs.

[GetContented]

@bensu Fair enough - I really don't mind... by making the server have good defaults (ie using a path in the policy in the examples) this will probably "side-step" that issue.

The main reason I wanted it in the client is to save another round-trip to the database. (in other words, I currently create a record in the DB with the generated file name from the client, that returns an ID, and I use that combined with the path to build my object key). If I didn't have the client option, it would mean I'd have to save the record, sign & upload the file, then save the filename to that initial record again.

(Also, by the way, before the PR, it's by default unsafe, too - when I first tested the code it uploaded to the root! I think the server needs to check the user's auth and set the object begins-with policy to whatever is appropriate for that user).

[martinklepsch]

This changes a bunch of things and those things should probably be documented in the changelog. I don't have time or incentive to prepare the changelog myself right now but once we have a proper changelog + commits are squashed I'm happy to merge this.

Sorry for letting this sit so long.

/cc @kennyjwilli

[GetContented]

Hi. I don't know how to do those things, unfortunately. Any help would be appreciated (or if someone else could do this, it would be grand). I'm not working on the clojurescript version of my software much at the moment, so I don't have any time or incentive either.

Maybe I should just close this and we won't bother?

[kennyjwilli]

I agree with @GetContented. The key-fn can be on the client to provide the most flexibility. The path should always be verified (and modified if needed) server side. I will take a look at the PR and see what I can do.

[kennyjwilli]

@martinklepsch and @GetContented I have updated #24 to include all changes. I believe this merge will satisfy all parties. @martinklepsch please review.

[GetContented]

LGTM