authz changes between 1.9 and 1.10

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?

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?

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. 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:

Re: authz changes between 1.9 and 1.10

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.

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.

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.

Re: authz changes between 1.9 and 1.10

> On 20.07.2018 14:59, Philip Martin wrote:
>> In 1.9 it was possible to repeat, or reopen, a section:
>
> It's an intentional change that is documented in the design wiki page.

And only there, it didn't make the release notes.

>> In 1.9 any repeat acl lines that were the exact same match, such as:

> This is also documented in the design page (Inheritance and
> Disambiguation, item 8).

But not explicitly as a change from 1.9, and again not in the release
notes.

>> Finally, issue 4762. In 1.9 if both global and per-repository sections
>> matched they were combined, so:
>
> 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 ...

Again, it's not explicitly documented as a change from 1.9 and it's not
in the release notes.

> 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.

The most important thing is to document the change in behaviour of the
non-glob rules between 1.9 and 1.10.

The problem I have is that I still don't know if the changes are
intentional. Of these undocumented (in the release notes) changes there
is one that appears to be intentional and two that could be accidental.
At least the first, intentional, change produces a run-time error if it
occurs, the other two just lead to different access being granted, one
less access the other more access. Anyone using a non-trivial authz
file in 1.9 has to be very careful upgrading to 1.10.

Is it worth me working on a fix? Can we declare 1.10.0 and 1.10.1 buggy
and change the behaviour in future 1.10.x? Or are we stuck with 1.10
being different from 1.9?

Re: authz changes between 1.9 and 1.10

Philip Martin wrote on Tue, 24 Jul 2018 23:37 +0100:

> Branko Čibej <[hidden email]> writes:
> > 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.
>
> The most important thing is to document the change in behaviour of the
> non-glob rules between 1.9 and 1.10.
>

+1; we should document any incompatible changes (regardless of whether
they were intentional or not).

> The problem I have is that I still don't know if the changes are
> intentional. Of these undocumented (in the release notes) changes there
> is one that appears to be intentional and two that could be accidental.
> At least the first, intentional, change produces a run-time error if it
> occurs, the other two just lead to different access being granted, one
> less access the other more access. Anyone using a non-trivial authz
> file in 1.9 has to be very careful upgrading to 1.10.
>

Sounds like we should encourage people to write unit tests for their
authz files. This would be fairly easy to implement using 'svnauthz
accessof'. We could ship something in tools/ that takes two
inputs, an authz file and a set of expectations, and validates the authz
file against the expectations.

> Is it worth me working on a fix? Can we declare 1.10.0 and 1.10.1 buggy
> and change the behaviour in future 1.10.x? Or are we stuck with 1.10
> being different from 1.9?

It's an intentional change that is documented in the design wiki page.

And only there, it didn't make the release notes.

Yes ...

In 1.9 any repeat acl lines that were the exact same match, such as:

This is also documented in the design page (Inheritance and
Disambiguation, item 8).

But not explicitly as a change from 1.9, and again not in the release
notes.

... and yes ...

Finally, issue 4762. In 1.9 if both global and per-repository sections
matched they were combined, so:

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 ...

Again, it's not explicitly documented as a change from 1.9 and it's not
in the release notes.

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.

The most important thing is to document the change in behaviour of the
non-glob rules between 1.9 and 1.10.
The problem I have is that I still don't know if the changes are
intentional. Of these undocumented (in the release notes) changes there
is one that appears to be intentional and two that could be accidental.
At least the first, intentional, change produces a run-time error if it
occurs, the other two just lead to different access being granted, one
less access the other more access. Anyone using a non-trivial authz
file in 1.9 has to be very careful upgrading to 1.10.
Is it worth me working on a fix? Can we declare 1.10.0 and 1.10.1 buggy
and change the behaviour in future 1.10.x? Or are we stuck with 1.10
being different from 1.9?

It's worth working on a fix, IMO. My suggestion would be:

Keep the single-rule behaviour as it turned up in 1.10, just
document it. It's necessary for glob rules and making an
exception for "old-style" rules will limit the ways we can
improve the authz system in future. Also it'd make the next
point quite a bit harder.

Change the ACE override/merge behaviour back to the 1.9 way,
as there's no reason I can think of that it could be either.

#4762 takes a bit of thought. It's relatively easy to revert
to 1.9 behaviour for non-glob rules, because it can be done at
parsing time. For glob rules, it'd have to be done at resolve
time, as otherwise there's not consistent meaning of exact path
match.

Document everything in release notes ... or maybe provide a
link to the (or a new, more user-friendly) Wiki page.

Re: authz changes between 1.9 and 1.10

> Philip Martin wrote on Tue, 24 Jul 2018 23:37 +0100:
>> Branko Čibej <[hidden email]> writes:
>>> 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.
>> The most important thing is to document the change in behaviour of the
>> non-glob rules between 1.9 and 1.10.
>>
> +1; we should document any incompatible changes (regardless of whether
> they were intentional or not).
>
>> The problem I have is that I still don't know if the changes are
>> intentional. Of these undocumented (in the release notes) changes there
>> is one that appears to be intentional and two that could be accidental.
>> At least the first, intentional, change produces a run-time error if it
>> occurs, the other two just lead to different access being granted, one
>> less access the other more access. Anyone using a non-trivial authz
>> file in 1.9 has to be very careful upgrading to 1.10.
>>
> Sounds like we should encourage people to write unit tests for their
> authz files. This would be fairly easy to implement using 'svnauthz
> accessof'. We could ship something in tools/ that takes two
> inputs, an authz file and a set of expectations, and validates the authz
> file against the expectations.

I think serious admins already do this :) But sure, if someone has the
time and inclination to write such tools, they'll surely be useful.

Re: authz changes between 1.9 and 1.10

> It's worth working on a fix, IMO. My suggestion would be:
>
> * Keep the single-rule behaviour as it turned up in 1.10, just
> document it. It's necessary for glob rules and making an exception
> for "old-style" rules will limit the ways we can improve the authz
> system in future. Also it'd make the next point quite a bit harder.

Agreed. It is a regression from 1.9 but it's a hard error so it will
not change behaviour silently.

> * Change the ACE override/merge behaviour back to the 1.9 way, as
> there's no reason I can think of that it could be either.

I have a patch to do this. At present I am distinguishing between glob
and non-glob rules and only apply the 1.9 way to non-glob rules. This
means that a 1.9 authz file retains the 1.9 behaviour, and that the 1.10
glob rules retain the the 1.10 behaviour for anyone who has started
using them. But is also means glob and non-glob rules behave
differently.

Is it better to preserve as much 1.10 behaviour as possible because
people may have started using it, or is is better to have consistency
between glob and non-glob rules?

> * #4762 takes a bit of thought. It's relatively easy to revert to 1.9
> behaviour for non-glob rules, because it can be done at parsing
> time. For glob rules, it'd have to be done at resolve time, as
> otherwise there's not consistent meaning of exact path match.

I have a patch to do this as well, still testing. Again I am
distinguishing between glob and non-glob rules, so the inheritance only
applies to non-glob rules. And once again I wonder if it would be
better for 1.10 glob rules to change?

Re: authz changes between 1.9 and 1.10

On 30.07.2018 02:38, Philip Martin wrote:

> Branko Čibej <[hidden email]> writes:
>
>> It's worth working on a fix, IMO. My suggestion would be:
>>
>> * Keep the single-rule behaviour as it turned up in 1.10, just
>> document it. It's necessary for glob rules and making an exception
>> for "old-style" rules will limit the ways we can improve the authz
>> system in future. Also it'd make the next point quite a bit harder.
> Agreed. It is a regression from 1.9 but it's a hard error so it will
> not change behaviour silently.
>
>> * Change the ACE override/merge behaviour back to the 1.9 way, as
>> there's no reason I can think of that it could be either.
> I have a patch to do this. At present I am distinguishing between glob
> and non-glob rules and only apply the 1.9 way to non-glob rules. This
> means that a 1.9 authz file retains the 1.9 behaviour, and that the 1.10
> glob rules retain the the 1.10 behaviour for anyone who has started
> using them. But is also means glob and non-glob rules behave
> differently.
>
> Is it better to preserve as much 1.10 behaviour as possible because
> people may have started using it, or is is better to have consistency
> between glob and non-glob rules?

It's definitely better to have consistent behaviour across all rule
types. Otherwise there'll be no end of user questions about it ... and
we'll keep second-guessing ourselves, too. Also imagine this:

[:glob:/path]
foo = rw
foo = r

What are the access rights for 'foo'? It's a glob rule but it doesn't
have any wildcards in the path, so it's functionally equivalent to
[/path] ... but with different behaviour about calculating the access
rights? I shudder to think that users would figure this out and start
using the :glob: prefix to select a different consolidation algorithm.

>> * #4762 takes a bit of thought. It's relatively easy to revert to 1.9
>> behaviour for non-glob rules, because it can be done at parsing
>> time. For glob rules, it'd have to be done at resolve time, as
>> otherwise there's not consistent meaning of exact path match.
> I have a patch to do this as well, still testing. Again I am
> distinguishing between glob and non-glob rules, so the inheritance only
> applies to non-glob rules. And once again I wonder if it would be
> better for 1.10 glob rules to change?

Re: authz changes between 1.9 and 1.10

Branko Čibej wrote on Mon, 30 Jul 2018 03:07 +0200:

> On 30.07.2018 02:38, Philip Martin wrote:
> > Branko Čibej <[hidden email]> writes:
> >
> >> It's worth working on a fix, IMO. My suggestion would be:
> >>
> >> * Keep the single-rule behaviour as it turned up in 1.10, just
> >> document it. It's necessary for glob rules and making an exception
> >> for "old-style" rules will limit the ways we can improve the authz
> >> system in future. Also it'd make the next point quite a bit harder.
> > Agreed. It is a regression from 1.9 but it's a hard error so it will
> > not change behaviour silently.
> >
> >> * Change the ACE override/merge behaviour back to the 1.9 way, as
> >> there's no reason I can think of that it could be either.
> > I have a patch to do this. At present I am distinguishing between glob
> > and non-glob rules and only apply the 1.9 way to non-glob rules. This
> > means that a 1.9 authz file retains the 1.9 behaviour, and that the 1.10
> > glob rules retain the the 1.10 behaviour for anyone who has started
> > using them. But is also means glob and non-glob rules behave
> > differently.
> >
> > Is it better to preserve as much 1.10 behaviour as possible because
> > people may have started using it, or is is better to have consistency
> > between glob and non-glob rules?
>
> It's definitely better to have consistent behaviour across all rule
> types.

- The 1.9 behaviour is reasonable: it follows a simple rule (the same
one our CLI --option=argument parsing uses, and our
~/.subversion/config files parsing too, I believe), so an admin might
have reasonably assumed it to be intentional. It _is_ a feature of Subversion
that all ConfigParser-ish config files are parsed the same way, after all.

- 1.10 makes an incompatible change to the 1.9 semantics. The change
is undocumented in the release notes, so a 1.9 admin can't be
expected to be aware of the change, but _is_ documented in the design
wiki, so a 1.10 admin may have seen it in the design wiki and started
to rely on it.

We effectively made two contradictory compatibility promises; we can't
honour both of them. I think our only option is to make it a syntax error.

"In the face of ambiguity, refuse the temptation to guess."

Cheers,

Daniel

P.S. I _would_ have had this discussion during the design phase if I had
noticed the issue back then. Sorry to have missed it.

Re: authz changes between 1.9 and 1.10

I like the idea of achieving consistency by making the duplicate entries
into an error: it changes the behaviour of 1.10 but anyone affected gets
an error. It's also a simpler version of my existing patch.

Consistency is more of a problem for the inheritance case:

[/path]
userA = rw
[repo:/path]
userB = rw

because any change will silently change the behaviour of 1.10. The glob
implementation made file order (sequence number in the implementation)
important and my experimental inheritance patch arbitrarily picks the
per-repository sequence number when inheriting but without any real
justification. The choice has no effect when using the glob
implementation on a 1.9 authz file, but the choice starts to matter when
glob rules are present.

The release notes for 1.10 didn't specify how glob rules are
prioritised, so anyone using them had to read the design docs or
experiment. How inheritance affects glob rules is the outstanding
behaviour question. Do the glob inherit like non-glob rules? How does
the sequence number of inherited rules get defined? One option is to
make :glob: into an error, and introduce :GLOB: with defined rules for
inheritance.

Here's a quick patch to implement the new duplicate error, and to
inherit non-glob rules only:

+ /* Accumulates the access entries when processing a rule. */
+ apr_hash_t *rule_entries;
+
+ /* For duplicating the access entries when processing a rule. */
+ apr_pool_t *rule_entries_pool;
+
/* The parser's scratch pool. This may not be the same pool as
passed to the constructor callbacks, that is supposed to be an
iteration pool maintained by the generic parser.
@@ -229,6 +235,9 @@ create_ctor_baton(apr_pool_t *result_pool,
svn_membuf__create(&cb->rule_path_buffer, 0, parser_pool);
cb->rule_string_buffer = svn_stringbuf_create_empty(parser_pool);

A comment here explaining why this case needs to *remain* an error would be
helpful, otherwise I'm sure in a year or two someone will think "Hey, this case
is just a syntax error now, so it's fair game to make it a non-error and give
it a defined meaning"…

Re: authz changes between 1.9 and 1.10

> Daniel Shahaf <[hidden email]> writes:
>
>> Branko Čibej wrote on Mon, 30 Jul 2018 03:07 +0200:
>>> It's definitely better to have consistent behaviour across all rule
>>> types.
>> +1
> I like the idea of achieving consistency by making the duplicate entries
> into an error: it changes the behaviour of 1.10 but anyone affected gets
> an error. It's also a simpler version of my existing patch.
>
> Consistency is more of a problem for the inheritance case:
>
> [/path]
> userA = rw
> [repo:/path]
> userB = rw
>
> because any change will silently change the behaviour of 1.10. The glob
> implementation made file order (sequence number in the implementation)
> important and my experimental inheritance patch arbitrarily picks the
> per-repository sequence number when inheriting but without any real
> justification.

Shouldn't it be the other way around? At request processing time, only
one rule will match. If it's a per-repository rule, it should be
possible to check if a generic rule with the same path exists. This part
can even be pre-calculated in the parser, so expensive lookup at
processing time can be avoided.

> The choice has no effect when using the glob
> implementation on a 1.9 authz file, but the choice starts to matter when
> glob rules are present.
>
> The release notes for 1.10 didn't specify how glob rules are
> prioritised, so anyone using them had to read the design docs or
> experiment. How inheritance affects glob rules is the outstanding
> behaviour question. Do the glob inherit like non-glob rules? How does
> the sequence number of inherited rules get defined? One option is to
> make :glob: into an error, and introduce :GLOB: with defined rules for
> inheritance.

I think it's fair to say that the current behaviour in 1.10.x is a bug,
then explain in an announcement what we're doing about it.

It was never the intention of the new authz code to *silently* change
behaviour from 1.9.

Re: authz changes between 1.9 and 1.10

Most of these issues have already been addressed by Brane,
but as he wished for my input, here it is.

These are the guiding principles for the 1.10 authz design:

(1) ACLs are only evaluated on a per-user bases; ACLs that
don't mention this user (or any of their groups) are ignored.
Rationale: We don't want to explicitly repeat inherited access
specs that don't change for the respective path / section.

(2) A more specific rule (fully) replaces any more general rule.
Rationale: You want to specify access exactly where it applies
and be sure that those are exactly the rights that will be applied.

(3) If there are multiple, equally specific rules, the last one
replaces any previous one.
Rationale: The last one, so you can specify catch-all denial ACLs.
Replacement, again, to ensure that exactly the rights specified
last will be in effect.

Those principles seemed quite reasonable and, most importantly,
workable with all the potential confusion caused by glob-rules.

Given that the previous authz code lacked similarly explicit
guidelines, I felt that any discrepancy between 1.9 and 1.10
would be a strong indication of a undesirable behavior in 1.9.
I still think this is true but we must neither break 1.9 authz.

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?

Collides with rule (3), which is essential for reasoning on glob-rules.
Since it seemed an accidental feature inherited from the config parser,
we explicitly flagged it as invalid in 1.10. It would otherwise become

[/some/path]
otheruser = rw

> 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?

Ouch. That is a bad one and an oversight in the design - I think.

According to (3), 1.9 behaves correctly. I guess we consider it
an unordered collection in 1.10 and then a union is the only thing
that approximates intent when a user is a member of different
groups with different access rights. Strict ordering becomes
very useful when assigning rights to groups:

[/some/path]
@Users = rw
@BadUsers = r

I guess the fix in 1.10 is simple but will change 1.10 behavior.
My proposal here:

* apply replacement semantics here as in 1.9
* error out / warn on repeated lines with the same user or group
(this is hardly ever intentional)
* provide a script / tool that takes 1.10 authz and checks for
changed behavior ("r" after "rw" rules etc.)

The last one is a bit of work but would be really handy.

> 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.

According to (2), 1.10 behaves correctly: "user" has rw access,
except for a specific repository. I was not aware that 1.9 has
different behavior.

> 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.
>
User documentation on the new authz is inadequate.

Re: authz changes between 1.9 and 1.10

On 08.09.2018 11:17, Stefan Fuhrmann wrote:
> These are the guiding principles for the 1.10 authz design:
>
> (1) ACLs are only evaluated on a per-user bases; ACLs that
> don't mention this user (or any of their groups) are ignored.
> Rationale: We don't want to explicitly repeat inherited access
> specs that don't change for the respective path / section.

This is not entirely true, as seen in the fix for SVN-4793. If a user is
"not mentioned" in an inverted selector, those rights do propagate to
the global level. For example:

[groups]
readers = foo, bar

[/]
~@readers = rw
@readers = r

In this case 'user' has read-write access to '[/]' even though she's not
mentioned anywhere in the authz file or the specific ACL for '[/]'.

>> 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?
> Ouch. That is a bad one and an oversight in the design - I think.
>
> According to (3), 1.9 behaves correctly. I guess we consider it
> an unordered collection in 1.10 and then a union is the only thing
> that approximates intent when a user is a member of different
> groups with different access rights. Strict ordering becomes
> very useful when assigning rights to groups:
>
> [/some/path]
> @Users = rw
> @BadUsers = r

We already have strict ordering within an ACL (authz_acl_t in
libsvn_repos/authz.h):

/* All other user- or group-specific access rights.
Aliases are replaced with their definitions, rules for the same
user or group are merged. */
apr_array_header_t *user_access;

The "merge" semantics was intentional; if we decide it's a bug (and I
think it is), it's fairly easy to change. I would lean in the direction
of making repeating the same access entry selection a hard error at
parsing time. This requires changes to the merge semantics implemented
in add_access_entry() and merge_alias_ace() in
libsvn_repos/authz_parse.c. The nice part is that it would catch errors
like this:

[aliases]
afoo = foo
abar = bar

[/]
&afoo = rw
foo = r
~&abar = rw
~bar = r

With the current implementation we translate the ACL to:

[/]
foo = rw
foo = r
~bar = rw
~bar = r

and even with strict ordering I'd say this is a bug and not intentional.

> I guess the fix in 1.10 is simple but will change 1.10 behavior.
> My proposal here:
>
> * apply replacement semantics here as in 1.9
> * error out / warn on repeated lines with the same user or group
> (this is hardly ever intentional)

^^^ this

> * provide a script / tool that takes 1.10 authz and checks for
> changed behavior ("r" after "rw" rules etc.)
>
> The last one is a bit of work but would be really handy.

The remaining ambiguity that I would prefer _not_ to resolve at parsing
time is this:

[groups]
@readers = foo, bar, user
@writers = baz, quz, user

[/]
@writers = rw
@readers = r

With strict ordering and replacement semantics 'user' would get
read-only rights. But that doesn't seem right; surely if someone is a
member of both 'readers' and 'writers' groups, they should get merged
access rights of both?

Re: authz changes between 1.9 and 1.10

On 02.12.2018 08:25, Branko Čibej wrote:

> On 08.09.2018 11:17, Stefan Fuhrmann wrote:
>> These are the guiding principles for the 1.10 authz design:
>>
>> (1) ACLs are only evaluated on a per-user bases; ACLs that
>> don't mention this user (or any of their groups) are ignored.
>> Rationale: We don't want to explicitly repeat inherited access
>> specs that don't change for the respective path / section.
>
> This is not entirely true, as seen in the fix for SVN-4793. If a user is
> "not mentioned" in an inverted selector, those rights do propagate to
> the global level. For example:
>
> [groups]
> readers = foo, bar
>
> [/]
> ~@readers = rw
> @readers = r
>
>
> In this case 'user' has read-write access to '[/]' even though she's not
> mentioned anywhere in the authz file or the specific ACL for '[/]'.
>
>
>>> 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?
>> Ouch. That is a bad one and an oversight in the design - I think.
>>
>> According to (3), 1.9 behaves correctly. I guess we consider it
>> an unordered collection in 1.10 and then a union is the only thing
>> that approximates intent when a user is a member of different
>> groups with different access rights. Strict ordering becomes
>> very useful when assigning rights to groups:
>>
>> [/some/path]
>> @Users = rw
>> @BadUsers = r
> We already have strict ordering within an ACL (authz_acl_t in
> libsvn_repos/authz.h):
>
> /* All other user- or group-specific access rights.
> Aliases are replaced with their definitions, rules for the same
> user or group are merged. */
> apr_array_header_t *user_access;
>
>
>
> The "merge" semantics was intentional; if we decide it's a bug (and I
> think it is), it's fairly easy to change. I would lean in the direction
> of making repeating the same access entry selection a hard error at
> parsing time. This requires changes to the merge semantics implemented
> in add_access_entry() and merge_alias_ace() in
> libsvn_repos/authz_parse.c. The nice part is that it would catch errors
> like this:
>
> [aliases]
> afoo = foo
> abar = bar
>
> [/]
> &afoo = rw
> foo = r
> ~&abar = rw
> ~bar = r
>
>
> With the current implementation we translate the ACL to:
>
> [/]
> foo = rw
> foo = r
> ~bar = rw
> ~bar = r
>
>
> and even with strict ordering I'd say this is a bug and not intentional.

Re: authz changes between 1.9 and 1.10

On 02.12.2018 08:43, Branko Čibej wrote:

> On 02.12.2018 08:25, Branko Čibej wrote:
>> On 08.09.2018 11:17, Stefan Fuhrmann wrote:
>>> These are the guiding principles for the 1.10 authz design:
>>>
>>> (1) ACLs are only evaluated on a per-user bases; ACLs that
>>> don't mention this user (or any of their groups) are ignored.
>>> Rationale: We don't want to explicitly repeat inherited access
>>> specs that don't change for the respective path / section.
>> This is not entirely true, as seen in the fix for SVN-4793. If a user is
>> "not mentioned" in an inverted selector, those rights do propagate to
>> the global level. For example:
>>
>> [groups]
>> readers = foo, bar
>>
>> [/]
>> ~@readers = rw
>> @readers = r
>>
>>
>> In this case 'user' has read-write access to '[/]' even though she's not
>> mentioned anywhere in the authz file or the specific ACL for '[/]'.
>>
>>
>>>> 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?
>>> Ouch. That is a bad one and an oversight in the design - I think.
>>>
>>> According to (3), 1.9 behaves correctly. I guess we consider it
>>> an unordered collection in 1.10 and then a union is the only thing
>>> that approximates intent when a user is a member of different
>>> groups with different access rights. Strict ordering becomes
>>> very useful when assigning rights to groups:
>>>
>>> [/some/path]
>>> @Users = rw
>>> @BadUsers = r
>> We already have strict ordering within an ACL (authz_acl_t in
>> libsvn_repos/authz.h):
>>
>> /* All other user- or group-specific access rights.
>> Aliases are replaced with their definitions, rules for the same
>> user or group are merged. */
>> apr_array_header_t *user_access;
>>
>>
>>
>> The "merge" semantics was intentional; if we decide it's a bug (and I
>> think it is), it's fairly easy to change. I would lean in the direction
>> of making repeating the same access entry selection a hard error at
>> parsing time. This requires changes to the merge semantics implemented
>> in add_access_entry() and merge_alias_ace() in
>> libsvn_repos/authz_parse.c. The nice part is that it would catch errors
>> like this:
>>
>> [aliases]
>> afoo = foo
>> abar = bar
>>
>> [/]
>> &afoo = rw
>> foo = r
>> ~&abar = rw
>> ~bar = r
>>
>>
>> With the current implementation we translate the ACL to:
>>
>> [/]
>> foo = rw
>> foo = r
>> ~bar = rw
>> ~bar = r
>>
>>
>> and even with strict ordering I'd say this is a bug and not intentional.
>
> Note that this should also be an error:
>
> [/]
> $anonymous = r
> ~$authenticated = rw

I have a patch ready, here are some examples of what it does (currently,
all these examples are valid and produce merged access rights):

Re: authz changes between 1.9 and 1.10

Branko Čibej wrote:
> I have a patch ready, here are some examples of what it does [...]

Not following the details, all I would ask ("all", he says) is that a complete description of the new semantics be available for review, and should be published along with the changes. Otherwise I fear the community has no real way of reviewing the actual semantics against the intended semantics.

One basis we have for such documentation could be the document that Doug Robinson contributed in dev@ thread "Subversion AuthZ Wildcards" on 2017-02-27 [1]. AFAIK it was the best or only documentation we had of the new semantics.

Oooh, Yeah. I started by making the page tree in confluence make some
kind of sense, and found we have two roadmap pages, both by the same
author. :) There are a few pages I haven't been able to classify, but
certainly a new chapter is necessary to gather actual implemented
feature documentation (which may, or may not, be different from the
related design notes).

Re: authz changes between 1.9 and 1.10

> On 02.12.2018 08:43, Branko Čibej wrote:
>> On 02.12.2018 08:25, Branko Čibej wrote:
>>> On 08.09.2018 11:17, Stefan Fuhrmann wrote:
>>>> These are the guiding principles for the 1.10 authz design:
>>>>
>>>> (1) ACLs are only evaluated on a per-user bases; ACLs that
>>>> don't mention this user (or any of their groups) are ignored.
>>>> Rationale: We don't want to explicitly repeat inherited access
>>>> specs that don't change for the respective path / section.
>>> This is not entirely true, as seen in the fix for SVN-4793. If a user is
>>> "not mentioned" in an inverted selector, those rights do propagate to
>>> the global level. For example:
>>>
>>> [groups]
>>> readers = foo, bar
>>>
>>> [/]
>>> ~@readers = rw
>>> @readers = r
>>>
>>>
>>> In this case 'user' has read-write access to '[/]' even though she's not
>>> mentioned anywhere in the authz file or the specific ACL for '[/]'.
>>>
>>>
>>>>> 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?
>>>> Ouch. That is a bad one and an oversight in the design - I think.
>>>>
>>>> According to (3), 1.9 behaves correctly. I guess we consider it
>>>> an unordered collection in 1.10 and then a union is the only thing
>>>> that approximates intent when a user is a member of different
>>>> groups with different access rights. Strict ordering becomes
>>>> very useful when assigning rights to groups:
>>>>
>>>> [/some/path]
>>>> @Users = rw
>>>> @BadUsers = r
>>> We already have strict ordering within an ACL (authz_acl_t in
>>> libsvn_repos/authz.h):
>>>
>>> /* All other user- or group-specific access rights.
>>> Aliases are replaced with their definitions, rules for the same
>>> user or group are merged. */
>>> apr_array_header_t *user_access;
>>>
>>>
>>>
>>> The "merge" semantics was intentional; if we decide it's a bug (and I
>>> think it is), it's fairly easy to change. I would lean in the direction
>>> of making repeating the same access entry selection a hard error at
>>> parsing time. This requires changes to the merge semantics implemented
>>> in add_access_entry() and merge_alias_ace() in
>>> libsvn_repos/authz_parse.c. The nice part is that it would catch errors
>>> like this:
>>>
>>> [aliases]
>>> afoo = foo
>>> abar = bar
>>>
>>> [/]
>>> &afoo = rw
>>> foo = r
>>> ~&abar = rw
>>> ~bar = r
>>>
>>>
>>> With the current implementation we translate the ACL to:
>>>
>>> [/]
>>> foo = rw
>>> foo = r
>>> ~bar = rw
>>> ~bar = r
>>>
>>>
>>> and even with strict ordering I'd say this is a bug and not intentional.
>> Note that this should also be an error:
>>
>> [/]
>> $anonymous = r
>> ~$authenticated = rw
> I have a patch ready, here are some examples of what it does (currently,
> all these examples are valid and produce merged access rights):

Re: authz changes between 1.9 and 1.10

> On 02.12.2018 15:15, Julian Foad wrote:
>> Branko Čibej wrote:
>>> I have a patch ready, here are some examples of what it does [...]
>> Not following the details, all I would ask ("all", he says) is that a complete description of the new semantics be available for review, and should be published along with the changes. Otherwise I fear the community has no real way of reviewing the actual semantics against the intended semantics.
>>
>> One basis we have for such documentation could be the document that Doug Robinson contributed in dev@ thread "Subversion AuthZ Wildcards" on 2017-02-27 [1]. AFAIK it was the best or only documentation we had of the new semantics.
>>
>> Might you or anyone else be prepared to do that?
>>
>> [1] https://svn.haxx.se/dev/archive-2017-02/0188.shtml or https://lists.apache.org/thread.html/be50f6e5b1a92a244033bd7f13449c8d02e92bbe7e29dc89209f62f8@%3Cdev.subversion.apache.org%3E>>
>
> Oooh, Yeah. I started by making the page tree in confluence make some
> kind of sense, and found we have two roadmap pages, both by the same
> author. :) There are a few pages I haven't been able to classify, but
> certainly a new chapter is necessary to gather actual implemented
> feature documentation (which may, or may not, be different from the
> related design notes).