You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Senthil Kumaran S <se...@collab.net> on 2008/09/01 12:05:39 UTC
Re: Subversion sometimes needlessly asks for confirmation to store
already stored plaintext passwords
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Karl,
Karl Fogel wrote:
> + /* Nothing was present in the auth cache, so indicate that these
> + credentials should be saved if saving is allowed by the run-time
> + configuration. */
>
> I'm just saying that it would be a bit better if that kind of
> explanation were collected at the variable's declaration, so the reader
> could understand the full semantics from the start -- instead of having
> to collect the information bit by bit from reading through the code.
> It's not a big deal, though; I think it's pretty clear as-is.
The updated patch has more clarification in the doc string for 'need_to_save'
variable. Please let me know if the patch is good so that I can commit it to trunk.
[[[
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.
Found by: arfrever
Review by: kfogel
]]]
Thank You.
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFIu9qR9o1G+2zNQDgRAoxtAJ9mhrTUocyqufD6x602OJ+CvjZ4ZACfcQQK
JD+BxwiW6Wcmp2PBSDGa1+I=
=qc48
-----END PGP SIGNATURE-----
Re: Subversion sometimes needlessly asks for confirmation to store
already stored plaintext passwords
Posted by Senthil Kumaran S <se...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Karl Fogel wrote:
> I like the log message -- it explains the core meaning of the change
> very clearly.
The credits goes to stsp :) (http://svn.haxx.se/dev/archive-2008-07/0541.shtml).
> This conditional structure is a bit odd, because you check 'err' (the
> pointer) after clearing 'err' (the value).
>
> Usually we set errors to NULL after clearing them, so that we're not
> keeping a pointer that points to garbage. Thus, it would be better to
> do something like this:
>
> err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
> realmstring, config_dir, pool);
> if (err)
> {
> svn_error_clear(err);
> err = NULL;
> }
> else if (creds_hash)
> {
> /* We have something in the auth cache for this realm. */
> [...]
> }
>
> Rest looks good to me!
I ve made the above modification and committed in r32874.
Thank You.
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFIvuPM9o1G+2zNQDgRAuBAAKC+j7+RzgdTj83E+mBXwU0jU/M+MwCgp5pl
ubG8ULTPGqKL+/nwCRJvaic=
=qFTR
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Subversion sometimes needlessly asks for confirmation to store already stored plaintext passwords
Posted by Karl Fogel <kf...@red-bean.com>.
Senthil Kumaran S <se...@collab.net> writes:
> The updated patch has more clarification in the doc string for
> 'need_to_save' variable. Please let me know if the patch is good so
> that I can commit it to trunk.
>
> [[[
> 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.
>
> Found by: arfrever
> Review by: kfogel
> ]]]
I like the log message -- it explains the core meaning of the change
very clearly.
> --- subversion/libsvn_subr/simple_providers.c (revision 32779)
> +++ subversion/libsvn_subr/simple_providers.c (working copy)
> @@ -127,45 +145,88 @@
> svn_boolean_t non_interactive = apr_hash_get(parameters,
> SVN_AUTH_PARAM_NON_INTERACTIVE,
> APR_HASH_KEY_STRING) != NULL;
> + const char *default_username = NULL; /* Default username from cache. */
> + const char *default_password = NULL; /* Default password from cache. */
>
> - svn_boolean_t may_save = username || password;
> + /* This checks if we should save the CREDS, iff saving the credentials is
> + allowed by the run-time configuration. */
> + 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;
>
> - /* 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)
> {
This conditional structure is a bit odd, because you check 'err' (the
pointer) after clearing 'err' (the value).
Usually we set errors to NULL after clearing them, so that we're not
keeping a pointer that points to garbage. Thus, it would be better to
do something like this:
err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
realmstring, config_dir, pool);
if (err)
{
svn_error_clear(err);
err = NULL;
}
else if (creds_hash)
{
/* We have something in the auth cache for this realm. */
[...]
}
Rest looks good to me!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org