[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: Mon, 28 Jul 2008 19:20:46 +0530

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Senthil Kumaran S wrote:
> Though this fixes the problem, it did not pass the tests. The recently
> added "basic auth tests" in basic_tests.py breaks due to this change.
>
> Basically in my previous email this is "use case 2". I am working on
> another patch for the same, which have more changes (modifies the flow
> of this function) inside "svn_auth__simple_first_creds_helper" which I
> will post to the dev list once completed.
>
> But if anyone feel that the change which I am working on is not needed,
> please let me know, in which case we need to invalidate "basic auth
> tests" in basic_tests.py, But I would love to work on this, though it
> will change the logic of private function
> "svn_auth__simple_first_creds_helper" entirely.

I am attaching an updated patch along with this email which passes all the
tests and also satisfies use cases 1), 3)a & 3)b as explained here
http://svn.haxx.se/dev/archive-2008-07/0533.shtml

Use case 2 is tested in basic_tests.py test number 46 ie., "basic auth test",
which is the reverse of what I ve explained in the above thread.

[[[
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 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. 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.

Patch by: stylesen
Found by: arfrever
]]]

Thank You.
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFIjc609o1G+2zNQDgRAsktAJ40dOt/HuEjNTfgn8uEj3s1W3MvGACfVtd5
GBb7G4yzxe9dtpaqJPF3+60=
=pMeE
-----END PGP SIGNATURE-----

Index: subversion/libsvn_subr/simple_providers.c
===================================================================
--- subversion/libsvn_subr/simple_providers.c (revision 32316)
+++ subversion/libsvn_subr/simple_providers.c (working copy)
@@ -94,6 +94,23 @@
   return TRUE;
 }
 
+/* Retrieves the username from CREDS. */
+static svn_boolean_t
+simple_username_get(const char **username,
+ apr_hash_t *creds,
+ const char *realmstring,
+ svn_boolean_t non_interactive)
+{
+ svn_string_t *str;
+ str = apr_hash_get(creds, AUTHN_USERNAME_KEY, APR_HASH_KEY_STRING);
+ if (str && str->data)
+ {
+ *username = str->data;
+ return TRUE;
+ }
+ return FALSE;
+}
+
 /* Common implementation for simple_first_creds and
    windows_simple_first_creds. Uses PARAMETERS, REALMSTRING and the
    simple auth provider's username and password cache to fill a set of
@@ -122,45 +139,85 @@
   svn_boolean_t non_interactive = apr_hash_get(parameters,
                                                SVN_AUTH_PARAM_NON_INTERACTIVE,
                                                APR_HASH_KEY_STRING) != NULL;
-
- svn_boolean_t may_save = username || password;
+ const char *def_username = NULL;
+ const char *def_password = NULL;
+ svn_boolean_t may_save = FALSE;
+ apr_hash_t *creds_hash = NULL;
   svn_error_t *err;
+ svn_string_t *str;
+ svn_boolean_t have_passtype = FALSE;
 
- /* If we don't have a username and a password yet, we try the auth cache */
- if (! (username && password))
+ /* Try to load credentials from a file on disk, based on the
+ realmstring. Don't throw an error, though: if something went
+ wrong reading the file, no big deal. What really matters is that
+ we failed to get the creds, so allow the auth system to try the
+ next provider. */
+ err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
+ realmstring, config_dir, pool);
+ svn_error_clear(err);
+
+ /* We have something in the auth cache for this realm. */
+ if (! err && creds_hash)
     {
- apr_hash_t *creds_hash = NULL;
+ /* The password type in the auth data must match the
+ mangler's type, otherwise the password must be
+ interpreted by another provider. */
+ str = apr_hash_get(creds_hash, AUTHN_PASSTYPE_KEY, APR_HASH_KEY_STRING);
+ if (str && str->data)
+ if (passtype && (0 == strcmp(str->data, passtype)))
+ have_passtype = TRUE;
 
- /* Try to load credentials from a file on disk, based on the
- realmstring. Don't throw an error, though: if something went
- wrong reading the file, no big deal. What really matters is that
- we failed to get the creds, so allow the auth system to try the
- next provider. */
- err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
- realmstring, config_dir, pool);
- svn_error_clear(err);
- if (! err && creds_hash)
+ /* See if we need to save this username if it is not present in
+ auth cache. */
+ if (username)
         {
- svn_string_t *str;
- if (! username)
+ if (!simple_username_get(&def_username, creds_hash, realmstring,
+ non_interactive))
             {
- str = apr_hash_get(creds_hash, AUTHN_USERNAME_KEY,
- APR_HASH_KEY_STRING);
- if (str && str->data)
- username = str->data;
+ may_save = TRUE;
             }
+ else
+ {
+ if (0 == strcmp(def_username, username))
+ may_save = FALSE;
+ else
+ may_save = TRUE;
+ }
+ }
 
+ /* See if we need to save this password if it is not present in
+ auth cache. */
+ if (password)
+ {
+ if (have_passtype)
+ {
+ if (!password_get(&def_password, creds_hash, realmstring,
+ username, non_interactive, pool))
+ {
+ may_save = TRUE;
+ }
+ else
+ {
+ if (0 == strcmp(def_password, password))
+ may_save = FALSE;
+ else
+ may_save = TRUE;
+ }
+ }
+ }
+
+ /* If we don't have a username and a password yet, we try the
+ auth cache */
+ if (! (username && password))
+ {
+ if (! username)
+ if (!simple_username_get(&username, creds_hash, realmstring,
+ non_interactive))
+ username = NULL;
+
           if (username && ! password)
             {
- svn_boolean_t have_passtype;
- /* The password type in the auth data must match the
- mangler's type, otherwise the password must be
- interpreted by another provider. */
- str = apr_hash_get(creds_hash, AUTHN_PASSTYPE_KEY,
- APR_HASH_KEY_STRING);
- have_passtype = (str && str->data);
- if (have_passtype && passtype
- && 0 != strcmp(str->data, passtype))
+ if (! have_passtype)
                 password = NULL;
               else
                 {
@@ -171,12 +228,18 @@
                   /* 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)
+ if (password && ! have_passtype)
                     may_save = TRUE;
                 }
             }
         }
     }
+ else
+ {
+ /* Nothing was present in the auth cache, so save the credentials if
+ we are asked to do so. */
+ may_save = TRUE;
+ }
 
   /* Ask the OS for the username if we have a password but no
      username. */

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-07-28 15:52:26 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.