Fix function_clause exception on invalid DB security objects #1770

wenli200133 · 2018-11-28T13:57:23Z

Overview

This PR will fix function_clause error on invalid DB security objects.

for example when the request body of PUT db/_security endpoint is not a correct json format, if you check the log file you will find the function_clause error.

[error] 2018-06-13T16:50:50.479454Z node1@127.0.0.1 <0.3642.0> 3108587a16 rpc couch_db:set_security/2 function_clause [{couch_db,validate_names_and_roles,[[<<"role_a">>]],[{file,"src/couch_db.erl"},{line,594}]},{couch_db,validate_security_object,1,[{file,"src/couch_db.erl"},{line,590}]},{couch_db,set_security,2,[{file,"src/couch_db.erl"},{line,574}]},{fabric_rpc,with_db,3,[{file,"src/fabric_rpc.erl"},{line,289}]},{rexi_server,init_p,3,[{file,"src/rexi_server.erl"},{line,139}]}]                                                                              [error] 2018-06-13T16:50:50.479648Z node1@127.0.0.1 <0.3576.0> 3108587a16 Failed to set security object on {'node1@127.0.0.1',<<"shards/40000000-5fffffff/abc.1528908305">>} :: {error,function_clause}                                         [error] 2018-06-13T16:50:50.480220Z node1@127.0.0.1 <0.3647.0> 3108587a16 rpc couch_db:set_security/2 function_clause [{couch_db,validate_names_and_roles,[[<<"role_a">>]],[{file,"src/couch_db.erl"},{line,594}]},{couch_db,validate_security_object,1,[{file,"src/couch_db.erl"},{line,590}]},{couch_db,set_security,2,[{file,"src/couch_db.erl"},{line,574}]},{fabric_rpc,with_db,3,[{file,"src/fabric_rpc.erl"},{line,289}]},{rexi_server,init_p,3,[{file,"src/rexi_server.erl"},{line,139}]}]                                                                              [error] 2018-06-13T16:50:50.480371Z node1@127.0.0.1 <0.3646.0> 3108587a16 rpc couch_db:set_security/2 function_clause [
{couch_db,validate_names_and_roles,[[<<"role_a">>]],[{file,"src/couch_db.erl"},{line,594}]},
{couch_db,validate_security_object,1,[{file,"src/couch_db.erl"},{line,590}]},
{couch_db,set_security,2,[{file,"src/couch_db.erl"},{line,574}]},
{fabric_rpc,with_db,3,[{file,"src/fabric_rpc.erl"},{line,289}]},
{rexi_server,init_p,3,[{file,"src/rexi_server.erl"},{line,139}]}]

after fix it, function_clause error full stack trace will disappeared instead of

[error] 2018-11-29T02:30:24.013166Z node1@127.0.0.1 <0.2699.0> 985a72f160 Failed to set security object on {'node1@127.0.0.1',<<"shards/e0000000-ffffffff/test.1543202135">>} :: "admins or members must be a JSON list of strings"

Testing recommendations

case

test with incorrect members format
curl -i -X PUT http://localhost:5984/test/_security -u "adm:pass" -H 'accept: application/json' -d '{"members":["foo"]}'
check logs:

node2.log:8979:[error] 2018-11-29T14:08:05.480944Z node2@127.0.0.1 <0.550.0> 71a87732c7 Failed to set security object on {'node3@127.0.0.1',<<"shards/c0000000-dfffffff/test.1543202135">>} :: "admins or members must be a JSON list of strings"

return
{"error":"error","reason":"no_majority"}

test with incorrect members roles format

curl -i -X PUT http://localhost:15984/test/_security -u "adm:pass" -H 'accept: application/json' -d '{"members":{"names": ["user1","user2"],"roles": "developers"}}'

check log:

node1.log:19740:[error] 2018-11-29T14:03:44.740886Z node1@127.0.0.1 <0.5831.0> 76fbe3cd55 Failed to set security object on {'node2@127.0.0.1',<<"shards/e0000000-ffffffff/test.1543202135">>} :: "roles field vallue must be a list of strings"

return
{"error":"error","reason":"no_majority"}

test with correct members roles and names format
curl -i -X PUT http://localhost:5984/test/_security -u "adm:pass" -H 'accept: application/json' -d '{"members":{"names": ["user1","user2"],"roles": ["developers"]}}'

return
{"ok":true}

test with correct admins and members roles and names format
curl -i -X PUT http://localhost:5984/test/_security -u "adm:pass" -H 'accept: application/json' -d '{"admins":{"roles":["admins"],"names":["foo"]},"members":{"names": ["user1","user2"],"roles": ["developers"]}}'

return
{"ok":true}

Related Issues or Pull Requests

#1384

Checklist

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

eiri

Your tests are broken. You can easily check this by changing any assertion to invalid and see that tests still pass. In general it's a good practice to start writing tests from a fallen state to be sure you actually have them testing something.

The issue here is that all ?_ macro return test objects, i.e. functions and you are overwrapping your tests with ?_test, so you have functions, that return function that return functions. Naturally that tests nothing.

Remove all ?_test wrappers from your tests.
Don't mix ?_ and ? macro in a same test, as your are doing in should_return_error_for_security_object_with_incorrect_roles for example, this is incorrect by definition.
Change all the assertion to ?_ ones and return them as a list at the end of your function Instantiators where you have multiple assertions.

Also the test names are exceeding 80 chars. We are usually more relaxed on that in the tests, but it'd be good if you can rename security_object to sec_obj, that doesn't change clarity of the test naming much, but making code length more reasonable.

eiri

A good fix for a real issue. Requires a bit different approach, but overall - good job. Really appreciate the added tests.

eiri · 2018-11-28T15:49:37Z

src/chttpd/test/chttpd_security_tests.erl

+    {ok, Status, _, RespBody} = test_request:put(SecurityUrl, [?CONTENT_JSON, ?AUTH], Body),
+    ?_assertEqual(500, Status),
+    ResultJson = ?JSON_DECODE(RespBody),
+    ?assertMatch({[


Use ?_ assertEqual here, you are not checking guarded pattern, but objects equality.

eiri · 2018-11-28T15:49:48Z

src/chttpd/test/chttpd_security_tests.erl

+    {ok, Status, _, RespBody} = test_request:put(SecurityUrl, [?CONTENT_JSON, ?AUTH], Body),
+    ?_assertEqual(500, Status),
+    ResultJson = ?JSON_DECODE(RespBody),
+    ?assertMatch({[


Same as above

eiri · 2018-11-28T15:49:58Z

src/chttpd/test/chttpd_security_tests.erl

+    {ok, Status, _, RespBody} = test_request:put(SecurityUrl, [?CONTENT_JSON, ?AUTH], Body),
+    ?_assertEqual(500, Status),
+    ResultJson = ?JSON_DECODE(RespBody),
+    ?assertMatch({[


Same as above

eiri · 2018-11-28T15:58:34Z

src/couch/src/couch_db.erl

@@ -730,12 +730,22 @@ validate_security_object(SecProps) ->
    % we fallback to readers here for backwards compatibility
    Members = couch_util:get_value(<<"members">>, SecProps,
        couch_util:get_value(<<"readers">>, SecProps, {[]})),
-    ok = validate_names_and_roles(Admins),
-    ok = validate_names_and_roles(Members),
+    case Admins of


This is inefficient way to go about this, you are essentially splitting validation logic in two places instead of fixing it in the dedicated validator.

Better (and shorter) approach would be to add a "catch all" validator function to raise errors, like this:

@@ -759,6 +749,8 @@ validate_names_and_roles(Props) when is_list(Props) -> _ -> throw("roles must be a JSON list of strings") end, ok. +validate_names_and_roles(_) -> + throw("members must be a JSON list of strings").

eiri · 2018-11-28T16:05:03Z

One more thing - can you please change your commit message (and PR titile) to something more descriptive? It'll read really cryptic to people out of context when merged. Something like a title of the original issue - "Fix function_clause exception on invalid DB security objects" or similar?

wenli200133 · 2018-11-29T04:44:34Z

Hi @eiri Could you help me review again if you have a free time, I updated the codes according to your comments, thank you for your time.

jiangphcn · 2018-11-30T02:03:28Z

generally, looks better after adding @eiri 's comments. Just minor comments, there are still some lines longer than 80 characters.

eiri

Almost there, but some tests still need fixing for return of multiple assertion and usage of Equal vs Match

src/chttpd/test/chttpd_security_tests.erl

src/couch/src/couch_db.erl

wenli200133 · 2018-12-03T14:36:19Z

Hi @eiri could you help review again? thanks.

eiri

Looks good now. Needs squashing before merging, obviously.

wohali · 2018-12-07T21:49:21Z

@wenli200133 Please edit the description so it says "Closes #1384" so the ticket auto-closes when this merges. Thank you for a great PR!

@jiangphcn You can go ahead and merge this on @wenli200133 's behalf. Be sure #1384 gets closed.

- fix function_clause error on invalid DB security objects when the request body of PUT db/_security endpoint is not a correct json format Closes apache#1384

jiangphcn · 2018-12-08T02:05:43Z

@wohali sure, just rebased and merged this PR. Looks that #1384 is auto-closed.

@wenli200133 thanks for your great work.

wenli200133 changed the title ~~fix couchdb 1384 function clause error~~ Fix couchdb 1384 function clause error Nov 28, 2018

eiri requested changes Nov 28, 2018

View reviewed changes

wenli200133 changed the title ~~Fix couchdb 1384 function clause error~~ Fix function_clause exception on invalid DB security objects Nov 29, 2018

eiri requested changes Nov 30, 2018

View reviewed changes

wenli200133 force-pushed the COUCHDB-1384-function-clause-error branch 2 times, most recently from 3b10fc8 to fa45642 Compare December 6, 2018 03:23

eiri approved these changes Dec 7, 2018

View reviewed changes

jiangphcn force-pushed the COUCHDB-1384-function-clause-error branch from fa45642 to 2e52cb5 Compare December 8, 2018 01:22

Fix function_clause error

fcb272e

- fix function_clause error on invalid DB security objects when the request body of PUT db/_security endpoint is not a correct json format Closes apache#1384

jiangphcn force-pushed the COUCHDB-1384-function-clause-error branch from 2e52cb5 to fcb272e Compare December 8, 2018 01:25

jiangphcn merged commit 2ab1e87 into apache:master Dec 8, 2018

wohali mentioned this pull request Feb 7, 2019

2.3.1 Release Proposal #1908

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix function_clause exception on invalid DB security objects #1770

Fix function_clause exception on invalid DB security objects #1770

wenli200133 commented Nov 28, 2018 •

edited

eiri left a comment

eiri left a comment

eiri Nov 28, 2018

eiri Nov 28, 2018

eiri Nov 28, 2018

eiri Nov 28, 2018

eiri commented Nov 28, 2018

wenli200133 commented Nov 29, 2018

jiangphcn commented Nov 30, 2018

eiri left a comment

wenli200133 commented Dec 3, 2018

eiri left a comment

wohali commented Dec 7, 2018

jiangphcn commented Dec 8, 2018

Fix function_clause exception on invalid DB security objects #1770

Fix function_clause exception on invalid DB security objects #1770

Conversation

wenli200133 commented Nov 28, 2018 • edited

Overview

Testing recommendations

Related Issues or Pull Requests

Checklist

eiri left a comment

Choose a reason for hiding this comment

eiri left a comment

Choose a reason for hiding this comment

eiri Nov 28, 2018

Choose a reason for hiding this comment

eiri Nov 28, 2018

Choose a reason for hiding this comment

eiri Nov 28, 2018

Choose a reason for hiding this comment

eiri Nov 28, 2018

Choose a reason for hiding this comment

eiri commented Nov 28, 2018

wenli200133 commented Nov 29, 2018

jiangphcn commented Nov 30, 2018

eiri left a comment

Choose a reason for hiding this comment

wenli200133 commented Dec 3, 2018

eiri left a comment

Choose a reason for hiding this comment

wohali commented Dec 7, 2018

jiangphcn commented Dec 8, 2018

wenli200133 commented Nov 28, 2018 •

edited