Merge pull request #1178 from basho/bugfix/multiple-ip-in-policy

Fix policy parser to accept multiple IP address per statement [JIRA: RCS-222] Reviewed-by: shino
basho · Jul 31, 2015 · f1e5b14 · f1e5b14
2 parents 0574683 + 0ec4bee
commit f1e5b14
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 19 deletions.
diff --git a/src/riak_cs_s3_policy.erl b/src/riak_cs_s3_policy.erl
@@ -531,8 +531,8 @@ eval_ip_address(Req, Conds) ->
         {error, _} ->
             false;
         {Peer, _} ->
-            IPConds = [ IPCond || {'aws:SourceIp', IPCond} <- Conds ],
-            eval_all_ip_addr(IPConds, Peer)
+            IPConds = [IPCond || {'aws:SourceIp', IPCond} <- Conds ],
+            eval_all_ip_addr(lists:flatten(IPConds), Peer)
     end.
 
 eval_all_ip_addr([], _) -> false;
@@ -568,7 +568,10 @@ statement_to_pairs(#statement{sid=Sid, effect=E, principal=P, action=A,
 
 -spec condition_block_from_condition_pair(condition_pair()) -> {binary(), list()}.
 condition_block_from_condition_pair({AtomKey, Conds})->
-    Fun = fun({'aws:SourceIp', IP}) -> {'aws:SourceIp', print_ip(IP)};
+    Fun = fun({'aws:SourceIp', IPs}) when is_list(IPs) ->
+                  {'aws:SourceIp', [print_ip(IP) || IP <- IPs]};
+              ({'aws:SourceIp', IP}) when is_tuple(IP) ->
+                  {'aws:SourceIp', print_ip(IP)};
              (Cond) -> Cond
           end,
     {atom_to_binary(AtomKey, latin1),  lists:map(Fun, Conds)}.
@@ -794,6 +797,14 @@ condition_({<<"aws:SourceIp">>, Bin}) when is_binary(Bin)->
         IP ->
             {'aws:SourceIp', IP}
     end;
+condition_({<<"aws:SourceIp">>, BinIPs}) when is_list(BinIPs)->
+    IPs = [case parse_ip(Bin) of
+               {error, _} ->
+                   throw({error, malformed_policy_condition});
+               IP ->
+                   IP
+           end || Bin <- BinIPs],
+    {'aws:SourceIp', IPs};
 condition_({<<"aws:UserAgent">>, Bin}) -> % TODO: check string condition
     {'aws:UserAgent', Bin};
 condition_({<<"aws:Referer">>, Bin}) -> % TODO: check string condition

diff --git a/src/riak_cs_wm_utils.erl b/src/riak_cs_wm_utils.erl
@@ -741,6 +741,7 @@ handle_policy_eval_result(_, true, OwnerId, RD, Ctx) ->
     %% Policy says yes while ACL says no
     shift_to_owner(RD, Ctx, OwnerId, Ctx#context.riak_client);
 handle_policy_eval_result(User, _, _, RD, Ctx) ->
+    %% Policy says no
     #context{riak_client=RcPid,
              response_module=ResponseMod,
              user=User,

diff --git a/test/riak_cs_s3_policy_eqc.erl b/test/riak_cs_s3_policy_eqc.erl
@@ -33,25 +33,30 @@
 -include_lib("webmachine/include/wm_reqdata.hrl").
 
 -define(TEST_ITERATIONS, 500).
--define(SET_MODULE, twop_set).
 -define(QC_OUT(P),
         eqc:on_output(fun(Str, Args) -> io:format(user, Str, Args) end, P)).
 
+-define(TIMEOUT, 60).
+
 eqc_test_()->
-    {spawn,
+    {inparallel,
      [
-      {timeout, 20, ?_assertEqual(true,
-                                  quickcheck(numtests(?TEST_ITERATIONS,
-                                                      ?QC_OUT(prop_ip_filter()))))},
-      {timeout, 20, ?_assertEqual(true,
-                                  quickcheck(numtests(?TEST_ITERATIONS,
-                                                      ?QC_OUT(prop_secure_transport()))))},
-      {timeout, 20, ?_assertEqual(true,
-                                  quickcheck(numtests(?TEST_ITERATIONS,
-                                                      ?QC_OUT(prop_eval()))))},
-      {timeout, 20, ?_assertEqual(true,
-                                  quickcheck(numtests(?TEST_ITERATIONS,
-                                                      ?QC_OUT(prop_policy_v1()))))}
+      {timeout, ?TIMEOUT,
+       ?_assertEqual(true,
+                     quickcheck(numtests(?TEST_ITERATIONS,
+                                         ?QC_OUT(prop_ip_filter()))))},
+      {timeout, ?TIMEOUT,
+       ?_assertEqual(true,
+                     quickcheck(numtests(?TEST_ITERATIONS,
+                                         ?QC_OUT(prop_secure_transport()))))},
+      {timeout, ?TIMEOUT,
+       ?_assertEqual(true,
+                     quickcheck(numtests(?TEST_ITERATIONS,
+                                         ?QC_OUT(prop_eval()))))},
+      {timeout, ?TIMEOUT,
+       ?_assertEqual(true,
+                     quickcheck(numtests(?TEST_ITERATIONS,
+                                         ?QC_OUT(prop_policy_v1()))))}
      ]}.
 
 %% accept case of ip filtering
@@ -182,11 +187,14 @@ condition_pair() ->
 %%           {date_condition(),    [{'aws:CurrentTime', binary_char_string()}]},
 %%           {numeric_condition(), [{'aws:EpochTime', nat()}]},
            {'Bool',              [{'aws:SecureTransport', bool()}]},
-           {ip_addr_condition(), [{'aws:SourceIp',  ip_with_mask()}]}
+           {ip_addr_condition(), [{'aws:SourceIp',  one_or_more_ip_with_mask()}]}
 %%           {string_condition(),  [{'aws:UserAgent', binary_char_string()}]},
 %%           {string_condition(),  [{'aws:Referer',   binary_char_string()}]}
           ]).
 
+one_or_more_ip_with_mask() ->
+    oneof([ip_with_mask(), non_empty(list(ip_with_mask()))]).
+
 %% TODO: FIXME: add a more various form of path
 path() ->
     "test/*".

diff --git a/test/riak_cs_s3_policy_test.erl b/test/riak_cs_s3_policy_test.erl
@@ -84,7 +84,7 @@ sample_plain_allow_policy()->
       "     \"s3:PutObjectAcl\""
       "   ],"
       "   \"Condition\":{"
-      "     \"IpAddress\": { \"aws:SourceIp\":\"192.168.0.1/8\" }"
+      "     \"IpAddress\": { \"aws:SourceIp\":[\"192.168.0.1/8\", \"192.168.0.2/17\"] }"
       "   },"
       "   \"Effect\": \"Allow\","
       "   \"Resource\": \"arn:aws:s3:::test\","