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/03/30 16:29:56 UTC
[GitHub] [couchdb] Terreii opened a new pull request #3483: password requirement
Terreii opened a new pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483
<!-- Thank you for your contribution!
Please file this form by replacing the Markdown comments
with your text. If a section needs no action - remove it.
Also remember, that CouchDB uses the Review-Then-Commit (RTC) model
of code collaboration. Positive feedback is represented +1 from committers
and negative is a -1. The -1 also means veto, and needs to be addressed
to proceed. Once there are no objections, the PR can be merged by a
CouchDB committer.
See: http://couchdb.apache.org/bylaws.html#decisions for more info. -->
## Overview
<!-- Please give a short brief for the pull request,
what problem it solves or how it makes things better. -->
This pull request adds a new setting for password requirements.
Closes #2951.
## Testing recommendations
<!-- Describe how we can test your changes.
Does it provides any behaviour that the end users
could notice? -->
A user should not be able to change their password, if it does not
match the requirements set by the admin.
## Related Issues or Pull Requests
<!-- If your changes affects multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [x] Code is written and works correctly
- [x] Changes are covered by tests
- [x] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
--
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] Terreii commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r605685134
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ Loop = fun(RegExp) ->
+ check_password_with_regexp(ClearPassword, RegExp)
+ end,
+ lists:foreach(Loop, RegExpList),
+ ok;
+ {ok, NonListValue} ->
+ couch_log:error(
+ "[couch_httpd_auth] password_reqexp value of '~p' is invalid.",
Review comment:
Is your request to change the error logs to be more like [this](https://github.com/apache/couchdb/blob/776f920021dd15c51355e9a904bdc5a2250b19d9/src/mem3/src/mem3_rep.erl#L496)
Like this so:
`couch_log:error("~p : ~p couldn't be parsed. '~p' is not a list.", [?MODULE, "password_regexp", NonListValue])`
--
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] Terreii commented on pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#issuecomment-822698523
Thanks đ
--
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] wohali commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
wohali commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r604282003
##########
File path: rel/overlay/etc/default.ini
##########
@@ -279,6 +279,7 @@ iterations = 10 ; iterations for password hashing
; min_iterations = 1
; max_iterations = 1000000000
; password_scheme = pbkdf2
+; password_reqexp = ""
Review comment:
This should be `regexp` not `reqexp`.
--
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] Terreii commented on pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#issuecomment-817168080
There wasn't a change without the `with_db` tag. But all password requirement tests without it.
But I could, with the `with a string in a list` test now being removed, combine the remaining error path tests into one test. Reducing the time added to "only" 10s. Or I could combine them all into one password requirements test.
--
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 #3483: password requirement
Posted by GitBox <gi...@apache.org>.
jaydoane commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r604307785
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ Loop = fun(RegExp) ->
+ check_password_with_regexp(ClearPassword, RegExp)
+ end,
+ lists:foreach(Loop, RegExpList),
+ ok;
+ {ok, NonListValue} ->
+ couch_log:error(
+ "[couch_httpd_auth] password_reqexp value of '~p' is invalid.",
+ [NonListValue]
+ ),
+ throw({forbidden, ?PASSWORD_SERVER_ERROR});
+ {error, ErrorInfo} ->
+ couch_log:error("[couch_httpd_auth] password_reqexp value of '~p' is invalid. ~p",
+ [ValidateConfig, ErrorInfo]),
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end
+ end.
+
+% Check the password if it matches a RegExp.
+% First is with a Reason string.
+check_password_with_regexp(Password, {RegExp, Reason})
+ when is_list(RegExp) andalso is_list(Reason) andalso length(Reason) > 0 ->
+ check_password(Password, RegExp,
+ lists:concat([?REQUIREMENT_ERROR, " ", Reason]));
+% With a not correct Reason string.
+check_password_with_regexp(Password, {RegExp, _Reason}) when is_list(RegExp) ->
+ check_password(Password, RegExp, ?REQUIREMENT_ERROR);
+% Without a Reason string.
+check_password_with_regexp(Password, {RegExp}) when is_list(RegExp) ->
+ check_password(Password, RegExp, ?REQUIREMENT_ERROR);
+% Not correct RegExpValue.
+check_password_with_regexp(_, RegExpValue) ->
+ couch_log:error(
+ "[couch_httpd_auth] password_reqexp value of '~p' is invalid.", [RegExpValue]),
+ throw({forbidden, ?PASSWORD_SERVER_ERROR}).
+
+check_password(Password, RegExp, ErrorMsg) ->
+ case re:run(Password, RegExp, [{capture, none}]) of
+ match ->
Review comment:
Indentation
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ Loop = fun(RegExp) ->
+ check_password_with_regexp(ClearPassword, RegExp)
+ end,
+ lists:foreach(Loop, RegExpList),
+ ok;
Review comment:
Stylistically, I think this is preferred:
```erlang
lists:foreach(fun(RegExp) ->
check_password_with_regexp(ClearPassword, RegExp)
end, RegExpList);
```
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ Loop = fun(RegExp) ->
+ check_password_with_regexp(ClearPassword, RegExp)
+ end,
+ lists:foreach(Loop, RegExpList),
+ ok;
+ {ok, NonListValue} ->
+ couch_log:error(
+ "[couch_httpd_auth] password_reqexp value of '~p' is invalid.",
+ [NonListValue]
+ ),
+ throw({forbidden, ?PASSWORD_SERVER_ERROR});
Review comment:
Is `forbidden` the correct error here? I would argue 400 is more appropriate, since the password is "malformed" based on not passing the regexp
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
Review comment:
This line should be indented 4 spaces
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ Loop = fun(RegExp) ->
+ check_password_with_regexp(ClearPassword, RegExp)
+ end,
+ lists:foreach(Loop, RegExpList),
+ ok;
+ {ok, NonListValue} ->
+ couch_log:error(
+ "[couch_httpd_auth] password_reqexp value of '~p' is invalid.",
Review comment:
`?MODULE` is preferred to hard-coding the module name here. Also, please omit the `[]`
--
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] janl commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
janl commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r606804968
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ Loop = fun(RegExp) ->
+ check_password_with_regexp(ClearPassword, RegExp)
+ end,
+ lists:foreach(Loop, RegExpList),
+ ok;
+ {ok, NonListValue} ->
+ couch_log:error(
+ "[couch_httpd_auth] password_reqexp value of '~p' is invalid.",
+ [NonListValue]
+ ),
+ throw({forbidden, ?PASSWORD_SERVER_ERROR});
Review comment:
403 for âwe canât hash any PW because we are misconfiguredâ is fine because we used that before, although a 5xx code might be more appropriate, but no need to change all that now.
400 for âyour password is badâ is ok too. Sadly, there is no 4xx for âyou failed whatever custom validation I have going on hereâ, only ones specific to other HTTP behaviour like `Accept` or `Expect` headers,
--
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] janl commented on pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
janl commented on pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#issuecomment-813037271
this is neat, letâs get it into 3.2.0 :)
--
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] janl merged pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
janl merged pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483
--
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] Terreii commented on pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#issuecomment-819039852
Down to 5s - 6s.
--
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] bessbd commented on pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#issuecomment-814159145
Once this is merged, I think it needs a docs update. @Terreii : are you interested in opening a PR for docs, too?
--
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] janl commented on pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
janl commented on pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#issuecomment-817108082
> Just some minor line length issues remain for me. Tests pass, although they seem to add about 20 seconds to the suite on my machine:
>
> ```
> UsersDbTest
> * test users password requirements (76.2ms)
> * test users password requirements with non list value (4973.3ms)
> * test users db (5267.9ms)
> * test users password requirements with not correct syntax (4968.3ms)
> * test users password requirements with a string in a list (4969.9ms)
> ```
>
looks like wrapping all tests cases in `with_db` starts a new couch server each time, we could probably get away with only starting it once for all these tests.
--
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] janl commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
janl commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r611022028
##########
File path: test/elixir/test/users_db_test.exs
##########
@@ -298,4 +298,142 @@ defmodule UsersDbTest do
assert resp.body["userCtx"]["name"] == "foo@example.org"
end
+
+ @tag :with_db
+ test "users password requirements", context do
Review comment:
`context` seems unused:
```
[2021-04-09T17:17:07.011Z] Excluding tags: [:without_quorum_test, :with_quorum_test, :pending, :skip_on_****]
[2021-04-09T17:17:07.011Z]
[2021-04-09T17:17:14.578Z] warning: variable "string" is unused (if the variable is not meant to be used, prefix it with an underscore)
[2021-04-09T17:17:14.578Z] test/elixir/test/utf8_test.exs:32: UTF8Test."test UTF8 support"/1
[2021-04-09T17:17:14.578Z]
[2021-04-09T17:17:14.578Z] warning: variable "context" is unused (if the variable is not meant to be used, prefix it with an underscore)
[2021-04-09T17:17:14.578Z] test/elixir/test/users_db_test.exs:303: UsersDbTest."test users password requirements"/1
[2021-04-09T17:17:14.578Z]
[2021-04-09T17:17:14.578Z] warning: variable "context" is unused (if the variable is not meant to be used, prefix it with an underscore)
[2021-04-09T17:17:14.578Z] test/elixir/test/users_db_test.exs:391: UsersDbTest."test users password requirements with non list value"/1
[2021-04-09T17:17:14.578Z]
[2021-04-09T17:17:14.578Z] warning: variable "context" is unused (if the variable is not meant to be used, prefix it with an underscore)
[2021-04-09T17:17:14.578Z] test/elixir/test/users_db_test.exs:416: UsersDbTest."test users password requirements with not correct syntax"/1
```
--
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] janl commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
janl commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r606804417
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ Loop = fun(RegExp) ->
+ check_password_with_regexp(ClearPassword, RegExp)
+ end,
+ lists:foreach(Loop, RegExpList),
+ ok;
+ {ok, NonListValue} ->
+ couch_log:error(
+ "[couch_httpd_auth] password_reqexp value of '~p' is invalid.",
Review comment:
@jaydoane if another auth module supports PW validation it can hardcode its own name, so I think this is fine here. â And I prefer the convention of `[section] key` for referring to config value.
--
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] bessbd commented on pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#issuecomment-824088741
Should we port this to `main`?
--
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] Terreii commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r610140175
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,72 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_regexp setting.
+% Throws if not.
+% In this function the [couch_httpd_auth] password_regexp config is parsed.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_regexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ lists:foreach(fun(RegExpTuple) ->
+ case get_password_regexp_and_error_msg(RegExpTuple) of
+ {ok, RegExp, PasswordErrorMsg} ->
+ check_password(ClearPassword, RegExp, PasswordErrorMsg);
+ {error} ->
+ couch_log:error(
+ "[couch_httpd_auth] password_regexp value of '~p' is invalid.",
+ [RegExpTuple]
+ ),
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end
+ end, RegExpList),
Review comment:
Done
--
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] Terreii edited a comment on pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii edited a comment on pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#issuecomment-817168080
There wasn't a change without the `with_db` tag. But all password requirement tests pass without it.
But I could, with the `with a string in a list` test now being removed, combine the remaining error path tests into one test. Reducing the time added to "only" 10s. Or I could combine them all into one password requirements test.
--
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 #3483: password requirement
Posted by GitBox <gi...@apache.org>.
jaydoane commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r607371090
##########
File path: rel/overlay/etc/default.ini
##########
@@ -279,6 +279,8 @@ iterations = 10 ; iterations for password hashing
; min_iterations = 1
; max_iterations = 1000000000
; password_scheme = pbkdf2
+; List of tuples of an Erlang RegExp and an optional error message. Where a new password must match all RegExp. Example: [{".{10,}", "Password min length is 10 characters."}]
Review comment:
I wonder if it should just be `Regexp` (instead of `{Regexp}`) when there's no `Reason`. At the risk of bikeshedding, It seems a little strange to require the string be wrapped in a tuple when there's no positional information, although I don't have a strong opinion on this, other than it's so easy to pattern match in Erlang, that "extra" syntax like that is usually unnecessary.
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,72 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_regexp setting.
+% Throws if not.
+% In this function the [couch_httpd_auth] password_regexp config is parsed.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_regexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ lists:foreach(fun(RegExpTuple) ->
+ case get_password_regexp_and_error_msg(RegExpTuple) of
+ {ok, RegExp, PasswordErrorMsg} ->
+ check_password(ClearPassword, RegExp, PasswordErrorMsg);
+ {error} ->
+ couch_log:error(
+ "[couch_httpd_auth] password_regexp value of '~p' is invalid.",
+ [RegExpTuple]
+ ),
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end
+ end, RegExpList),
Review comment:
Stylistically, this seems fine now, except several lines exceed 80 chars.
--
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] janl commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
janl commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r611021689
##########
File path: rel/overlay/etc/default.ini
##########
@@ -279,6 +279,8 @@ iterations = 10 ; iterations for password hashing
; min_iterations = 1
; max_iterations = 1000000000
; password_scheme = pbkdf2
+; List of tuples of an Erlang RegExp and an optional error message. Where a new password must match all RegExp. Example: [{".{10,}", "Password min length is 10 characters."}]
Review comment:
Alternatively it could encourage folks to provide proper Reasons, which is likely beneficial down the road :) â I wouldnât worry about shortcuts here.
--
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] Terreii commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r605209798
##########
File path: rel/overlay/etc/default.ini
##########
@@ -279,6 +279,7 @@ iterations = 10 ; iterations for password hashing
; min_iterations = 1
; max_iterations = 1000000000
; password_scheme = pbkdf2
+; password_reqexp = ""
Review comment:
đŽ this is now fixed.
Should it be `password_regexp` or would be `password_requirements` be better?
--
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] Terreii commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r605210394
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ Loop = fun(RegExp) ->
+ check_password_with_regexp(ClearPassword, RegExp)
+ end,
+ lists:foreach(Loop, RegExpList),
+ ok;
Review comment:
I changed it the the latest commit a little. What do you think?
--
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] Terreii commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r605214355
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
Review comment:
Done
--
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] Terreii commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r605211722
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ Loop = fun(RegExp) ->
+ check_password_with_regexp(ClearPassword, RegExp)
+ end,
+ lists:foreach(Loop, RegExpList),
+ ok;
+ {ok, NonListValue} ->
+ couch_log:error(
+ "[couch_httpd_auth] password_reqexp value of '~p' is invalid.",
Review comment:
This is the configuration name, inspired by the error message of [`save_doc/1`](https://github.com/apache/couchdb/blob/776f920021dd15c51355e9a904bdc5a2250b19d9/src/couch/src/couch_users_db.erl#L88).
--
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] Terreii commented on pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#issuecomment-830282253
> Should we port this to `main`?
I did port it on #3539.
--
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] Terreii commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
Terreii commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r605214013
##########
File path: src/couch/src/couch_users_db.erl
##########
@@ -86,7 +90,63 @@ save_doc(#doc{body={Body}} = Doc) ->
Doc#doc{body={Body4}};
{_ClearPassword, Scheme} ->
couch_log:error("[couch_httpd_auth] password_scheme value of '~p' is invalid.", [Scheme]),
- throw({forbidden, "Server cannot hash passwords at this time."})
+ throw({forbidden, ?PASSWORD_SERVER_ERROR})
+ end.
+
+% Validate if a new password matches all RegExp in the password_reqexp setting.
+% Throws if not.
+validate_password(ClearPassword) ->
+ case config:get("couch_httpd_auth", "password_reqexp", "") of
+ "" ->
+ ok;
+ "[]" ->
+ ok;
+ ValidateConfig ->
+ case couch_util:parse_term(ValidateConfig) of
+ {ok, RegExpList} when is_list(RegExpList) ->
+ % Check the password on every RegExp.
+ Loop = fun(RegExp) ->
+ check_password_with_regexp(ClearPassword, RegExp)
+ end,
+ lists:foreach(Loop, RegExpList),
+ ok;
+ {ok, NonListValue} ->
+ couch_log:error(
+ "[couch_httpd_auth] password_reqexp value of '~p' is invalid.",
+ [NonListValue]
+ ),
+ throw({forbidden, ?PASSWORD_SERVER_ERROR});
Review comment:
If in [`save_doc/1`](https://github.com/apache/couchdb/blob/776f920021dd15c51355e9a904bdc5a2250b19d9/src/couch/src/couch_users_db.erl#L89) the `[couch_httpd_auth] password_scheme` config has an invalid value, the user will receive an 403 status. I also used 403 for all instances where a part of `[couch_httpd_auth] password_regexp` is wrong.
In the latest commit, I changed the status code for "malformed" password to 400.
--
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] janl commented on a change in pull request #3483: password requirement
Posted by GitBox <gi...@apache.org>.
janl commented on a change in pull request #3483:
URL: https://github.com/apache/couchdb/pull/3483#discussion_r606803756
##########
File path: rel/overlay/etc/default.ini
##########
@@ -279,6 +279,8 @@ iterations = 10 ; iterations for password hashing
; min_iterations = 1
; max_iterations = 1000000000
; password_scheme = pbkdf2
+; List of tuples of an Erlang RegExp and an optional error message. Where a new password must match all RegExp. Example: [{".{10,}", "Password min length is 10 characters."}]
Review comment:
nitpick, please keep lines under 80 chars.
--
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