You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/06/03 18:33:09 UTC
[GitHub] [couchdb] nickva opened a new pull request #3599: Improve basic auth credentials handling in replicator
nickva opened a new pull request #3599:
URL: https://github.com/apache/couchdb/pull/3599
This is a port of commit ecd266b0e87f44e1080cabdb4c28e4758f5a4406 from 3.x to main. Including the same commit message from there for completeness below. Then, towards the end, there is a description of changes required to port this PR to main.
Previously, there were two ways to pass in basic auth credentials for endpoints -- using URL's userinfo part, and encoding them in an `"Authorization": "basic..."` header. Neither one is ideal for these reasons:
* Passwords in userinfo don't allow using ":", "@" and other characters. However, even after switching to always unquoting them like we did recently [1], it could break authentication for usernames or passwords previously
containing "+" or "%HH" patterns, as "+" might now be decoded to a " ".
* Base64 encoded headers need an extra step to encode them. Also, quite often these encoded headers are confused as being "encrypted" and shared in a clear channel.
To improve this, revert the recent commit to unquote URL userinfo parts to restore backwards compatibility, and introduce a way to pass in basic auth credentials in the "auth" object. The "auth" object was already added a while
back to allow authentication plugins to store their credentials in it. The format is:
```
"source": {
"url": "https://host/db",
"auth": {
"basic": {
"username":"myuser",
"password":"mypassword"
}
}
}
```
{"auth" : "basic" : {...}} object is checked first, and if credentials are provided, they will be used. If they are not then userinfo and basic auth header will be parsed.
Internally, there was a good amount duplication related to parsing credentials from userinfo and headers in replication ID generation logic and in the auth session plugin. As a cleanup, consolidate that logic in the `couch_replicator_parse` module.
The commit is quite different from the 3.x one for these two reasons:
* `main` uses two types of replication endpoint "objects": `#httpdb` records and `HttpDb` maps. In most cases it uses maps which can be serialized and deserialized to and from json. But in lower level, connection handling code
in couch_replicator_httpc, it uses `#httpdb` records. This explain the need to still handle both representations. Auth session plugin, for instance, uses the lower level #httpdb records while replicator ID handling code uses the map based one.
* `main` has all the parsing of replication documents and `_replicate` request bodies in a separate `couch_replicator_parse`. So, most of the code which handles normalizing basic auth creds is there instead of `couch_replicator_docs` or `couch_replicator_utils` like it is in 3.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a change in pull request #3599: Improve basic auth credentials handling in replicator
Posted by GitBox <gi...@apache.org>.
jaydoane commented on a change in pull request #3599:
URL: https://github.com/apache/couchdb/pull/3599#discussion_r645217968
##########
File path: src/couch_replicator/src/couch_replicator_parse.erl
##########
@@ -565,4 +677,243 @@ t_error_on_local_endpoint(_) ->
?assertThrow({bad_rep_doc, Expect}, parse_rep_doc(RepDoc)).
+remove_basic_auth_from_headers_test_() ->
+ B64 = list_to_binary(b64creds("user", "pass")),
+ [?_assertEqual({{User, Pass}, NoAuthHeaders},
+ remove_basic_auth_from_headers(Headers)) ||
+ {{User, Pass, NoAuthHeaders}, Headers} <- [
+ {
+ {undefined, undefined, #{}},
+ #{}
+ },
+ {
+ {undefined, undefined, #{<<"h">> => <<"v">>}},
+ #{<<"h">> => <<"v">>}
+ },
+ {
+ {undefined, undefined, #{<<"Authorization">> => <<"junk">>}},
+ #{<<"Authorization">> => <<"junk">>}
+ },
+ {
+ {undefined, undefined, #{}},
+ #{<<"Authorization">> => <<"basic X">>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"AuThorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"bAsIc ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{<<"h">> => <<"v">>}},
+ #{
+ <<"Authorization">> => <<"Basic ", B64/binary>>,
+ <<"h">> => <<"v">>
+ }
+ }
+ ]
+ ].
+
+
+b64creds(User, Pass) ->
+ base64:encode_to_string(User ++ ":" ++ Pass).
+
+
+set_basic_auth_creds_test() ->
+ Check = fun(User, Pass, Props) ->
+ HttpDb = #{<<"auth_props">> => Props},
+ HttpDb1 = set_basic_auth_creds(User, Pass, HttpDb),
+ maps:get(<<"auth_props">>, HttpDb1)
+ end,
+
+ ?assertEqual(#{}, Check(undefined, undefined, #{})),
+
+ ?assertEqual(#{<<"other">> => #{}}, Check(undefined, undefined,
+ #{<<"other">> => #{}})),
+
+ ?assertEqual(#{
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{})),
+
+ ?assertEqual(#{
+ <<"other">> => #{},
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{<<"other">> => #{}})).
+
+
+normalize_basic_creds_test_() ->
+ DefaultHeaders = couch_replicator_utils:default_headers_map(),
+ [?_assertEqual(Expect, normalize_basic_auth(Input)) || {Input, Expect} <- [
+ {
+ #{
+ <<"url">> => <<"http://u:p@x.y/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://x.y/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@h:80/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://h:80/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"https://u:p@h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"https://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p@")
Review comment:
Yay I can finally put `@` in my password 🎉
##########
File path: src/couch_replicator/src/couch_replicator_parse.erl
##########
@@ -565,4 +677,243 @@ t_error_on_local_endpoint(_) ->
?assertThrow({bad_rep_doc, Expect}, parse_rep_doc(RepDoc)).
+remove_basic_auth_from_headers_test_() ->
+ B64 = list_to_binary(b64creds("user", "pass")),
+ [?_assertEqual({{User, Pass}, NoAuthHeaders},
+ remove_basic_auth_from_headers(Headers)) ||
+ {{User, Pass, NoAuthHeaders}, Headers} <- [
+ {
+ {undefined, undefined, #{}},
+ #{}
+ },
+ {
+ {undefined, undefined, #{<<"h">> => <<"v">>}},
+ #{<<"h">> => <<"v">>}
+ },
+ {
+ {undefined, undefined, #{<<"Authorization">> => <<"junk">>}},
+ #{<<"Authorization">> => <<"junk">>}
+ },
+ {
+ {undefined, undefined, #{}},
+ #{<<"Authorization">> => <<"basic X">>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"AuThorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"bAsIc ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{<<"h">> => <<"v">>}},
+ #{
+ <<"Authorization">> => <<"Basic ", B64/binary>>,
+ <<"h">> => <<"v">>
+ }
+ }
+ ]
+ ].
+
+
+b64creds(User, Pass) ->
+ base64:encode_to_string(User ++ ":" ++ Pass).
+
+
+set_basic_auth_creds_test() ->
+ Check = fun(User, Pass, Props) ->
+ HttpDb = #{<<"auth_props">> => Props},
+ HttpDb1 = set_basic_auth_creds(User, Pass, HttpDb),
+ maps:get(<<"auth_props">>, HttpDb1)
+ end,
+
+ ?assertEqual(#{}, Check(undefined, undefined, #{})),
+
+ ?assertEqual(#{<<"other">> => #{}}, Check(undefined, undefined,
+ #{<<"other">> => #{}})),
+
+ ?assertEqual(#{
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{})),
+
+ ?assertEqual(#{
+ <<"other">> => #{},
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{<<"other">> => #{}})).
+
+
+normalize_basic_creds_test_() ->
+ DefaultHeaders = couch_replicator_utils:default_headers_map(),
+ [?_assertEqual(Expect, normalize_basic_auth(Input)) || {Input, Expect} <- [
+ {
+ #{
+ <<"url">> => <<"http://u:p@x.y/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://x.y/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@h:80/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://h:80/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"https://u:p@h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"https://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p@")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p@"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p@%40")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p@%40"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"aUthoriZation">> => basic_b64("U", "p")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("U", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"Authorization">> => basic_b64("u2", "p2")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u1", "p1"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
+ <<"auth_props">> => auth_props("u2", "p2"),
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u2", "p2"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
Review comment:
I wonder if it would be surprising for someone putting different creds in multiple locations in the replication doc, that all but one set just gets ignored? Would it be better to fail with an error on multiple creds or would that not be backwards compatible?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3599: Improve basic auth credentials handling in replicator
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3599:
URL: https://github.com/apache/couchdb/pull/3599#discussion_r645234779
##########
File path: src/couch_replicator/src/couch_replicator_parse.erl
##########
@@ -565,4 +677,243 @@ t_error_on_local_endpoint(_) ->
?assertThrow({bad_rep_doc, Expect}, parse_rep_doc(RepDoc)).
+remove_basic_auth_from_headers_test_() ->
+ B64 = list_to_binary(b64creds("user", "pass")),
+ [?_assertEqual({{User, Pass}, NoAuthHeaders},
+ remove_basic_auth_from_headers(Headers)) ||
+ {{User, Pass, NoAuthHeaders}, Headers} <- [
+ {
+ {undefined, undefined, #{}},
+ #{}
+ },
+ {
+ {undefined, undefined, #{<<"h">> => <<"v">>}},
+ #{<<"h">> => <<"v">>}
+ },
+ {
+ {undefined, undefined, #{<<"Authorization">> => <<"junk">>}},
+ #{<<"Authorization">> => <<"junk">>}
+ },
+ {
+ {undefined, undefined, #{}},
+ #{<<"Authorization">> => <<"basic X">>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"AuThorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"bAsIc ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{<<"h">> => <<"v">>}},
+ #{
+ <<"Authorization">> => <<"Basic ", B64/binary>>,
+ <<"h">> => <<"v">>
+ }
+ }
+ ]
+ ].
+
+
+b64creds(User, Pass) ->
+ base64:encode_to_string(User ++ ":" ++ Pass).
+
+
+set_basic_auth_creds_test() ->
+ Check = fun(User, Pass, Props) ->
+ HttpDb = #{<<"auth_props">> => Props},
+ HttpDb1 = set_basic_auth_creds(User, Pass, HttpDb),
+ maps:get(<<"auth_props">>, HttpDb1)
+ end,
+
+ ?assertEqual(#{}, Check(undefined, undefined, #{})),
+
+ ?assertEqual(#{<<"other">> => #{}}, Check(undefined, undefined,
+ #{<<"other">> => #{}})),
+
+ ?assertEqual(#{
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{})),
+
+ ?assertEqual(#{
+ <<"other">> => #{},
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{<<"other">> => #{}})).
+
+
+normalize_basic_creds_test_() ->
+ DefaultHeaders = couch_replicator_utils:default_headers_map(),
+ [?_assertEqual(Expect, normalize_basic_auth(Input)) || {Input, Expect} <- [
+ {
+ #{
+ <<"url">> => <<"http://u:p@x.y/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://x.y/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@h:80/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://h:80/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"https://u:p@h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"https://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p@")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p@"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p@%40")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p@%40"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"aUthoriZation">> => basic_b64("U", "p")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("U", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"Authorization">> => basic_b64("u2", "p2")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u1", "p1"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
+ <<"auth_props">> => auth_props("u2", "p2"),
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u2", "p2"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
Review comment:
I thought since we allowed multiple ones previously we'd keep that behavior for compatibility. Next, I was planning on reverting the ibrowse commit and also issue a doc PR indicating that we're deprecating userinfo and if users to have multiple credentials specified which ones get picked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva merged pull request #3599: Improve basic auth credentials handling in replicator
Posted by GitBox <gi...@apache.org>.
nickva merged pull request #3599:
URL: https://github.com/apache/couchdb/pull/3599
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3599: Improve basic auth credentials handling in replicator
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3599:
URL: https://github.com/apache/couchdb/pull/3599#discussion_r645234779
##########
File path: src/couch_replicator/src/couch_replicator_parse.erl
##########
@@ -565,4 +677,243 @@ t_error_on_local_endpoint(_) ->
?assertThrow({bad_rep_doc, Expect}, parse_rep_doc(RepDoc)).
+remove_basic_auth_from_headers_test_() ->
+ B64 = list_to_binary(b64creds("user", "pass")),
+ [?_assertEqual({{User, Pass}, NoAuthHeaders},
+ remove_basic_auth_from_headers(Headers)) ||
+ {{User, Pass, NoAuthHeaders}, Headers} <- [
+ {
+ {undefined, undefined, #{}},
+ #{}
+ },
+ {
+ {undefined, undefined, #{<<"h">> => <<"v">>}},
+ #{<<"h">> => <<"v">>}
+ },
+ {
+ {undefined, undefined, #{<<"Authorization">> => <<"junk">>}},
+ #{<<"Authorization">> => <<"junk">>}
+ },
+ {
+ {undefined, undefined, #{}},
+ #{<<"Authorization">> => <<"basic X">>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"AuThorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"bAsIc ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{<<"h">> => <<"v">>}},
+ #{
+ <<"Authorization">> => <<"Basic ", B64/binary>>,
+ <<"h">> => <<"v">>
+ }
+ }
+ ]
+ ].
+
+
+b64creds(User, Pass) ->
+ base64:encode_to_string(User ++ ":" ++ Pass).
+
+
+set_basic_auth_creds_test() ->
+ Check = fun(User, Pass, Props) ->
+ HttpDb = #{<<"auth_props">> => Props},
+ HttpDb1 = set_basic_auth_creds(User, Pass, HttpDb),
+ maps:get(<<"auth_props">>, HttpDb1)
+ end,
+
+ ?assertEqual(#{}, Check(undefined, undefined, #{})),
+
+ ?assertEqual(#{<<"other">> => #{}}, Check(undefined, undefined,
+ #{<<"other">> => #{}})),
+
+ ?assertEqual(#{
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{})),
+
+ ?assertEqual(#{
+ <<"other">> => #{},
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{<<"other">> => #{}})).
+
+
+normalize_basic_creds_test_() ->
+ DefaultHeaders = couch_replicator_utils:default_headers_map(),
+ [?_assertEqual(Expect, normalize_basic_auth(Input)) || {Input, Expect} <- [
+ {
+ #{
+ <<"url">> => <<"http://u:p@x.y/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://x.y/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@h:80/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://h:80/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"https://u:p@h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"https://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p@")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p@"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p@%40")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p@%40"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"aUthoriZation">> => basic_b64("U", "p")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("U", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"Authorization">> => basic_b64("u2", "p2")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u1", "p1"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
+ <<"auth_props">> => auth_props("u2", "p2"),
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u2", "p2"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
Review comment:
I thought since we allowed multiple ones previously we'd keep that behavior for compatibility. Next, I was planning on reverting the ibrowse commit and also issue a doc PR indicating that we're deprecating userinfo and if users to have multiple credentials specified which ones get picked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva merged pull request #3599: Improve basic auth credentials handling in replicator
Posted by GitBox <gi...@apache.org>.
nickva merged pull request #3599:
URL: https://github.com/apache/couchdb/pull/3599
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a change in pull request #3599: Improve basic auth credentials handling in replicator
Posted by GitBox <gi...@apache.org>.
jaydoane commented on a change in pull request #3599:
URL: https://github.com/apache/couchdb/pull/3599#discussion_r645217968
##########
File path: src/couch_replicator/src/couch_replicator_parse.erl
##########
@@ -565,4 +677,243 @@ t_error_on_local_endpoint(_) ->
?assertThrow({bad_rep_doc, Expect}, parse_rep_doc(RepDoc)).
+remove_basic_auth_from_headers_test_() ->
+ B64 = list_to_binary(b64creds("user", "pass")),
+ [?_assertEqual({{User, Pass}, NoAuthHeaders},
+ remove_basic_auth_from_headers(Headers)) ||
+ {{User, Pass, NoAuthHeaders}, Headers} <- [
+ {
+ {undefined, undefined, #{}},
+ #{}
+ },
+ {
+ {undefined, undefined, #{<<"h">> => <<"v">>}},
+ #{<<"h">> => <<"v">>}
+ },
+ {
+ {undefined, undefined, #{<<"Authorization">> => <<"junk">>}},
+ #{<<"Authorization">> => <<"junk">>}
+ },
+ {
+ {undefined, undefined, #{}},
+ #{<<"Authorization">> => <<"basic X">>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"AuThorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"bAsIc ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{<<"h">> => <<"v">>}},
+ #{
+ <<"Authorization">> => <<"Basic ", B64/binary>>,
+ <<"h">> => <<"v">>
+ }
+ }
+ ]
+ ].
+
+
+b64creds(User, Pass) ->
+ base64:encode_to_string(User ++ ":" ++ Pass).
+
+
+set_basic_auth_creds_test() ->
+ Check = fun(User, Pass, Props) ->
+ HttpDb = #{<<"auth_props">> => Props},
+ HttpDb1 = set_basic_auth_creds(User, Pass, HttpDb),
+ maps:get(<<"auth_props">>, HttpDb1)
+ end,
+
+ ?assertEqual(#{}, Check(undefined, undefined, #{})),
+
+ ?assertEqual(#{<<"other">> => #{}}, Check(undefined, undefined,
+ #{<<"other">> => #{}})),
+
+ ?assertEqual(#{
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{})),
+
+ ?assertEqual(#{
+ <<"other">> => #{},
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{<<"other">> => #{}})).
+
+
+normalize_basic_creds_test_() ->
+ DefaultHeaders = couch_replicator_utils:default_headers_map(),
+ [?_assertEqual(Expect, normalize_basic_auth(Input)) || {Input, Expect} <- [
+ {
+ #{
+ <<"url">> => <<"http://u:p@x.y/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://x.y/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@h:80/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://h:80/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"https://u:p@h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"https://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p@")
Review comment:
Yay I can finally put `@` in my password 🎉
##########
File path: src/couch_replicator/src/couch_replicator_parse.erl
##########
@@ -565,4 +677,243 @@ t_error_on_local_endpoint(_) ->
?assertThrow({bad_rep_doc, Expect}, parse_rep_doc(RepDoc)).
+remove_basic_auth_from_headers_test_() ->
+ B64 = list_to_binary(b64creds("user", "pass")),
+ [?_assertEqual({{User, Pass}, NoAuthHeaders},
+ remove_basic_auth_from_headers(Headers)) ||
+ {{User, Pass, NoAuthHeaders}, Headers} <- [
+ {
+ {undefined, undefined, #{}},
+ #{}
+ },
+ {
+ {undefined, undefined, #{<<"h">> => <<"v">>}},
+ #{<<"h">> => <<"v">>}
+ },
+ {
+ {undefined, undefined, #{<<"Authorization">> => <<"junk">>}},
+ #{<<"Authorization">> => <<"junk">>}
+ },
+ {
+ {undefined, undefined, #{}},
+ #{<<"Authorization">> => <<"basic X">>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"AuThorization">> => <<"Basic ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{}},
+ #{<<"Authorization">> => <<"bAsIc ", B64/binary>>}
+ },
+ {
+ {"user", "pass", #{<<"h">> => <<"v">>}},
+ #{
+ <<"Authorization">> => <<"Basic ", B64/binary>>,
+ <<"h">> => <<"v">>
+ }
+ }
+ ]
+ ].
+
+
+b64creds(User, Pass) ->
+ base64:encode_to_string(User ++ ":" ++ Pass).
+
+
+set_basic_auth_creds_test() ->
+ Check = fun(User, Pass, Props) ->
+ HttpDb = #{<<"auth_props">> => Props},
+ HttpDb1 = set_basic_auth_creds(User, Pass, HttpDb),
+ maps:get(<<"auth_props">>, HttpDb1)
+ end,
+
+ ?assertEqual(#{}, Check(undefined, undefined, #{})),
+
+ ?assertEqual(#{<<"other">> => #{}}, Check(undefined, undefined,
+ #{<<"other">> => #{}})),
+
+ ?assertEqual(#{
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{})),
+
+ ?assertEqual(#{
+ <<"other">> => #{},
+ <<"basic">> => #{
+ <<"username">> => <<"u">>,
+ <<"password">> => <<"p">>
+ }
+ }, Check("u", "p", #{<<"other">> => #{}})).
+
+
+normalize_basic_creds_test_() ->
+ DefaultHeaders = couch_replicator_utils:default_headers_map(),
+ [?_assertEqual(Expect, normalize_basic_auth(Input)) || {Input, Expect} <- [
+ {
+ #{
+ <<"url">> => <<"http://u:p@x.y/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://x.y/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@h:80/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://h:80/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"https://u:p@h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"https://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u:p@[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://[2001:db8:a1b:12f9::1]/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p@")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p@"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"authorization">> => basic_b64("u", "p@%40")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u", "p@%40"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"aUthoriZation">> => basic_b64("U", "p")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("U", "p"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
+ <<"auth_props">> => #{},
+ <<"headers">> => maps:merge(DefaultHeaders, #{
+ <<"Authorization">> => basic_b64("u2", "p2")
+ })
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u1", "p1"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
+ <<"auth_props">> => auth_props("u2", "p2"),
+ <<"headers">> => DefaultHeaders
+ },
+ #{
+ <<"url">> => <<"http://h/db">>,
+ <<"auth_props">> => auth_props("u2", "p2"),
+ <<"headers">> => DefaultHeaders
+ }
+ },
+ {
+ #{
+ <<"url">> => <<"http://u1:p1@h/db">>,
Review comment:
I wonder if it would be surprising for someone putting different creds in multiple locations in the replication doc, that all but one set just gets ignored? Would it be better to fail with an error on multiple creds or would that not be backwards compatible?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org