Re: Subversion sometimes needlessly asks for confirmation to store already stored plaintext passwords

On Fri, Jul 18, 2008 at 05:20:59PM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Fri, 18 Jul 2008 at 15:20 +0200:
> > [[[
> > Fix unnecessary plaintext password saving prompt when the username
> > is supplied on the command line and the password is already cached.
> >
> > * subversion/libsvn_subr/simple_providers.c
> > (svn_auth__simple_first_creds_helper): Start out with may_save = FALSE.
> > The old code did set creds->may_save to TRUE whenever a username
> > was supplied on the command line, regardless of whether a password
> > was already cached or not. This unconditionally triggered the whole
> > auth caching logic (including the 'may I save your password in
> > plaintext?' prompt) to be run again, even if a password was already
> > cached.
> >
> > Patch by: stylesen
> > Found by: arfrever
> > Approved by: stsp
> > ]]]
> >
> > Please commit if you agree with my log message.
> >
>
> After this patch, svn_auth__simple_first_creds_helper() will never set
> 'may_save' to TRUE except when the auth cache exists and is of a older
> format that contains a password but not a passtype:
>
> /* If the auth data didn't contain a password type,
> force a write to upgrade the format of the auth
> data file. */
> if (password && passtype && !have_passtype)
> may_save = TRUE;
>
> Don't we need to set may_save to 'TRUE' in some cases as well?
>
> Daniel
> (who didn't reply in the morning, because he thinks he's missing
> something obvious, again)

No, we don't need to set may_save to TRUE in any other case.

All that svn_auth__simple_first_creds_helper() (part of the
"simple file" provider) does is look whether there are some credentials
cached already. If there are any, there's not much point in caching
them again, right? So creds->may_save can be as FALSE as it wants to be.

If the creds don't authenticate the next provider will be tried
anyway, because the simple file provider has no 'next_credentials'
method (see the definition of svn_auth_provider_t in svn_auth.h
if you don't know the difference between 'first_credentials' and
'next_credentials').

If svn_auth__simple_first_creds_helper() cannot find any creds in the
auth cache, it says "I have no credentials" (by setting *credentials
to NULL) and, again, the next provider is tried.

Among the next providers is likely to be the 'prompt provider', which
prompts the user interactively and creates its own creds object.

There are two invocations of the prompt provider in simple_providers.c,
both of which use '! no_auth_cache' to set the 'may_save' parameter
while prompting the user for a username and password. Both calls,
if successful, create an entirely new credentials object:

One is in simple_prompt_first_creds:
  SVN_ERR(prompt_for_simple_creds((svn_auth_cred_simple_t **) credentials_p,
                                  pb, parameters, realmstring, TRUE,
                                  ! no_auth_cache, pool));

The other one is in simple_prompt_next_creds:
  SVN_ERR(prompt_for_simple_creds((svn_auth_cred_simple_t **) credentials_p,
                                  pb, parameters, realmstring, FALSE,
                                  ! no_auth_cache, pool));

Here's the signature of prompt_for_simple_creds for reference:

static svn_error_t *
prompt_for_simple_creds(svn_auth_cred_simple_t **cred_p,
                        simple_prompt_provider_baton_t *pb,
                        apr_hash_t *parameters,
                        const char *realmstring,
                        svn_boolean_t first_time,
                        svn_boolean_t may_save,
                        apr_pool_t *pool)

The may_save parameter is copied straight into the returned credentials
objects. So unless the user passes --no-auth-cache, the credentials
returned from these calls have their may_save member set to TRUE,
and they will be cached in the usual way.

Did this relieve your concerns?

Stefan