-
Notifications
You must be signed in to change notification settings - Fork 94
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
Setting ACLs via headers at PUT Object creation #631
Setting ACLs via headers at PUT Object creation #631
Conversation
I've also now been able to reproduce this with s3cmd. |
The core issue here is that we only look for 'canned' ACLs in HTTP headers, both for the normal PUT object resource and the |
Note to self: 'If you use these ACL specific headers, you cannot use x-amz-acl header to set a canned ACL.', from here. |
As far as I can tell, s3cmd actually doesn't correctly do this with S3 either. We should still support it though. |
This is going to slip to 1.5. WIP branch is at |
|
Returns HTTP 400:
|
Yes. See PUT bucket acl. |
Returns HTTP 400:
|
And if you specify all three:
you get the HTTP 400 unexpected content warning. |
We'll need to support the same acl headers in Initiate multipart upload. |
Previously, ACLs could only be set via canned-ACL headers, or through the XML document PUT to the ?acl subresource. S3 also supports ACLs being set through non-canned ACL headers, like `x-amz-grant-read` and `x-amz-grant-write`. The specifics of the syntax are described in each of the resources, like in [PUT Bucket](http://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketPUT.html). More information on ACLs can also be found on the [ACL Overview](http://docs.aws.amazon.com/AmazonS3/latest/dev/ACLOverview.html). With this change, there are now three ways to specify an ACL for a bucket or object: 1. a canned-ACL header 2. specific ACL-grants in the headers 3. an XML document PUT to the ACL subresource Only _one_ of these can be provided on a given request. HTTP 400 will be returned if the user provides more than one of these ACLs, with an error message equivalent to what S3 provides. This change applies to the following resources: * riak_cs_wm_object * riak_cs_wm_object_acl * riak_cs_wm_bucket * riak_cs_wm_bucket_acl * riak_cs_wm_object_upload (initiate multi-part upload) The following lists several other notable changes: * Along the way, this commit also fixes a bug in `riak_cs_acl_utils:add_grant/2`: `lists:splitwith` only splits on the first occurrence. `lists:partition` provides the expected behavior here. An EQC test has been added. * Remove unused record field This record is never persisted, to changing the field would only matter for live-code changes, which we tend not to do. * Add riak_test and tests to both the Clojure and Python test suites.
@@ -92,8 +93,7 @@ | |||
api :: atom() | |||
}). | |||
|
|||
-record(key_context, {context :: #context{}, |
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.
Nice catch. I didn't even know it was never used.
I'll take this review, but the time is up for today. Will resume tomorrow.
Here's one high-level qestion: does the ACL thing have nothing to do with Swift API? |
Nope. |
Note: in R16B03 Compiled test/twop_set_eqc.erl
test/riak_cs_get_fsm_pulse.erl:none: internal error in lint_module;
crash reason: {function_clause,
[{erl_internal,bif,
[{atom,50,setup},{integer,50,0}],
[{file,"erl_internal.erl"},{line,248}]},
{erl_lint,expr,3,[{file,"erl_lint.erl"},{line,2018}]},
{erl_lint,'-expr_list/3-fun-0-',3,
[{file,"erl_lint.erl"},{line,2151}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1248}]},
{erl_lint,expr_list,3,[{file,"erl_lint.erl"},{line,2150}]},
{erl_lint,'-expr_list/3-fun-0-',3,
[{file,"erl_lint.erl"},{line,2151}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1248}]},
{erl_lint,expr_list,3,
[{file,"erl_lint.erl"},{line,2150}]}]}
ERROR: eunit failed while processing /home/kuenishi/src/riak_cs: rebar_abort |
One new warning seems to appear with this branch:
Reid: fixed in 2f32ef8 |
eqc_test_() -> | ||
{spawn, | ||
[ | ||
{timeout, 60, ?_assertEqual(true, quickcheck(numtests(?TEST_ITERATIONS, ?QC_OUT(prop_add_grant_idempotent()))))} |
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.
Trivial, but maybe this line is too long?
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.
I agree it's long, but I've always had trouble in Erlang getting long lines to be readable once wrapped. This is what I came up with, but I think it's quite a bit less readable. Thoughts?
So here comes another question; probably we don't need but just want to be sure. Do we do nny canonical id validation when PUTting ACL with "id=deadbeef01010101badbadbad" style? Other part of the code looks nice and straightforward though I'm not a good clojure reader. |
I've been using the basho-patched R16B02. |
Fixes dialyzer error
The test was written, and passing, but it wasn't being run because it wasn't in the test suite description.
Prevent starts_and_ends_with_quotes from raising an exception by calling `hd` on an empty list. Further, ensure at least one character could be between the quotes.
+1 to merge. Nice work! (Confirmed that Note that this code will conflict against #761 . Please merge with care. |
I still have to fix the test that wasn't included, too. I'll probably just end up rebasing and squashing this whole branch against develop one more time before merging too. |
This test was previously written, but just not run
Blagh, looks like 3be0660 doesn't work actually. Will look for a fix now. |
So, the issue I'm running into is that not all of the resources use the |
Quick note before reading diffs/conflicts. I added |
One possible, but not-so-beautiful, fix is (I don't execute/test it):
|
Yeah, that was basically my thought as well. Hmm. |
What are the downsides of putting the |
Put more concretely, what i'm suggesting is moving |
No strong objection :) I decided to use
So if I include
|
Just an idea: instead of using raw riak object, holding a record (brand new one or |
+1 to this idea. So far that's enough to proceed other works - but is there any reason why ACL is stored as metadata, not bucket object itself? To do a sole refactoring, we'd better move from |
Yeah, @shino makes some good points. Not all resources that have a context operate on just a single bucket (though there is already a bucket field in the context now (facepalm)). Maybe I will just make a separate local_context for |
Alright, still need to squash and stuff, but feeling pretty good about the changes here. Not the most elegant solution, but it passes the tests, and could be worse. |
Those changes looks pretty good to me. |
Dialyzer, riak_test, xref, eunit, pulse, all tests are also green to me. A newly squashed branch named
|
Alright, what I intend to be the final branch/commit has been pushed here. It's just been squashed from the previous branch. |
+1 to that branch ( |
Merged in 3039694. |
Originally reported by Eugene Doudine via the riak-users mailing list.
Attempts to set ACLs via
x-amz-grant-*
headers during object creation (PUT
) are ignored. Setting them after an object is created via the XML body of aPUT
with the query string parameter?acl
appended works.Test case using the AWS Ruby SDK:
Debugging output of the above script running against a local Riak CS instance with two active users: