Skip to content
Browse files

Merge branch 'bugfix/ku-policy-on-multipart' into release/1.3

  • Loading branch information...
2 parents 0c9dff3 + fed9a9a commit 72e01d8ff4f33304d025de76385454f725ca889d @kellymclaughlin kellymclaughlin committed Mar 18, 2013
View
38 client_tests/python/boto_test.py
@@ -444,6 +444,42 @@ def test_transport_addr_policy(self):
self.assertEqual(response.reason, 'Forbidden')
+class MultipartUploadTestsUnderPolicy(S3ApiVerificationTestBase):
+
+ def test_small_strings_upload_1(self):
+ bucket = self.conn.create_bucket(self.bucket_name)
+ parts = ['this is part one', 'part two is just a rewording',
+ 'surprise that part three is pretty much the same',
+ 'and the last part is number four']
+ stringio_parts = [StringIO(p) for p in parts]
+ expected_md5 = md5.new(''.join(parts)).hexdigest()
+
+ key_name = str(uuid.uuid4())
+ key = Key(bucket, key_name)
+
+ ## anyone may PUT this object
+ policy = '''
+{"Version":"2008-10-17","Statement":[{"Sid":"Stmtaaa0","Effect":"Allow","Principal":"*","Action":["s3:PutObject"],"Resource":"arn:aws:s3:::%s/*","Condition":{"Bool":{"aws:SecureTransport":false}}}]}
+''' % bucket.name
+ self.assertTrue(bucket.set_policy(policy, headers={'content-type':'application/json'}))
+
+ upload = upload_multipart(bucket, key_name, stringio_parts)
+ actual_md5 = md5_from_key(key)
+ self.assertEqual(expected_md5, actual_md5)
+
+ ## anyone without https may not do any operation
+ policy = '''
+{"Version":"2008-10-17","Statement":[{"Sid":"Stmtaaa0","Effect":"Deny","Principal":"*","Action":["s3:PutObject"],"Resource":"arn:aws:s3:::%s/*","Condition":{"Bool":{"aws:SecureTransport":false}}}]}
+''' % bucket.name
+ self.assertTrue(bucket.set_policy(policy, headers={'content-type':'application/json'}))
+
+ try:
+ upload = upload_multipart(bucket, key_name, stringio_parts)
+ self.fail()
+ except S3ResponseError as e:
+ self.assertEqual(e.status, 403)
+ self.assertEqual(e.reason, 'Forbidden')
+
+
if __name__ == "__main__":
unittest.main()
-# unittest.main(defaultTest = 'BucketPolicyTest')
View
7 include/s3_api.hrl
@@ -31,15 +31,16 @@
| 's3:PutObject' | 's3:PutObjectAcl'
| 's3:PutObjectVersionAcl'
| 's3:DeleteObject' | 's3:DeleteObjectVersion'
- | 's3:ListMultipartUploadParts' %to be supported
- | 's3:AbortMultipartUpload' %to be supported
+ | 's3:ListMultipartUploadParts'
+ | 's3:AbortMultipartUpload'
%| 's3:GetObjectTorrent' we never do this
%| 's3:GetObjectVersionTorrent' we never do this
| 's3:RestoreObject'.
-define(SUPPORTED_OBJECT_ACTION,
[ 's3:GetObject', 's3:GetObjectAcl', 's3:PutObject', 's3:PutObjectAcl',
- 's3:DeleteObject' ]).
+ 's3:DeleteObject',
+ 's3:ListMultipartUploadParts', 's3:AbortMultipartUpload' ]).
-type s3_bucket_action() :: 's3:CreateBucket'
| 's3:DeleteBucket'
View
7 src/riak_cs_s3_policy.erl
@@ -372,11 +372,13 @@ eval_statement(#access_v1{method=M, target=T, req=Req, bucket=B, key=K} = _Acces
make_action(Method, Target) ->
case {Method, Target} of
{'PUT', object} -> {ok, 's3:PutObject'};
+ {'PUT', object_part} -> {ok, 's3:PutObject'};
{'PUT', object_acl} -> {ok, 's3:PutObjectAcl'};
{'PUT', bucket_acl} -> {ok, 's3:PutBucketAcl'};
{'PUT', bucket_policy} -> {ok, 's3:PutBucketPolicy'};
{'GET', object} -> {ok, 's3:GetObject'};
+ {'GET', object_part} -> {ok, 's3:ListMultipartUploadParts'};
{'GET', object_acl} -> {ok, 's3:GetObjectAcl'};
{'GET', bucket} -> {ok, 's3:ListBucket'};
{'GET', no_bucket } -> {ok, 's3:ListAllMyBuckets'};
@@ -386,6 +388,7 @@ make_action(Method, Target) ->
{'GET', bucket_uploads} -> {ok, 's3:ListBucketMultipartUploads'};
{'DELETE', object} -> {ok, 's3:DeleteObject'};
+ {'DELETE', object_part} -> {ok, 's3:AbortMultipartUpload'};
{'DELETE', bucket} -> {ok, 's3:DeleteBucket'};
{'DELETE', bucket_policy} -> {ok, 's3:DeleteBucketPolicy'};
@@ -394,15 +397,15 @@ make_action(Method, Target) ->
%% PUT Object includes POST Object,
%% including Initiate Multipart Upload, Upload Part, Complete Multipart Upload
{'POST', object} -> {ok, 's3:PutObject'};
+ {'POST', object_part} -> {ok, 's3:PutObject'};
%% same as {'GET' bucket}
{'HEAD', bucket} -> {ok, 's3:ListBucket'};
%% 400 (MalformedPolicy): Policy has invalid action
{'PUT', bucket} -> {ok, 's3:CreateBucket'};
- {'HEAD', _} ->
- {error, no_action}
+ {'HEAD', _} -> {error, no_action}
end.
eval_condition(Req, {AtomKey, Cond}) ->
View
2 src/riak_cs_wm_bucket_uploads.erl
@@ -55,7 +55,7 @@ malformed_request(RD,Ctx=#context{local_context=LocalCtx0}) ->
-spec authorize(#wm_reqdata{}, #context{}) -> {boolean() | {halt, non_neg_integer()}, #wm_reqdata{}, #context{}}.
authorize(RD, Ctx) ->
- riak_cs_wm_utils:bucket_access_authorize_helper(bucket_uploads, true, RD, Ctx).
+ riak_cs_wm_utils:bucket_access_authorize_helper(bucket_uploads, false, RD, Ctx).
%% @doc Get the list of methods this resource supports.
-spec allowed_methods() -> [atom()].
View
48 src/riak_cs_wm_object_upload.erl
@@ -61,51 +61,9 @@ authorize(RD, Ctx0=#context{local_context=LocalCtx0, riakc_pid=RiakPid}) ->
riak_cs_acl_utils:requested_access(Method, false),
LocalCtx = riak_cs_wm_utils:ensure_doc(LocalCtx0, RiakPid),
Ctx = Ctx0#context{requested_perm=RequestedAccess,local_context=LocalCtx},
- check_permission(Method, RD, Ctx, LocalCtx#key_context.manifest).
-
-%% @doc Final step of {@link forbidden/2}: Authentication succeeded,
-%% now perform ACL check to verify access permission.
--spec check_permission(atom(), #wm_reqdata{}, #context{}, lfs_manifest() | notfound) ->
- {boolean() | {halt, non_neg_integer()}, #wm_reqdata{}, #context{}}.
-check_permission('GET', RD, Ctx, notfound) ->
- {{halt, 404}, riak_cs_access_log_handler:set_user(Ctx#context.user, RD), Ctx};
-check_permission('HEAD', RD, Ctx, notfound) ->
- {{halt, 404}, riak_cs_access_log_handler:set_user(Ctx#context.user, RD), Ctx};
-check_permission(_, RD, Ctx=#context{requested_perm=RequestedAccess,local_context=LocalCtx}, Mfst) ->
- #key_context{bucket=Bucket} = LocalCtx,
- RiakPid = Ctx#context.riakc_pid,
- case Ctx#context.user of
- undefined ->
- User = CanonicalId = undefined;
- User ->
- CanonicalId = User?RCS_USER.canonical_id
- end,
- case Mfst of
- notfound ->
- ObjectAcl = undefined;
- _ ->
- ObjectAcl = Mfst?MANIFEST.acl
- end,
- case riak_cs_acl:object_access(Bucket,
- ObjectAcl,
- RequestedAccess,
- CanonicalId,
- RiakPid) of
- true ->
- %% actor is the owner
- AccessRD = riak_cs_access_log_handler:set_user(User, RD),
- UserStr = User?RCS_USER.canonical_id,
- UpdLocalCtx = LocalCtx#key_context{owner=UserStr},
- {false, AccessRD, Ctx#context{local_context=UpdLocalCtx}};
- {true, OwnerId} ->
- %% bill the owner, not the actor
- AccessRD = riak_cs_access_log_handler:set_user(OwnerId, RD),
- UpdLocalCtx = LocalCtx#key_context{owner=OwnerId},
- {false, AccessRD, Ctx#context{local_context=UpdLocalCtx}};
- false ->
- %% ACL check failed, deny access
- riak_cs_wm_utils:deny_access(RD, Ctx)
- end.
+
+ riak_cs_wm_utils:object_access_authorize_helper(object_part, false, RD, Ctx).
+
%% @doc Get the list of methods this resource supports.
-spec allowed_methods() -> [atom()].
View
63 src/riak_cs_wm_object_upload_part.erl
@@ -65,58 +65,21 @@ authorize(RD, Ctx0=#context{local_context=LocalCtx0, riakc_pid=RiakPid}) ->
RequestedAccess =
riak_cs_acl_utils:requested_access(Method, false),
LocalCtx = riak_cs_wm_utils:ensure_doc(LocalCtx0, RiakPid),
- Ctx = Ctx0#context{requested_perm=RequestedAccess,local_context=LocalCtx},
- check_permission(Method, RD, Ctx, LocalCtx#key_context.manifest).
+ Ctx = Ctx0#context{requested_perm=RequestedAccess, local_context=LocalCtx},
-%% @doc Final step of {@link forbidden/2}: Authentication succeeded,
-%% now perform ACL check to verify access permission.
--spec check_permission(atom(), #wm_reqdata{}, #context{}, lfs_manifest() | notfound) ->
- {boolean() | {halt, non_neg_integer()}, #wm_reqdata{}, #context{}}.
-check_permission(_, RD, Ctx=#context{requested_perm=RequestedAccess,
- local_context=LocalCtx}, Mfst) ->
- #key_context{bucket=Bucket} = LocalCtx,
- RiakPid = Ctx#context.riakc_pid,
- case Ctx#context.user of
- undefined ->
- User = CanonicalId = undefined;
- User ->
- CanonicalId = User?RCS_USER.canonical_id
- end,
- case Mfst of
- notfound ->
- case wrq:method(RD) of
- 'GET' ->
- %% Object ownership will be checked by
- %% riak_cs_mp_utils:do_part_common(), so we give blanket
- %% permission here.
- ObjectAcl = skip;
- _ ->
- ObjectAcl = undefined
- end;
+ %% Final step of {@link forbidden/2}: Authentication succeeded,
+ case {Method, LocalCtx#key_context.manifest} of
+ {'GET', notfound} ->
+ %% List Parts
+ %% Object ownership will be checked by riak_cs_mp_utils:do_part_common(),
+ %% so we give blanket permission here - Never check ACL but Policy.
+ riak_cs_wm_utils:object_access_authorize_helper(object_part, true, true, RD, Ctx);
+ {'HEAD', notfound} ->
+ {{halt, 404},
+ riak_cs_access_log_handler:set_user(Ctx#context.user, RD), Ctx};
_ ->
- ObjectAcl = Mfst?MANIFEST.acl
- end,
- case if ObjectAcl == skip -> true;
- true -> riak_cs_acl:object_access(Bucket,
- ObjectAcl,
- RequestedAccess,
- CanonicalId,
- RiakPid)
- end of
- true ->
- %% actor is the owner
- AccessRD = riak_cs_access_log_handler:set_user(User, RD),
- UserStr = User?RCS_USER.canonical_id,
- UpdLocalCtx = LocalCtx#key_context{owner=UserStr},
- {false, AccessRD, Ctx#context{local_context=UpdLocalCtx}};
- {true, OwnerId} ->
- %% bill the owner, not the actor
- AccessRD = riak_cs_access_log_handler:set_user(OwnerId, RD),
- UpdLocalCtx = LocalCtx#key_context{owner=OwnerId},
- {false, AccessRD, Ctx#context{local_context=UpdLocalCtx}};
- false ->
- %% ACL check failed, deny access
- riak_cs_wm_utils:deny_access(RD, Ctx)
+ %% Initiate/Complete/Abort multipart
+ riak_cs_wm_utils:object_access_authorize_helper(object_part, true, false, RD, Ctx)
end.
%% @doc Get the list of methods this resource supports.
View
42 src/riak_cs_wm_utils.erl
@@ -46,6 +46,7 @@
shift_to_owner/4,
bucket_access_authorize_helper/4,
object_access_authorize_helper/4,
+ object_access_authorize_helper/5,
bucket_owner/2
]).
@@ -525,16 +526,25 @@ is_acl_request(_) ->
-type halt_or_bool() :: {halt, pos_integer()} | boolean().
-type authorized_response() :: {halt_or_bool(), RD :: term(), Ctx :: term()}.
+
-spec object_access_authorize_helper(AccessType::atom(), boolean(),
RD::term(), Ctx::term()) ->
authorized_response().
-object_access_authorize_helper(AccessType, Deletable,
+object_access_authorize_helper(AccessType, Deletable, RD, Ctx) ->
+ object_access_authorize_helper(AccessType, Deletable, false, RD, Ctx).
+
+-spec object_access_authorize_helper(AccessType::atom(), boolean(), boolean(),
+ RD::term(), Ctx::term()) ->
+ authorized_response().
+object_access_authorize_helper(AccessType, Deletable, SkipAcl,
RD, #context{policy_module=PolicyMod,
local_context=LocalCtx,
riakc_pid=RiakPid}=Ctx)
when ( AccessType =:= object_acl orelse
+ AccessType =:= object_part orelse
AccessType =:= object )
- andalso is_boolean(Deletable) ->
+ andalso is_boolean(Deletable)
+ andalso is_boolean(SkipAcl) ->
#key_context{bucket=Bucket} = LocalCtx,
case translate_bucket_policy(PolicyMod, Bucket, RiakPid) of
@@ -547,10 +557,10 @@ object_access_authorize_helper(AccessType, Deletable,
%% so we can assume to bucket does not exist.
riak_cs_s3_response:api_error(no_such_bucket, RD, Ctx);
Policy ->
- check_object_authorization(AccessType, Deletable, Policy, RD, Ctx)
+ check_object_authorization(AccessType, Deletable, SkipAcl, Policy, RD, Ctx)
end.
-check_object_authorization(AccessType, Deletable, Policy,
+check_object_authorization(AccessType, Deletable, SkipAcl, Policy,
RD, #context{user=User,
policy_module=PolicyMod,
local_context=LocalCtx,
@@ -561,12 +571,15 @@ check_object_authorization(AccessType, Deletable, Policy,
RequestedAccess = requested_access_helper(AccessType, Method),
ObjectAcl = extract_object_acl(Manifest),
Access = PolicyMod:reqdata_to_access(RD, AccessType, CanonicalId),
- case {riak_cs_acl:object_access(Bucket,
- ObjectAcl,
- RequestedAccess,
- CanonicalId,
- RiakPid),
- PolicyMod:eval(Access, Policy)} of
+ Acl = case SkipAcl of
+ true -> true;
+ false -> riak_cs_acl:object_access(Bucket,
+ ObjectAcl,
+ RequestedAccess,
+ CanonicalId,
+ RiakPid)
+ end,
+ case {Acl, PolicyMod:eval(Access, Policy)} of
{true, false} ->
%% return forbidden or 404 based on the `Method' and `Deletable'
@@ -600,10 +613,12 @@ extract_canonical_id(undefined) ->
extract_canonical_id(?RCS_USER{canonical_id=CanonicalID}) ->
CanonicalID.
--spec requested_access_helper(object | object_acl, atom()) ->
+-spec requested_access_helper(object | object_part | object_acl, atom()) ->
acl_perm().
requested_access_helper(object, Method) ->
riak_cs_acl_utils:requested_access(Method, false);
+requested_access_helper(object_part, Method) ->
+ requested_access_helper(object, Method);
requested_access_helper(object_acl, Method) ->
riak_cs_acl_utils:requested_access(Method, true).
@@ -643,12 +658,13 @@ translate_bucket_policy(PolicyMod, Bucket, RiakPid) ->
authorized_response().
actor_is_owner_but_denied_policy(User, RD, Ctx, Method, Deletable)
when Method =:= 'PUT' orelse
- (Deletable andalso Method =:= 'DELETE') ->
+ Method =:= 'POST' orelse
+ (Deletable andalso Method =:= 'DELETE') ->
AccessRD = riak_cs_access_log_handler:set_user(User, RD),
riak_cs_wm_utils:deny_access(AccessRD, Ctx);
actor_is_owner_but_denied_policy(User, RD, Ctx, Method, Deletable)
when Method =:= 'GET' orelse
- (Deletable andalso Method =:= 'HEAD') ->
+ (Deletable andalso Method =:= 'HEAD') ->
{{halt, 404}, riak_cs_access_log_handler:set_user(User, RD), Ctx}.
-spec actor_is_owner_and_allowed_policy(User :: rcs_user(),

0 comments on commit 72e01d8

Please sign in to comment.
Something went wrong with that request. Please try again.