You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com> on 2008/05/25 12:29:33 UTC
Re: svn commit: r31428 - in trunk/subversion: include libsvn_subr
2008-05-24 22:22:10 arfrever@tigris.org napisał(a):
> Author: arfrever
> Date: Sat May 24 13:22:10 2008
> New Revision: 31428
>
> Log:
> Implement the 'password-stores' config option.
>
> * subversion/include/svn_config.h
> (SVN_CONFIG_OPTION_PASSWORD_STORES): Define.
>
> * subversion/libsvn_subr/config_file.c
> (svn_config_ensure): Update the default 'config' file.
>
> * subversion/libsvn_subr/cmdline.c
> (svn_cmdline_setup_auth_baton): Use the 'password-stores' option
> to determine order of dynamically loaded auth simple providers.
>
> Modified:
> trunk/subversion/include/svn_config.h
> trunk/subversion/libsvn_subr/cmdline.c
> trunk/subversion/libsvn_subr/config_file.c
>
> Modified: trunk/subversion/include/svn_config.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_config.h?pathrev=31428&r1=31427&r2=31428
> ==============================================================================
> --- trunk/subversion/include/svn_config.h Sat May 24 10:07:45 2008 (r31427)
> +++ trunk/subversion/include/svn_config.h Sat May 24 13:22:10 2008 (r31428)
> @@ -80,8 +80,9 @@ typedef struct svn_config_t svn_config_t
>
> #define SVN_CONFIG_CATEGORY_CONFIG "config"
> #define SVN_CONFIG_SECTION_AUTH "auth"
> -/* The "auth" section has been deprecated.
> - * Its options have been moved to SVN_CONFIG_CATEGORY_SERVERS. */
> +#define SVN_CONFIG_OPTION_PASSWORD_STORES "password-stores"
> +/* The majority of options of the "auth" section
> + * has been moved to SVN_CONFIG_CATEGORY_SERVERS. */
> #define SVN_CONFIG_SECTION_HELPERS "helpers"
> #define SVN_CONFIG_OPTION_EDITOR_CMD "editor-cmd"
> #define SVN_CONFIG_OPTION_DIFF_CMD "diff-cmd"
>
> Modified: trunk/subversion/libsvn_subr/cmdline.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/cmdline.c?pathrev=31428&r1=31427&r2=31428
> ==============================================================================
> --- trunk/subversion/libsvn_subr/cmdline.c Sat May 24 10:07:45 2008 (r31427)
> +++ trunk/subversion/libsvn_subr/cmdline.c Sat May 24 13:22:10 2008 (r31428)
> @@ -435,17 +435,45 @@ svn_cmdline_setup_auth_baton(svn_auth_ba
> svn_auth_get_keychain_simple_provider(&provider, pool);
> APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> #endif
> +#if defined(SVN_HAVE_GNOME_KEYRING) || defined(SVN_HAVE_KWALLET)
> + const char *password_stores_config_option;
> + svn_config_get(cfg,
> + &password_stores_config_option,
> + SVN_CONFIG_SECTION_AUTH,
> + SVN_CONFIG_OPTION_PASSWORD_STORES,
> + "gnome-keyring,kwallet");
> +
> + apr_array_header_t *password_stores
> + = svn_cstring_split(password_stores_config_option, " ,", TRUE, pool);
> +
> + int i;
> + for (i = 0; i < password_stores->nelts; i++)
> + {
> + const char *password_store = APR_ARRAY_IDX(password_stores, i,
> + const char *);
> #ifdef SVN_HAVE_GNOME_KEYRING
> - if (get_auth_simple_provider(&provider, "gnome_keyring", pool))
> - {
> - APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> - }
> + if (apr_strnatcmp(password_store, "gnome-keyring") == 0)
> + {
> + if (get_auth_simple_provider(&provider, "gnome_keyring", pool))
> + {
> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> + }
> + continue;
> + }
> #endif
> #ifdef SVN_HAVE_KWALLET
> - if (get_auth_simple_provider(&provider, "kwallet", pool))
> - {
> - APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> + if (apr_strnatcmp(password_store, "kwallet") == 0)
> + {
> + if (get_auth_simple_provider(&provider, "kwallet", pool))
> + {
> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> + }
> + continue;
> + }
> +#endif
> + /* TODO: Error. */
Which error code should be used for invalid value of DAV-unrelated configuration option?
> }
> +// SVN_ERR(add_dynamically_loaded_auth_simple_providers_to_array(&providers, cfg, pool));
> #endif
> if (non_interactive == FALSE)
> {
--
Arfrever Frehtes Taifersar Arahesis
Re: svn commit: r31428 - in trunk/subversion: include libsvn_subr
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Arfrever Frehtes Taifersar Arahesis wrote on Mon, 26 May 2008 at 12:29 +0200:
> 2008/5/26 Daniel Shahaf <d....@daniel.shahaf.co.il>:
> > e.g. If I have two clients, one with kwallet and one with both kwallet and
> > gnome-keyring, and my password-stores option is 'gnome-keyring,kwallet',
> > then I'll want the first client to use kwallet, not to complain that
> > 'gnome-keyring' is not a known value...
>
> OK.
>
> I will replace:
> <snip>
Agreed, thanks.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31428 - in trunk/subversion: include libsvn_subr
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
2008/5/26 Daniel Shahaf <d....@daniel.shahaf.co.il>:
> Arfrever Frehtes Taifersar Arahesis wrote on Mon, 26 May 2008 at 11:23 +0200:
>> 2008-05-26 08:06 Daniel Shahaf <d....@daniel.shahaf.co.il> napisał(a):
>> > Karl Fogel wrote on Sun, 25 May 2008 at 22:30 -0400:
>> >> Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com> writes:
>> >> >> Log:
>> >> >> Implement the 'password-stores' config option.
>> >> >>
>> >> >> --- trunk/subversion/libsvn_subr/cmdline.c (r31427)
>> >> >> +++ trunk/subversion/libsvn_subr/cmdline.c (r31428)
>> >> >> @@ -435,17 +435,45 @@ svn_cmdline_setup_auth_baton(svn_auth_ba
>> >> >>
>> >> >> [...]
>> >> >>
>> >> >> #ifdef SVN_HAVE_KWALLET
>> >> >> - if (get_auth_simple_provider(&provider, "kwallet", pool))
>> >> >> - {
>> >> >> - APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>> >> >> + if (apr_strnatcmp(password_store, "kwallet") == 0)
>> >> >> + {
>> >> >> + if (get_auth_simple_provider(&provider, "kwallet", pool))
>> >> >> + {
>> >> >> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>> >> >> + }
>> >> >> + continue;
>> >> >> + }
>> >> >> +#endif
>> >> >> + /* TODO: Error. */
>> >
>> > Don't you need to also allow the case where password_store == ""?
>>
>> svn_cstring_split() doesn't allow password_store to be "".
>>
>
> Sorry, I misread svn_cstring_split()'s docstring. It says that the
> array will not contain empty elements -- I missed the "not".
>
>> When the 'password-stores' config option is set to "", then
>> password_stores->nelts should be 0, so the code inside the 'for (i =
>> 0; i < password_stores->nelts; i++)' loop shouldn't be used.
>>
>> >> > Which error code should be used for invalid value of DAV-unrelated
>> >> > configuration option?
>> >>
>> >
>> > Nothing? It will fall back to plaintext passwords and get a warning
>> > there...
>>
>> When the 'http-library' option is set to invalid value, then the
>> "Invalid config: unknown HTTP library '%s'" error is printed instead
>> of falling back to Neon.
>> I would prefer consistent behavior.
>>
>
> That error is only printed when both serf and neon are enabled at
> compile time. Therefore, ignoring gnome-keyring when it is not enabled
> at compile time might be considered consistent...
>
> e.g. If I have two clients, one with kwallet and one with both kwallet and
> gnome-keyring, and my password-stores option is 'gnome-keyring,kwallet',
> then I'll want the first client to use kwallet, not to complain that
> 'gnome-keyring' is not a known value...
OK.
I will replace:
<snip>
#ifdef SVN_HAVE_GNOME_KEYRING
if (apr_strnatcmp(password_store, "gnome-keyring") == 0)
{
if (get_auth_simple_provider(&provider, "gnome_keyring", pool))
{
APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *)
= provider;
}
continue;
}
#endif
</snip>
With:
<snip>
if (apr_strnatcmp(password_store, "gnome-keyring") == 0)
{
#ifdef SVN_HAVE_GNOME_KEYRING
if (get_auth_simple_provider(&provider, "gnome_keyring", pool))
{
APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *)
= provider;
}
#endif
continue;
}
</snip>
(And the same for KWallet-related code.)
Re: svn commit: r31428 - in trunk/subversion: include libsvn_subr
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Arfrever Frehtes Taifersar Arahesis wrote on Mon, 26 May 2008 at 11:23 +0200:
> 2008-05-26 08:06 Daniel Shahaf <d....@daniel.shahaf.co.il> napisał(a):
> > Karl Fogel wrote on Sun, 25 May 2008 at 22:30 -0400:
> >> Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com> writes:
> >> >> Log:
> >> >> Implement the 'password-stores' config option.
> >> >>
> >> >> --- trunk/subversion/libsvn_subr/cmdline.c (r31427)
> >> >> +++ trunk/subversion/libsvn_subr/cmdline.c (r31428)
> >> >> @@ -435,17 +435,45 @@ svn_cmdline_setup_auth_baton(svn_auth_ba
> >> >>
> >> >> [...]
> >> >>
> >> >> #ifdef SVN_HAVE_KWALLET
> >> >> - if (get_auth_simple_provider(&provider, "kwallet", pool))
> >> >> - {
> >> >> - APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> >> >> + if (apr_strnatcmp(password_store, "kwallet") == 0)
> >> >> + {
> >> >> + if (get_auth_simple_provider(&provider, "kwallet", pool))
> >> >> + {
> >> >> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> >> >> + }
> >> >> + continue;
> >> >> + }
> >> >> +#endif
> >> >> + /* TODO: Error. */
> >
> > Don't you need to also allow the case where password_store == ""?
>
> svn_cstring_split() doesn't allow password_store to be "".
>
Sorry, I misread svn_cstring_split()'s docstring. It says that the
array will not contain empty elements -- I missed the "not".
> When the 'password-stores' config option is set to "", then
> password_stores->nelts should be 0, so the code inside the 'for (i =
> 0; i < password_stores->nelts; i++)' loop shouldn't be used.
>
> >> > Which error code should be used for invalid value of DAV-unrelated
> >> > configuration option?
> >>
> >
> > Nothing? It will fall back to plaintext passwords and get a warning
> > there...
>
> When the 'http-library' option is set to invalid value, then the
> "Invalid config: unknown HTTP library '%s'" error is printed instead
> of falling back to Neon.
> I would prefer consistent behavior.
>
That error is only printed when both serf and neon are enabled at
compile time. Therefore, ignoring gnome-keyring when it is not enabled
at compile time might be considered consistent...
e.g. If I have two clients, one with kwallet and one with both kwallet and
gnome-keyring, and my password-stores option is 'gnome-keyring,kwallet',
then I'll want the first client to use kwallet, not to complain that
'gnome-keyring' is not a known value...
Re: svn commit: r31428 - in trunk/subversion: include libsvn_subr
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
2008-05-26 08:06 Daniel Shahaf <d....@daniel.shahaf.co.il> napisał(a):
> Karl Fogel wrote on Sun, 25 May 2008 at 22:30 -0400:
>> Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com> writes:
>> >> Log:
>> >> Implement the 'password-stores' config option.
>> >>
>> >> --- trunk/subversion/libsvn_subr/cmdline.c (r31427)
>> >> +++ trunk/subversion/libsvn_subr/cmdline.c (r31428)
>> >> @@ -435,17 +435,45 @@ svn_cmdline_setup_auth_baton(svn_auth_ba
>> >>
>> >> [...]
>> >>
>> >> #ifdef SVN_HAVE_KWALLET
>> >> - if (get_auth_simple_provider(&provider, "kwallet", pool))
>> >> - {
>> >> - APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>> >> + if (apr_strnatcmp(password_store, "kwallet") == 0)
>> >> + {
>> >> + if (get_auth_simple_provider(&provider, "kwallet", pool))
>> >> + {
>> >> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>> >> + }
>> >> + continue;
>> >> + }
>> >> +#endif
>> >> + /* TODO: Error. */
>
> Don't you need to also allow the case where password_store == ""?
svn_cstring_split() doesn't allow password_store to be "".
When the 'password-stores' config option is set to "", then
password_stores->nelts should be 0, so the code inside the 'for (i =
0; i < password_stores->nelts; i++)' loop shouldn't be used.
>> > Which error code should be used for invalid value of DAV-unrelated
>> > configuration option?
>>
>
> Nothing? It will fall back to plaintext passwords and get a warning
> there...
When the 'http-library' option is set to invalid value, then the
"Invalid config: unknown HTTP library '%s'" error is printed instead
of falling back to Neon.
I would prefer consistent behavior.
Re: svn commit: r31428 - in trunk/subversion: include libsvn_subr
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Karl Fogel wrote on Sun, 25 May 2008 at 22:30 -0400:
> Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com> writes:
> >> Log:
> >> Implement the 'password-stores' config option.
> >>
> >> --- trunk/subversion/libsvn_subr/cmdline.c (r31427)
> >> +++ trunk/subversion/libsvn_subr/cmdline.c (r31428)
> >> @@ -435,17 +435,45 @@ svn_cmdline_setup_auth_baton(svn_auth_ba
> >>
> >> [...]
> >>
> >> #ifdef SVN_HAVE_KWALLET
> >> - if (get_auth_simple_provider(&provider, "kwallet", pool))
> >> - {
> >> - APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> >> + if (apr_strnatcmp(password_store, "kwallet") == 0)
> >> + {
> >> + if (get_auth_simple_provider(&provider, "kwallet", pool))
> >> + {
> >> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
> >> + }
> >> + continue;
> >> + }
> >> +#endif
> >> + /* TODO: Error. */
Don't you need to also allow the case where password_store == ""?
> >
> > Which error code should be used for invalid value of DAV-unrelated
> > configuration option?
>
Nothing? It will fall back to plaintext passwords and get a warning
there...
> SVN_ERR_AUTHN_NO_PROVIDER ?
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31428 - in trunk/subversion: include libsvn_subr
Posted by Karl Fogel <kf...@red-bean.com>.
Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com> writes:
>> Log:
>> Implement the 'password-stores' config option.
>>
>> --- trunk/subversion/libsvn_subr/cmdline.c (r31427)
>> +++ trunk/subversion/libsvn_subr/cmdline.c (r31428)
>> @@ -435,17 +435,45 @@ svn_cmdline_setup_auth_baton(svn_auth_ba
>>
>> [...]
>>
>> #ifdef SVN_HAVE_KWALLET
>> - if (get_auth_simple_provider(&provider, "kwallet", pool))
>> - {
>> - APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>> + if (apr_strnatcmp(password_store, "kwallet") == 0)
>> + {
>> + if (get_auth_simple_provider(&provider, "kwallet", pool))
>> + {
>> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>> + }
>> + continue;
>> + }
>> +#endif
>> + /* TODO: Error. */
>
> Which error code should be used for invalid value of DAV-unrelated
> configuration option?
SVN_ERR_AUTHN_NO_PROVIDER ?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org