You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@couchdb.apache.org by va...@apache.org on 2019/09/19 22:52:34 UTC
[couchdb] branch prototype/fdb-layer updated: Check members after
db is opened
This is an automated email from the ASF dual-hosted git repository.
vatamane pushed a commit to branch prototype/fdb-layer
in repository https://gitbox.apache.org/repos/asf/couchdb.git
The following commit(s) were added to refs/heads/prototype/fdb-layer by this push:
new 2051bd6 Check members after db is opened
2051bd6 is described below
commit 2051bd6e610a09d40cd86c1e0694d33be58dc26e
Author: Nick Vatamaniuc <va...@apache.org>
AuthorDate: Thu Sep 19 16:42:35 2019 -0400
Check members after db is opened
This brings this to parity with master `couch_db:open/2` logic:
https://github.com/apache/couchdb/blob/master/src/couch/src/couch_db.erl#L166
There are two separate cases that have to be handled:
1) Db was already opened and cached. In that case, call `check_is_member(Db)`
which reads the security from the Db to ensure we don't authorize against a
stale security doc. Otherwise, the delay could be as long as the last write
that went through that node. A potential future optimization could be to have
a timestamp and only get the new security context if last refresh hasn't
happened recently.
2) Db was not cached, and was just opened. To avoid running another two read
transactions to get the security doc after the main transaction finished, call
a version of check_is_member which gets the security doc passed in as an
argument.
As a bonus, `check_is_members(Db, SecDoc)` version ends up saving one extra
security read since we don't read twice in is_member and is_admin calls.
`delete/2` was updated to pass ?ADMIN_CTX to `open/2` since it only cares about
getting a `database_does_not_exist` error thrown. There is a check for server
admin at the HTTP API level that would care of authentication / authorization.
---
src/fabric/src/fabric2_db.erl | 29 ++++++++++++++++++---------
src/fabric/test/fabric2_db_security_tests.erl | 26 ++++++++++++++++++++----
2 files changed, 42 insertions(+), 13 deletions(-)
diff --git a/src/fabric/src/fabric2_db.erl b/src/fabric/src/fabric2_db.erl
index 8927ce3..a316517 100644
--- a/src/fabric/src/fabric2_db.erl
+++ b/src/fabric/src/fabric2_db.erl
@@ -175,14 +175,17 @@ create(DbName, Options) ->
open(DbName, Options) ->
case fabric2_server:fetch(DbName) of
#{} = Db ->
- {ok, maybe_set_user_ctx(Db, Options)};
+ Db1 = maybe_set_user_ctx(Db, Options),
+ ok = check_is_member(Db1),
+ {ok, 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
- #{} = Db0 ->
+ #{security_doc := SecDoc} = Db0 ->
+ ok = check_is_member(Db0, SecDoc),
Db1 = maybe_add_sys_db_callbacks(Db0),
ok = fabric2_server:store(Db1),
{ok, Db1#{tx := undefined}};
@@ -193,8 +196,10 @@ open(DbName, Options) ->
delete(DbName, Options) ->
- % This will throw if the db does not exist
- {ok, Db} = open(DbName, Options),
+ % Delete doesn't check user_ctx, that's done at the HTTP API level
+ % here we just care to get the `database_does_not_exist` error thrown
+ Options1 = lists:keystore(user_ctx, 1, Options, ?ADMIN_CTX),
+ {ok, Db} = open(DbName, Options1),
Resp = fabric2_fdb:transactional(Db, fun(TxDb) ->
fabric2_fdb:delete(TxDb)
end),
@@ -236,11 +241,14 @@ list_dbs(UserFun, UserAcc0, Options) ->
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 ->
true;
false ->
- {SecProps} = get_security(Db),
UserCtx = get_user_ctx(Db),
{Admins} = get_admins(SecProps),
is_authorized(Admins, UserCtx)
@@ -259,7 +267,11 @@ check_is_admin(Db) ->
check_is_member(Db) ->
- case is_member(Db) of
+ check_is_member(Db, get_security(Db)).
+
+
+check_is_member(Db, SecDoc) ->
+ case is_member(Db, SecDoc) of
true ->
ok;
false ->
@@ -977,9 +989,8 @@ maybe_set_user_ctx(Db, Options) ->
end.
-is_member(Db) ->
- {SecProps} = get_security(Db),
- case is_admin(Db) of
+is_member(Db, {SecProps}) when is_list(SecProps) ->
+ case is_admin(Db, {SecProps}) of
true ->
true;
false ->
diff --git a/src/fabric/test/fabric2_db_security_tests.erl b/src/fabric/test/fabric2_db_security_tests.erl
index b4df3b4..4a54083 100644
--- a/src/fabric/test/fabric2_db_security_tests.erl
+++ b/src/fabric/test/fabric2_db_security_tests.erl
@@ -38,7 +38,10 @@ security_test_() ->
fun check_is_not_member_role/1,
fun check_admin_is_member/1,
fun check_is_member_of_public_db/1,
- fun check_set_user_ctx/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
]}
}
}.
@@ -48,7 +51,7 @@ setup() ->
Ctx = test_util:start_couch([fabric]),
DbName = ?tempdb(),
PubDbName = ?tempdb(),
- {ok, Db1} = fabric2_db:create(DbName, [{user_ctx, ?ADMIN_USER}]),
+ {ok, Db1} = fabric2_db:create(DbName, [?ADMIN_CTX]),
SecProps = {[
{<<"admins">>, {[
{<<"names">>, [<<"admin_name1">>, <<"admin_name2">>]},
@@ -60,7 +63,7 @@ setup() ->
]}}
]},
ok = fabric2_db:set_security(Db1, SecProps),
- {ok, Db2} = fabric2_db:open(DbName, []),
+ {ok, Db2} = fabric2_db:open(DbName, [?ADMIN_CTX]),
{ok, PubDb} = fabric2_db:create(PubDbName, []),
{Db2, PubDb, Ctx}.
@@ -157,8 +160,23 @@ check_is_member_of_public_db({_, PubDb, _}) ->
check_set_user_ctx({Db0, _, _}) ->
DbName = fabric2_db:name(Db0),
- UserCtx = #user_ctx{name = <<"foo">>, roles = [<<"bar">>]},
+ 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_open_forbidden({Db0, _, _}) ->
+ DbName = fabric2_db:name(Db0),
+ UserCtx = #user_ctx{name = <<"foo">>, roles = [<<"bar">>]},
+ ?assertThrow({forbidden, _}, fabric2_db:open(DbName, [{user_ctx, UserCtx}])).
+
+
+check_fail_open_no_opts({Db0, _, _}) ->
+ DbName = fabric2_db:name(Db0),
+ ?assertThrow({unauthorized, _}, fabric2_db:open(DbName, [])).
+
+
+check_fail_open_name_null({Db0, _, _}) ->
+ DbName = fabric2_db:name(Db0),
+ UserCtx = #user_ctx{name = null},
+ ?assertThrow({unauthorized, _}, fabric2_db:open(DbName, [{user_ctx, UserCtx}])).