Skip to content

Return friendly error message when creating user with invalid password#1087

Merged
rnewson merged 2 commits intoapache:masterfrom
cloudant:issue-1051-create-user-with-non-string-pwd
Jan 6, 2018
Merged

Return friendly error message when creating user with invalid password#1087
rnewson merged 2 commits intoapache:masterfrom
cloudant:issue-1051-create-user-with-non-string-pwd

Conversation

@jiangphcn
Copy link
Copy Markdown
Contributor

Overview

Before this PR, couch returns unknown_error/function_clause when creating a user with non-string password.

curl -u foo:bar http://127.0.0.1:15984/_users  -X POST -H "Content-Type: application/json" -d @test.json
{"error":"unknown_error","reason":"function_clause","ref":1311610859}

This PR is aimed to return one friendly error message when creating user with invalid password.

curl -u foo:bar http://127.0.0.1:15984/_users  -X POST -H "Content-Type: application/json" -d @test.json
{"error":"forbidden","reason":"Password value of '123' is invalid."}

more test.json
{
    "_id": "org.couchdb.user:test",
    "name": "test",
    "type": "user",
    "roles": [],
    "password": 123
}

Testing recommendations

make check skip_deps+=couch_epi apps=chttpd tests=all_test_
==> chttpd (eunit)
    Running test function(s):
      chttpd_security_tests:all_sec_test_/0
======================== EUnit ========================
chttpd security tests
Application crypto was left running!
  chttpd_security_tests:120: should_allow_admin_db_compaction...[0.002 s] ok
  chttpd_security_tests:137: should_allow_valid_password_to_create_user...ok
  chttpd_security_tests:147: should_disallow_invalid_password_to_create_user...ok
  chttpd_security_tests:155: should_disallow_anonymous_db_compaction...ok
  chttpd_security_tests:163: should_disallow_db_member_db_compaction...ok
  chttpd_security_tests:166: should_allow_db_admin_db_compaction...[0.002 s] ok
  chttpd_security_tests:176: should_allow_admin_view_compaction...[0.008 s] ok
  chttpd_security_tests:191: should_disallow_anonymous_view_compaction...ok
  chttpd_security_tests:194: should_allow_admin_db_view_cleanup...[0.002 s] ok
  chttpd_security_tests:209: should_disallow_anonymous_db_view_cleanup...ok
[os_mon] cpu supervisor port (cpu_sup): Erlang has closed
  [done in 6.539 s]
=======================================================
  All 10 tests passed.

Related Issues or Pull Requests

issue #1051

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

Copy link
Copy Markdown
Member

@rnewson rnewson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good patch, well done. I think it would be good to do the same for invalid "salt" fields following the same method. I've approved this as it is but can re-review if you decide to add better errors for bad "salt" values.

simple(Password, Salt) when is_binary(Password), is_binary(Salt) ->
?l2b(couch_util:to_hex(crypto:hash(sha, <<Password/binary, Salt/binary>>))).
?l2b(couch_util:to_hex(crypto:hash(sha, <<Password/binary, Salt/binary>>)));
simple(Password, Salt) when is_binary(Salt) ->
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about another clause for when the salt field is not a string?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Added

{ok, Result} = pbkdf2(Password, Salt, Iterations, ?SHA1_OUTPUT_LENGTH),
Result.
Result;
pbkdf2(Password, Salt, Iterations) when is_binary(Salt),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above.

@jiangphcn
Copy link
Copy Markdown
Contributor Author

Thanks so much @rnewson for your review and comments. I added another clause for salt with commit d7218cc. Would you please review when you get time?

@jiangphcn jiangphcn force-pushed the issue-1051-create-user-with-non-string-pwd branch from d7218cc to f913ff7 Compare January 5, 2018 09:35
  - Return friendly error message instead of returning unknown_error
    and function_clause when creating a use with non-string password.

issue 1051
Copy link
Copy Markdown
Member

@rnewson rnewson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thanks!

@jiangphcn
Copy link
Copy Markdown
Contributor Author

Thanks again, Bob.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants