Re: svn commit: r30723 - in branches/dont-save-plaintext-passwords-by-default: . subversion/include subversion/libsvn_subr subversion/svn

On Mon, Apr 21, 2008 at 10:50:25PM +0200, Stefan Sperling wrote:
> On Mon, Apr 21, 2008 at 10:44:19AM -0700, David Glasser wrote:
> > On Mon, Apr 21, 2008 at 6:31 AM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> > > Stefan Sperling wrote:
> > >
> > > > On Sun, Apr 20, 2008 at 04:42:56PM -0700, David Glasser wrote:
> > > >
> > > > > Hmm, why not just have the "global" servers section be the normal
> > > > > place to configure this? Why use the other config file at all?
> > > > >
> > > >
> > > > Because most users will look at the 'config' file first, I guess.
> > > > Also, there, it's right next to 'store-passwords'.
> > > >
> > > > The docstrings reference each other, so I think it's sorta OK...
> > > >
> > > > The other idea I had was to have the option be valid in both
> > > > the config file and the servers file [global] section, and
> > > > have svn print a warning when the two disagree and fall back
> > > > to 'prompt'. But I discarded that as overkill.
> > > >
> > >
> > > That we ever had the store-passwords option in the 'config' file instead of
> > > in the 'servers' file (bound to RA-related things) might have been a
> > > mistake. Let's evaluate the correct location for this new option without
> > > concern for the dubious decisions of the past.
> >
> > +1
>
> Right, so if we stored 'store-plaintext-passwords' in 'servers' only,
> we should also move 'store-passwords' and 'store-auth-creds' to 'servers'.
> That is, the whole [auth] section of the 'config' file will go to the
> 'servers' file, with a comment in 'config' informing users about the move.
>
> Agreed?

Actually, storing 'store-plaintext-passwords' in 'servers' is *required*
to fix the following bug I just realised exists (and is really silly
once you realise it's there, I do really stupid things sometimes):

In svn/main.c on the branch, we're currently grabbing URLs from the
command line so we can use those to match server groups in the 'servers'
file. The approach is totally naive and wrong. We're not even checking
(and we can't, in main.c) which of the URLs we're actually connecting to
(they might be part of, say, a log message argument), and on top of that,
the URL we're connecting to might not even be retrieved from the command
line! "svn ci ..." causes auth data to be cached for repos that require
auth only for write operations, like our own repo does. Doh!

So this is clearly an issue of "layer violation". 'store-plaintext-passwords'
has to move out of 'config' and svn_cmdline_setup_auth_baton, and go into
'servers', and be evaluated by RA layers. This probably means that quite a
bit of code will need to be refactored/rewritten on the branch :/

Does anyone oppose moving the whole [auth] section from 'config' to
'servers' for consistency?

-- 
Stefan Sperling <stsp_at_elego.de>                    Software Monkey
 
German law requires the following banner :(
elego Software Solutions GmbH                            HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12        Tel:  +49 30 23 45 86 96 
13355 Berlin                              Fax:  +49 30 23 45 86 95
http://www.elego.de                               CEO: Olaf Wagner
 
Store password unencrypted (yes/no)? No