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
Received on 2019-01-27 21:59:58 CET