Re: authz's inverted group access rules are only effective for users mentioned in authz
On 27.11.2018 22:33, Branko Čibej wrote: > On 27.11.2018 10:41, Branko Čibej wrote: >> On 26.11.2018 18:32, Pavel Goran wrote: >>> Hello mailing list, >>> >>> I'm migrating a repository from an ancient Subversion installation: >>> >>> svn, version 1.6.17 (r1128011) >>> compiled Jun 2 2011, 23:35:08 >>> (on Windows) >>> >>> to a recent version: >>> >>> svn, version 1.11.0 (r1845130) >>>compiled Oct 29 2018, 14:33:24 on x86_64-unknown-linux-gnu >>> (on NixOS Linux) >>> >>> And I'm encountering a regression in handling of the authz file by svnserve. >>> >>> Steps to reproduce: >> [...] >>> # svnserve -r /data/svn -d >>> >>> # svn --username myuser --password mypassword --no-auth-cache ls >>> svn://localhost/myrepo >>> svn: E170001: Authorization failed >>> >>> The user "myuser" is not included in group "readonly", so I expect the rule >>> "~@readonly = rw" to take effect, but apparently this doesn't happen. >>> >>> This setup worked fine in the old installation (version 1.6.17). Also, it >>> starts working if I add the user "myuser" to any other group: >>> >>> # in authz, [groups] section >>> unrelatedgroup = myuser >>> >>> or use it in any access rule (even if the rule specifies no access): >>> >>> # in authz, [/] section >>> myuser = >>> >>> I looked at the sources (subversion/libsvn_repos/authz_parse.c), and my >>> guess is that this behaviour results from not setting up user's rights with >>> a call to prepare_global_rights(). When a user is mentioned in authz, this >>> function gets called, and authorization starts working for the user. >>> >>> Possibly correction of this problem could involve adding the new field >>> "unkn_rights" (and "has_unkn_rights") to struct authz_full_t, so that this >>> field would receive access rights from all inverted user-related access >>> rules (~user, ~, ~@group). Then, svn_authz__get_global_rights() would >>> combine authz->has_unkn_rights with authz->has_authn_rights (instead of just >>> returning authz->has_authn_rights) when user_rights is NULL. >>> >>> Pavel Goran >>> >>> P.S. I'm not subscribed to the mailing list; please CC me directly when >>> replying. >> Thanks for the detailed report! Yes, this is indeed a bug. I see that >> you've taken time to analyse the code; could you prepare a patch with >> the fix you propose? >> >> Also can you please create an issue in Jira: >> https://issues.apache.org/jira/projects/SVN/ > > I added a test case for this in r1847598, using the reproduction steps > described above. https://issues.apache.org/jira/browse/SVN-4793 -- Brane
Re: authz's inverted group access rules are only effective for users mentioned in authz
On 27.11.2018 10:41, Branko Čibej wrote: > On 26.11.2018 18:32, Pavel Goran wrote: >> Hello mailing list, >> >> I'm migrating a repository from an ancient Subversion installation: >> >> svn, version 1.6.17 (r1128011) >> compiled Jun 2 2011, 23:35:08 >> (on Windows) >> >> to a recent version: >> >> svn, version 1.11.0 (r1845130) >>compiled Oct 29 2018, 14:33:24 on x86_64-unknown-linux-gnu >> (on NixOS Linux) >> >> And I'm encountering a regression in handling of the authz file by svnserve. >> >> Steps to reproduce: > [...] >> # svnserve -r /data/svn -d >> >> # svn --username myuser --password mypassword --no-auth-cache ls >> svn://localhost/myrepo >> svn: E170001: Authorization failed >> >> The user "myuser" is not included in group "readonly", so I expect the rule >> "~@readonly = rw" to take effect, but apparently this doesn't happen. >> >> This setup worked fine in the old installation (version 1.6.17). Also, it >> starts working if I add the user "myuser" to any other group: >> >> # in authz, [groups] section >> unrelatedgroup = myuser >> >> or use it in any access rule (even if the rule specifies no access): >> >> # in authz, [/] section >> myuser = >> >> I looked at the sources (subversion/libsvn_repos/authz_parse.c), and my >> guess is that this behaviour results from not setting up user's rights with >> a call to prepare_global_rights(). When a user is mentioned in authz, this >> function gets called, and authorization starts working for the user. >> >> Possibly correction of this problem could involve adding the new field >> "unkn_rights" (and "has_unkn_rights") to struct authz_full_t, so that this >> field would receive access rights from all inverted user-related access >> rules (~user, ~, ~@group). Then, svn_authz__get_global_rights() would >> combine authz->has_unkn_rights with authz->has_authn_rights (instead of just >> returning authz->has_authn_rights) when user_rights is NULL. >> >> Pavel Goran >> >> P.S. I'm not subscribed to the mailing list; please CC me directly when >> replying. > Thanks for the detailed report! Yes, this is indeed a bug. I see that > you've taken time to analyse the code; could you prepare a patch with > the fix you propose? > > Also can you please create an issue in Jira: > https://issues.apache.org/jira/projects/SVN/ I added a test case for this in r1847598, using the reproduction steps described above. -- Brane
Re: svn commit: r1847572 - in /subversion/trunk/subversion: libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c
Branko Čibej writes: > > + else > > +return err_not_directory(root, directory, pool); > > +} > > } > > } > > > > Would be nice to wrap this return value in svn_error_trace(), as that > makes it easier to find where the error came from. Makes sense, committed in https://svn.apache.org/r1847596 Thanks, Evgeny Kotkov
Re: [PATCH] A test for "Can't get entries" error
Evgeny Kotkov wrote:
> Apparently, this behavior is caused by a problem in the specific optimization
> within the FSFS open_path() routine.
>
> I constructed an FS regression test and committed it and the fix in:
>https://svn.apache.org/r1847572
>
> (I'll try to nominate it for backport once I get some time.)
Excellent! Thanks!
I had opened issue SVN-4791 for this. I have updated the log message and the
issue to cross-reference each other.
https://issues.apache.org/jira/browse/SVN-4791
I expect we could improve the issue summary line which is currently "FSFS Can't
get entries of non-directory".
I'm glad you included a FS-level test for it. In case anyone is interested, I
attach a patch that backports the initial RA-level test to 1.8. 1.9 and 1.10.
I haven't reviewed or tested your r1847572 fix or test.
--
- Julian
Backport the initial RA-level test for SVN-4791 to 1.8, 1.9, 1.10.
* 1.8.x/subversion/tests/libsvn_ra/ra-test.c
* 1.9.x/subversion/tests/libsvn_ra/ra-test.c
* 1.10.x/subversion/tests/libsvn_ra/ra-test.c
(cant_get_entries_of_non_directory): New test.
(test_funcs): Run it.
--This line, and those below, will be ignored--
Index: 1.10.x/subversion/tests/libsvn_ra/ra-test.c
===
--- 1.10.x/subversion/tests/libsvn_ra/ra-test.c (revision 1847591)
+++ 1.10.x/subversion/tests/libsvn_ra/ra-test.c (working copy)
@@ -19,13 +19,13 @@
*specific language governing permissions and limitations
*under the License.
*
*/
-
+
#include
#include
#include
#include
#include "svn_error.h"
@@ -1781,12 +1781,134 @@ commit_locked_file(const svn_test_opts_t
SVN_TEST_ASSERT(propval);
SVN_TEST_STRING_ASSERT(propval->data, "propval");
return SVN_NO_ERROR;
}
+static svn_error_t *
+cant_get_entries_of_non_directory(const svn_test_opts_t *opts, apr_pool_t *pool)
+{
+ svn_ra_session_t *session;
+
+ SVN_ERR(make_and_open_repos(,
+ "cant_get_entries_of_non_directory", opts,
+ pool));
+
+ {
+const svn_delta_editor_t *editor;
+void *edit_baton;
+void *root_baton;
+void *dir_baton;
+void *file_baton;
+
+SVN_ERR(svn_ra_get_commit_editor3(session, , _baton,
+ apr_hash_make(pool), NULL,
+ NULL, NULL, FALSE, pool));
+SVN_ERR(editor->open_root(edit_baton, 0, pool, _baton));
+SVN_ERR(editor->add_directory("A", root_baton, NULL, SVN_INVALID_REVNUM,
+ pool, _baton));
+SVN_ERR(editor->add_file("A/mu", dir_baton, NULL, SVN_INVALID_REVNUM,
+ pool, _baton));
+SVN_ERR(editor->close_file(file_baton, NULL, pool));
+SVN_ERR(editor->close_directory(dir_baton, pool));
+SVN_ERR(editor->close_directory(root_baton, pool));
+SVN_ERR(editor->close_edit(edit_baton, pool));
+ }
+ {
+const svn_delta_editor_t *editor;
+void *edit_baton;
+void *root_baton;
+void *dir_baton;
+const char* repos_root_url;
+const char* A_url;
+
+SVN_ERR(svn_ra_get_repos_root2(session, _root_url, pool));
+A_url = svn_path_url_add_component2(repos_root_url, "A", pool);
+
+SVN_ERR(svn_ra_get_commit_editor3(session, , _baton,
+ apr_hash_make(pool), NULL,
+ NULL, NULL, FALSE, pool));
+SVN_ERR(editor->open_root(edit_baton, 1, pool, _baton));
+SVN_ERR(editor->add_directory("B", root_baton, A_url, 1,
+ pool, _baton));
+SVN_ERR(editor->close_directory(dir_baton, pool));
+SVN_ERR(editor->close_directory(root_baton, pool));
+SVN_ERR(editor->close_edit(edit_baton, pool));
+ }
+ {
+const svn_delta_editor_t *editor;
+void *edit_baton;
+void *root_baton;
+
+SVN_ERR(svn_ra_get_commit_editor3(session, , _baton,
+ apr_hash_make(pool), NULL,
+ NULL, NULL, FALSE, pool));
+SVN_ERR(editor->open_root(edit_baton, 2, pool, _baton));
+SVN_ERR(editor->delete_entry("B/mu", 2, root_baton, pool));
+SVN_ERR(editor->close_directory(root_baton, pool));
+SVN_ERR(editor->close_edit(edit_baton, pool));
+ }
+ {
+const svn_delta_editor_t *editor;
+void *edit_baton;
+void *root_baton;
+void *dir_baton;
+void *subdir_baton;
+void *file_baton;
+
+SVN_ERR(svn_ra_get_commit_editor3(session, , _baton,
+ apr_hash_make(pool), NULL,
+ NULL, NULL, FALSE, pool));
+SVN_ERR(editor->open_root(edit_baton, 3, pool, _baton));
+SVN_ERR(editor->open_directory("B", root_baton, 3, pool, _baton));
+SVN_ERR(editor->add_directory("B/mu", root_baton, NULL, SVN_INVALID_REVNUM,
+ pool, _baton));
Re: svn commit: r1847572 - in /subversion/trunk/subversion: libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c
On 27.11.2018 19:10, kot...@apache.org wrote:
> Author: kotkov
> Date: Tue Nov 27 18:10:01 2018
> New Revision: 1847572
>
> URL: http://svn.apache.org/viewvc?rev=1847572=rev
> Log:
> fsfs: Fix an issue with the DAG open_path() that was causing unexpected
> SVN_ERR_FS_NOT_DIRECTORY errors when attempting to open a path with
> `open_path_node_only | open_path_allow_null` flags.
[...]
> @@ -1016,12 +1035,26 @@ open_path(parent_path_t **parent_path_p,
>SVN_ERR(dag_node_cache_get(, root, directory, pool));
>
>/* Did the shortcut work? */
> - if (here)
> + if (here && svn_fs_fs__dag_node_kind(here) == svn_node_dir)
> {
>apr_size_t dirname_len = strlen(directory);
>path_so_far->len = dirname_len;
>rest = path + dirname_len + 1;
> }
> + else if (here)
> +{
> + /* The parent node is not a directory. We are looking for some
> + sub-path, so that sub-path will not exist. That will be
> o.k.
> + if we are just here to check for the path's existence, but
> + should result in an error otherwise. */
> + if (flags & open_path_allow_null)
> +{
> + *parent_path_p = NULL;
> + return SVN_NO_ERROR;
> +}
> + else
> +return err_not_directory(root, directory, pool);
> +}
> }
> }
>
Would be nice to wrap this return value in svn_error_trace(), as that
makes it easier to find where the error came from.
> @@ -1144,8 +1177,6 @@ open_path(parent_path_t **parent_path_p,
>/* The path isn't finished yet; we'd better be in a directory. */
>if (svn_fs_fs__dag_node_kind(child) != svn_node_dir)
> {
> - const char *msg;
> -
>/* Since this is not a directory and we are looking for some
> sub-path, that sub-path will not exist. That will be o.k.,
> if we are just here to check for the path's existence. */
> @@ -1156,14 +1187,7 @@ open_path(parent_path_t **parent_path_p,
> }
>
>/* It's really a problem ... */
> - msg = root->is_txn_root
> - ? apr_psprintf(iterpool,
> - _("Failure opening '%s' in transaction '%s'"),
> - path, root->txn)
> - : apr_psprintf(iterpool,
> - _("Failure opening '%s' in revision %ld"),
> - path, root->rev);
> - SVN_ERR_W(SVN_FS__ERR_NOT_DIRECTORY(fs, path_so_far->data), msg);
> + return err_not_directory(root, path_so_far->data, iterpool);
> }
And here.
-- Brane
Re: [PATCH] A test for "Can't get entries" error
Dmitry Pavlenko writes: > I've run into an error: when performing 2 specially constructed updates one > after another within the same session, SVN fails with > > $ ./ra-test 15 > svn_tests: E160016: Can't get entries of non-directory > XFAIL: ra-test 15: check that there's no "Can't get entries" error > > error. I believe these updates constructed that way are valid, so the problem > is somewhere in FSFS code. It's also interesting that if these updates are > run separately (e.g. by adding "if (FALSE)" to one or another), they succeed. Apparently, this behavior is caused by a problem in the specific optimization within the FSFS open_path() routine. I constructed an FS regression test and committed it and the fix in: https://svn.apache.org/r1847572 (I'll try to nominate it for backport once I get some time.) Thanks, Evgeny Kotkov
Re: truncated author names in 'svn ls -v' output
On 27.11.2018 12:22, Branko Čibej wrote: > On 26.11.2018 04:15, Daniel Shahaf wrote: >> Branko Čibej wrote on Sun, 25 Nov 2018 23:28 +0100: >>> On 25.11.2018 18:42, Branko Čibej wrote: On 25.11.2018 11:58, Daniel Shahaf wrote: > Branko Čibej wrote on Sun, 25 Nov 2018 06:06 +0100: >> $ svn ls --verbose --human-readable >> 1847281 stsp Nov 23 16:04 ./ >> 1716820 rhuijben 175B Nov 27 2015 .editorconfig > 'svn info' doesn't print file sizes at all. If it did, it could take > the new flag too. svn info -H --show-item=size? :) >>> But seriouisly '--human-readable' and '--show-item' would be incompatible. >> I don't see why. It would make perfect sense to have: >> . >> % svn info --human-readable --show-item=repos-size >> 42KiB >> % >> . >> for interactive usage. > > This? > > > $ svn info -RH --show-item=repos-size > ^/subversion/developer-resources/difftools > 66 B > https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/README [...] On second thought, I think I should revert this. Consider the inconsistency with --show-item=last-changed-date: $ svn info ^/subversion/trunk/CHANGES --human-readable --show-item=last-changed-date 2018-11-26T02:50:47.513639Z but $ svn info ^/subversion/trunk/CHANGES | grep '^Last Changed Date' Last Changed Date: 2018-11-26 03:50:47 +0100 (Mon, 26 Nov 2018) The output from --show-item was always meant to be strictly for scripts. -- Brane
Re: truncated author names in 'svn ls -v' output
On 26.11.2018 04:15, Daniel Shahaf wrote: > Branko Čibej wrote on Sun, 25 Nov 2018 23:28 +0100: >> On 25.11.2018 18:42, Branko Čibej wrote: >>> On 25.11.2018 11:58, Daniel Shahaf wrote: Branko Čibej wrote on Sun, 25 Nov 2018 06:06 +0100: > $ svn ls --verbose --human-readable > 1847281 stsp Nov 23 16:04 ./ > 1716820 rhuijben 175B Nov 27 2015 .editorconfig 'svn info' doesn't print file sizes at all. If it did, it could take the new flag too. >>> svn info -H --show-item=size? :) >> But seriouisly '--human-readable' and '--show-item' would be incompatible. > I don't see why. It would make perfect sense to have: > . > % svn info --human-readable --show-item=repos-size > 42KiB > % > . > for interactive usage. This? $ svn info -RH --show-item=repos-size ^/subversion/developer-resources/difftools 66 B https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/README 57 B https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/pics/README 20 KiB https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/pics/FileMerge1.jpg 30 KiB https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/pics/FileMerge2.jpg 90 KiB https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/pics/FileMerge3.jpg 115 KiB https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/pics/FileMerge4.jpg 84 KiB https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/pics/FileMerge5.jpg 1.1 KiB https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/pics/xxdiff-README 17 KiB https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/pics/xxdiff-1.png 464 B https://svn.apache.org/repos/asf/subversion/developer-resources/difftools/pics/FileMerge-README -- Brane
Re: authz's inverted group access rules are only effective for users mentioned in authz
On 26.11.2018 18:32, Pavel Goran wrote: > Hello mailing list, > > I'm migrating a repository from an ancient Subversion installation: > > svn, version 1.6.17 (r1128011) > compiled Jun 2 2011, 23:35:08 > (on Windows) > > to a recent version: > > svn, version 1.11.0 (r1845130) >compiled Oct 29 2018, 14:33:24 on x86_64-unknown-linux-gnu > (on NixOS Linux) > > And I'm encountering a regression in handling of the authz file by svnserve. > > Steps to reproduce: [...] > # svnserve -r /data/svn -d > > # svn --username myuser --password mypassword --no-auth-cache ls > svn://localhost/myrepo > svn: E170001: Authorization failed > > The user "myuser" is not included in group "readonly", so I expect the rule > "~@readonly = rw" to take effect, but apparently this doesn't happen. > > This setup worked fine in the old installation (version 1.6.17). Also, it > starts working if I add the user "myuser" to any other group: > > # in authz, [groups] section > unrelatedgroup = myuser > > or use it in any access rule (even if the rule specifies no access): > > # in authz, [/] section > myuser = > > I looked at the sources (subversion/libsvn_repos/authz_parse.c), and my > guess is that this behaviour results from not setting up user's rights with > a call to prepare_global_rights(). When a user is mentioned in authz, this > function gets called, and authorization starts working for the user. > > Possibly correction of this problem could involve adding the new field > "unkn_rights" (and "has_unkn_rights") to struct authz_full_t, so that this > field would receive access rights from all inverted user-related access > rules (~user, ~, ~@group). Then, svn_authz__get_global_rights() would > combine authz->has_unkn_rights with authz->has_authn_rights (instead of just > returning authz->has_authn_rights) when user_rights is NULL. > > Pavel Goran > > P.S. I'm not subscribed to the mailing list; please CC me directly when > replying. Thanks for the detailed report! Yes, this is indeed a bug. I see that you've taken time to analyse the code; could you prepare a patch with the fix you propose? Also can you please create an issue in Jira: https://issues.apache.org/jira/projects/SVN/ -- Brane
Re: truncated author names in 'svn ls -v' output
On 26.11.2018 16:30, Vincent Lefevre wrote: > On 2018-11-26 15:18:47 +0100, Branko Čibej wrote: >> Do please read the rest of the thread. A solution has already been >> implemented on trunk. > Except that this solution is not satisfactory for me. I guess that > the ultimate solution is to use --xml + a wrapper (thus one could > add things like coloring), though that's still limited as one > doesn't get the MIME type (svn:mime-type). I'm delighted that you have a solution. Job done. -- Brane
