Re: [PATCH] Clear neon_auth_types before adding http_auth_types from server config

On Sat, 17 Mar 2007, Jens Peters wrote:

> Hello there,
>
> Attached is a small patch for the configuration handling of http
> authentication types in neon>0.26 .It seemed like all http-auth-types where
> read nicely from the server configuration, but neon_auth_types was at this
> time already filled with the defaults (BASIC and DIGEST plus NEGOTIATE if
> SSL). Now you can e.g. really force BASIC, thus using other user
> credentials than the current, even if your server supports SSPI and SSL.

Nice catch, Jens!

> Furthermore I added some info to the help output that in the case of SSPI
> --username and --password will be completely ignored. That way windows
> users can get a clue why nothing acts different in an SSPI environment when
> specifying a different user.

This seems reasonable to me.
 
> (I assumed that neon can make use of SSPI on Windows only, is this correct?)

Looking at Neon's source, it uses NE_AUTH_NEGOTIATE for both the
"Negotiate" (GSSPI) and "NTLM" (SSPI) authentication.

> [[[
> * subversion/libsvn_ra_dav/session.c
> clear neon_auth_types before reading http-auth-type from
> server configuration
> * subversion/svn/main.c
> Add help output text that --username ARG and --password ARG
> is ignored if SSPI authentication is used
> ]]]
>
>
> Index: subversion/libsvn_ra_dav/session.c
> ===================================================================
> --- subversion/libsvn_ra_dav/session.c (revision 23887)
> +++ subversion/libsvn_ra_dav/session.c (working copy)
> @@ -502,6 +502,9 @@
> {
> char *token, *last;
> char *auth_types_list = apr_palloc(pool, strlen(http_auth_types) +
> 1);
> +
> + /* clear neon_auth_types */
> + *neon_auth_types = 0;
> apr_collapse_spaces(auth_types_list, http_auth_types);
> while ((token = apr_strtok(auth_types_list, ";", &last)) != NULL)
> {

This approach clobbers the defaults we set in the caller. Instead,
let's wait to see if we've read any auth type configuration before
setting our defaults in svn_ra_dav__open() (as a fall back):

Index: subversion/libsvn_ra_dav/session.c
===================================================================
--- subversion/libsvn_ra_dav/session.c (revision 23898)
+++ subversion/libsvn_ra_dav/session.c (working copy)
@@ -651,7 +651,7 @@
   svn_config_t *cfg;
   const char *server_group;
   char *itr;
- unsigned int neon_auth_types;
+ unsigned int neon_auth_types = 0;
   neonprogress_baton_t *neonprogress_baton =
     apr_pcalloc(pool, sizeof(*neonprogress_baton));
 
@@ -712,11 +712,6 @@
     int timeout;
     int debug;
 
-#ifdef SVN_NEON_0_26
- neon_auth_types = NE_AUTH_BASIC | NE_AUTH_DIGEST;
- if (is_ssl_session)
- neon_auth_types |= NE_AUTH_NEGOTIATE;
-#endif
     SVN_ERR(get_server_settings(&proxy_host,
                                 &proxy_port,
                                 &proxy_username,
@@ -729,6 +724,17 @@
                                 uri->host,
                                 pool));
 
+#ifdef SVN_NEON_0_26
+ if (neon_auth_types == 0)
+ {
+ /* If there were no auth types in the configuration file,
+ provide the appropriate defaults. */
+ neon_auth_types = NE_AUTH_BASIC | NE_AUTH_DIGEST;
+ if (is_ssl_session)
+ neon_auth_types |= NE_AUTH_NEGOTIATE;
+ }
+#endif
+
     if (debug)
       ne_debug_init(stderr, debug);
 
I've committed this to trunk in r23900.

> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 23887)
> +++ subversion/svn/main.c (working copy)
> @@ -92,10 +92,19 @@
> N_("show program version information")},
> {"verbose", 'v', 0, N_("print extra information")},
> {"show-updates", 'u', 0, N_("display update information")},
> +#ifdef WIN32 && HAVE_SSPI

This check isn't quite right: GSSPI is also a factor, and is it
appropriate to use && in conjunction with an #ifdef?

> + {"username", svn_cl__auth_username_opt, 1,N_(
> + "specify a username ARG\n"
> + " ARG is ignored if SSPI authentication is used.")},
> + {"password", svn_cl__auth_password_opt, 1,N_(
> + "specify a password ARG\n"
> + " ARG is ignored if SSPI authentication is used.")},

I haven't compiled it, but the formatting on this warning text looks
like it's going to push this out too far (exceeding 79 columns).

> +#else
> {"username", svn_cl__auth_username_opt, 1,
> N_("specify a username ARG")},
> {"password", svn_cl__auth_password_opt, 1,
> N_("specify a password ARG")},
> +#endif
> #ifndef AS400
> {"extensions", 'x', 1,
> N_("Default: '-u'. When Subversion is invoking an\n"