Senthil Kumaran S <senthil_at_collab.net> writes:
> 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).
Oops, sorry Senthil. I owed a response to your response to my response
to your patch :-).
> 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.
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).
I think that 'may_save=FALSE' should mean that no new passwords get
cached -- whether or not a password is *currently* cached for a given
username. But we can deal with that outside this change.
>>> +/* 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.
Thanks.
>>> +{
>>> + 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.
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."
>> 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>
Well, sure, but that's a field in a struct. Just because it has the
same name as a local variable used elsewhere doesn't mean they have the
same semantics (at least, no reader could assume that).
>> 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.
Sounds good to me.
> [[[
> 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/
>
> 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;
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.)
With def_username and def_password, does "def" mean "default"? If so,
just write it out -- that way their meaning is instantly clear.
> - /* 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;
> + }
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.)
-Karl
---------------------------------------------------------------------
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-23 20:48:46 CEST