Senthil Kumaran S <senthil_at_collab.net> writes:
> 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.
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?
Is that what would happen?
Some code comments below.
> * 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
> ]]]
>
> --- 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)
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...
> +{
> + 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?
(As I look through simple_providers.c, I'm unclear on this point...)
> /* 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;
> + }
> + }
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"?
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...
> + /* 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"...
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.
-Karl
> + 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-08-05 18:28:15 CEST