[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Senthil Kumaran S <senthil_at_collab.net>
Date: Tue, 26 Aug 2008 18:54:23 +0530

Hash: SHA1

Hi Karl,

Karl Fogel wrote:
> Oops, sorry Senthil. I owed a response to your response to my response
> to your patch :-).

:) My comments inline.

>> If there is a change in credentials then the changed creds will be cached in
>> the auth area afresh. This is what happens now. This was the behavior
>> previously in older versions, so I didn't want to break it, because it breaks
>> tests.
> Hmm. That seems counterintuitive to me, but if it's current behavior,
> let's keep it for now, and raise this as a separate issue (unrelated to
> your patch).


> Well, the current caller uses it temporarily, but simple_username_get()
> is providing an interface, and we don't know who will use it in the
> future. It might be good to state explicitly that:
> "*USERNAME will have the same lifetime as CREDS."

Documented the above in "simple_username_get()".

>> - svn_boolean_t may_save = username || password;
>> + const char *def_username = NULL;
>> + const char *def_password = NULL;
>> + svn_boolean_t need_to_save = FALSE;
>> + apr_hash_t *creds_hash = NULL;
>> svn_error_t *err;
>> + svn_string_t *str;
>> + svn_boolean_t have_passtype = FALSE;
> I think the subsequent code would become much more comprehensible if you
> document some of these variables here, in particular need_to_save. (For
> example, at the end when you assign "creds->may_save = need_to_save;",
> the semantic transfer from one to the other will be confusing unless the
> reader totally understands what need_to_save's semantics are.)

Added comments for the above variables.

> With def_username and def_password, does "def" mean "default"? If so,
> just write it out -- that way their meaning is instantly clear.

Done this in new patch.

Another instance of the above in "prompt_for_simple_creds()" was corrected in

>> + else
>> + {
>> + /* Nothing was present in the auth cache, so save the credentials if
>> + we are asked to do so. */
>> + need_to_save = TRUE;
>> + }
> Where's the check for "if we are asked to do so", though? This new
> 'else' is the counter-condition to this 'if':
> /* We have something in the auth cache for this realm. */
> if (! err && creds_hash)
> {
> [...]
> }
> Or do you mean "... so indicate that these credentials should be saved
> if saving is allowed by the run-time configuration"? (Since the
> run-time config is checked in svn_auth__simple_save_creds_helper(), I
> guess.)

Yes this is checked in svn_auth__simple_save_creds_helper() before saving, so I
changed the comment in the attached patch according to the above (as you have

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
  (simple_username_get): New function to get username for simple provider.
  (svn_auth__simple_first_creds_helper): Start out with need_to_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. Here we check for different combinations
   of username and password either supplied in the command line or
   already cached in the auth area, based on that we toggle creds->may_save
   through need_to_save.

Patch by: stylesen
Found by: arfrever
Review by: kfogel

Thank You.
- --
Senthil Kumaran S
Version: GnuPG v1.4.6 (GNU/Linux)


To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org

Received on 2008-08-26 15:24:58 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.