Re: [PATCH] don't store plain-text passwords by default

On Sat, Apr 19, 2008 at 08:26:55PM +0300, Daniel Shahaf wrote:
> > + * @a *default_value_was_used is non-NULL, it is set to TRUE if the option
> ^
> "If "
>

Not fixed, because the function will be removed. See David's comment
about making 'store-plaintext-passwords' a 3-way option.

> > +svn_error_t *
> > +svn_config_get_bool(svn_config_t *cfg, svn_boolean_t *valuep,
> > + const char *section, const char *option,
> > + svn_boolean_t default_value)
> > +{
> > + svn_boolean_t dummy;
>
> Local variable not needed after r30710.

Same here.

> > + const char *answer = NULL;
> > + svn_boolean_t answered = FALSE;
> > + const char *prompt_string = _("Store password unencrypted (yes/no)? ");
> >
> > + SVN_ERR(svn_cmdline_printf(pool, "\n"));
>
> You could call svn_cmdline_printf() just once (passing a multiline
> string); don't know if it matters.

Indeed. Fixed, thanks.

> > + svn_boolean_t store_plaintext_passwords; /* do the unthinkable */
>
> New struct member unneeded since r30701.

Nice catch. Fixed, also.

> Also, in three places whitespace is missing:
>
> > + if (! default_value_was_used)
> > + {
> > + if (store_plaintext_password_val)
> > + svn_auth_set_parameter(*ab, SVN_AUTH_PARAM_STORE_PLAINTEXT_PASSWORDS,
> > + "");
> > + else
> > + svn_auth_set_parameter(*ab,
> > + SVN_AUTH_PARAM_DONT_STORE_PLAINTEXT_PASSWORDS,
> > + "");
>
> Here,

That code will be rewritten anyway.

> > --- subversion/libsvn_subr/simple_providers.c (.../trunk) (revision 30656)
> > +++ subversion/libsvn_subr/simple_providers.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30710)
> > @@ -238,13 +246,21 @@ simple_save_creds_helper(svn_boolean_t *saved,
> > + svn_boolean_t non_interactive = apr_hash_get(parameters,
> > SVN_AUTH_PARAM_NON_INTERACTIVE,
> > APR_HASH_KEY_STRING) != NULL;
>
> here,

Woah, that must've been vim, it certainly wasn't me :)

> > - svn_boolean_t password_stored = TRUE;
> > -
> > + simple_provider_baton_t *b = (simple_provider_baton_t*)provider_baton;
> ^
> and here.

Fixed.

> I'll leave you alone now. :)

Thanks so much for your input! :)

-- 
Stefan Sperling <stsp_at_elego.de>                 Software Developer
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                 Geschaeftsfuehrer: Olaf Wagner