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? Note that the current behaviour is to merge. -- Brane