Re: Review of dont-* branch

On Tue, Apr 29, 2008 at 08:29:46PM +0300, Daniel Shahaf wrote:
> > /** 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?

Yes, I will do that.

> > @@ -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? ;)

:)

I will simply drop that part of the docstring.

> > +/** @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/

Thanks, will fix.

> > @@ -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/.

Yes.

> 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.)

Mmmmmh, good question.

I think the caching should only be done if the client provides a
prompt callback. Currently the code tries to look up answers in the
cache unconditionally. I will fix that, and make the prompt callback's
docstring mention the pool lifetime issue wrt svn_auth_provider_t's
save_credentials.

This way, simple_save_creds_helper will segfault if, and only if:

  1. The client provides a plaintext prompt callback
  2. And the client passes a pool to save_credentials that does not
     survive across RA sessions
  3. And the user causes two RA sessions to be opened (e.g. by
     supplying multiple URLs on the command line)

I think this is a reasonable compromise, given that we change semantics
of old API (save_credentials) only if new API is also used (the prompt
callback), and that the three conditions taken together are quite some
corner case. I don't think client developers will mind this much.

Unfortunately, it looks like we cannot provide the true semantics of the
old API in this case without defaulting to storing plaintext passwords
again. There is, AFAIK, no way for us to determine the lifetime of the
pool we're handed.

> > 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?

Yes, thanks.
 
> > - /* 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()?

I don't really understand what your question is getting at.

svn_ra_open3 _also_ sets SVN_AUTH_PARAM_* parameters. It just gets
the values from the servers file, instead of the config file.

The providers are shielded from the configuration details, e.g.
which file parameters come from. They only ever see SVN_AUTH_PARAM_*
parameters.

Does that make sense? What was your question?

> 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?

I'm not comfortable with cross-referencing function names in comments
in different files (let alone libraries), because the comments may get
obsoleted and cause confusion rather then help, if not updated properly.

I will make the comments in svn_cmdline_setup_auth_baton mention
that "the RA layer" may override auth settings. That should be
generic enough to be valid for at least a good long while.

I don't think putting a comment about options specified in 'config'
into the RA layer code makes much sense. It should be clear that
the RA layer gets its settings from 'servers', and that the command
line client reads 'config' independently of what the RA layer is doing.
If this is not apparent to the reader, than they simply haven't
read enough of our code yet :)

Note, for example, the code in libsvn_ra_neon/session.c, which has
been reading 'servers' for ages, and never mentions 'config' in
any way. When cross-referencing 'config' from svn_ra_open3(), we
might as well add comments to libsvn_ra_neon about 'config'.
And that does not really make sense, I think. We'd be documenting
behaviour of one library in another.
 
> 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?

I guess we could use apr_strnatcasecmp() instead.
This maps characters to their upper-case equivalent via 'toupper',
which works with non-ASCII characters on some system, based on what
the C library does, whether the system supports locales, what day
of week it is, whether the sun is out or whether it's raining,
and apparently some other magic factors.

> > @@ -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.

Yes, thanks.
 
> > 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?

Well, what's the alternative?

> > 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.

Sure, I will do that.

> Thanks Stefan for your work on this branch; it looks good to me.
>
> Daniel

Thanks heaps for your review :)

-- 
Stefan Sperling <stsp_at_elego.de>                    Software Monkey
 
German law requires the following banner :(
elego Software Solutions GmbH                            HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12        Tel:  +49 30 23 45 86 96 
13355 Berlin                              Fax:  +49 30 23 45 86 95
http://www.elego.de                               CEO: Olaf Wagner
 
Store password unencrypted (yes/no)? No