Check membership when calling get_security/1 in fabric2_db

Previously, membership check was disabled when fetching the security doc. That
was not correct, as membership should be checked before every db operation,
including when fetching the security doc itself.

Also, most of the security tests relied on patching the user context in the
`Db` handle then calling `check_*` functions. Those functions however call
`get_security/1` before doing the actual check, and in some cases, like when
checking for admin, the failure was coming from the membership check in
`get_security/1` instead. Also some tests were going through the regular
request path of opening a new db by name. In order, make the tests more
uniform, switch all the tests to apply the tested `UserCtx` in the open call.

2 files changed, 64 insertions, 73 deletions

diff --git a/src/fabric/src/fabric2_db.erl b/src/fabric/src/fabric2_db.erl
index d957ec954..88840e702 100644
--- a/src/fabric/src/fabric2_db.erl
+++ b/src/fabric/src/fabric2_db.erl
@@ -22,7 +22,6 @@
     list_dbs/1,
     list_dbs/3,
 
-    is_admin/1,
     check_is_admin/1,
     check_is_member/1,
 
@@ -239,10 +238,6 @@ list_dbs(UserFun, UserAcc0, Options) ->
     end).
 
 
-is_admin(Db) ->
-    is_admin(Db, get_security(Db)).
-
-
 is_admin(Db, {SecProps}) when is_list(SecProps) ->
     case fabric2_db_plugin:check_is_admin(Db) of
         true ->
@@ -291,10 +286,6 @@ require_member_check(#{} = Db) ->
     Db#{security_fun := fun check_is_member/2}.
 
 
-no_security_check(#{} = Db) ->
-    Db#{security_fun := undefined}.
-
-
 name(#{name := DbName}) ->
     DbName.
 
@@ -382,7 +373,7 @@ get_revs_limit(#{} = Db) ->
 
 get_security(#{} = Db) ->
     #{security_doc := SecDoc} = fabric2_fdb:transactional(Db, fun(TxDb) ->
-        fabric2_fdb:ensure_current(no_security_check(TxDb))
+        fabric2_fdb:ensure_current(TxDb)
     end),
     SecDoc.
 
diff --git a/src/fabric/test/fabric2_db_security_tests.erl b/src/fabric/test/fabric2_db_security_tests.erl
index e5f3ad2c0..501545484 100644
--- a/src/fabric/test/fabric2_db_security_tests.erl
+++ b/src/fabric/test/fabric2_db_security_tests.erl
@@ -26,12 +26,10 @@ security_test_() ->
             fun setup/0,
             fun cleanup/1,
             {with, [
-                fun is_admin_name/1,
-                fun is_not_admin_name/1,
-                fun is_admin_role/1,
-                fun is_not_admin_role/1,
                 fun check_is_admin/1,
                 fun check_is_not_admin/1,
+                fun check_is_admin_role/1,
+                fun check_is_not_admin_role/1,
                 fun check_is_member_name/1,
                 fun check_is_not_member_name/1,
                 fun check_is_member_role/1,
@@ -63,123 +61,125 @@ setup() ->
         ]}}
     ]},
     ok = fabric2_db:set_security(Db1, SecProps),
-    {ok, Db2} = fabric2_db:open(DbName, [?ADMIN_CTX]),
-    {ok, PubDb} = fabric2_db:create(PubDbName, []),
-    {Db2, PubDb, Ctx}.
+    {ok, _} = fabric2_db:create(PubDbName, [?ADMIN_CTX]),
+    {DbName, PubDbName, Ctx}.
 
 
-cleanup({Db, PubDb, Ctx}) ->
-    ok = fabric2_db:delete(fabric2_db:name(Db), []),
-    ok = fabric2_db:delete(fabric2_db:name(PubDb), []),
+cleanup({DbName, PubDbName, Ctx}) ->
+    ok = fabric2_db:delete(DbName, []),
+    ok = fabric2_db:delete(PubDbName, []),
     test_util:stop_couch(Ctx).
 
 
-is_admin_name({Db, _, _}) ->
-    UserCtx = #user_ctx{name = <<"admin_name1">>},
-    ?assertEqual(true, fabric2_db:is_admin(Db#{user_ctx := UserCtx})).
-
-
-is_not_admin_name({Db, _, _}) ->
-    UserCtx = #user_ctx{name = <<"member1">>},
-    ?assertEqual(false, fabric2_db:is_admin(Db#{user_ctx := UserCtx})).
 
+check_is_admin({DbName, _, _}) ->
+    UserCtx = #user_ctx{name = <<"admin_name1">>},
+    {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
+    ?assertEqual(ok, fabric2_db:check_is_admin(Db)).
 
-is_admin_role({Db, _, _}) ->
-    UserCtx = #user_ctx{roles = [<<"admin_role1">>]},
-    ?assertEqual(true, fabric2_db:is_admin(Db#{user_ctx := UserCtx})).
 
+check_is_not_admin({DbName, _, _}) ->
+    {ok, Db1} = fabric2_db:open(DbName, [{user_ctx, #user_ctx{}}]),
+    ?assertThrow(
+        {unauthorized, <<"You are not authorized", _/binary>>},
+        fabric2_db:check_is_admin(Db1)
+    ),
 
-is_not_admin_role({Db, _, _}) ->
-    UserCtx = #user_ctx{roles = [<<"member_role1">>]},
-    ?assertEqual(false, fabric2_db:is_admin(Db#{user_ctx := UserCtx})).
+    UserCtx = #user_ctx{name = <<"member_name1">>},
+    {ok, Db2} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
+    ?assertThrow(
+        {forbidden, <<"You are not a db or server admin.">>},
+        fabric2_db:check_is_admin(Db2)
+    ).
 
 
-check_is_admin({Db, _, _}) ->
-    UserCtx = #user_ctx{name = <<"admin_name1">>},
-    ?assertEqual(ok, fabric2_db:check_is_admin(Db#{user_ctx := UserCtx})).
+check_is_admin_role({DbName, _, _}) ->
+    UserCtx = #user_ctx{roles = [<<"admin_role1">>]},
+    {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
+    ?assertEqual(ok, fabric2_db:check_is_admin(Db)).
 
 
-check_is_not_admin({Db, _, _}) ->
-    UserCtx = #user_ctx{name = <<"member_name1">>},
-    ?assertThrow(
-        {unauthorized, <<"You are not a db or server admin.">>},
-        fabric2_db:check_is_admin(Db#{user_ctx := #user_ctx{}})
-    ),
+check_is_not_admin_role({DbName, _, _}) ->
+    UserCtx = #user_ctx{
+        name = <<"member_name1">>,
+        roles = [<<"member_role1">>]
+    },
+    {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
     ?assertThrow(
         {forbidden, <<"You are not a db or server admin.">>},
-        fabric2_db:check_is_admin(Db#{user_ctx := UserCtx})
+        fabric2_db:check_is_admin(Db)
     ).
 
 
-check_is_member_name({Db, _, _}) ->
+check_is_member_name({DbName, _, _}) ->
     UserCtx = #user_ctx{name = <<"member_name1">>},
-    ?assertEqual(ok, fabric2_db:check_is_member(Db#{user_ctx := UserCtx})).
+    {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
+    ?assertEqual(ok, fabric2_db:check_is_member(Db)).
 
 
-check_is_not_member_name({Db, _, _}) ->
-    UserCtx = #user_ctx{name = <<"foo">>},
+check_is_not_member_name({DbName, _, _}) ->
+    {ok, Db1} = fabric2_db:open(DbName, [{user_ctx, #user_ctx{}}]),
     ?assertThrow(
         {unauthorized, <<"You are not authorized", _/binary>>},
-        fabric2_db:check_is_member(Db#{user_ctx := #user_ctx{}})
+        fabric2_db:check_is_member(Db1)
     ),
+
+    UserCtx = #user_ctx{name = <<"foo">>},
+    {ok, Db2} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
     ?assertThrow(
         {forbidden, <<"You are not allowed to access", _/binary>>},
-        fabric2_db:check_is_member(Db#{user_ctx := UserCtx})
+        fabric2_db:check_is_member(Db2)
     ).
 
 
-check_is_member_role({Db, _, _}) ->
+check_is_member_role({DbName, _, _}) ->
     UserCtx = #user_ctx{name = <<"foo">>, roles = [<<"member_role1">>]},
-    ?assertEqual(ok, fabric2_db:check_is_member(Db#{user_ctx := UserCtx})).
+    {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
+    ?assertEqual(ok, fabric2_db:check_is_member(Db)).
 
 
-check_is_not_member_role({Db, _, _}) ->
+check_is_not_member_role({DbName, _, _}) ->
     UserCtx = #user_ctx{name = <<"foo">>, roles = [<<"bar">>]},
+    {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
     ?assertThrow(
         {forbidden, <<"You are not allowed to access", _/binary>>},
-        fabric2_db:check_is_member(Db#{user_ctx := UserCtx})
+        fabric2_db:check_is_member(Db)
     ).
 
 
-check_admin_is_member({Db, _, _}) ->
+check_admin_is_member({DbName, _, _}) ->
     UserCtx = #user_ctx{name = <<"admin_name1">>},
-    ?assertEqual(ok, fabric2_db:check_is_member(Db#{user_ctx := UserCtx})).
+    {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
+    ?assertEqual(ok, fabric2_db:check_is_member(Db)).
 
 
-check_is_member_of_public_db({_, PubDb, _}) ->
+check_is_member_of_public_db({_, PubDbName, _}) ->
+    {ok, Db1} = fabric2_db:open(PubDbName, [{user_ctx, #user_ctx{}}]),
+    ?assertEqual(ok, fabric2_db:check_is_member(Db1)),
+
     UserCtx = #user_ctx{name = <<"foo">>, roles = [<<"bar">>]},
-    ?assertEqual(
-        ok,
-        fabric2_db:check_is_member(PubDb#{user_ctx := #user_ctx{}})
-    ),
-    ?assertEqual(
-        ok,
-        fabric2_db:check_is_member(PubDb#{user_ctx := UserCtx})
-    ).
+    {ok, Db2} = fabric2_db:open(PubDbName, [{user_ctx, UserCtx}]),
+    ?assertEqual(ok, fabric2_db:check_is_member(Db2)).
 
 
-check_set_user_ctx({Db0, _, _}) ->
-    DbName = fabric2_db:name(Db0),
+check_set_user_ctx({DbName, _, _}) ->
     UserCtx = #user_ctx{name = <<"foo">>, roles = [<<"admin_role1">>]},
     {ok, Db1} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
     ?assertEqual(UserCtx, fabric2_db:get_user_ctx(Db1)).
 
 
-check_forbidden({Db0, _, _}) ->
-    DbName = fabric2_db:name(Db0),
+check_forbidden({DbName, _, _}) ->
     UserCtx = #user_ctx{name = <<"foo">>, roles = [<<"bar">>]},
     {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
     ?assertThrow({forbidden, _}, fabric2_db:get_db_info(Db)).
 
 
-check_fail_no_opts({Db0, _, _}) ->
-    DbName = fabric2_db:name(Db0),
+check_fail_no_opts({DbName, _, _}) ->
     {ok, Db} = fabric2_db:open(DbName, []),
     ?assertThrow({unauthorized, _}, fabric2_db:get_db_info(Db)).
 
 
-check_fail_name_null({Db0, _, _}) ->
-    DbName = fabric2_db:name(Db0),
+check_fail_name_null({DbName, _, _}) ->
     UserCtx = #user_ctx{name = null},
     {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
     ?assertThrow({unauthorized, _}, fabric2_db:get_db_info(Db)).