Daniel Shahaf <d.s_at_daniel.shahaf.co.il> writes:
> [[[
> Username/password caching bugfixes.
>
> * subversion/libsvn_subr/simple_providers.c
> (simple_save_creds_helper):
> Cache the username even if we're not allowed to cache the password,
> and respect SVN_AUTH_PARAM_NO_AUTH_CACHE.
>
> Suggested by: stsp
> Patch by: Daniel Shahaf <d.s_at_daniel.shahaf.co.il>
> ]]]
As mentioned in previous mail: clarify that the second part is to fix an
API violation, and that there is currently no user-visible manifestation
of the problem.
> Index: subversion/libsvn_subr/simple_providers.c
> ===================================================================
> --- subversion/libsvn_subr/simple_providers.c (revision 30750)
> +++ subversion/libsvn_subr/simple_providers.c (working copy)
> @@ -234,18 +234,25 @@ simple_save_creds_helper(svn_boolean_t *saved,
> apr_hash_t *creds_hash = NULL;
> const char *config_dir;
> svn_error_t *err;
> - const char *dont_store_passwords =
> - apr_hash_get(parameters,
> - SVN_AUTH_PARAM_DONT_STORE_PASSWORDS,
> - APR_HASH_KEY_STRING);
> + svn_boolean_t dont_store_passwords =
> + (NULL != apr_hash_get(parameters,
> + SVN_AUTH_PARAM_DONT_STORE_PASSWORDS,
> + APR_HASH_KEY_STRING));
> svn_boolean_t non_interactive = apr_hash_get(parameters,
> SVN_AUTH_PARAM_NON_INTERACTIVE,
> APR_HASH_KEY_STRING) != NULL;
Heh -- I've always used "!!" for that trick, but "NULL !=" works too.
> + svn_boolean_t no_auth_cache = apr_hash_get(parameters,
> + SVN_AUTH_PARAM_NO_AUTH_CACHE,
> + APR_HASH_KEY_STRING) != NULL;
> + svn_boolean_t username_stored = FALSE;
> svn_boolean_t password_stored = TRUE;
For consistency, better to do the boolean-folding the same way for
both 'dont_store_passwords' and 'no_auth_cache' -- that is, put the
"NULL !=" in front, or at put it at the end, but whichever way it's
done, be consistent. And...
> *saved = FALSE;
>
> if (! creds->may_save)
> + no_auth_cache = TRUE;
> +
> + if (no_auth_cache)
> return SVN_NO_ERROR;
...why not just fold this condition into the initialization of
no_auth_cache in the first place?
> config_dir = apr_hash_get(parameters,
> @@ -254,9 +261,17 @@ simple_save_creds_helper(svn_boolean_t *saved,
>
> /* Put the credentials in a hash and save it to disk */
> creds_hash = apr_hash_make(pool);
> - apr_hash_set(creds_hash, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> - APR_HASH_KEY_STRING,
> - svn_string_create(creds->username, pool));
> +
> + /* Maybe cache the username. */
> + if (! no_auth_cache)
> + {
> + apr_hash_set(creds_hash, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> + APR_HASH_KEY_STRING,
> + svn_string_create(creds->username, pool));
> + username_stored = TRUE;
> + }
> +
> + /* Maybe cache the password. */
> if (! dont_store_passwords)
> {
> password_stored = password_set(creds_hash, realmstring, creds->username,
> @@ -276,7 +291,8 @@ simple_save_creds_helper(svn_boolean_t *saved,
> *saved = FALSE;
> }
>
> - if (password_stored)
> + /* If we cached anything, write it to disk. */
> + if (username_stored || password_stored)
> {
> err = svn_config_write_auth_data(creds_hash, SVN_AUTH_CRED_SIMPLE,
> realmstring, config_dir, pool);
Looks good to me, modulo the above stylistic nits. I assume this passes
'make check'? (I'll start a 'make check' run right now, but won't be
able to see the results till tomorrow morning.)
-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-04-22 22:29:16 CEST