-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi,
I would like to get some review on this patch, so that it will get into trunk
(this does not have an issue in our issue tracker, fyi).
Thank You.
Senthil Kumaran S wrote:
> Hi Karl,
>
> Karl Fogel wrote:
>> What happens if all of the following are true:
>
>> - the user passes '--username=my_user_name'
>> - there is a cached password for my_user_name and this repository
>> - the cached password is incorrect (according to the repository)
>> - the user's ~/.subversion configuration says may_save = FALSE
>
>> ? The correct behavior then would be to prompt the user for a new
>> username/password (after discovering that the old password doesn't work,
>> the client prompts for both username and password, I assume), use the
>> new creds for this operation, but *not* save them. I think. Does that
>> make sense to you?
>
> 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.
>
>>> +/* 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)
>> Doc string doesn't mention the other parameters, but also doesn't say
>> this is an implementation of some pre-existing function type. Should do
>> one or the other...
>
> Done in the new patch.
>
>>> +{
>>> + 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;
>>> +}
>> Is there any need to mention lifetime issues with the returned data, or
>> are there conventions already in place about the 'creds' hash and data
>> retrieved from it that would be obvious to anyone reading this code in
>> context?
>
> Since the values from creds_hash is used temporarily there is no lifetime
> issues to mention here, IMHO. We use these values in order to set
> "svn_auth_cred_simple_t *creds" which must be persistent and it _is_ throughout
> the code.
>
>> I don't understand the logic of these different settings of 'may_save'.
>
>> Is the 'may_save' variable supposed to represent what the configuration
>> says about the question of whether or not we can save creds? Or does it
>> represent the question of whether or not we got different data from what
>> was in the cache -- in other words, it's really "need_to_save"?
>
> - From svn_auth_cred_simple_t doc string:
>
> <snip>
> /** Indicates if the credentials may be saved (to disk). For example, a
> * GUI prompt implementation with a remember password checkbox shall set
> * @a may_save to TRUE if the checkbox is checked.
> */
> svn_boolean_t may_save;
> </snip>
>
>> If the variable only represents one thing, then maybe it's just that its
>> name needs to be changed. But if it represents several different
>> questions, it might be good to have several different variables, so the
>> readers can understand what questions are being considered...
>
> This variable just addresses, whether we need to save the credentials supplied.
> So I am changing the variable name in the updated patch to need_to_save.
>
>>> + /* See if we need to save this password if it is not present in
>>> + auth cache. */
>> This comment indicates it's about "need_to_save" not "may_save"...
>
> In all the places this variable is used to predict whether we need to save the
> creds, so changing its name.
>
>> The rest of the change I found hard to understand, but I think my
>> confusion all stems from one basic source: the difference between "may
>> save" and "need to save (if saving is allowed)". One is about what the
>> user has given Subvesrion permission to do; the other is about what
>> Subversion *should* do, assuming it has the necessary permissions from
>> the user at all.
>
> I will try to summarize the change as follows:
>
> 1) Get the username and password supplied by the user in command line.
> 2) We check whether there is any creds cached in the auth area for this REALM.
> 3) If 2) is TRUE, then get the def_username from the auth cache, else we need
> to save whatever username we get from the following steps to the auth cache, if
> the user indicated us to save auth creds ie., by "store-auth-creds" in the
> servers file (but the storing part check is done in save_credentials function).
> What we really are bothered here is to toggle the creds->may_save, in order to
> make a decision whether we need to save these credentials or not.
> 4) If 3) gives a def_username, then compare this username with whatever
> username we have (if any) from 1). If they are same then we need not change the
> username creds in the auth cache, else we need to save the username creds in
> the auth cache.
> 5) If 2) is TRUE, then get the def_password from the auth cache, else we need
> to save whatever password we get from the following steps to the auth cache.
> 6) If 5) gives a def_password, then compare this password with whatever
> password we have (if any) from 1). If they are same then we need not change the
> password creds in the auth cache, else we need to save the password creds in
> the auth cache.
>
> Steps 1) to 6) above are checks made in order to see whether the creds supplied
> in command line matches with the stored creds in the auth cache, if not we will
> make the change in the auth cache.
>
> 7) Get the username and password from the auth cache, if it is not supplied in
> the command line. Here there is one check to need_to_save, if the passtype is
> not cached for these credentials.
> 8) If 2) fails then, there was nothing in the auth cache for this REALM, so we
> set need_to_save as TRUE, which will be handled by save_credentials related
> functions based on "store-auth-creds" value in servers file.
>
> The remaining part of the code ie., the following is obvious:
> <snip>
> /* Ask the OS for the username if we have a password but no
> username. */
> if (password && ! username)
> username = svn_user_get_name(pool);
>
> if (username && password)
> {
> svn_auth_cred_simple_t *creds = apr_pcalloc(pool, sizeof(*creds));
> creds->username = username;
> creds->password = password;
> creds->may_save = need_to_save;
> *credentials = creds;
> }
> else
> *credentials = NULL;
> </snip>
>
> [[[
> 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
> ]]]
>
> Thank You.
- ------------------------------------------------------------------------
Index: subversion/libsvn_subr/simple_providers.c
===================================================================
- --- subversion/libsvn_subr/simple_providers.c (revision 32393)
+++ subversion/libsvn_subr/simple_providers.c (working copy)
@@ -94,6 +94,24 @@
return TRUE;
}
+/* Set **USERNAME to the username retrieved from CREDS; ignore
+ other parameters. */
+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 +140,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 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)
{
- - 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;
+ need_to_save = TRUE;
}
+ else
+ {
+ if (0 == strcmp(def_username, username))
+ need_to_save = FALSE;
+ else
+ need_to_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))
+ {
+ need_to_save = TRUE;
+ }
+ else
+ {
+ if (0 == strcmp(def_password, password))
+ need_to_save = FALSE;
+ else
+ need_to_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 +229,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)
- - may_save = TRUE;
+ if (password && ! have_passtype)
+ need_to_save = TRUE;
}
}
}
}
+ else
+ {
+ /* Nothing was present in the auth cache, so save the credentials if
+ we are asked to do so. */
+ need_to_save = TRUE;
+ }
/* Ask the OS for the username if we have a password but no
username. */
@@ -188,7 +252,7 @@
svn_auth_cred_simple_t *creds = apr_pcalloc(pool, sizeof(*creds));
creds->username = username;
creds->password = password;
- - creds->may_save = may_save;
+ creds->may_save = need_to_save;
*credentials = creds;
}
else
- ------------------------------------------------------------------------
- ---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFIrqNz9o1G+2zNQDgRAjHQAJ9PtiXuLCzUV1CXyA14+I1k0eUwzACgoJqj
0Kk11uyEm5F6n+X8D/wEsx0=
=9zj2
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
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-22 13:31:26 CEST