You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2020/10/01 19:36:35 UTC
svn commit: r1882186 - in /subversion/trunk/subversion: libsvn_repos/authz.c
tests/cmdline/svnauthz_tests.py tests/libsvn_repos/authz-test.c
tests/libsvn_repos/repos-test.c
Author: stsp
Date: Thu Oct 1 19:36:35 2020
New Revision: 1882186
URL: http://svn.apache.org/viewvc?rev=1882186&view=rev
Log:
Fix issue #4762 "authz doesn't combine global and repository rules"
The new authz implementation of SVN 1.10 introduced an incompatible change
in the interpretation of authz rules: Global rules access were not
considered if per-repository access rules were also supplied.
This change seems entirely unnecessary and is still causing problems today
for deployments which upgrade from earlier versions, such as from SVN 1.8.
Existing authz files no longer produce expected results and adjusting
large existing authz rule files to avoid this problem is a lot of work.
* subversion/libsvn_repos/authz.c
(authz_check_access): New helper function, extracted from ...
(svn_repos_authz_check_access): ... here. Always consider the global
rule set in addition to the repository-specific one. The results
from both rulesets are now merged as was the case before SVN 1.10.
* subversion/tests/cmdline/svnauthz_tests.py
(svnauthz_accessof_file_test,
svnauthz_accessof_repo_test,
svnauthz_accessof_groups_file_test,
svnauthz_accessof_groups_repo_test,
svnauthz_accessof_is_file_test,
svnauthz_accessof_is_repo_test): Adjust test expectations accordingly.
* subversion/tests/libsvn_repos/authz-test.c
(reposful_reposless_stanzas_inherit): Remove XFAIL marker.
* subversion/tests/libsvn_repos/repos-test.c
(test_authz_prefixes): Adjust test expectations. This test shows that
the behaviour change was likely deliberate. This test assumed that
global rules would only apply if listed after per-repository rules.
Change test expectations such that global rules are always taken
into account.
Modified:
subversion/trunk/subversion/libsvn_repos/authz.c
subversion/trunk/subversion/tests/cmdline/svnauthz_tests.py
subversion/trunk/subversion/tests/libsvn_repos/authz-test.c
subversion/trunk/subversion/tests/libsvn_repos/repos-test.c
Modified: subversion/trunk/subversion/libsvn_repos/authz.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/authz.c?rev=1882186&r1=1882185&r2=1882186&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/authz.c (original)
+++ subversion/trunk/subversion/libsvn_repos/authz.c Thu Oct 1 19:36:35 2020
@@ -1674,23 +1674,13 @@ svn_repos_authz_parse2(svn_authz_t **aut
return SVN_NO_ERROR;
}
-svn_error_t *
-svn_repos_authz_check_access(svn_authz_t *authz, const char *repos_name,
- const char *path, const char *user,
- svn_repos_authz_access_t required_access,
- svn_boolean_t *access_granted,
- apr_pool_t *pool)
+static svn_error_t *
+authz_check_access(svn_authz_t *authz, const char *path,
+ authz_user_rules_t *rules,
+ svn_repos_authz_access_t required_access,
+ authz_access_t required,
+ svn_boolean_t *access_granted, apr_pool_t *pool)
{
- const authz_access_t required =
- ((required_access & svn_authz_read ? authz_access_read_flag : 0)
- | (required_access & svn_authz_write ? authz_access_write_flag : 0));
-
- /* Pick or create the suitable pre-filtered path rule tree. */
- authz_user_rules_t *rules = get_user_rules(
- authz,
- (repos_name ? repos_name : AUTHZ_ANY_REPOSITORY),
- user);
-
/* In many scenarios, users have uniform access to a repository
* (blanket access or no access at all).
*
@@ -1736,3 +1726,39 @@ svn_repos_authz_check_access(svn_authz_t
return SVN_NO_ERROR;
}
+
+svn_error_t *
+svn_repos_authz_check_access(svn_authz_t *authz, const char *repos_name,
+ const char *path, const char *user,
+ svn_repos_authz_access_t required_access,
+ svn_boolean_t *access_granted,
+ apr_pool_t *pool)
+{
+ svn_error_t *err;
+ const authz_access_t required =
+ ((required_access & svn_authz_read ? authz_access_read_flag : 0)
+ | (required_access & svn_authz_write ? authz_access_write_flag : 0));
+ authz_user_rules_t *rules;
+ svn_boolean_t access_granted_by_any = FALSE;
+ svn_boolean_t access_granted_by_repos = FALSE;
+
+ *access_granted = FALSE;
+
+ rules = get_user_rules(authz, AUTHZ_ANY_REPOSITORY, user);
+ err = authz_check_access(authz, path, rules, required_access, required,
+ &access_granted_by_any, pool);
+ if (err)
+ return err;
+
+ if (repos_name)
+ {
+ rules = get_user_rules(authz, repos_name, user);
+ err = authz_check_access(authz, path, rules, required_access, required,
+ &access_granted_by_repos, pool);
+ if (err)
+ return err;
+ }
+
+ *access_granted = (access_granted_by_any || access_granted_by_repos);
+ return SVN_NO_ERROR;
+}
Modified: subversion/trunk/subversion/tests/cmdline/svnauthz_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svnauthz_tests.py?rev=1882186&r1=1882185&r2=1882186&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svnauthz_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svnauthz_tests.py Thu Oct 1 19:36:35 2020
@@ -224,8 +224,8 @@ def svnauthz_accessof_file_test(sbox):
svntest.actions.run_and_verify_svnauthz(["r\n"], None, 0, False, "accessof",
authz_path, "--path", "/jokes")
- # Anonymous access on /jokes on slapstick repo should be no
- svntest.actions.run_and_verify_svnauthz(["no\n"], None, 0, False, "accessof",
+ # Anonymous access on /jokes on slapstick repo should be r via rules on [/]
+ svntest.actions.run_and_verify_svnauthz(["r\n"], None, 0, False, "accessof",
authz_path, "--path", "/jokes",
"--repository", "slapstick")
@@ -289,8 +289,8 @@ def svnauthz_accessof_repo_test(sbox):
svntest.actions.run_and_verify_svnauthz(["r\n"], None, 0, False, "accessof",
authz_url, "--path", "/jokes")
- # Anonymous access on /jokes on slapstick repo should be no
- svntest.actions.run_and_verify_svnauthz(["no\n"], None, 0, False, "accessof",
+ # Anonymous access on /jokes on slapstick repo should be r via rules on [/]
+ svntest.actions.run_and_verify_svnauthz(["r\n"], None, 0, False, "accessof",
authz_url, "--path", "/jokes",
"--repository", "slapstick")
@@ -356,8 +356,8 @@ def svnauthz_accessof_groups_file_test(s
"--repository", "comedy")
# User stafford (@musicians) specified on /jokes with the repo comedy
- # will be no.
- svntest.actions.run_and_verify_svnauthz(["no\n"], None,
+ # will be rw.
+ svntest.actions.run_and_verify_svnauthz(["rw\n"], None,
0, False, "accessof", authz_path,
"--groups-file", groups_path,
"--path", "jokes",
@@ -440,8 +440,8 @@ def svnauthz_accessof_groups_repo_test(s
"--repository", "comedy")
# User stafford (@musicians) specified on /jokes with the repo comedy
- # will be no.
- svntest.actions.run_and_verify_svnauthz(["no\n"], None,
+ # will be rw via rules on [/].
+ svntest.actions.run_and_verify_svnauthz(["rw\n"], None,
0, False, "accessof", authz_url,
"--groups-file", groups_url,
"--path", "jokes",
@@ -508,19 +508,19 @@ def svnauthz_accessof_is_file_test(sbox)
authz_path, "--path", "/jokes",
"--is", "no")
- # Anonymous access on /jokes on slapstick repo should be no
- # Test --is no returns 0.
+ # Anonymous access on /jokes on slapstick repo should be r via rules on [/]
+ # Test --is r returns 0.
svntest.actions.run_and_verify_svnauthz(None, None, 0, False, "accessof",
authz_path, "--path", "/jokes",
"--repository", "slapstick",
- "--is", "no")
+ "--is", "r")
# Test --is rw returns 3.
svntest.actions.run_and_verify_svnauthz(None, None, 3, False, "accessof",
authz_path, "--path", "/jokes",
"--repository", "slapstick",
"--is", "rw")
- # Test --is r returns 3.
- svntest.actions.run_and_verify_svnauthz(None, None, 3, False, "accessof",
+ # Test --is r returns 0.
+ svntest.actions.run_and_verify_svnauthz(None, None, 0, False, "accessof",
authz_path, "--path", "/jokes",
"--repository", "slapstick",
"--is", "r")
@@ -666,19 +666,19 @@ def svnauthz_accessof_is_repo_test(sbox)
authz_url, "--path", "/jokes",
"--is", "no")
- # Anonymous access on /jokes on slapstick repo should be no
- # Test --is no returns 0.
+ # Anonymous access on /jokes on slapstick repo should be r via rules on [/]
+ # Test --is r returns 0.
svntest.actions.run_and_verify_svnauthz(None, None, 0, False, "accessof",
authz_url, "--path", "/jokes",
"--repository", "slapstick",
- "--is", "no")
+ "--is", "r")
# Test --is rw returns 3.
svntest.actions.run_and_verify_svnauthz(None, None, 3, False, "accessof",
authz_url, "--path", "/jokes",
"--repository", "slapstick",
"--is", "rw")
- # Test --is r returns 3.
- svntest.actions.run_and_verify_svnauthz(None, None, 3, False, "accessof",
+ # Test --is r returns 0.
+ svntest.actions.run_and_verify_svnauthz(None, None, 0, False, "accessof",
authz_url, "--path", "/jokes",
"--repository", "slapstick",
"--is", "r")
Modified: subversion/trunk/subversion/tests/libsvn_repos/authz-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_repos/authz-test.c?rev=1882186&r1=1882185&r2=1882186&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_repos/authz-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_repos/authz-test.c Thu Oct 1 19:36:35 2020
@@ -522,7 +522,7 @@ static struct svn_test_descriptor_t test
"test svn_authz__get_global_rights"),
SVN_TEST_PASS2(issue_4741_groups,
"issue 4741 groups"),
- SVN_TEST_XFAIL2(reposful_reposless_stanzas_inherit,
+ SVN_TEST_PASS2(reposful_reposless_stanzas_inherit,
"[foo:/] inherits [/]"),
SVN_TEST_NULL
};
Modified: subversion/trunk/subversion/tests/libsvn_repos/repos-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_repos/repos-test.c?rev=1882186&r1=1882185&r2=1882186&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_repos/repos-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_repos/repos-test.c Thu Oct 1 19:36:35 2020
@@ -1601,20 +1601,7 @@ test_authz_prefixes(apr_pool_t *pool)
enum { PATH_COUNT = 2 };
const char *test_paths[PATH_COUNT] = { "/", "/A" };
- /* Definition of the paths to test and expected replies for each. */
- struct check_access_tests test_set1[] = {
- /* Test that read rules are correctly used. */
- { "", "greek", NULL, svn_authz_read, FALSE },
- /* Test that write rules are correctly used. */
- { "", "greek", "plato", svn_authz_read, TRUE },
- { "", "greek", "plato", svn_authz_write, FALSE },
- /* Sentinel */
- { NULL, NULL, NULL, svn_authz_none, FALSE }
- };
-
- /* To be used when global rules are specified after per-repos rules.
- * In that case, the global rules still win. */
- struct check_access_tests test_set2[] = {
+ struct check_access_tests test_set[] = {
/* Test that read rules are correctly used. */
{ "", "greek", NULL, svn_authz_read, TRUE },
{ "", "greek", NULL, svn_authz_write, FALSE },
@@ -1634,7 +1621,6 @@ test_authz_prefixes(apr_pool_t *pool)
const char *repo1 = (combi & 4) ? "greek:" : "";
const char *repo2 = (combi & 4) ? "" : "greek:";
const char *test_path = test_paths[combi / 8];
- struct check_access_tests *test_set = (combi & 4) ? test_set2 : test_set1;
/* Create and parse the authz rules. */
svn_pool_clear(iterpool);
Re: svn commit: r1882186 - in /subversion/trunk/subversion:
libsvn_repos/authz.c tests/cmdline/svnauthz_tests.py
tests/libsvn_repos/authz-test.c tests/libsvn_repos/repos-test.c
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Oct 03, 2020 at 06:07:49AM +0000, Daniel Shahaf wrote:
> Sounds like this change merits an entry in the 1.14 release notes (if
> it's backported) or in the 1.15 release notes if it's not backported.
> Would you please add a placeholder (just a section header or a ToC
> link) or file a corresponding issue, so we don't forget to document
> this change?
The release notes already cover this problem as an authz incompatibility
issue and already mention that this behaviour might change again in a
future release.
My plan was to update that already existing section if this behaviour
is changed in a future release.
Re: svn commit: r1882186 - in /subversion/trunk/subversion:
libsvn_repos/authz.c tests/cmdline/svnauthz_tests.py
tests/libsvn_repos/authz-test.c tests/libsvn_repos/repos-test.c
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Oct 03, 2020 at 06:07:49AM +0000, Daniel Shahaf wrote:
> Sounds like this change merits an entry in the 1.14 release notes (if
> it's backported) or in the 1.15 release notes if it's not backported.
> Would you please add a placeholder (just a section header or a ToC
> link) or file a corresponding issue, so we don't forget to document
> this change?
The release notes already cover this problem as an authz incompatibility
issue and already mention that this behaviour might change again in a
future release.
My plan was to update that already existing section if this behaviour
is changed in a future release.
Re: Fixing issue #4762 -- authz doesn't combine global and
repository rules -- svn 1.10 regression [was: r1882186 ...]
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Oct 06, 2020 at 06:26:06PM +0200, Stefan Sperling wrote:
> I will see about whether this can be fixed in a follow-up patch.
> Otherwise I will revert my commit and try to find a better approach.
See https://svn.apache.org/r1882326 for a better fix, based on
improved understanding of the bug.
None of the other authz tests required adjustment this time around!
I hope this gives us a better idea about of the impact of this change.
The log message explains the issue and my solution:
[[[
Fix issue #4762 "authz doesn't combine global and repository rules"
The new authz implementation of SVN 1.10 introduced an incompatible
change in the interpretation of authz rules:
Global access rules can be configured for a particular user and a specific
path. Such global rules were ignored if a repository-specific rule is also
present for the same path, even if this repository-specific rule does not
apply to the user in question.
This change seems unnecessary because it broke backwards compatibility with
existing authz files, from SVN 1.9 and older, for no discernible benefit.
The change was probably not intentional as this situation was not covered
by the test suite before a test case was added in r1835049.
Restore the behaviour of SVN 1.9: It is now again possible to override
per-path access rules for specific users (and groups) at the global level.
Such global rules are overridden by repository-specific rules only if
both the user and the path match the repository-specific rule.
* subversion/libsvn_repos/authz.c
(create_user_authz): Prefer rules which apply to both the user and
the path over rules which apply only to the path. If both a global
and a repository-specific rule apply to user and path then prefer
the repository-specific one.
* subversion/tests/libsvn_repos/authz-test.c
(reposful_reposless_stanzas_inherit): Remove XFAIL marker.
]]]
My impression is still that this is a low-risk change which we could
ship in a patch release. I would argue that existing rulesets are being
misinterpreted due to a gap in our test coverage. It seems unlikely that
someone would intentionally write rulesets which take advantage of this
quirk.
Of course, we could see breakage in case someone does rely on the new
behaviour. Regardless, shipping this fix doesn't seem unreasonable to me.
Cheers,
Stefan
Re: Fixing issue #4762 -- authz doesn't combine global and
repository rules -- svn 1.10 regression [was: r1882186 ...]
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Oct 06, 2020 at 03:24:01PM +0100, Julian Foad wrote:
> A third group is pre-existing deployments in which the admins have now
> adjusted their rules to match the 1.10+ behaviour. I can't guess which
> group is biggest, which ones matter more or less than others, nor in what
> proportion of deployments the semantic change would make any actual
> difference to the authz rules in effect.
There is indeed a regression in my proposed fix (r1882186).
An unintended behaviour change now exists on trunk, where the
result of an access check differs from both 1.9 and 1.14.
This regression is exposed by the test suite. I missed it because I did
not pay close enough attention to the test results which had changed.
Given this authz file, derived from permutation 11 of test_authz_prefixes()
from before r1882186:
[[[
[/A]
* = r
plato = rw
[greek:/A]
* =
plato = r
]]]
I see the following results for the following query:
svnauthz accessof --username plato --repository greek --path /A
trunk returns rw
1.14 returns r
1.9 returns r
I will see about whether this can be fixed in a follow-up patch.
Otherwise I will revert my commit and try to find a better approach.
Do we have an exact specification somewhere about how merging of
global and repository-specific rules was done in 1.9? It seems global
rules were applied only if no explicit rule was defined for the path
being accessed? With the root directory being a special "fallback" case?
The specific example given in issue #4762 deals with the root directory,
so perhaps this is actually a special case which needs a more specific fix?
Re: Fixing issue #4762 -- authz doesn't combine global and repository
rules -- svn 1.10 regression [was: r1882186 ...]
Posted by Julian Foad <ju...@apache.org>.
Nathan Hartman wrote:
> Stefan Sperling wrote:
>> In my opinion the original change was a mistake which should be corrected.
To be clear: I don't disagree (and thank you for debugging and fixing
it); I understand your reasoning; I just don't have the context to judge
the impact and want to be sure we don't create a new problem when
deploying a fix for the original problem.
>> I think the benefits of seamless upgrades of existing deployments outweight
>> concerns over potential regressions in fresh deployments (which, let's admit
>> it, are getting rare these days).
A third group is pre-existing deployments in which the admins have now
adjusted their rules to match the 1.10+ behaviour. I can't guess which
group is biggest, which ones matter more or less than others, nor in
what proportion of deployments the semantic change would make any actual
difference to the authz rules in effect.
- Julian
Re: Fixing issue #4762 -- authz doesn't combine global and repository
rules -- svn 1.10 regression [was: r1882186 ...]
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Oct 6, 2020 at 9:36 AM Stefan Sperling <st...@elego.de> wrote:
> In my opinion the original change was a mistake which should be corrected.
> I think the benefits of seamless upgrades of existing deployments outweight
> concerns over potential regressions in fresh deployments (which, let's admit
> it, are getting rare these days).
> If a fix would have to wait for 1.15, so be it.
Perhaps this merits a post to users@ to get feedback from the wider community?
Nathan
Re: Fixing issue #4762 -- authz doesn't combine global and
repository rules -- svn 1.10 regression [was: r1882186 ...]
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Oct 06, 2020 at 12:21:27PM +0100, Julian Foad wrote:
> Doug Robinson: Would you please cast your eye over this, as an affected
> party? Basically I'd ask if you want to offer your take on fixing this
> authz semantics regression?
>
> I haven't been following this issue; I've just noticed it. On quick
> inspection it seems to me after two LTS releases with changed semantics, it
> might not be appropriate to change it back unconditionally in point
> releases, as this could be just as bad for those who have begun relying on
> the new semantics. Instead it would seem better to seek some way to take
> account of both groups of users, those who are relying on each version of
> the semantics. But I am not familiar with the nuances of this particular
> change and how it may affect users in practice, to judge what is best.
Note that the regression tests added for 1.10 still allowed for global
and per-repo rules to be merged. The code assumes global rules to
be listed _after_ repository-specific rules:
https://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_repos/repos-test.c?r1=1882186&r2=1882185&pathrev=1882186
[[[
/* To be used when global rules are specified after per-repos rules.
* In that case, the global rules still win. */
]]]
Conceivably, somebody could now have written and tested a ruleset which
prevents access to a path only because global rules are listed before
per-repository rules.
Unfortunately we cannot know whether such cases exist in the wild.
My problem right now is that people are upgrading servers with hundresds
of repositories and hundreds of lines of already vetted authz files and
they will need to test their entire ruleset again. In this light, the
benefit of the behaviour change made in 1.10 is entirely unclear to me.
Perhaps we could add a flag somewhere which specifies whether global
rules should be merged with per-repository rules, but that doesn't make
the suggested fix less problematic. In a patch release we usually promise
that a roll-back with existing config files remains possible. If we take
this strictly, then any fix for issue #4762 would have to wait for 1.15,
regardless of whether we retain both and new behaviours or just revert
back to the old behaviour.
In my opinion the original change was a mistake which should be corrected.
I think the benefits of seamless upgrades of existing deployments outweight
concerns over potential regressions in fresh deployments (which, let's admit
it, are getting rare these days).
If a fix would have to wait for 1.15, so be it.
Fixing issue #4762 -- authz doesn't combine global and repository
rules -- svn 1.10 regression [was: r1882186 ...]
Posted by Julian Foad <ju...@apache.org>.
Doug Robinson: Would you please cast your eye over this, as an affected
party? Basically I'd ask if you want to offer your take on fixing this
authz semantics regression?
I haven't been following this issue; I've just noticed it. On quick
inspection it seems to me after two LTS releases with changed semantics,
it might not be appropriate to change it back unconditionally in point
releases, as this could be just as bad for those who have begun relying
on the new semantics. Instead it would seem better to seek some way to
take account of both groups of users, those who are relying on each
version of the semantics. But I am not familiar with the nuances of
this particular change and how it may affect users in practice, to judge
what is best.
References:
* 2018-07-04 users@ email "svn 1.10: mod_authz no longer combines ACL
entries"
https://lists.apache.org/thread.html/1aef30b70b4063ecfa24f17be0e1c9998837b4f8b3f7e8b03de4e240@%3Cusers.subversion.apache.org%3E
* 2018-07-10 issue 4762 "authz doesn't combine global and repository rules"
https://subversion.apache.org/issue/4762
* 2020-10-01 "Fix issue #4762 ..."
http://svn.apache.org/r1882186
(Subject line changed for visibility; cc added.)
Daniel Shahaf wrote:
> stsp@apache.org wrote on Thu, 01 Oct 2020 19:36 +00:00:
>> Fix issue #4762 "authz doesn't combine global and repository rules"
>>
>> The new authz implementation of SVN 1.10 introduced an incompatible change
>> in the interpretation of authz rules: Global rules access were not
>> considered if per-repository access rules were also supplied.
>>
>> This change seems entirely unnecessary
>
> I'm wary of this justification. The semantics being changed here were
> covered by tests and, according to your very log message, were likely
> implemented deliberately. Reverting such semantics requires better
> arguments than "they seem unnecessary". That argument is exactly how
> the Debian OpenSSL bug was introduced.
>
> I think we should more seriously investigate the possibility that _this_
> change, r1882186, will break some use-case or another.
>
> (To be clear, I'm disagreeing only with the _justification_ for the
> change, not with the change itself. I'm not commenting on the
> change itself.)
>
>> and is still causing problems today
>> for deployments which upgrade from earlier versions, such as from SVN 1.8.
>> Existing authz files no longer produce expected results and adjusting
>> large existing authz rule files to avoid this problem is a lot of work.
>
> Sounds like this change merits an entry in the 1.14 release notes (if
> it's backported) or in the 1.15 release notes if it's not backported.
> Would you please add a placeholder (just a section header or a ToC
> link) or file a corresponding issue, so we don't forget to document
> this change?
>
>> * subversion/libsvn_repos/authz.c
>> (authz_check_access): New helper function, extracted from ...
>> (svn_repos_authz_check_access): ... here. Always consider the global
>> rule set in addition to the repository-specific one. The results
>> from both rulesets are now merged as was the case before SVN 1.10.
>>
>> * subversion/tests/cmdline/svnauthz_tests.py
>> (svnauthz_accessof_file_test,
>> svnauthz_accessof_repo_test,
>> svnauthz_accessof_groups_file_test,
>> svnauthz_accessof_groups_repo_test,
>> svnauthz_accessof_is_file_test,
>> svnauthz_accessof_is_repo_test): Adjust test expectations accordingly.
>>
>> * subversion/tests/libsvn_repos/authz-test.c
>> (reposful_reposless_stanzas_inherit): Remove XFAIL marker.
>>
>> * subversion/tests/libsvn_repos/repos-test.c
>> (test_authz_prefixes): Adjust test expectations. This test shows that
>> the behaviour change was likely deliberate. This test assumed that
>> global rules would only apply if listed after per-repository rules.
>> Change test expectations such that global rules are always taken
>> into account.
Re: svn commit: r1882186 - in /subversion/trunk/subversion: libsvn_repos/authz.c tests/cmdline/svnauthz_tests.py tests/libsvn_repos/authz-test.c tests/libsvn_repos/repos-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stsp@apache.org wrote on Thu, 01 Oct 2020 19:36 +00:00:
> Fix issue #4762 "authz doesn't combine global and repository rules"
>
> The new authz implementation of SVN 1.10 introduced an incompatible change
> in the interpretation of authz rules: Global rules access were not
> considered if per-repository access rules were also supplied.
>
> This change seems entirely unnecessary
I'm wary of this justification. The semantics being changed here were
covered by tests and, according to your very log message, were likely
implemented deliberately. Reverting such semantics requires better
arguments than "they seem unnecessary". That argument is exactly how
the Debian OpenSSL bug was introduced.
I think we should more seriously investigate the possibility that _this_
change, r1882186, will break some use-case or another.
(To be clear, I'm disagreeing only with the _justification_ for the
change, not with the change itself. I'm not commenting on the
change itself.)
> and is still causing problems today
> for deployments which upgrade from earlier versions, such as from SVN 1.8.
> Existing authz files no longer produce expected results and adjusting
> large existing authz rule files to avoid this problem is a lot of work.
Sounds like this change merits an entry in the 1.14 release notes (if
it's backported) or in the 1.15 release notes if it's not backported.
Would you please add a placeholder (just a section header or a ToC
link) or file a corresponding issue, so we don't forget to document
this change?
> * subversion/libsvn_repos/authz.c
> (authz_check_access): New helper function, extracted from ...
> (svn_repos_authz_check_access): ... here. Always consider the global
> rule set in addition to the repository-specific one. The results
> from both rulesets are now merged as was the case before SVN 1.10.
>
> * subversion/tests/cmdline/svnauthz_tests.py
> (svnauthz_accessof_file_test,
> svnauthz_accessof_repo_test,
> svnauthz_accessof_groups_file_test,
> svnauthz_accessof_groups_repo_test,
> svnauthz_accessof_is_file_test,
> svnauthz_accessof_is_repo_test): Adjust test expectations accordingly.
>
> * subversion/tests/libsvn_repos/authz-test.c
> (reposful_reposless_stanzas_inherit): Remove XFAIL marker.
>
> * subversion/tests/libsvn_repos/repos-test.c
> (test_authz_prefixes): Adjust test expectations. This test shows that
> the behaviour change was likely deliberate. This test assumed that
> global rules would only apply if listed after per-repository rules.
> Change test expectations such that global rules are always taken
> into account.
Re: svn commit: r1882186 - in /subversion/trunk/subversion: libsvn_repos/authz.c tests/cmdline/svnauthz_tests.py tests/libsvn_repos/authz-test.c tests/libsvn_repos/repos-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stsp@apache.org wrote on Thu, 01 Oct 2020 19:36 +00:00:
> Fix issue #4762 "authz doesn't combine global and repository rules"
>
> The new authz implementation of SVN 1.10 introduced an incompatible change
> in the interpretation of authz rules: Global rules access were not
> considered if per-repository access rules were also supplied.
>
> This change seems entirely unnecessary
I'm wary of this justification. The semantics being changed here were
covered by tests and, according to your very log message, were likely
implemented deliberately. Reverting such semantics requires better
arguments than "they seem unnecessary". That argument is exactly how
the Debian OpenSSL bug was introduced.
I think we should more seriously investigate the possibility that _this_
change, r1882186, will break some use-case or another.
(To be clear, I'm disagreeing only with the _justification_ for the
change, not with the change itself. I'm not commenting on the
change itself.)
> and is still causing problems today
> for deployments which upgrade from earlier versions, such as from SVN 1.8.
> Existing authz files no longer produce expected results and adjusting
> large existing authz rule files to avoid this problem is a lot of work.
Sounds like this change merits an entry in the 1.14 release notes (if
it's backported) or in the 1.15 release notes if it's not backported.
Would you please add a placeholder (just a section header or a ToC
link) or file a corresponding issue, so we don't forget to document
this change?
> * subversion/libsvn_repos/authz.c
> (authz_check_access): New helper function, extracted from ...
> (svn_repos_authz_check_access): ... here. Always consider the global
> rule set in addition to the repository-specific one. The results
> from both rulesets are now merged as was the case before SVN 1.10.
>
> * subversion/tests/cmdline/svnauthz_tests.py
> (svnauthz_accessof_file_test,
> svnauthz_accessof_repo_test,
> svnauthz_accessof_groups_file_test,
> svnauthz_accessof_groups_repo_test,
> svnauthz_accessof_is_file_test,
> svnauthz_accessof_is_repo_test): Adjust test expectations accordingly.
>
> * subversion/tests/libsvn_repos/authz-test.c
> (reposful_reposless_stanzas_inherit): Remove XFAIL marker.
>
> * subversion/tests/libsvn_repos/repos-test.c
> (test_authz_prefixes): Adjust test expectations. This test shows that
> the behaviour change was likely deliberate. This test assumed that
> global rules would only apply if listed after per-repository rules.
> Change test expectations such that global rules are always taken
> into account.