This is an automated email from the ASF dual-hosted git repository.
jan pushed a commit to branch rebase/access-2023
in repository https://gitbox.apache.org/repos/asf/couchdb.git
The following commit(s) were added to refs/heads/rebase/access-2023 by this
push:
new b1c630867 chore: address various rerview notes by @rnewson
b1c630867 is described below
commit b1c630867980ca36edc4327f177e7376e128cc31
Author: Jan Lehnardt <[email protected]>
AuthorDate: Thu Aug 17 12:52:11 2023 +0200
chore: address various rerview notes by @rnewson
---
rel/overlay/etc/default.ini | 2 +-
src/chttpd/src/chttpd_db.erl | 26 +++++++++++++-------------
src/couch/src/couch_access_native_proc.erl | 4 +---
src/couch/src/couch_db.erl | 23 +++++++----------------
src/couch/src/couch_db_updater.erl | 8 ++------
src/couch/test/eunit/couchdb_access_tests.erl | 6 +++---
6 files changed, 27 insertions(+), 42 deletions(-)
diff --git a/rel/overlay/etc/default.ini b/rel/overlay/etc/default.ini
index ac93e02f2..890d819a6 100644
--- a/rel/overlay/etc/default.ini
+++ b/rel/overlay/etc/default.ini
@@ -404,7 +404,7 @@ authentication_db = _users
; Per document access settings
[per_doc_access]
-;enabled = false
+;enable = false
; CSP (Content Security Policy) Support
[csp]
diff --git a/src/chttpd/src/chttpd_db.erl b/src/chttpd/src/chttpd_db.erl
index 148ca9806..6911b5ecc 100644
--- a/src/chttpd/src/chttpd_db.erl
+++ b/src/chttpd/src/chttpd_db.erl
@@ -961,14 +961,14 @@ db_doc_req(#httpd{method = 'DELETE'} = Req, Db, DocId) ->
% 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
+ Rev = chttpd:qs_value(Req, "rev"),
+ case Rev of
undefined ->
Body = {[{<<"_deleted">>, true}]};
Rev ->
Body = {[{<<"_rev">>, ?l2b(Rev)}, {<<"_deleted">>, true}]}
end,
- Doc = #doc{revs = Revs, body = Body, deleted = true, access =
Doc0#doc.access},
+ Doc = #doc{revs = Rev, body = Body, deleted = true, access =
Doc0#doc.access},
send_updated_doc(Req, Db, DocId, couch_doc_from_req(Req, Db, DocId, Doc));
db_doc_req(#httpd{method = 'GET', mochi_req = MochiReq} = Req, Db, DocId) ->
#doc_query_args{
@@ -1940,10 +1940,11 @@ parse_doc_query(Req) ->
lists:foldl(fun parse_doc_query/2, #doc_query_args{}, chttpd:qs(Req)).
parse_shards_opt(Req) ->
+ AccessValue = list_to_existing_atom(chttpd:qs_value(Req, "access",
"false")),
[
{n, parse_shards_opt("n", Req, config:get_integer("cluster", "n", 3))},
{q, parse_shards_opt("q", Req, config:get_integer("cluster", "q", 2))},
- {access, parse_shards_opt("access", Req, chttpd:qs_value(Req,
"access", false))},
+ {access, parse_shards_opt("access", Req, AccessValue)},
{placement,
parse_shards_opt(
"placement", Req, config:get("cluster", "placement")
@@ -1972,27 +1973,26 @@ parse_shards_opt("placement", Req, Default) ->
throw({bad_request, Err})
end
end;
-parse_shards_opt("access", Req, Value) when is_list(Value) ->
- parse_shards_opt("access", Req, list_to_existing_atom(Value));
-parse_shards_opt("access", _Req, Value) when Value =:= true ->
- case config:get_boolean("per_doc_access", "enabled", false) of
+parse_shards_opt("access", _Req, true) ->
+ case config:get_boolean("per_doc_access", "enable", false) of
true ->
true;
false ->
- Err = ?l2b(["The `access` option is not available on this CouchDB
installation."]),
+ Err = <<"The `access` option is not available on this CouchDB
installation.">>,
throw({bad_request, Err})
end;
-parse_shards_opt("access", _Req, Value) when Value =:= false ->
+parse_shards_opt("access", _Req, false) ->
false;
parse_shards_opt("access", _Req, _Value) ->
- Err = ?l2b(["The `access` value should be a boolean."]),
+ Err = <<"The `access` value should be a boolean.">>,
throw({bad_request, Err});
parse_shards_opt(Param, Req, Default) ->
Val = chttpd:qs_value(Req, Param, Default),
- Err = ?l2b(["The `", Param, "` value should be a positive integer."]),
case couch_util:validate_positive_int(Val) of
true -> Val;
- false -> throw({bad_request, Err})
+ false ->
+ Err = ?l2b(["The `", Param, "` value should be a positive
integer."]),
+ throw({bad_request, Err})
end.
parse_engine_opt(Req) ->
diff --git a/src/couch/src/couch_access_native_proc.erl
b/src/couch/src/couch_access_native_proc.erl
index 8c82cfccc..494221a5e 100644
--- a/src/couch/src/couch_access_native_proc.erl
+++ b/src/couch/src/couch_access_native_proc.erl
@@ -132,8 +132,6 @@ map_doc(_St, {Doc}) ->
Access
),
ById ++ BySeq;
- Else ->
- % TODO: no comprende: should not be needed once we implement
- % _access field validation
+ _Else ->
[[], []]
end.
diff --git a/src/couch/src/couch_db.erl b/src/couch/src/couch_db.erl
index c1e9da0da..5b603072f 100644
--- a/src/couch/src/couch_db.erl
+++ b/src/couch/src/couch_db.erl
@@ -140,7 +140,6 @@
]).
-include_lib("couch/include/couch_db.hrl").
-
-include("couch_db_int.hrl").
-define(DBNAME_REGEX,
@@ -821,20 +820,11 @@ check_access(Db, Access) ->
} = Db#db.user_ctx,
case Access of
[] ->
- % if doc has no _access, userCtX must be admin
+ % 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
- case is_admin(Db) of
- true ->
- true;
- _ ->
- case {check_name(UserName, Access), check_roles(UserRoles,
Access)} of
- {true, _} -> true;
- {_, true} -> true;
- _ -> false
- end
- end
+ is_admin(Db) or (check_name(UserName, Access) or
check_roles(UserRoles, Access))
end.
check_name(null, _Access) -> false;
@@ -989,7 +979,7 @@ validate_doc_update(#db{} = Db, #doc{id = <<"_design/",
_/binary>>} = Doc, _GetD
case couch_doc:has_access(Doc) of
true ->
validate_ddoc(Db, Doc);
- _Else ->
+ false ->
case catch check_is_admin(Db) of
ok -> validate_ddoc(Db, Doc);
Error -> Error
@@ -1421,13 +1411,13 @@ update_docs(Db, Docs0, Options, ?REPLICATED_CHANGES) ->
false ->
% we’re done here
{ok, DocErrors};
- _ ->
- AccessViolations = lists:filter(fun({_Ref, Tag}) -> Tag =:= access
end, Results),
+ true ->
+ AccessViolations = lists:filter(fun({_Ref, Tag}) -> Tag == access
end, Results),
case length(AccessViolations) of
0 ->
% we’re done here
{ok, DocErrors};
- _ ->
+ N when N > 0 ->
% dig out FDIs from Docs matching our tags/refs
DocsDict = lists:foldl(
fun(Doc, Dict) ->
@@ -1472,6 +1462,7 @@ update_docs_interactive(Db, Docs0, Options) ->
{ok, DocBuckets, LocalDocs, DocErrors} =
before_docs_update(Db, Docs, PrepValidateFun, ?INTERACTIVE_EDIT),
+
if
(AllOrNothing) and (DocErrors /= []) ->
RefErrorDict = dict:from_list([{doc_tag(Doc), Doc} || Doc <-
Docs]),
diff --git a/src/couch/src/couch_db_updater.erl
b/src/couch/src/couch_db_updater.erl
index f78a043b9..21ae6e931 100644
--- a/src/couch/src/couch_db_updater.erl
+++ b/src/couch/src/couch_db_updater.erl
@@ -266,7 +266,6 @@ sort_and_tag_grouped_docs(Client, GroupedDocs, UserCtx) ->
% 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,
- % couch_log:notice("~n s_a_t_g_d: GroupedDocs: ~p, UserCtx: ~p ~n",
[GroupedDocs, UserCtx]),
lists:map(
fun(DocGroup) ->
[{Client, maybe_tag_doc(D), UserCtx} || D <- DocGroup]
@@ -740,7 +739,6 @@ update_docs_int(Db, DocsList, LocalDocs, ReplicatedChanges)
->
{DocsListValidated, OldDocInfosValidated} = validate_docs_access(
Db, DocsList, OldDocInfos
),
- % couch_log:notice("~n~n u_d_i: DocsList: ~p~n, OldDocInfos: ~p~n,
DocsListValidated: ~p~n, OldDocInfosValidated: ~p~n~n~n", [DocsList,
OldDocInfos, DocsListValidated, OldDocInfosValidated]),
{ok, AccOut} = merge_rev_trees(DocsListValidated, OldDocInfosValidated,
AccIn),
#merge_acc{
add_infos = NewFullDocInfos,
@@ -783,15 +781,13 @@ check_access(Db, UserCtx, Access) ->
validate_docs_access(Db, DocsList, OldDocInfos) ->
case couch_db:has_access_enabled(Db) of
true -> validate_docs_access_int(Db, DocsList, OldDocInfos);
- _Else -> {DocsList, OldDocInfos}
+ false -> {DocsList, OldDocInfos}
end.
validate_docs_access_int(Db, DocsList, OldDocInfos) ->
validate_docs_access(Db, DocsList, OldDocInfos, [], []).
validate_docs_access(_Db, [], [], DocsListValidated, OldDocInfosValidated) ->
- % TODO: check if need to reverse this? maybe this is the cause of the test
reverse issue?
- % {lists:reverse(DocsListValidated), lists:reverse(OldDocInfosValidated)};
{lists:reverse(DocsListValidated), lists:reverse(OldDocInfosValidated)};
validate_docs_access(
Db, [Docs | DocRest], [OldInfo | OldInfoRest], DocsListValidated,
OldDocInfosValidated
@@ -833,7 +829,7 @@ validate_docs_access(
% we sent out all docs as invalid access, drop the old doc info
associated with it
0 ->
{DocsListValidated, OldDocInfosValidated};
- _ ->
+ N when N > 0 ->
{[NewDocs | DocsListValidated], [OldInfo |
OldDocInfosValidated]}
end,
validate_docs_access(
diff --git a/src/couch/test/eunit/couchdb_access_tests.erl
b/src/couch/test/eunit/couchdb_access_tests.erl
index bce0cfd83..bd19c9a51 100644
--- a/src/couch/test/eunit/couchdb_access_tests.erl
+++ b/src/couch/test/eunit/couchdb_access_tests.erl
@@ -50,7 +50,7 @@ before_all() ->
ok = config:set("admins", "a", binary_to_list(Hashed), false),
ok = config:set("couchdb", "uuid", "21ac467c1bc05e9d9e9d2d850bb1108f",
false),
ok = config:set("log", "level", "debug", false),
- ok = config:set("per_doc_access", "enabled", "true", false),
+ ok = config:set("per_doc_access", "enable", "true", false),
% cleanup and setup
{ok, _, _, _} = test_request:delete(url() ++ "/db", ?ADMIN_REQ_HEADERS),
@@ -172,9 +172,9 @@ make_test_cases(Mod, Funs) ->
%
should_not_let_create_access_db_if_disabled(_PortType, Url) ->
- ok = config:set("per_doc_access", "enabled", "false", false),
+ ok = config:set("per_doc_access", "enable", "false", false),
{ok, Code, _, _} = test_request:put(url() ++ "/db?q=1&n=1&access=true",
?ADMIN_REQ_HEADERS, ""),
- ok = config:set("per_doc_access", "enabled", "true", false),
+ ok = config:set("per_doc_access", "enable", "true", false),
?_assertEqual(400, Code).
should_not_let_anonymous_user_create_doc(_PortType, Url) ->
