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
