iilyak commented on a change in pull request #1176: Implement pluggable 
authentication and session support for replicator
URL: https://github.com/apache/couchdb/pull/1176#discussion_r169178743
 
 

 ##########
 File path: src/couch_replicator/src/couch_replicator_utils.erl
 ##########
 @@ -174,3 +175,93 @@ filter_state(State, States, Info) ->
         false ->
             skip
     end.
+
+
+remove_basic_auth_from_headers(Headers) ->
+    Headers1 = mochiweb_headers:make(Headers),
+    case mochiweb_headers:get_value("Authorization", Headers1) of
+        undefined ->
+            {{undefined, undefined}, Headers};
+        Auth when length(Auth) =< 6 ->
 
 Review comment:
   What is the rationale for this approach. It seems rather complex. Here are 
the possible downsides of the code as it is written:
   * use of magic number 6 (I got it is the lenght("Basic ") but I have to stop 
for a minute)
   * unnecessary use of length/1
   * subsequent use of lists:split/2
   * what if we decide to use other types (Bearer or Digest or something else)
   
   If I understand the structure should be always `Authorization: <type> 
<credentials>`
   This means that the same could be written as:
   ```
   Auth ->
       case lists:splitwith(fun(X) -> X =/= $\s end, Auth) of
           {AType, " " ++ Base64} ->
               maybe_remove_basic_auth(string:to_lower(AType), Base64, Headers1)
   ```
    
   OR if we use binaries 
   ```
   case binary:split(?l2b(Auth), <<" ">>) of
      {AType, Base64} ->
          maybe_remove_basic_auth(string:to_lower(?b2l(AType)), Base64, 
Headers1);
   ...
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to