Handle well-indented XML including comments

src/riak_cs_acl_utils.erl

@@ -43,8 +43,6 @@
          validate_acl/2
         ]).
 
--type xmlElement() :: #xmlElement{}.
--type xmlText() :: #xmlText{}.
 
 %% ===================================================================
 %% Public API
@@ -83,12 +81,16 @@ canned_acl(HeaderVal, Owner, BucketOwner) ->
 %% @doc Convert an XML document representing an ACL into
 %% an internal representation.
 -spec acl_from_xml(string(), string(), pid()) -> {ok, #acl_v2{}} |
-                                                 {error, 'invalid_argument'} |
-                                                 {error, 'unresolved_grant_email'}.
+    {error, 'invalid_argument'} |
+    {error, 'unresolved_grant_email'} |
+    {error, 'malformed_acl_error'}.
 acl_from_xml(Xml, KeyId, RiakPid) ->
-    {ParsedData, _Rest} = xmerl_scan:string(Xml, []),
-    BareAcl = ?ACL{owner={[], [], KeyId}},
-    process_acl_contents(ParsedData#xmlElement.content, BareAcl, RiakPid).
+    case riak_cs_xml:scan(Xml) of
+        {error, malformed_xml} -> {error, malformed_acl_error};
+        {ok, ParsedData} ->
+            BareAcl = ?ACL{owner={[], [], KeyId}},
+            process_acl_contents(ParsedData#xmlElement.content, BareAcl, RiakPid)
+    end.
 
 %% @doc Convert an internal representation of an ACL
 %% into XML.
@@ -257,16 +259,16 @@ name_for_canonical(CanonicalId, RiakPid) ->
     end.
 
 %% @doc Process the top-level elements of the
--spec process_acl_contents([xmlElement()], acl(), pid()) ->
-                                  {ok, #acl_v2{}} |
-                                  {error, invalid_argument} |
-                                  {error, unresolved_grant_email}.
+-spec process_acl_contents([riak_cs_xml:xmlElement()], acl(), pid()) ->
+    {ok, #acl_v2{}} |
+    {error, invalid_argument} |
+    {error, unresolved_grant_email}.
 process_acl_contents([], Acl, _) ->
     {ok, Acl};
-process_acl_contents([HeadElement | RestElements], Acl, RiakPid) ->
-    Content = HeadElement#xmlElement.content,
-    _ = lager:debug("Element name: ~p", [HeadElement#xmlElement.name]),
-    ElementName = HeadElement#xmlElement.name,
+process_acl_contents([#xmlElement{content=Content,
+                                  name=ElementName}
+                      | RestElements], Acl, RiakPid) ->
+    _ = lager:debug("Element name: ~p", [ElementName]),
     UpdAclRes =
         case ElementName of
             'Owner' ->
@@ -282,10 +284,15 @@ process_acl_contents([HeadElement | RestElements], Acl, RiakPid) ->
             process_acl_contents(RestElements, UpdAcl, RiakPid);
         {error, _}=Error ->
             Error
-    end.
+    end;
+process_acl_contents([#xmlComment{} | RestElements], Acl, RiakPid) ->
+    process_acl_contents(RestElements, Acl, RiakPid);
+process_acl_contents([#xmlText{value=" "} | RestElements], Acl, RiakPid) ->
+    %% skip normalized space
+    process_acl_contents(RestElements, Acl, RiakPid).
 
 %% @doc Process an XML element containing acl owner information.
--spec process_owner([xmlElement()], acl(), pid()) -> {ok, #acl_v2{}}.
+-spec process_owner([riak_cs_xml:xmlNode()], acl(), pid()) -> {ok, #acl_v2{}}.
 process_owner([], Acl=?ACL{owner={[], CanonicalId, KeyId}}, RiakPid) ->
     {ok, DisplayName} = name_for_canonical(CanonicalId, RiakPid),
     case name_for_canonical(CanonicalId, RiakPid) of
@@ -296,36 +303,44 @@ process_owner([], Acl=?ACL{owner={[], CanonicalId, KeyId}}, RiakPid) ->
     end;
 process_owner([], Acl, _) ->
     {ok, Acl};
-process_owner([HeadElement | RestElements], Acl, RiakPid) ->
+process_owner([#xmlElement{content=[Content],
+                           name=ElementName} |
+               RestElements], Acl, RiakPid) ->
     Owner = Acl?ACL.owner,
-    [Content] = HeadElement#xmlElement.content,
-    Value = Content#xmlText.value,
-    ElementName = HeadElement#xmlElement.name,
-    case ElementName of
-        'ID' ->
-            _ = lager:debug("Owner ID value: ~p", [Value]),
-            {OwnerName, _, OwnerKeyId} = Owner,
-            UpdOwner = {OwnerName, Value, OwnerKeyId};
-        'DisplayName' ->
-            _ = lager:debug("Owner Name content: ~p", [Value]),
-            {_, OwnerId, OwnerKeyId} = Owner,
-            UpdOwner = {Value, OwnerId, OwnerKeyId};
+    case Content of
+        #xmlText{value=Value} ->
+            UpdOwner =
+                case ElementName of
+                    'ID' ->
+                        _ = lager:debug("Owner ID value: ~p", [Value]),
+                        {OwnerName, _, OwnerKeyId} = Owner,
+                        {OwnerName, Value, OwnerKeyId};
+                    'DisplayName' ->
+                        _ = lager:debug("Owner Name content: ~p", [Value]),
+                        {_, OwnerId, OwnerKeyId} = Owner,
+                        {Value, OwnerId, OwnerKeyId};
+                    _ ->
+                        _ = lager:debug("Encountered unexpected element: ~p", [ElementName]),
+                        Owner
+            end,
+            process_owner(RestElements, Acl?ACL{owner=UpdOwner}, RiakPid);
         _ ->
-            _ = lager:debug("Encountered unexpected element: ~p", [ElementName]),
-            UpdOwner = Owner
-    end,
-    process_owner(RestElements, Acl?ACL{owner=UpdOwner}, RiakPid).
+            process_owner(RestElements, Acl, RiakPid)
+    end;
+process_owner([_ | RestElements], Acl, RiakPid) ->
+    %% this pattern matches with text, comment, etc..
+    process_owner(RestElements, Acl, RiakPid).
 
 %% @doc Process an XML element containing the grants for the acl.
--spec process_grants([xmlElement()], acl(), pid()) ->
-                            {ok, #acl_v2{}} |
-                            {error, invalid_argument} |
-                            {error, unresolved_grant_email}.
+-spec process_grants([riak_cs_xml:xmlNode()], acl(), pid()) ->
+    {ok, #acl_v2{}} |
+    {error, invalid_argument} |
+    {error, unresolved_grant_email}.
 process_grants([], Acl, _) ->
     {ok, Acl};
-process_grants([HeadElement | RestElements], Acl, RiakPid) ->
-    Content = HeadElement#xmlElement.content,
-    ElementName = HeadElement#xmlElement.name,
+process_grants([#xmlElement{content=Content,
+                            name=ElementName} |
+                RestElements], Acl, RiakPid) ->
     UpdAcl =
         case ElementName of
             'Grant' ->
@@ -341,20 +356,22 @@ process_grants([HeadElement | RestElements], Acl, RiakPid) ->
                 Acl
         end,
     case UpdAcl of
-        {error, _} ->
-            UpdAcl;
-        _ ->
-            process_grants(RestElements, UpdAcl, RiakPid)
-    end.
+        {error, _} -> UpdAcl;
+        _ -> process_grants(RestElements, UpdAcl, RiakPid)
+    end;
+process_grants([ #xmlComment{} | RestElements], Acl, RiakPid) ->
+    process_grants(RestElements, Acl, RiakPid);
+process_grants([ #xmlText{} | RestElements], Acl, RiakPid) ->
+    process_grants(RestElements, Acl, RiakPid).
 
 %% @doc Process an XML element containing the grants for the acl.
--spec process_grant([xmlElement()], acl_grant(), acl_owner(), pid()) ->
-                           acl_grant() | {error, atom()}.
+-spec process_grant([riak_cs_xml:xmlElement()], acl_grant(), acl_owner(), pid()) ->
+    acl_grant() | {error, atom()}.
 process_grant([], Grant, _, _) ->
     Grant;
-process_grant([HeadElement | RestElements], Grant, AclOwner, RiakPid) ->
-    Content = HeadElement#xmlElement.content,
-    ElementName = HeadElement#xmlElement.name,
+process_grant([#xmlElement{content=Content,
+                           name=ElementName} |
+               RestElements], Grant, AclOwner, RiakPid) ->
     _ = lager:debug("ElementName: ~p", [ElementName]),
     _ = lager:debug("Content: ~p", [Content]),
     UpdGrant =
@@ -372,14 +389,18 @@ process_grant([HeadElement | RestElements], Grant, AclOwner, RiakPid) ->
             Error;
         _ ->
             process_grant(RestElements, UpdGrant, AclOwner, RiakPid)
-    end.
+    end;
+process_grant([#xmlComment{}|RestElements], Grant, Owner, RiakPid) ->
+    process_grant(RestElements, Grant, Owner, RiakPid);
+process_grant([#xmlText{}|RestElements], Grant, Owner, RiakPid) ->
+    process_grant(RestElements, Grant, Owner, RiakPid).
 
 %% @doc Process an XML element containing information about
 %% an ACL permission grantee.
--spec process_grantee([xmlElement()], acl_grant(), acl_owner(), pid()) ->
-                             acl_grant() |
-                             {error, invalid_argument} |
-                             {error, unresolved_grant_email}.
+-spec process_grantee([riak_cs_xml:xmlElement()], acl_grant(), acl_owner(), pid()) ->
+    acl_grant() |
+    {error, invalid_argument} |
+    {error, unresolved_grant_email}.
 process_grantee([], {{[], CanonicalId}, _Perms}, {DisplayName, CanonicalId, _}, _) ->
     {{DisplayName, CanonicalId}, _Perms};
 process_grantee([], {{[], CanonicalId}, _Perms}, _, RiakPid) ->
@@ -393,10 +414,10 @@ process_grantee([], {{[], CanonicalId}, _Perms}, _, RiakPid) ->
     end;
 process_grantee([], Grant, _, _) ->
     Grant;
-process_grantee([HeadElement | RestElements], Grant, AclOwner, RiakPid) ->
-    [Content] = HeadElement#xmlElement.content,
+process_grantee([#xmlElement{content=[Content],
+                             name=ElementName} |
+                 RestElements], Grant, AclOwner, RiakPid) ->
     Value = Content#xmlText.value,
-    ElementName = HeadElement#xmlElement.name,
     case ElementName of
         'ID' ->
             _ = lager:debug("ID value: ~p", [Value]),
@@ -433,20 +454,20 @@ process_grantee([HeadElement | RestElements], Grant, AclOwner, RiakPid) ->
             UpdGrant;
         _ ->
             process_grantee(RestElements, UpdGrant, AclOwner, RiakPid)
-    end.
+    end;
+process_grantee([#xmlText{}|RestElements], Grant, Owner, RiakPid) ->
+    process_grant(RestElements, Grant, Owner, RiakPid).
 
 %% @doc Process an XML element containing information about
 %% an ACL permission.
--spec process_permission([xmlText()], acl_grant()) -> acl_grant().
+-spec process_permission([riak_cs_xml:xmlText()], acl_grant()) -> acl_grant().
 process_permission([Content], Grant) ->
     Value = list_to_existing_atom(Content#xmlText.value),
     {Grantee, Perms} = Grant,
-    case lists:member(Value, Perms) of
-        true ->
-            UpdPerms = Perms;
-        false ->
-            UpdPerms = [Value | Perms]
-    end,
+    UpdPerms = case lists:member(Value, Perms) of
+                   true -> Perms;
+                   false -> [Value | Perms]
+               end,
     {Grantee, UpdPerms}.
 
 
@@ -538,4 +559,29 @@ canned_acl_test() ->
     ?assertMatch({acl_v2,{"tester1","TESTID1","TESTKEYID1"},
                   [{{"tester1","TESTID1"},['FULL_CONTROL']}], _}, BucketOwnerFCAcl3).
 
+
+indented_xml_with_comments() ->
+    Xml="<!-- comments here --> <AccessControlPolicy> <!-- comments here -->"
+        "  <Owner> <!-- blah blah blah -->"
+        "    <ID>eb874c6afce06925157eda682f1b3c6eb0f3b983bbee3673ae62f41cce21f6b1</ID>"
+        " <!-- c -->   <DisplayName>admin</DisplayName> <!--"
+        " \n --> </Owner>"
+        "  <AccessControlList> <!-- c -->"
+        "    <Grant>  <!-- c -->"
+        "      <Grantee xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" xsi:type=\"CanonicalUser\">"
+        "    <!-- c -->     <ID>eb874c6afce06925157eda682f1b3c6eb0f3b983bbee3673ae62f41cce21f6b1</ID>  <!-- c -->"
+        "        <DisplayName>admin</DisplayName> <!-- c -->"
+        "      </Grantee> <!-- c -->"
+        "      <Permission>FULL_CONTROL</Permission> <!-- c -->"
+        "    </Grant> <!-- c -->"
+        "  </AccessControlList> <!-- c -->"
+        "</AccessControlPolicy>    <!-- c -->",
+    Xml.
+comment_space_test() ->
+    Xml = indented_xml_with_comments(),
+    %% if cs782 alive, error:{badrecord,xmlElement} thrown here.
+    {ok, ?ACL{} = _Acl} = riak_cs_acl_utils:acl_from_xml(Xml, boom, foo),
+    ok.
+
+
 -endif.

src/riak_cs_s3_response.erl

@@ -119,6 +119,9 @@ error_code(invalid_argument) -> "InvalidArgument";
 error_code(invalid_range) -> "InvalidRange";
 error_code(invalid_bucket_name) -> "InvalidBucketName";
 error_code(unresolved_grant_email) -> "UnresolvableGrantByEmailAddress";
+error_code(unexpected_content) -> "UnexpectedContent";
+error_code(canned_acl_and_header_grant) -> "InvalidRequest";
+error_code(malformed_acl_error) -> "MalformedACLError";
 error_code(ErrorName) ->
     ok = lager:debug("Unknown Error Name: ~p", [ErrorName]),
     "ServiceUnavailable".
@@ -161,6 +164,9 @@ status_code(invalid_argument) -> 400;
 status_code(unresolved_grant_email) -> 400;
 status_code(invalid_range) -> 416;
 status_code(invalid_bucket_name) -> 400;
+status_code(unexpected_content) -> 400;
+status_code(canned_acl_and_header_grant) -> 400;
+status_code(malformed_acl_error) -> 400;
 status_code(_) -> 503.
 
 -spec respond(term(), #wm_reqdata{}, #context{}) ->
@@ -263,7 +269,9 @@ error_code_to_atom(ErrorCode) ->
 %% and XML document.
 -spec xml_error_code(string()) -> string().
 xml_error_code(Xml) ->
-    {ParsedData, _Rest} = xmerl_scan:string(Xml, []),
+    %% here comes response from velvet (Stanchion),
+    %% this scan should match, otherwise bug.
+    {ok, ParsedData} = riak_cs_xml:scan(Xml),
     process_xml_error(ParsedData#xmlElement.content).
 
 %% @doc Process the top-level elements of the

src/riak_cs_wm_object_upload_part.erl

@@ -195,9 +195,10 @@ content_types_accepted(RD, Ctx) ->
     %% e.g., PUT /ObjectName?partNumber=PartNumber&uploadId=UploadId
     riak_cs_mp_utils:make_content_types_accepted(RD, Ctx, accept_body).
 
-parse_body(Body) ->
+parse_body(Body0) ->
     try
-        {ParsedData, _Rest} = xmerl_scan:string(re:replace(Body, """, "", [global, {return, list}]), []),
+        Body = re:replace(Body0, """, "", [global, {return, list}]),
+        {ok, ParsedData} = riak_cs_xml:scan(Body),
         #xmlElement{name='CompleteMultipartUpload'} = ParsedData,
         Nums = [list_to_integer(T#xmlText.value) ||
                    T <- xmerl_xpath:string("//CompleteMultipartUpload/Part/PartNumber/text()", ParsedData)],

src/riak_cs_wm_user.erl

@@ -140,10 +140,10 @@ accept_json(RD, Ctx) ->
 accept_xml(RD, Ctx=#context{user=undefined}) ->
     riak_cs_dtrace:dt_wm_entry(?MODULE, <<"accept_xml">>),
     Body = binary_to_list(wrq:req_body(RD)),
-    case catch xmerl_scan:string(Body, []) of
-        {'EXIT', _} ->
+    case riak_cs_xml:scan(Body) of
+        {error, malformed_xml} ->
             riak_cs_s3_response:api_error(invalid_user_update, RD, Ctx);
-        {ParsedData, _Rest} ->
+        {ok, ParsedData} ->
             ValidItems = lists:foldl(fun user_xml_filter/2,
                                      [],
                                      ParsedData#xmlElement.content),
@@ -157,10 +157,10 @@ accept_xml(RD, Ctx=#context{user=undefined}) ->
 accept_xml(RD, Ctx) ->
     riak_cs_dtrace:dt_wm_entry(?MODULE, <<"accept_xml">>),
     Body = binary_to_list(wrq:req_body(RD)),
-    case catch xmerl_scan:string(Body, []) of
-        {'EXIT', _} ->
+    case riak_cs_xml:scan(Body) of
+        {error, malformed_xml} ->
             riak_cs_s3_response:api_error(invalid_user_update, RD, Ctx);
-        {ParsedData, _Rest} ->
+        {ok, ParsedData} ->
             UpdateItems = lists:foldl(fun user_xml_filter/2,
                                       [],
                                       ParsedData#xmlElement.content),

src/riak_cs_xml.erl

@@ -30,9 +30,11 @@
 -ifdef(TEST).
 -include_lib("eunit/include/eunit.hrl").
 -endif.
+-include_lib("xmerl/include/xmerl.hrl").
 
 %% Public API
--export([export_xml/1,
+-export([scan/1,
+         export_xml/1,
          to_xml/1]).
 
 -define(XML_SCHEMA_INSTANCE, "http://www.w3.org/2001/XMLSchema-instance").
@@ -42,10 +44,34 @@
 -type internal_node() :: {atom(), [internal_node() | external_node()]} |
                          {atom(), attributes(), [internal_node() | external_node()]}.
 
+%% these types should be defined in xmerl.hrl or xmerl.erl
+%% for now they're defined here for convenience.
+-type xmlElement() :: #xmlElement{}.
+-type xmlText() :: #xmlText{}.
+-type xmlComment() :: #xmlComment{}.
+-type xmlPI() :: #xmlPI{}.
+-type xmlDocument() :: #xmlDocument{}.
+-type xmlNode() ::  xmlElement() | xmlText() | xmlComment() |
+                    xmlPI() | xmlDocument().
+-export_type([xmlNode/0, xmlElement/0, xmlText/0]).
+
 %% ===================================================================
 %% Public API
 %% ===================================================================
 
+
+%% @doc parse XML and produce xmlElement (other comments and else are bad)
+%% in R15B03 (and maybe later version), xmerl_scan:string/2 may return any
+%% xml nodes, such as defined as xmlNode() above. It it unsafe because
+%% `String` is the Body sent from client, which can be anything.
+-spec scan(string()) -> {ok, xmlElement()} | {error, malformed_xml}.
+scan(String) ->
+    case catch xmerl_scan:string(String, [{space, normalize}]) of
+        {'EXIT', _E} ->  {error, malformed_xml};
+        { #xmlElement{} = ParsedData, _Rest} -> {ok, ParsedData};
+        _E -> {error, malformed_xml}
+    end.
+
 %% This function is temporary and should be removed once all XML
 %% handling has been moved into this module.
 %% @TODO Remove this asap!