On 27.01.2019 19:58, Daniel Shahaf wrote: > [ Sorry for the late reply; I've been offline. ] > > Branko Čibej wrote on Tue, Jan 22, 2019 at 15:54:40 +0100: >> On 19.01.2019 22:24, Branko Čibej wrote: >>> Thinking about this some more: If we add a warning callback to the >>> parser (I have that implemented locally), we may as well relax the >>> requirement for a group being /defined/, not just non-empty. For >>> example, this authz file will also fail to parse with the same error as >>> shown above: >>> >>> [/] >>> @nosuchgroup = rw > I think that in the case the RHS is empty (= "no" in `svnauthz accessof` > terms), converting an error to a silent no-op could be a regression, > whereby an @nosuchgroup=no directive would result in some authenticated > principals having read access that they shouldn't have, rather than in > a hard error. That is: it would be better to hard fail and block all > access, than to silently give more access than the admin intended. > > From PEP 20: > > Errors should never pass silently. > > In the face of ambiguity, refuse the temptation to guess. > > Next, «@nosuchgroup=(empty string)» and «@nosuchgroup=rw» should be > either both valid or both errors, so I think the latter shouldn't be > changed either. > > Anyone who wants the proposed semantics can achieve them today by > defining nosuchgroup to an empty group. > >>> and this one will also complain about an undefined group, but with a >>> different message: >>> >>> [groups] >>> somegroup = @nosuchgroup >>> >>> >>> In both cases, instead of exiting with an error, just ignoring the ACE >>> or group membership wouldn't affect the semantics of the authz file. If >>> we can print warnings about such issues in 'svnauthz', users would still >>> have a way to find such bugs in their authz files. >>> >>> Relaxing the restrictions on undefined groups (and maybe aliases, too?) >>> would be a change for the next minor release. >>> >>> Thoughts? >> >> If no-one has an opinion, I'm going to do this (closing #4803) and also >> fix #4794 in the same way. > Brane and I discussed this on IRC > <https://colabti.org/irclogger/irclogger_log/svn-dev?date=2019-01-22#l36>; > the gist is: > > - #4794: > > + is a regression whereby the semantics of authz file syntax were > changed > > + we shouldn't change the semantics in a patch release > > + in the next minor release, we could either restore the original > semantics, or make the syntax in question a fatal error > > - no objections to warning about using defined, but empty, groups > («@definedtoempty» on the LHS) > > I think that covers it…
It does. The current state is that empty groups are allowed but ignored, and the use of undefined groups is an error. For #4794, I propose to solve it by reverting back to the 1.9.x behaviour (probably in 1.12) and issuing a warning, since we now have the infrastructure to do that. The overriding reason for this is being that we already have a last-match-wins rule for ACLs, so we may as well have the same rule for ACEs, so that the logic of reading the authz file is consistent. -- Brane
