Stefan Sperling wrote on Thu, 1 May 2008 at 17:28 +0200:
> On Tue, Apr 29, 2008 at 08:29:46PM +0300, Daniel Shahaf wrote:
> > > @@ -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
> >
> > 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.
>
+1 to the compromise, due to item (1) on your list (not segfaulting for
clients that don't use the new-in-1.6 plaintext prompt callback).
> Unfortunately, it looks like we cannot provide the true semantics of the
> old API in this case without defaulting to storing plaintext passwords
> again.
Agreed: the old API defaulted to saving passwords.
> There is, AFAIK, no way for us to determine the lifetime of the
> pool we're handed.
>
Nor should there be, if I understand the pools chapter of HACKING correctly.
> > > 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)
> > > - /* 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.
>
Oops, right. I was confused. Did I look at svn_ra_open3 of trunk
(which, obviously, doesn't use these parameters)?
> 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?
Your answer makes sense; thanks.
> What was your question?
>
My question was, "This is how I understand the situation; did
I understand correctly".
> > 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.
>
Agreed, but note that as you phrased it it's very generic; it applies to me,
too. (If I won't be updated properly (e.g., by being taught new cool
programming languages), I would eventually get obsoleted and confusing too ;))
> 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.
>
+1; clearly explained.
The situation isn't symmetrical -- an libsvn_ra reader ought to suspect that a
config has already been read, but libsvn_ra itself doesn't care about this and
overrides the parameters unconditionally.
> > 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.
>
:)
So the choice is, on non-ASCII inputs, between never working correctly and
working correctly inconsistently. I don't have an opinion on this (both
consistency and correctness are important, and I don't know apr_strnatcasecmp
well enough.)
> > > 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?
>
/me discovers that the PASSWORD_TYPE constants are private to simple_providers.c
Document this? If someone adds a password type and greps for
SIMPLE_PASSWORD_TYPE (try this), they won't know that somewhere in the file
there is a list of "special" password types that SIMPLE isn't one of them...
> > Thanks Stefan for your work on this branch; it looks good to me.
> >
> > Daniel
>
> Thanks heaps for your review :)
>
>
:)
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-05-01 18:53:36 CEST