Good morning Stefan,
Reviewing (at your request):
Stefan Sperling wrote:
> $ svn diff https://svn.collab.net/repos/svn/trunk@30801 \
> https://svn.collab.net/repos/svn/branches/dont-save-plaintext-passwords-by-default
> Index: subversion/include/svn_cmdline.h
> ===================================================================
> --- subversion/include/svn_cmdline.h (.../trunk) (revision 30801)
> +++ subversion/include/svn_cmdline.h (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -140,10 +140,23 @@ int svn_cmdline_handle_exit_error(svn_error_t *err
> apr_pool_t *pool,
> const char *prefix);
>
> +/** A cancellation function/baton pair, and the path to the configuration
> + * directory. To be passed as the baton argument to the
> + * @c svn_cmdline_*_prompt functions.
> + *
> + * @since New in 1.6.
> + */
> +typedef struct svn_cmdline_prompt_baton2_t {
> + svn_cancel_func_t cancel_func;
> + void *cancel_baton;
> + const char *config_dir;
> +} svn_cmdline_prompt_baton2_t;
> +
> /** A cancellation function/baton pair to be passed as the baton argument
> * to the @c svn_cmdline_*_prompt functions.
> *
> * @since New in 1.4.
> + * @deprecated Provided for backward compatibility with the 1.5 API.
> */
> typedef struct svn_cmdline_prompt_baton_t {
Should svn_cmdline_prompt_baton_t's docstring point to svn_cmdline_prompt_baton2_t?
> @@ -253,6 +266,18 @@ svn_cmdline_auth_ssl_client_cert_pw_prompt
> svn_boolean_t may_save,
> apr_pool_t *pool);
>
> +/** An implementation of @c svn_auth_plaintext_prompt_func_t that
> + * prompts the user whether storing unencypted passwords to disk
> + * is OK on the command line.
^^^^^^^^^^^^^^^^^^^
"Storing unencrypted passwords ... on the command line"? As opposed to
storing them (the passwords) on the kitchen table? ;)
> + *
> + * @since New in 1.6.
> + */
> +svn_error_t *
> +svn_cmdline_auth_plaintext_prompt(svn_boolean_t *may_save_plaintext,
> Index: subversion/include/svn_auth.h
> ===================================================================
> --- subversion/include/svn_auth.h (.../trunk) (revision 30801)
> +++ subversion/include/svn_auth.h (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -524,6 +559,12 @@ const void * svn_auth_get_parameter(svn_auth_baton
> #define SVN_AUTH_PARAM_DONT_STORE_PASSWORDS SVN_AUTH_PARAM_PREFIX \
> "dont-store-passwords"
>
> +/** @brief Indicates whether providers may save passwords to disk in
> + * plaintext. Property value can be either SVN_CONFIG_TRUE,
> + * SVN_CONFIG_FALSE, or SVN_CONFIG_PROMPT. */
^^^^^^
s/PROMPT/ASK/
> @@ -587,9 +627,12 @@ svn_error_t * svn_auth_next_credentials(void **cre
> /** Save a set of credentials.
> *
> * Ask @a state to store the most recently returned credentials,
> - * presumably because they successfully authenticated. Use @a pool
> - * for temporary allocation. If no credentials were ever returned, do
> - * nothing.
> + * presumably because they successfully authenticated.
> + * All allocations should be done in @a pool, which can be
^^^^^^
Since it's a function we implement, not a callback, might as well
s/can be/is/.
If we do not rev this API, we need to make sure it continues to work for
pre-1.6 callers as well. In this case, what happens if the pool does
*not* survive across RA sessions? Does it only not cache the user's
answers to the prompt, or could it segfault? (I think the former is
fine.)
(The same considerations apply to save_credentials in
svn_auth_provider_t, whose docstring was similarly updated.)
> + * assumed to survive across RA sessions; auth providers that store
> + * passwords in plaintext rely on this.
> + *
> + * If no credentials were ever returned, do nothing.
> */
> svn_error_t * svn_auth_save_credentials(svn_auth_iterstate_t *state,
> apr_pool_t *pool);
> Index: subversion/libsvn_subr/config_file.c
> ===================================================================
> --- subversion/libsvn_subr/config_file.c (.../trunk) (revision 30801)
> +++ subversion/libsvn_subr/config_file.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -897,10 +900,19 @@ svn_config_ensure(const char *config_dir, apr_pool
> + "[auth]" NL
> "### Section for authentication and authorization customizations." NL
> - "[auth]" NL
Was this change involuntary?
> Index: subversion/libsvn_subr/cmdline.c
> ===================================================================
> --- subversion/libsvn_subr/cmdline.c (.../trunk) (revision 30801)
> +++ subversion/libsvn_subr/cmdline.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -459,21 +472,24 @@ svn_cmdline_setup_auth_baton(svn_auth_baton_t **ab
> svn_auth_set_parameter(*ab, SVN_AUTH_PARAM_CONFIG_DIR,
> config_dir);
>
> + /* Determine whether storing passwords in any form is allowed.
> + * This is the deprecated location for this option, the new
> + * location is SVN_CONFIG_CATEGORY_SERVERS. */
> SVN_ERR(svn_config_get_bool(cfg, &store_password_val,
> SVN_CONFIG_SECTION_AUTH,
> SVN_CONFIG_OPTION_STORE_PASSWORDS,
> - TRUE));
> + SVN_CONFIG_DEFAULT_OPTION_STORE_PASSWORDS));
>
> if (! store_password_val)
> svn_auth_set_parameter(*ab, SVN_AUTH_PARAM_DONT_STORE_PASSWORDS, "");
>
> - /* There are two different ways the user can disable disk caching
> - of credentials: either via --no-auth-cache, or in the config
> - file ('store-auth-creds = no'). */
> + /* Determine whether we are allowed to write to the auth/ area.
> + * This is the deprecated location for this option, the new
> + * location is SVN_CONFIG_CATEGORY_SERVERS. */
So, if I understand correctly, the setting in ~/.subversion/config is
passed as an SVN_AUTH_PARAM_* parameter, and the setting(s) in
~/.subversion/servers are read and used in svn_ra_open3()?
It is not entirely obvious, when reading this function in isolation or
reading svn_ra_open3() in isolation, where the other config file's
options are handled. Maybe add cross-reference comments between them?
> Index: subversion/libsvn_subr/prompt.c
> ===================================================================
> --- subversion/libsvn_subr/prompt.c (.../trunk) (revision 30801)
> +++ subversion/libsvn_subr/prompt.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -376,7 +376,67 @@ svn_cmdline_auth_ssl_client_cert_pw_prompt
> +/* This implements 'svn_auth_plaintext_prompt_func_t'. */
> +svn_error_t *
> +svn_cmdline_auth_plaintext_prompt(svn_boolean_t *may_save_plaintext,
> + const char *realmstring,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + do
> + {
> + svn_error_t *err = prompt(&answer, prompt_string, FALSE, pb, pool);
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_CANCELLED)
> + {
> + svn_error_clear(err);
> + *may_save_plaintext = FALSE;
> + return SVN_NO_ERROR;
> + }
> + else
> + return err;
> + }
> + if (svn_cstring_casecmp(answer, _("yes")) == 0)
svn_cstring_casecmp() is:
/**
* Compare two strings @a atr1 and @a atr2, treating case-equivalent
* unaccented Latin (ASCII subset) letters as equal.
*/
int svn_cstring_casecmp(const char *str1, const char *str2);
Do we want to also consider non-ASCII characters as equal regardless of case?
>
> @@ -386,10 +446,12 @@ svn_cmdline_prompt_user2(const char **result,
> svn_cmdline_prompt_baton_t *baton,
> apr_pool_t *pool)
> {
> - return prompt(result, prompt_str, FALSE /* don't hide input */, baton, pool);
> + /* XXX: We know prompt doesn't use the new members
> + * of svn_cmdline_prompt_baton2_t. */
> + return prompt(result, prompt_str, FALSE /* don't hide input */,
> + (svn_cmdline_prompt_baton2_t *)baton, pool);
Add a reverse pointer, from prompt()'s docstring to this comment? If
prompt() started using the new members, this function will need to be
adjusted.
> Index: subversion/libsvn_subr/simple_providers.c
> ===================================================================
> --- subversion/libsvn_subr/simple_providers.c (.../trunk) (revision 30801)
> +++ subversion/libsvn_subr/simple_providers.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -258,46 +272,117 @@ simple_save_creds_helper(svn_boolean_t *saved,
> + /* Don't store passwords in any form if the user has told
> + * us not to do so. */
> + if (! dont_store_passwords)
> {
> + svn_boolean_t may_save_password = FALSE;
>
> + /* If the password is going to be stored encrypted, go right
> + * ahead and store it to disk. Else determine whether saving
> + * in plaintext is OK. */
> + if (strcmp(passtype, SVN_AUTH__WINCRYPT_PASSWORD_TYPE) == 0
> + || strcmp(passtype, SVN_AUTH__KEYCHAIN_PASSWORD_TYPE) == 0)
Do we want to hardcode which password types are encrypted?
> Index: TODO.branch
> ===================================================================
> --- TODO.branch (.../trunk) (revision 0)
> +++ TODO.branch (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
(included for completeness) File should be removed before merging to trunk.
> Property changes on: .
> ___________________________________________________________________
> Modified: svn:mergeinfo
> Merged /trunk:r30654-30800
>
>
Thanks Stefan for your work on this branch; it looks good to me.
Daniel
---------------------------------------------------------------------
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-29 19:30:10 CEST