-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
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.
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFImu109o1G+2zNQDgRAsF2AJ9nNVWkcaWuSuOAjQjouCuOjFfXHgCdEkAN
3594GGOd7bq8cFczqFXSiZg=
=zcAC
-----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-07 14:43:15 CEST