subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject Re: SVN 1.10 AuthZ file parsing too strict?!
Date Sun, 27 Jan 2019 18:58:28 GMT
[ 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

Mime
View raw message