You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.co.il> on 2008/04/22 17:19:13 UTC
[PATCH] Username caching
Attached patch fixes two issues:
1. If 'store-passwords' (in ~/.subversion/config) was set to FALSE,
then usernames would not be cached either.
2. simple_save_creds_helper only checked CREDS->may_save, but
ignored its SVN_AUTH_PARAM_NO_AUTH_CACHE parameter.
(stsp pointed this out)
I tested manually several combinations of --no-auth-cache,
store-passwords=FALSE and store-auth-creds=FALSE, and they worked. I do
not claim that my manual testing covered all possible situations.
[[[
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....@daniel.shahaf.co.il>
]]]
Thanks,
Daniel
Re: [PATCH] Username caching
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> Karl Fogel wrote on Tue, 22 Apr 2008 at 14:20 -0400:
>> Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
>> > 1. If 'store-passwords' (in ~/.subversion/config) was set to FALSE,
>> > then usernames would not be cached either.
>> >
>> > 2. simple_save_creds_helper only checked CREDS->may_save, but
>> > ignored its SVN_AUTH_PARAM_NO_AUTH_CACHE parameter.
>> > (stsp pointed this out)
>>
>> What was the user-visible manifestation of (2) ?
>>
>
> [ Summary: API violation, cmdline client probably unaffected. ]
Thanks -- got it. Just make sure the log message for the change
clarifies this.
Reviewing patch now.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Username caching
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
[ CCing Stefan as he pointed out (2) and worked with this code recently. ]
Karl Fogel wrote on Tue, 22 Apr 2008 at 14:20 -0400:
> Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> > 1. If 'store-passwords' (in ~/.subversion/config) was set to FALSE,
> > then usernames would not be cached either.
> >
> > 2. simple_save_creds_helper only checked CREDS->may_save, but
> > ignored its SVN_AUTH_PARAM_NO_AUTH_CACHE parameter.
> > (stsp pointed this out)
>
> What was the user-visible manifestation of (2) ?
>
[ Summary: API violation, cmdline client probably unaffected. ]
The documentation of SVN_AUTH_PARAM_NO_AUTH_CACHE requires:
/** @brief The application doesn't want any providers to save credentials
* to disk. Property value is irrelevant; only property's existence
* matters. */
#define SVN_AUTH_PARAM_NO_AUTH_CACHE SVN_AUTH_PARAM_PREFIX "no-auth-cache"
However, the simple provider (simple_first_creds_helper) does not inspect
SVN_AUTH_PARAM_NO_AUTH_CACHE when it sets CREDS->may_save. (It only
inspects the cached 'password type' and the API equivalents of --username
and --password.) This might be considered an API violation.
As to user-visible manifestation, however, I have not succeeded in
reproducing this situation (where may_save is TRUE and
SVN_AUTH_PARAM_NO_AUTH_CACHE is set) with the cmdline client, because
svn_auth_save_credentials checks and does not call the provider if
SVN_AUTH_PARAM_NO_AUTH_CACHE is set:
290 svn_auth_save_credentials(svn_auth_iterstate_t *state,
291 apr_pool_t *pool)
292 {
...
309 /* Do not save the creds if SVN_AUTH_PARAM_NO_AUTH_CACHE is set */
310 no_auth_cache = apr_hash_get(auth_baton->parameters,
311 SVN_AUTH_PARAM_NO_AUTH_CACHE,
312 APR_HASH_KEY_STRING);
313 if (no_auth_cache)
314 return SVN_NO_ERROR;
315
316 /* First, try to save the creds using the provider that produced them. */
317 provider = APR_ARRAY_IDX(state->table->providers,
318 state->provider_idx,
319 svn_auth_provider_object_t *);
320 if (provider->vtable->save_credentials)
321 SVN_ERR(provider->vtable->save_credentials(&save_succeeded,
In other words, by the time the simple provider (simple_save_creds_helper)
is called, it has already been established for it that
SVN_AUTH_PARAM_NO_AUTH_CACHE is not set.
Sorry for not explaining all this originally.
Daniel
> Thanks,
> -Karl
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Username caching
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> 1. If 'store-passwords' (in ~/.subversion/config) was set to FALSE,
> then usernames would not be cached either.
>
> 2. simple_save_creds_helper only checked CREDS->may_save, but
> ignored its SVN_AUTH_PARAM_NO_AUTH_CACHE parameter.
> (stsp pointed this out)
What was the user-visible manifestation of (2) ?
Thanks,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Username caching
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> Karl Fogel wrote on Tue, 22 Apr 2008 at 23:28 -0000:
>> > *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?
>
> Because
>
> apr_hash_get(SVN_AUTH....WHATEVER) != NULL
> || ! creds->may_save
>
> would be very long and would break symmetry with the other initialisations
> nearby.
Well, see r30760 -- it's kind of a matter of taste, but my feeling was
not to try to be the same as the other initializations if the actual
conditions for this one were different, which they are. So I did it all
at once, hopefully in a clear way.
Thanks for the fix!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Username caching
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Karl Fogel wrote on Tue, 22 Apr 2008 at 23:28 -0000:
> Daniel Shahaf <d....@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....@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.
>
Okay.
> > 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.
>
Copied existing code.
> > + 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...
>
Okay.
> > *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?
>
Because
apr_hash_get(SVN_AUTH....WHATEVER) != NULL
|| ! creds->may_save
would be very long and would break symmetry with the other initialisations
nearby.
> > 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'?
Yes. Passes svnserveautocheck on HEAD as of a few hours ago. (I don't
think username/password caching is well-covered by tests, though?)
> (I'll start a 'make check' run right now, but won't be
> able to see the results till tomorrow morning.)
>
> -Karl
>
Thanks for the review Karl.
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Username caching
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> Karl Fogel wrote on Tue, 22 Apr 2008 at 23:40 -0000:
>> By the way, no need for you to write a new patch, Daniel -- I'm testing
>> it already, with the tweaks I mentioned. I can add information to the
>> log message, too.
>
> Okay, thanks for letting me know that before I started working on it.
We do want all of your time, of course -- we just don't want to *waste*
it :-).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Username caching
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Karl Fogel wrote on Tue, 22 Apr 2008 at 23:40 -0000:
> Karl Fogel <kf...@red-bean.com> writes:
> > 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.)
>
> By the way, no need for you to write a new patch, Daniel -- I'm testing
> it already, with the tweaks I mentioned. I can add information to the
> log message, too.
>
> -Karl
>
Okay, thanks for letting me know that before I started working on it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Username caching
Posted by Karl Fogel <kf...@red-bean.com>.
Karl Fogel <kf...@red-bean.com> writes:
> 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.)
By the way, no need for you to write a new patch, Daniel -- I'm testing
it already, with the tweaks I mentioned. I can add information to the
log message, too.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Username caching
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@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....@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org