You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2020/08/24 14:05:51 UTC
[GitHub] [couchdb] rnewson commented on a change in pull request #3038: feat: per-document-access
rnewson commented on a change in pull request #3038:
URL: https://github.com/apache/couchdb/pull/3038#discussion_r475588753
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -106,15 +106,22 @@ handle_changes_req1(#httpd{}=Req, Db) ->
Etag = chttpd:make_etag({Info, Suffix}),
DeltaT = timer:now_diff(os:timestamp(), T0) / 1000,
couch_stats:update_histogram([couchdb, dbinfo], DeltaT),
- chttpd:etag_respond(Req, Etag, fun() ->
+ case chttpd:etag_respond(Req, Etag, fun() ->
Acc0 = #cacc{
feed = normal,
etag = Etag,
mochi = Req,
threshold = Max
},
fabric:changes(Db, fun changes_callback/2, Acc0, ChangesArgs)
- end);
+ end) of
+ % TODO: This may be a debugging leftover, undo by just returning
+ % chttpd:etag_respond()
+ {error, {forbidden, Message, _Stacktrace}} ->
Review comment:
can you determine if this is still needed?
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -123,7 +130,14 @@ handle_changes_req1(#httpd{}=Req, Db) ->
threshold = Max
},
try
- fabric:changes(Db, fun changes_callback/2, Acc0, ChangesArgs)
+ % TODO: This may be a debugging leftover, undo by just returning
+ % fabric:changes()
+ case fabric:changes(Db, fun changes_callback/2, Acc0, ChangesArgs) of
Review comment:
can you determine if this is still needed?
##########
File path: src/couch/src/couch_db.erl
##########
@@ -724,6 +747,73 @@ security_error_type(#user_ctx{name=null}) ->
security_error_type(#user_ctx{name=_}) ->
forbidden.
+is_per_user_ddoc(#doc{access=[]}) -> false;
+is_per_user_ddoc(#doc{access=[<<"_users">>]}) -> false;
+is_per_user_ddoc(_) -> true.
+
+
+validate_access(Db, Doc) ->
+ validate_access(Db, Doc, []).
+
+validate_access(Db, Doc, Options) ->
+ validate_access1(has_access_enabled(Db), Db, Doc, Options).
+
+validate_access1(false, _Db, _Doc, _Options) -> ok;
+validate_access1(true, Db, #doc{meta=Meta}=Doc, Options) ->
+ case proplists:get_value(conflicts, Meta) of
+ undefined -> % no conflicts
+ case is_read_from_ddoc_cache(Options) andalso is_per_user_ddoc(Doc) of
+ true -> throw({not_found, missing});
+ _False -> validate_access2(Db, Doc)
+ end;
+ _Else -> % only admins can read conflicted docs in _access dbs
+ case is_admin(Db) of
+ true -> ok;
+ _Else2 -> throw({forbidden, <<"document is in conflict">>})
+ end
+ end.
+validate_access2(Db, Doc) ->
+ validate_access3(check_access(Db, Doc)).
+
+validate_access3(true) -> ok;
+validate_access3(_) -> throw({forbidden, <<"can't touch this">>}).
+
+check_access(Db, #doc{access=Access}=Doc) ->
+ check_access(Db, Access);
+check_access(Db, Access) ->
+ #user_ctx{
+ name=UserName,
+ roles=UserRoles
+ } = Db#db.user_ctx,
+ case Access of
+ [] ->
+ % if doc has no _access, userCtX must be admin
+ is_admin(Db);
+ Access ->
+ % if doc has _access, userCtx must be admin OR matching user or role
+ % _access = ["a", "b", ]
+ case is_admin(Db) of
+ true ->
+ true;
+ _ ->
Review comment:
`false ->`
##########
File path: src/chttpd/src/chttpd_view.erl
##########
@@ -51,9 +51,17 @@ fabric_query_view(Db, Req, DDoc, ViewName, Args) ->
Max = chttpd:chunked_response_buffer_size(),
VAcc = #vacc{db=Db, req=Req, threshold=Max},
Options = [{user_ctx, Req#httpd.user_ctx}],
- {ok, Resp} = fabric:query_view(Db, Options, DDoc, ViewName,
- fun view_cb/2, VAcc, Args),
- {ok, Resp#vacc.resp}.
+ % TODO: This might just be a debugging leftover, we might be able
Review comment:
confirm this is still needed.
##########
File path: src/couch/src/couch_db.erl
##########
@@ -724,6 +747,73 @@ security_error_type(#user_ctx{name=null}) ->
security_error_type(#user_ctx{name=_}) ->
forbidden.
+is_per_user_ddoc(#doc{access=[]}) -> false;
+is_per_user_ddoc(#doc{access=[<<"_users">>]}) -> false;
+is_per_user_ddoc(_) -> true.
+
+
+validate_access(Db, Doc) ->
+ validate_access(Db, Doc, []).
+
+validate_access(Db, Doc, Options) ->
+ validate_access1(has_access_enabled(Db), Db, Doc, Options).
+
+validate_access1(false, _Db, _Doc, _Options) -> ok;
+validate_access1(true, Db, #doc{meta=Meta}=Doc, Options) ->
+ case proplists:get_value(conflicts, Meta) of
+ undefined -> % no conflicts
+ case is_read_from_ddoc_cache(Options) andalso is_per_user_ddoc(Doc) of
+ true -> throw({not_found, missing});
+ _False -> validate_access2(Db, Doc)
+ end;
+ _Else -> % only admins can read conflicted docs in _access dbs
+ case is_admin(Db) of
+ true -> ok;
+ _Else2 -> throw({forbidden, <<"document is in conflict">>})
+ end
+ end.
+validate_access2(Db, Doc) ->
+ validate_access3(check_access(Db, Doc)).
+
+validate_access3(true) -> ok;
+validate_access3(_) -> throw({forbidden, <<"can't touch this">>}).
+
+check_access(Db, #doc{access=Access}=Doc) ->
+ check_access(Db, Access);
+check_access(Db, Access) ->
+ #user_ctx{
+ name=UserName,
+ roles=UserRoles
+ } = Db#db.user_ctx,
+ case Access of
+ [] ->
+ % if doc has no _access, userCtX must be admin
+ is_admin(Db);
+ Access ->
+ % if doc has _access, userCtx must be admin OR matching user or role
+ % _access = ["a", "b", ]
+ case is_admin(Db) of
+ true ->
+ true;
+ _ ->
+ case {check_name(UserName, Access), check_roles(UserRoles, Access)} of
+ {true, _} -> true;
+ {_, true} -> true;
+ _ -> false
+ end
+ end
+ end.
+
+check_name(null, _Access) -> true;
+check_name(UserName, Access) ->
+ lists:member(UserName, Access).
+% nicked from couch_db:check_security
+
+check_roles(Roles, Access) ->
+ UserRolesSet = ordsets:from_list(Roles),
+ RolesSet = ordsets:from_list(Access ++ ["_users"]),
Review comment:
`["_users" | Access]`
##########
File path: src/couch/src/couch_db.erl
##########
@@ -1652,21 +1802,27 @@ open_doc_revs_int(Db, IdRevs, Options) ->
open_doc_int(Db, <<?LOCAL_DOC_PREFIX, _/binary>> = Id, Options) ->
case couch_db_engine:open_local_docs(Db, [Id]) of
[#doc{} = Doc] ->
- apply_open_options({ok, Doc}, Options);
+ case Doc#doc.body of
+ { Body } ->
Review comment:
`{Body} ->`
##########
File path: src/couch/src/couch_db_updater.erl
##########
@@ -676,15 +703,83 @@ update_docs_int(Db, DocsList, LocalDocs, MergeConflicts) ->
length(LocalDocs2)
),
- % Check if we just updated any design documents, and update the validation
- % funs if we did.
+ % Check if we just updated any non-access design documents,
+ % and update the validation funs if we did.
+ NonAccessIds = [Id || [{_Client, #doc{id=Id,access=[]}}|_] <- DocsList],
UpdatedDDocIds = lists:flatmap(fun
(<<"_design/", _/binary>> = Id) -> [Id];
(_) -> []
- end, Ids),
+ end, NonAccessIds),
{ok, commit_data(Db1), UpdatedDDocIds}.
+% check_access(Db, UserCtx, Access) ->
+% check_access(Db, UserCtx, couch_db:has_access_enabled(Db), Access).
+%
+% check_access(_Db, UserCtx, false, _Access) ->
+% true;
+
+% at this point, we already validated this Db is access enabled, so do the checks right away.
+check_access(Db, UserCtx, Access) -> couch_db:check_access(Db#db{user_ctx=UserCtx}, Access).
+
+% TODO: looks like we go into validation here unconditionally and only check in
+% check_access() whether the Db has_access_enabled(), we should do this
+% here on the outside. Might be our perf issue.
+% However, if it is, that means we have to speed this up as it would still
+% be too slow for when access is enabled.
+validate_docs_access(Db, UserCtx, DocsList, OldDocInfos) ->
+ case couch_db:has_access_enabled(Db) of
+ true -> validate_docs_access_int(Db, UserCtx, DocsList, OldDocInfos);
+ _Else -> { DocsList, OldDocInfos }
+ end.
+
+validate_docs_access_int(Db, UserCtx, DocsList, OldDocInfos) ->
+ validate_docs_access(Db, UserCtx, DocsList, OldDocInfos, [], []).
+
+validate_docs_access(_Db, UserCtx, [], [], DocsListValidated, OldDocInfosValidated) ->
+ { lists:reverse(DocsListValidated), lists:reverse(OldDocInfosValidated) };
+validate_docs_access(Db, UserCtx, [Docs | DocRest], [OldInfo | OldInfoRest], DocsListValidated, OldDocInfosValidated) ->
+ % loop over Docs as {Client, NewDoc}
+ % validate Doc
+ % if valid, then put back in Docs
+ % if not, then send_result and skip
+ NewDocs = lists:foldl(fun({ Client, Doc }, Acc) ->
+ % check if we are allowed to update the doc, skip when new doc
+ OldDocMatchesAccess = case OldInfo#full_doc_info.rev_tree of
+ [] -> true;
+ _ -> check_access(Db, UserCtx, OldInfo#full_doc_info.access)
+ end,
+
+ NewDocMatchesAccess = check_access(Db, UserCtx, Doc#doc.access),
+ case OldDocMatchesAccess andalso NewDocMatchesAccess of
+ true -> % if valid, then send to DocsListValidated, OldDocsInfo
+ % and store the access context on the new doc
+ [{Client, Doc} | Acc];
+ _Else2 -> % if invalid, then send_result tagged `access`(c.f. `conflict)
Review comment:
`false ->`
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -906,16 +925,18 @@ view_cb(Msg, Acc) ->
couch_mrview_http:view_cb(Msg, Acc).
db_doc_req(#httpd{method='DELETE'}=Req, Db, DocId) ->
- % check for the existence of the doc to handle the 404 case.
- couch_doc_open(Db, DocId, nil, []),
- case chttpd:qs_value(Req, "rev") of
+ % fetch the old doc revision, so we can compare access control
+ % in send_update_doc() later.
+ Doc0 = couch_doc_open(Db, DocId, nil, [{user_ctx, Req#httpd.user_ctx}]),
+ Revs = chttpd:qs_value(Req, "rev"),
+ case Revs of
undefined ->
Body = {[{<<"_deleted">>,true}]};
Rev ->
Body = {[{<<"_rev">>, ?l2b(Rev)},{<<"_deleted">>,true}]}
end,
- Doc = couch_doc_from_req(Req, Db, DocId, Body),
- send_updated_doc(Req, Db, DocId, Doc);
+ Doc = Doc0#doc{revs=Revs,body=Body,deleted=true},
Review comment:
rather than trying to remove what we don't want from Doc0, it seems safer to instead extract what we want to retain in Doc0 (presumably a new attribute called `access` or similar, and add that (and only that) to the artificially created Doc.
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -394,8 +409,12 @@ create_db_req(#httpd{}=Req, DbName) ->
{placement, P},
{props, DbProps}
] ++ EngineOpt,
+ Options1 = case Access of
Review comment:
this is a little untidy, seems we could just add `{access, Access}` alongside where n, q, placement, props is set if the Access variable was validated and set to the appropriate type above.
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -1255,10 +1276,12 @@ receive_request_data(Req, LenLeft) when LenLeft > 0 ->
receive_request_data(_Req, _) ->
throw(<<"expected more data">>).
+update_doc_result_to_json({#doc{id=Id,revs=Rev}, access}) ->
+ update_doc_result_to_json({{Id, Rev}, access});
Review comment:
it's difficult with an old/new code mix, but I think we should follow formatting rules here. A blank line between clauses of the same function pls (and a space after the , in the function head would be nice. note that you don't have one there but -do- have a space on the line below it.
##########
File path: src/couch/src/couch_access_native_proc.erl
##########
@@ -0,0 +1,143 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_access_native_proc).
+-behavior(gen_server).
+
+
+-export([
+ start_link/0,
+ set_timeout/2,
+ prompt/2
+]).
+
+-export([
+ init/1,
+ terminate/2,
+ handle_call/3,
+ handle_cast/2,
+ handle_info/2,
+ code_change/3
+]).
+
+
+-record(st, {
+ indexes = [],
+ timeout = 5000 % TODO: make configurable
+}).
+
+start_link() ->
+ gen_server:start_link(?MODULE, [], []).
+
+
+set_timeout(Pid, TimeOut) when is_integer(TimeOut), TimeOut > 0 ->
+ gen_server:call(Pid, {set_timeout, TimeOut}).
+
+
+prompt(Pid, Data) ->
+ gen_server:call(Pid, {prompt, Data}).
+
+
+init(_) ->
+ {ok, #st{}}.
+
+
+terminate(_Reason, _St) ->
+ ok.
+
+
+handle_call({set_timeout, TimeOut}, _From, St) ->
+ {reply, ok, St#st{timeout=TimeOut}};
+
+handle_call({prompt, [<<"reset">>]}, _From, St) ->
+ {reply, true, St#st{indexes=[]}};
+
+handle_call({prompt, [<<"reset">>, _QueryConfig]}, _From, St) ->
+ {reply, true, St#st{indexes=[]}};
+
+handle_call({prompt, [<<"add_fun">>, IndexInfo]}, _From, St) ->
+ {reply, true, St};
+
+handle_call({prompt, [<<"map_doc">>, Doc]}, _From, St) ->
+ {reply, map_doc(St, mango_json:to_binary(Doc)), St};
+
+handle_call({prompt, [<<"reduce">>, _, _]}, _From, St) ->
+ {reply, null, St};
+
+handle_call({prompt, [<<"rereduce">>, _, _]}, _From, St) ->
+ {reply, null, St};
+
+handle_call({prompt, [<<"index_doc">>, Doc]}, _From, St) ->
+ {reply, [[]], St};
+
+handle_call(Msg, _From, St) ->
+ {stop, {invalid_call, Msg}, {invalid_call, Msg}, St}.
+
+handle_cast(garbage_collect, St) ->
+ erlang:garbage_collect(),
+ {noreply, St};
+
+handle_cast(Msg, St) ->
+ {stop, {invalid_cast, Msg}, St}.
+
+
+handle_info(Msg, St) ->
+ {stop, {invalid_info, Msg}, St}.
+
+
+code_change(_OldVsn, St, _Extra) ->
+ {ok, St}.
+
+% return value is an array of arrays, first dimension is the different indexes
+% [0] will be by-access-id // for this test, later we should make this by-access
+% -seq, since that one we will always need, and by-access-id can be opt-in.
+% the second dimension is the number of emit kv pairs:
+% [ // the return value
+% [ // the first view
+% ['k1', 'v1'], // the first k/v pair for the first view
+% ['k2', 'v2'] // second, etc.
+% ],
+% [ // second view
+% ['l1', 'w1'] // first k/v par in second view
+% ]
+% ]
+% {"id":"account/bongel","key":"account/bongel","value":{"rev":"1-967a00dff5e02add41819138abb3284d"}},
+
+map_doc(_St, {Doc}) ->
+ case couch_util:get_value(<<"_access">>, Doc) of
+ undefined ->
+ [[],[]]; % do not index this doc
+ Access when is_list(Access) ->
+ Id = couch_util:get_value(<<"_id">>, Doc),
+ Rev = couch_util:get_value(<<"_rev">>, Doc),
+ Seq = couch_util:get_value(<<"_seq">>, Doc),
+ Deleted = couch_util:get_value(<<"_deleted">>, Doc, false),
+ BodySp = couch_util:get_value(<<"_body_sp">>, Doc),
+ % by-access-id
+ ById = case Deleted of
+ false ->
+ lists:map(fun(UserOrRole) -> [
+ [[UserOrRole, Id], Rev]
+ ] end, Access);
+ _True -> [[]]
+ end,
+
+ % by-access-seq
+ BySeq = lists:map(fun(UserOrRole) -> [
+ [[UserOrRole, Seq], [{rev, Rev}, {deleted, Deleted}, {body_sp, BodySp}]]
+ ] end, Access),
+ ById ++ BySeq;
+ Else ->
+ % TODO: no comprende: should not be needed once we implement
Review comment:
should we crash instead then? (that is, no Else clause for a condition we'll ensure cannot happen elsewhere)
##########
File path: src/couch/src/couch_db.erl
##########
@@ -284,26 +295,36 @@ delete_doc(Db, Id, Revisions) ->
open_doc(Db, IdOrDocInfo) ->
open_doc(Db, IdOrDocInfo, []).
-open_doc(Db, Id, Options) ->
+open_doc(Db, Id, Options0) ->
increment_stat(Db, [couchdb, database_reads]),
+ Options = case has_access_enabled(Db) of
+ true -> Options0 ++ [conflicts];
Review comment:
"only admins can see conflicts" isn't true generally, did we add that restriction somewhere for `_users` db?
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -721,21 +724,24 @@ seq_tree_split(#full_doc_info{}=Info) ->
update_seq = Seq,
deleted = Del,
sizes = SizeInfo,
- rev_tree = Tree
+ rev_tree = Tree,
+ access = Access
} = Info,
- {Seq, {Id, ?b2i(Del), split_sizes(SizeInfo), disk_tree(Tree)}}.
+ {Seq, {Id, ?b2i(Del), split_sizes(SizeInfo), disk_tree(Tree), split_access(Access)}}.
seq_tree_join(Seq, {Id, Del, DiskTree}) when is_integer(Del) ->
seq_tree_join(Seq, {Id, Del, {0, 0}, DiskTree});
-
Review comment:
same comment ^
##########
File path: src/couch/src/couch_db.erl
##########
@@ -1185,9 +1306,31 @@ update_docs(Db, Docs0, Options, replicated_changes) ->
DocBuckets2 = [[doc_flush_atts(Db, check_dup_atts(Doc))
|| Doc <- Bucket] || Bucket <- DocBuckets],
- {ok, _} = write_and_commit(Db, DocBuckets2,
+ {ok, Results} = write_and_commit(Db, DocBuckets2,
NonRepDocs, [merge_conflicts | Options]),
- {ok, DocErrors};
+
+ case couch_db:has_access_enabled(Db) of
+ false ->
+ % we’re done here
+ {ok, DocErrors};
+ _ ->
Review comment:
`true ->`
##########
File path: src/couch/src/couch_db.erl
##########
@@ -276,6 +281,12 @@ wait_for_compaction(#db{main_pid=Pid}=Db, Timeout) ->
ok
end.
+has_access_enabled(#db{access=true}) -> true;
+has_access_enabled(_) -> false.
Review comment:
again, I'm concerned we're sloppy on what value `access` can have. it's a boolean and should only have one of two possible values, everything else is a programming error and should not be silently defaulted to an acceptable value.
##########
File path: src/couch/src/couch_access_native_proc.erl
##########
@@ -0,0 +1,143 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(couch_access_native_proc).
+-behavior(gen_server).
+
+
+-export([
+ start_link/0,
+ set_timeout/2,
+ prompt/2
+]).
+
+-export([
+ init/1,
+ terminate/2,
+ handle_call/3,
+ handle_cast/2,
+ handle_info/2,
+ code_change/3
+]).
+
+
+-record(st, {
+ indexes = [],
+ timeout = 5000 % TODO: make configurable
+}).
+
+start_link() ->
+ gen_server:start_link(?MODULE, [], []).
+
+
+set_timeout(Pid, TimeOut) when is_integer(TimeOut), TimeOut > 0 ->
+ gen_server:call(Pid, {set_timeout, TimeOut}).
+
+
+prompt(Pid, Data) ->
+ gen_server:call(Pid, {prompt, Data}).
+
+
+init(_) ->
+ {ok, #st{}}.
+
+
+terminate(_Reason, _St) ->
+ ok.
+
+
+handle_call({set_timeout, TimeOut}, _From, St) ->
+ {reply, ok, St#st{timeout=TimeOut}};
+
+handle_call({prompt, [<<"reset">>]}, _From, St) ->
+ {reply, true, St#st{indexes=[]}};
+
+handle_call({prompt, [<<"reset">>, _QueryConfig]}, _From, St) ->
+ {reply, true, St#st{indexes=[]}};
+
+handle_call({prompt, [<<"add_fun">>, IndexInfo]}, _From, St) ->
+ {reply, true, St};
+
+handle_call({prompt, [<<"map_doc">>, Doc]}, _From, St) ->
+ {reply, map_doc(St, mango_json:to_binary(Doc)), St};
+
+handle_call({prompt, [<<"reduce">>, _, _]}, _From, St) ->
+ {reply, null, St};
+
+handle_call({prompt, [<<"rereduce">>, _, _]}, _From, St) ->
+ {reply, null, St};
+
+handle_call({prompt, [<<"index_doc">>, Doc]}, _From, St) ->
+ {reply, [[]], St};
+
+handle_call(Msg, _From, St) ->
+ {stop, {invalid_call, Msg}, {invalid_call, Msg}, St}.
+
+handle_cast(garbage_collect, St) ->
+ erlang:garbage_collect(),
+ {noreply, St};
+
+handle_cast(Msg, St) ->
+ {stop, {invalid_cast, Msg}, St}.
+
+
+handle_info(Msg, St) ->
+ {stop, {invalid_info, Msg}, St}.
+
+
+code_change(_OldVsn, St, _Extra) ->
+ {ok, St}.
+
+% return value is an array of arrays, first dimension is the different indexes
+% [0] will be by-access-id // for this test, later we should make this by-access
+% -seq, since that one we will always need, and by-access-id can be opt-in.
+% the second dimension is the number of emit kv pairs:
+% [ // the return value
+% [ // the first view
+% ['k1', 'v1'], // the first k/v pair for the first view
+% ['k2', 'v2'] // second, etc.
+% ],
+% [ // second view
+% ['l1', 'w1'] // first k/v par in second view
+% ]
+% ]
+% {"id":"account/bongel","key":"account/bongel","value":{"rev":"1-967a00dff5e02add41819138abb3284d"}},
+
+map_doc(_St, {Doc}) ->
+ case couch_util:get_value(<<"_access">>, Doc) of
+ undefined ->
+ [[],[]]; % do not index this doc
+ Access when is_list(Access) ->
+ Id = couch_util:get_value(<<"_id">>, Doc),
+ Rev = couch_util:get_value(<<"_rev">>, Doc),
+ Seq = couch_util:get_value(<<"_seq">>, Doc),
+ Deleted = couch_util:get_value(<<"_deleted">>, Doc, false),
+ BodySp = couch_util:get_value(<<"_body_sp">>, Doc),
+ % by-access-id
+ ById = case Deleted of
+ false ->
+ lists:map(fun(UserOrRole) -> [
+ [[UserOrRole, Id], Rev]
+ ] end, Access);
+ _True -> [[]]
Review comment:
this is coy, call it `_Else` if you intend to be defensive, but my preference is to say `true` explicitly. Any other case is a programming error and should crash (as opposed to a user error)
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -386,6 +400,7 @@ create_db_req(#httpd{}=Req, DbName) ->
N = chttpd:qs_value(Req, "n", config:get("cluster", "n", "3")),
Q = chttpd:qs_value(Req, "q", config:get("cluster", "q", "8")),
P = chttpd:qs_value(Req, "placement", config:get("cluster", "placement")),
+ Access = chttpd:qs_value(Req, "access", false),
Review comment:
I would also like to see that switch.
```
Access = case config:get_boolean("per_doc_access", "enabled", false) of
true ->
chttpd:qs_value(Req, "access", false);
false ->
false
end,
```
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -760,6 +766,8 @@ seq_tree_reduce(reduce, DocInfos) ->
seq_tree_reduce(rereduce, Reds) ->
lists:sum(Reds).
+join_access(Access) -> Access.
+split_access(Access) -> Access.
Review comment:
2 blank lines between functions pls
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -675,22 +675,25 @@ id_tree_split(#full_doc_info{}=Info) ->
update_seq = Seq,
deleted = Deleted,
sizes = SizeInfo,
- rev_tree = Tree
+ rev_tree = Tree,
+ access = Access
} = Info,
- {Id, {Seq, ?b2i(Deleted), split_sizes(SizeInfo), disk_tree(Tree)}}.
+ {Id, {Seq, ?b2i(Deleted), split_sizes(SizeInfo), disk_tree(Tree), split_access(Access)}}.
id_tree_join(Id, {HighSeq, Deleted, DiskTree}) ->
% Handle old formats before data_size was added
id_tree_join(Id, {HighSeq, Deleted, #size_info{}, DiskTree});
-
Review comment:
1 blank line between clauses of the same function pls.
##########
File path: src/couch/src/couch_db.erl
##########
@@ -1172,6 +1267,32 @@ doc_tag(#doc{meta=Meta}) ->
Else -> throw({invalid_doc_tag, Else})
end.
+validate_update(Db, Doc) ->
+ case catch validate_access(Db, Doc) of
+ ok -> Doc;
+ Error -> Error
+ end.
+
+
+validate_docs_access(Db, DocBuckets, DocErrors) ->
+ validate_docs_access1(Db, DocBuckets, {[], DocErrors}).
Review comment:
indentation
##########
File path: src/couch/src/couch_db.erl
##########
@@ -284,26 +295,36 @@ delete_doc(Db, Id, Revisions) ->
open_doc(Db, IdOrDocInfo) ->
open_doc(Db, IdOrDocInfo, []).
-open_doc(Db, Id, Options) ->
+open_doc(Db, Id, Options0) ->
increment_stat(Db, [couchdb, database_reads]),
+ Options = case has_access_enabled(Db) of
Review comment:
as a nit, if we're now numbering Options because it changes, I find it more readable to stick to the convention throughout, so this should be `Options1`.
##########
File path: src/couch/src/couch_db.erl
##########
@@ -863,9 +953,14 @@ group_alike_docs([Doc|Rest], [Bucket|RestBuckets]) ->
end.
validate_doc_update(#db{}=Db, #doc{id= <<"_design/",_/binary>>}=Doc, _GetDiskDocFun) ->
- case catch check_is_admin(Db) of
- ok -> validate_ddoc(Db, Doc);
- Error -> Error
+ case couch_doc:has_access(Doc) of
+ true ->
+ validate_ddoc(Db, Doc);
+ _Else ->
Review comment:
`false ->`
##########
File path: src/couch/src/couch_db.erl
##########
@@ -1652,21 +1802,27 @@ open_doc_revs_int(Db, IdRevs, Options) ->
open_doc_int(Db, <<?LOCAL_DOC_PREFIX, _/binary>> = Id, Options) ->
case couch_db_engine:open_local_docs(Db, [Id]) of
[#doc{} = Doc] ->
- apply_open_options({ok, Doc}, Options);
+ case Doc#doc.body of
+ { Body } ->
+ Access = couch_util:get_value(<<"_access">>, Body),
+ apply_open_options(Db, {ok, Doc#doc{access = Access}}, Options);
+ _Else ->
Review comment:
what could `_Else` be?
##########
File path: src/couch/src/couch_db.erl
##########
@@ -724,6 +747,73 @@ security_error_type(#user_ctx{name=null}) ->
security_error_type(#user_ctx{name=_}) ->
forbidden.
+is_per_user_ddoc(#doc{access=[]}) -> false;
+is_per_user_ddoc(#doc{access=[<<"_users">>]}) -> false;
+is_per_user_ddoc(_) -> true.
+
+
+validate_access(Db, Doc) ->
+ validate_access(Db, Doc, []).
+
+validate_access(Db, Doc, Options) ->
+ validate_access1(has_access_enabled(Db), Db, Doc, Options).
+
+validate_access1(false, _Db, _Doc, _Options) -> ok;
+validate_access1(true, Db, #doc{meta=Meta}=Doc, Options) ->
+ case proplists:get_value(conflicts, Meta) of
+ undefined -> % no conflicts
+ case is_read_from_ddoc_cache(Options) andalso is_per_user_ddoc(Doc) of
+ true -> throw({not_found, missing});
+ _False -> validate_access2(Db, Doc)
+ end;
+ _Else -> % only admins can read conflicted docs in _access dbs
+ case is_admin(Db) of
+ true -> ok;
+ _Else2 -> throw({forbidden, <<"document is in conflict">>})
+ end
+ end.
+validate_access2(Db, Doc) ->
+ validate_access3(check_access(Db, Doc)).
+
+validate_access3(true) -> ok;
+validate_access3(_) -> throw({forbidden, <<"can't touch this">>}).
+
+check_access(Db, #doc{access=Access}=Doc) ->
+ check_access(Db, Access);
+check_access(Db, Access) ->
+ #user_ctx{
+ name=UserName,
+ roles=UserRoles
+ } = Db#db.user_ctx,
+ case Access of
+ [] ->
+ % if doc has no _access, userCtX must be admin
+ is_admin(Db);
+ Access ->
+ % if doc has _access, userCtx must be admin OR matching user or role
+ % _access = ["a", "b", ]
+ case is_admin(Db) of
+ true ->
+ true;
+ _ ->
+ case {check_name(UserName, Access), check_roles(UserRoles, Access)} of
Review comment:
isn't this just `check_name(UserName, Access) orelse check_roles(UserRoles, Access)`?
##########
File path: src/couch/src/couch_db.erl
##########
@@ -724,6 +747,73 @@ security_error_type(#user_ctx{name=null}) ->
security_error_type(#user_ctx{name=_}) ->
forbidden.
+is_per_user_ddoc(#doc{access=[]}) -> false;
+is_per_user_ddoc(#doc{access=[<<"_users">>]}) -> false;
+is_per_user_ddoc(_) -> true.
+
+
+validate_access(Db, Doc) ->
+ validate_access(Db, Doc, []).
+
+validate_access(Db, Doc, Options) ->
+ validate_access1(has_access_enabled(Db), Db, Doc, Options).
+
+validate_access1(false, _Db, _Doc, _Options) -> ok;
+validate_access1(true, Db, #doc{meta=Meta}=Doc, Options) ->
+ case proplists:get_value(conflicts, Meta) of
+ undefined -> % no conflicts
+ case is_read_from_ddoc_cache(Options) andalso is_per_user_ddoc(Doc) of
+ true -> throw({not_found, missing});
+ _False -> validate_access2(Db, Doc)
+ end;
+ _Else -> % only admins can read conflicted docs in _access dbs
+ case is_admin(Db) of
+ true -> ok;
+ _Else2 -> throw({forbidden, <<"document is in conflict">>})
+ end
+ end.
+validate_access2(Db, Doc) ->
+ validate_access3(check_access(Db, Doc)).
+
+validate_access3(true) -> ok;
+validate_access3(_) -> throw({forbidden, <<"can't touch this">>}).
+
+check_access(Db, #doc{access=Access}=Doc) ->
+ check_access(Db, Access);
+check_access(Db, Access) ->
+ #user_ctx{
+ name=UserName,
+ roles=UserRoles
+ } = Db#db.user_ctx,
+ case Access of
+ [] ->
+ % if doc has no _access, userCtX must be admin
+ is_admin(Db);
+ Access ->
+ % if doc has _access, userCtx must be admin OR matching user or role
+ % _access = ["a", "b", ]
+ case is_admin(Db) of
+ true ->
+ true;
+ _ ->
+ case {check_name(UserName, Access), check_roles(UserRoles, Access)} of
+ {true, _} -> true;
+ {_, true} -> true;
+ _ -> false
+ end
+ end
+ end.
+
+check_name(null, _Access) -> true;
+check_name(UserName, Access) ->
+ lists:member(UserName, Access).
Review comment:
indentation
##########
File path: src/couch/src/couch_db.erl
##########
@@ -724,6 +747,73 @@ security_error_type(#user_ctx{name=null}) ->
security_error_type(#user_ctx{name=_}) ->
forbidden.
+is_per_user_ddoc(#doc{access=[]}) -> false;
+is_per_user_ddoc(#doc{access=[<<"_users">>]}) -> false;
+is_per_user_ddoc(_) -> true.
+
+
+validate_access(Db, Doc) ->
+ validate_access(Db, Doc, []).
+
+validate_access(Db, Doc, Options) ->
+ validate_access1(has_access_enabled(Db), Db, Doc, Options).
+
+validate_access1(false, _Db, _Doc, _Options) -> ok;
+validate_access1(true, Db, #doc{meta=Meta}=Doc, Options) ->
+ case proplists:get_value(conflicts, Meta) of
+ undefined -> % no conflicts
+ case is_read_from_ddoc_cache(Options) andalso is_per_user_ddoc(Doc) of
+ true -> throw({not_found, missing});
+ _False -> validate_access2(Db, Doc)
+ end;
+ _Else -> % only admins can read conflicted docs in _access dbs
+ case is_admin(Db) of
+ true -> ok;
+ _Else2 -> throw({forbidden, <<"document is in conflict">>})
+ end
+ end.
+validate_access2(Db, Doc) ->
+ validate_access3(check_access(Db, Doc)).
+
+validate_access3(true) -> ok;
Review comment:
these function names need improving (e.g, they could say what is is they are validating).
##########
File path: src/couch/src/couch_db_updater.erl
##########
@@ -248,7 +251,11 @@ sort_and_tag_grouped_docs(Client, GroupedDocs) ->
% The merge_updates function will fail and the database can end up with
% duplicate documents if the incoming groups are not sorted, so as a sanity
% check we sort them again here. See COUCHDB-2735.
- Cmp = fun([#doc{id=A}|_], [#doc{id=B}|_]) -> A < B end,
+ Cmp = fun
+ ([], []) -> false; % TODO: re-evaluate this addition, might be a
Review comment:
re-evaluate before we merge
##########
File path: src/couch/src/couch_db.erl
##########
@@ -284,26 +295,36 @@ delete_doc(Db, Id, Revisions) ->
open_doc(Db, IdOrDocInfo) ->
open_doc(Db, IdOrDocInfo, []).
-open_doc(Db, Id, Options) ->
+open_doc(Db, Id, Options0) ->
increment_stat(Db, [couchdb, database_reads]),
+ Options = case has_access_enabled(Db) of
+ true -> Options0 ++ [conflicts];
Review comment:
What this line means is "also fetch the conflicts when you open the doc", the motivation isn't clear here, but presumably will be made clear further down.
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -87,6 +87,13 @@ basic_name_pw(Req) ->
nil
end.
+extract_roles(UserProps) ->
+ Roles = couch_util:get_value(<<"roles">>, UserProps, []),
+ case lists:member(<<"_admin">>, Roles) of
+ true -> Roles;
+ _ -> Roles ++ [<<"_users">>]
Review comment:
`[<<"_users">> | Roles]`
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -167,7 +174,7 @@ proxy_auth_user(Req) ->
Roles = case header_value(Req, XHeaderRoles) of
undefined -> [];
Else ->
- [?l2b(R) || R <- string:tokens(Else, ",")]
+ [?l2b(R) || R <- string:tokens(Else, ",")] ++ [<<"_users">>]
Review comment:
as above
##########
File path: src/couch/src/couch_db.erl
##########
@@ -724,6 +747,73 @@ security_error_type(#user_ctx{name=null}) ->
security_error_type(#user_ctx{name=_}) ->
forbidden.
+is_per_user_ddoc(#doc{access=[]}) -> false;
+is_per_user_ddoc(#doc{access=[<<"_users">>]}) -> false;
+is_per_user_ddoc(_) -> true.
+
+
+validate_access(Db, Doc) ->
+ validate_access(Db, Doc, []).
+
+validate_access(Db, Doc, Options) ->
+ validate_access1(has_access_enabled(Db), Db, Doc, Options).
+
+validate_access1(false, _Db, _Doc, _Options) -> ok;
+validate_access1(true, Db, #doc{meta=Meta}=Doc, Options) ->
+ case proplists:get_value(conflicts, Meta) of
+ undefined -> % no conflicts
+ case is_read_from_ddoc_cache(Options) andalso is_per_user_ddoc(Doc) of
+ true -> throw({not_found, missing});
+ _False -> validate_access2(Db, Doc)
+ end;
+ _Else -> % only admins can read conflicted docs in _access dbs
+ case is_admin(Db) of
+ true -> ok;
+ _Else2 -> throw({forbidden, <<"document is in conflict">>})
Review comment:
why can't users see conflicted docs in "access" enabled dbs?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org