On 20.07.2018 14:59, Philip Martin wrote:
> In 1.9 it was possible to repeat, or reopen, a section:
>
> [/some/path]
> user = r
> [/some/path]
> otheruser = rw
>
> This was equivalent to a single section:
>
> [/some/path]
> user = r
> otheruser = rw
>
> In 1.10 this is rejected by the parser and cannot be used. Is this a
> bug in 1.10 or an acceptable behaviour change?
It's an intentional change that is documented in the design wiki page.
The old behaviour was not by design but a side effect of the way our
config parser works. For defining ACLs, it is far too sloppy and makes
large authz files hard to debug. It's also impossible to decide which
rule overrides, which was fine before we had glob patterns but is quite
important now (see below).
> In 1.9 any repeat acl lines that were the exact same match, such as:
>
> [/some/path]
> user = rw
> user = r
>
> resulted in the last line overriding all the other lines, so user=r in
> the example above. In 1.10 the lines combine, so user=rw in the example
> above. Is this a bug in 1.10, or a bug in 1.9 that is fixed in 1.10, or
> an acceptable behaviour change?
This is also documented in the design page (Inheritance and
Disambiguation, item 8).
I can't think of a good reason to change the behaviour though. This
change prevents giving access to a group and then restricting it for one
member of that group. Perhaps we should revert to the 1.9 behaviour ...
I wish Stefan2 would comment.
> Finally, issue 4762. In 1.9 if both global and per-repository sections
> matched they were combined, so:
>
> [/some/path]
> user = rw
> [repos:/some/path]
> user = r
>
> resulted in user=rw. The issue 4762 problem is that in 1.10 the
> per-repository section overrides any global section, so user=r above. I
> believe this is a 1.10 bug and that the 1.9 behaviour should be
> reinstated.
See Inheritance and Disambiguation, items 6 and 7: "If
repository-specific path rules as well as global path rules match a
given path, only the repository-specific ones will be considered." and
"If multiple path rules match a given repository path, only the one
specified last in the authz file shall apply."
So this is as designed. If this is a design bug, I wish someone had
pointed it out a few years ago ...
We have to be careful when changing the behaviour that we don't end up
reading the whole authz representation for each request, since this
would drastically reduce the performance improvements in 1.10. Of course
correctness comes before performance.
> However, consider glob rules:
>
> [:glob:/some/*]
> user = rw
> [:glob:repos:/some/*]
> user = r
>
> At present the per-repository section override the global section just
> like the buggy behaviour for non-glob sections. If we fix 4762 to
> reinstate the combining for non-glob sections should we change the
> behaviour of glob sections so they combine as well? What about a
> non-glob and glob section:
>
> [/some/path]
> user = rw
> [:glob:repos:/some/path]
> user = r
>
> Should these combine?
At the point of the decision, there are no wildcard patterns any more:
we already have a matched path and ACL. What's different in the 1.10
implementation is that, as per Item 7, the resolver will only use the
last-defined and most specific path or pattern to find the access rule —
so a glob pattern that happens to match the whole path will override
anything else, being the best match.
Note that we also don't have a rule that a non-glob rule that matches
the same path as a glob rule should override: we have Item 7 instead,
and the right way to handle this in authz files is to define glob rules
first, non-glob rules last.
> Glob sections are new so they could have different behaviour from
> non-glob sections, but is that what we want? There is a wiki page
> https://cwiki.apache.org/confluence/display/SVN/AuthzImprovements but
> given issue 4762 I not sure whether it describes the correct behaviour.
It describes designed behaviour. If we change it, we should do it
carefully, as I wrote above. Also I think it turns out that the authz
section in the release notes misses a behaviour change or two. It should
probably include the whole Inheritance and Disambiguation list, however
we end up changing it.
-- Brane
Received on 2018-07-21 13:38:48 CEST