[ 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…
Cheers,
Daniel
Received on 2019-01-27 19:58:42 CET