Check security properties in the main transaction

Previously we checked security properties in a separate transaction, after
opening the db or fetching it from the cache. To avoid running an extra
transaction move the check inside the main transaction right after the metadata
check runs. That ensure it will be consistent and it won't be accidentally
missed as all operations run the `ensure_current` metadata check.

Also remove the special `get_config/2` function in `fabric2_fdb` for getting
revs limit and security properties and just read them directly from the db map.

5 files changed, 73 insertions, 62 deletions

diff --git a/src/chttpd/src/chttpd_auth_request.erl b/src/chttpd/src/chttpd_auth_request.erl
index 7210905f0..26fee17c3 100644
--- a/src/chttpd/src/chttpd_auth_request.erl
+++ b/src/chttpd/src/chttpd_auth_request.erl
@@ -102,9 +102,8 @@ server_authorization_check(#httpd{method=Method, path_parts=[<<"_utils">>|_]}=Re
 server_authorization_check(#httpd{path_parts=[<<"_", _/binary>>|_]}=Req) ->
     require_admin(Req).
 
-db_authorization_check(#httpd{path_parts=[DbName|_],user_ctx=Ctx}=Req) ->
-    {ok, Db} = fabric2_db:open(DbName, [{user_ctx, Ctx}]),
-    fabric2_db:check_is_member(Db),
+db_authorization_check(#httpd{path_parts=[_DbName|_]}=Req) ->
+    % Db authorization checks are performed in fabric before every FDB operation
     Req.
 
 require_admin(Req) ->
diff --git a/src/fabric/src/fabric2_db.erl b/src/fabric/src/fabric2_db.erl
index 668d26d28..ab122ed2a 100644
--- a/src/fabric/src/fabric2_db.erl
+++ b/src/fabric/src/fabric2_db.erl
@@ -176,19 +176,18 @@ open(DbName, Options) ->
     case fabric2_server:fetch(DbName) of
         #{} = Db ->
             Db1 = maybe_set_user_ctx(Db, Options),
-            ok = check_is_member(Db1),
-            {ok, Db1};
+            {ok, require_member_check(Db1)};
         undefined ->
             Result = fabric2_fdb:transactional(DbName, Options, fun(TxDb) ->
                 fabric2_fdb:open(TxDb, Options)
             end),
             % Cache outside the transaction retry loop
             case Result of
-                #{security_doc := SecDoc} = Db0 ->
-                    ok = check_is_member(Db0, SecDoc),
+                #{} = Db0 ->
                     Db1 = maybe_add_sys_db_callbacks(Db0),
                     ok = fabric2_server:store(Db1),
-                    {ok, Db1#{tx := undefined}};
+                    Db2 = Db1#{tx := undefined},
+                    {ok, require_member_check(Db2)};
                 Error ->
                     Error
             end
@@ -256,7 +255,11 @@ is_admin(Db, {SecProps}) when is_list(SecProps) ->
 
 
 check_is_admin(Db) ->
-    case is_admin(Db) of
+    check_is_admin(Db, get_security(Db)).
+
+
+check_is_admin(Db, SecDoc) ->
+    case is_admin(Db, SecDoc) of
         true ->
             ok;
         false ->
@@ -280,6 +283,18 @@ check_is_member(Db, SecDoc) ->
     end.
 
 
+require_admin_check(#{} = Db) ->
+    Db#{security_fun := fun check_is_admin/2}.
+
+
+require_member_check(#{} = Db) ->
+    Db#{security_fun := fun check_is_member/2}.
+
+
+no_security_check(#{} = Db) ->
+    Db#{security_fun := undefined}.
+
+
 name(#{name := DbName}) ->
     DbName.
 
@@ -359,21 +374,17 @@ get_pid(#{}) ->
 
 
 get_revs_limit(#{} = Db) ->
-    fabric2_fdb:transactional(Db, fun(TxDb) ->
-        #{
-            revs_limit := RevsLimit
-        } = fabric2_fdb:ensure_current(TxDb),
-        RevsLimit
-    end).
+    #{revs_limit := RevsLimit} = fabric2_fdb:transactional(Db, fun(TxDb) ->
+        fabric2_fdb:ensure_current(TxDb)
+    end),
+    RevsLimit.
 
 
 get_security(#{} = Db) ->
-    fabric2_fdb:transactional(Db, fun(TxDb) ->
-        #{
-            security_doc := Security
-        } = fabric2_fdb:ensure_current(TxDb),
-        Security
-    end).
+    #{security_doc := SecDoc} = fabric2_fdb:transactional(Db, fun(TxDb) ->
+        fabric2_fdb:ensure_current(no_security_check(TxDb))
+    end),
+    SecDoc.
 
 
 get_update_seq(#{} = Db) ->
@@ -444,26 +455,26 @@ is_users_db(DbName) when is_binary(DbName) ->
     IsAuthCache orelse IsCfgUsersDb orelse IsGlobalUsersDb.
 
 
-set_revs_limit(#{} = Db, RevsLimit) ->
-    check_is_admin(Db),
+set_revs_limit(#{} = Db0, RevsLimit) ->
+    Db1 = require_admin_check(Db0),
     RevsLimBin = ?uint2bin(max(1, RevsLimit)),
-    Resp = fabric2_fdb:transactional(Db, fun(TxDb) ->
-        fabric2_fdb:set_config(TxDb, <<"revs_limit">>, RevsLimBin)
+    {Resp, Db2} = fabric2_fdb:transactional(Db1, fun(TxDb) ->
+        {fabric2_fdb:set_config(TxDb, <<"revs_limit">>, RevsLimBin), TxDb}
     end),
     if Resp /= ok -> Resp; true ->
-        fabric2_server:store(Db#{revs_limit := RevsLimit})
+        fabric2_server:store(Db2#{revs_limit := RevsLimit})
     end.
 
 
-set_security(#{} = Db, Security) ->
-    check_is_admin(Db),
+set_security(#{} = Db0, Security) ->
+    Db1 = require_admin_check(Db0),
     ok = fabric2_util:validate_security_object(Security),
     SecBin = ?JSON_ENCODE(Security),
-    Resp = fabric2_fdb:transactional(Db, fun(TxDb) ->
-        fabric2_fdb:set_config(TxDb, <<"security_doc">>, SecBin)
+    {Resp, Db2} = fabric2_fdb:transactional(Db1, fun(TxDb) ->
+        {fabric2_fdb:set_config(TxDb, <<"security_doc">>, SecBin), TxDb}
     end),
     if Resp /= ok -> Resp; true ->
-        fabric2_server:store(Db#{security_doc := Security})
+        fabric2_server:store(Db2#{security_doc := Security})
     end.
 
 
diff --git a/src/fabric/src/fabric2_fdb.erl b/src/fabric/src/fabric2_fdb.erl
index 2945ee93e..3db0cde7b 100644
--- a/src/fabric/src/fabric2_fdb.erl
+++ b/src/fabric/src/fabric2_fdb.erl
@@ -22,7 +22,7 @@
 
     create/2,
     open/2,
-    reopen/1,
+    ensure_current/1,
     delete/1,
     exists/1,
 
@@ -34,7 +34,6 @@
 
     get_info/1,
     get_config/1,
-    get_config/2,
     set_config/3,
 
     get_stat/2,
@@ -323,10 +322,12 @@ reopen(#{} = OldDb) ->
         tx := Tx,
         name := DbName,
         db_options := Options,
-        user_ctx := UserCtx
+        user_ctx := UserCtx,
+        security_fun := SecurityFun
     } = OldDb,
     Options1 = lists:keystore(user_ctx, 1, Options, {user_ctx, UserCtx}),
-    open(init_db(Tx, DbName, Options1), Options1).
+    NewDb = open(init_db(Tx, DbName, Options1), Options1),
+    NewDb#{security_fun := SecurityFun}.
 
 
 delete(#{} = Db) ->
@@ -437,19 +438,6 @@ get_config(#{} = Db) ->
     end, erlfdb:wait(Future)).
 
 
-get_config(#{} = Db, ConfigKey) ->
-    #{
-        tx := Tx,
-        db_prefix := DbPrefix
-    } = ensure_current(Db),
-
-    Key = erlfdb_tuple:pack({?DB_CONFIG, ConfigKey}, DbPrefix),
-    case erlfdb:wait(erlfdb:get(Tx, Key)) of
-        % config values are expected to be set so we blow if not_found
-        Val when Val =/= not_found -> Val
-    end.
-
-
 set_config(#{} = Db, ConfigKey, ConfigVal) ->
     #{
         tx := Tx,
@@ -912,6 +900,7 @@ init_db(Tx, DbName, Options) ->
         layer_prefix => Prefix,
         md_version => Version,
 
+        security_fun => undefined,
         db_options => Options
     }.
 
@@ -1353,12 +1342,20 @@ ensure_current(Db) ->
     ensure_current(Db, true).
 
 
-ensure_current(#{} = Db, CheckDbVersion) ->
-    require_transaction(Db),
-    case check_metadata_version(Db) of
+ensure_current(#{} = Db0, CheckDbVersion) ->
+    require_transaction(Db0),
+    Db2 = case check_metadata_version(Db0) of
         {current, Db1} -> Db1;
         {stale, Db1} -> check_db_version(Db1, CheckDbVersion)
-    end.
+    end,
+    case maps:get(security_fun, Db2) of
+        SecurityFun when is_function(SecurityFun, 2) ->
+            #{security_doc := SecDoc} = Db2,
+            ok = SecurityFun(Db2, SecDoc);
+        undefined ->
+            ok
+    end,
+    Db2.
 
 
 is_transaction_applied(Tx) ->
diff --git a/src/fabric/src/fabric2_server.erl b/src/fabric/src/fabric2_server.erl
index b5e2ee927..866e338bc 100644
--- a/src/fabric/src/fabric2_server.erl
+++ b/src/fabric/src/fabric2_server.erl
@@ -59,7 +59,8 @@ fetch(DbName) when is_binary(DbName) ->
 store(#{name := DbName} = Db0) when is_binary(DbName) ->
     Db1 = Db0#{
         tx := undefined,
-        user_ctx := #user_ctx{}
+        user_ctx := #user_ctx{},
+        security_fun := undefined
     },
     true = ets:insert(?MODULE, {DbName, Db1}),
     ok.
diff --git a/src/fabric/test/fabric2_db_security_tests.erl b/src/fabric/test/fabric2_db_security_tests.erl
index 4a54083ac..e5f3ad2c0 100644
--- a/src/fabric/test/fabric2_db_security_tests.erl
+++ b/src/fabric/test/fabric2_db_security_tests.erl
@@ -39,9 +39,9 @@ security_test_() ->
                 fun check_admin_is_member/1,
                 fun check_is_member_of_public_db/1,
                 fun check_set_user_ctx/1,
-                fun check_open_forbidden/1,
-                fun check_fail_open_no_opts/1,
-                fun check_fail_open_name_null/1
+                fun check_forbidden/1,
+                fun check_fail_no_opts/1,
+                fun check_fail_name_null/1
             ]}
         }
     }.
@@ -165,18 +165,21 @@ check_set_user_ctx({Db0, _, _}) ->
     ?assertEqual(UserCtx, fabric2_db:get_user_ctx(Db1)).
 
 
-check_open_forbidden({Db0, _, _}) ->
+check_forbidden({Db0, _, _}) ->
     DbName = fabric2_db:name(Db0),
     UserCtx = #user_ctx{name = <<"foo">>, roles = [<<"bar">>]},
-    ?assertThrow({forbidden, _}, fabric2_db:open(DbName, [{user_ctx, UserCtx}])).
+    {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
+    ?assertThrow({forbidden, _}, fabric2_db:get_db_info(Db)).
 
 
-check_fail_open_no_opts({Db0, _, _}) ->
+check_fail_no_opts({Db0, _, _}) ->
     DbName = fabric2_db:name(Db0),
-    ?assertThrow({unauthorized, _}, fabric2_db:open(DbName, [])).
+    {ok, Db} = fabric2_db:open(DbName, []),
+    ?assertThrow({unauthorized, _}, fabric2_db:get_db_info(Db)).
 
 
-check_fail_open_name_null({Db0, _, _}) ->
+check_fail_name_null({Db0, _, _}) ->
     DbName = fabric2_db:name(Db0),
     UserCtx = #user_ctx{name = null},
-    ?assertThrow({unauthorized, _}, fabric2_db:open(DbName, [{user_ctx, UserCtx}])).
+    {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
+    ?assertThrow({unauthorized, _}, fabric2_db:get_db_info(Db)).