Great, thanks Vlad!
Comments:
o SVN_ERR_RA_SVN_NO_MECHANISMS should be a documented return value of
svn_ra_svn__do_simple_auth().
o Aren't "simple" and "sasl" both doing SASL auth from the server's
perspective? If so, rather than differentiating between "simple" and
"sasl", wouldn't it be more accurate to label the APIs names and error
codes "internal" and "external"?
o It doesn't look like you really needed to remove the
svn_ra_svn__do_auth() interface; of course, what you decided on will
work fine too.
Thanks, Dan
On Mon, 05 Mar 2007, vgeorgescu@tigris.org wrote:
> Author: vgeorgescu
> Date: Mon Mar 5 12:20:04 2007
> New Revision: 23575
>
> Log:
> If Cyrus SASL can't find a suitable authentication mechanism, fall back to the
> built-in SASL implementation.
>
> * subversion/include/svn_error_codes.h
> (SVN_ERR_RA_SVN_NO_MECHANISMS): New error code.
>
> * subversion/libsvn_ra_svn/ra_svn.h
> (svn_ra_svn__do_auth): Remove.
> (svn_ra_svn__do_sasl_auth, svn_ra_svn__do_simple_auth): New.
>
> * subversion/libsvn_ra_svn/simple_auth.c
> Remove the #ifndef..#endif that surrounds the code in this file.
> (svn_ra_svn__do_auth): Rename to..
> (svn_ra_svn__do_simple_auth): ..this. Return a SVN_ERR_RA_SVN_NO_MECHANISMS
> error if mechanism negotiation fails.
>
> * subversion/libsvn_ra_svn/sasl_auth.c
> (try_auth): Return a SVN_ERR_RA_SVN_NO_MECHANISMS error if sasl_client_start
> returns SASL_NOMECH (i.e. no suitable mechs were found).
> (svn_ra_svn__do_auth): Rename to..
> (svn_ra_svn__do_sasl_auth): ..this. Call svn_ra_svn__do_simple_auth if
> try_auth returned a SVN_ERR_RA_SVN_NO_MECHANISMS error.
>
> * subversion/libsvn_ra_svn/client.c
> (DO_AUTH): New #define. Points to either svn_ra_svn__do_simple_auth or
> svn_ra_svn__do_sasl_auth depending on the value of SVN_HAVE_SASL.
> (handle_auth_request): Use DO_AUTH instead of svn_ra_svn__do_auth.
> (open_session): Use DO_AUTH instead of svn_ra_svn__do_auth. Tweak comments
> to reflect the current situation.
>
>
> Modified:
> trunk/subversion/include/svn_error_codes.h
> trunk/subversion/libsvn_ra_svn/client.c
> trunk/subversion/libsvn_ra_svn/ra_svn.h
> trunk/subversion/libsvn_ra_svn/sasl_auth.c
> trunk/subversion/libsvn_ra_svn/simple_auth.c
>
> Modified: trunk/subversion/include/svn_error_codes.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_error_codes.h?pathrev=23575&r1=23574&r2=23575
> ==============================================================================
> --- trunk/subversion/include/svn_error_codes.h (original)
> +++ trunk/subversion/include/svn_error_codes.h Mon Mar 5 12:20:04 2007
> @@ -770,6 +770,11 @@
> SVN_ERR_RA_SVN_CATEGORY_START + 6,
> "Client/server version mismatch")
>
> + SVN_ERRDEF(SVN_ERR_RA_SVN_NO_MECHANISMS,
> + SVN_ERR_RA_SVN_CATEGORY_START + 7,
> + "Cannot negotiate authentication mechanism")
> +
> +
> /* libsvn_auth errors */
>
> /* this error can be used when an auth provider doesn't have
>
> Modified: trunk/subversion/libsvn_ra_svn/client.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_ra_svn/client.c?pathrev=23575&r1=23574&r2=23575
> ==============================================================================
> --- trunk/subversion/libsvn_ra_svn/client.c (original)
> +++ trunk/subversion/libsvn_ra_svn/client.c Mon Mar 5 12:20:04 2007
> @@ -18,6 +18,8 @@
>
>
>
> +#include "svn_private_config.h"
> +
> #define APR_WANT_STRFUNC
> #include <apr_want.h>
> #include <apr_general.h>
> @@ -43,6 +45,12 @@
>
> #include "ra_svn.h"
>
> +#ifdef SVN_HAVE_SASL
> +#define DO_AUTH svn_ra_svn__do_sasl_auth
> +#else
> +#define DO_AUTH svn_ra_svn__do_simple_auth
> +#endif
> +
> typedef struct {
> svn_ra_svn__session_baton_t *sess_baton;
> apr_pool_t *pool;
> @@ -245,7 +253,7 @@
> SVN_ERR(svn_ra_svn_read_cmd_response(conn, pool, "lc", &mechlist, &realm));
> if (mechlist->nelts == 0)
> return SVN_NO_ERROR;
> - return svn_ra_svn__do_auth(sess, mechlist, realm, pool);
> + return DO_AUTH(sess, mechlist, realm, pool);
> }
>
> /* --- REPORTER IMPLEMENTATION --- */
> @@ -550,17 +558,18 @@
> * capability list, and the URL, and subsequently there is an auth
> * request. In version 1, we send back the protocol version, auth
> * mechanism, mechanism initial response, and capability list, and;
> - * then send the URL after authentication. svn_ra_svn__do_auth
> - * temporarily has support for the mixed-style response. */
> + * then send the URL after authentication. svn_ra_svn__do_sasl_auth
> + * and svn_ra_svn__do_simple_auth temporarily have support for the
> + * mixed-style response. */
> /* When we punt support for protocol version 1, we should:
> * - Eliminate this conditional and the similar one below
> - * - Remove v1 support from svn_ra_svn__auth_response and inline it
> - * into svn_ra_svn__do_auth
> - * - Remove the (realm == NULL) support from svn_ra_svn__do_auth
> + * - Remove v1 support from svn_ra_svn__auth_response
> + * - Remove the (realm == NULL) support from svn_ra_svn__do_sasl_auth
> + * and svn_ra_svn__do_simple_auth
> * - Remove the protocol version check from handle_auth_request */
> if (sess->protocol_version == 1)
> {
> - SVN_ERR(svn_ra_svn__do_auth(sess, mechlist, NULL, pool));
> + SVN_ERR(DO_AUTH(sess, mechlist, NULL, pool));
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "c", url));
> }
> else
>
> Modified: trunk/subversion/libsvn_ra_svn/ra_svn.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_ra_svn/ra_svn.h?pathrev=23575&r1=23574&r2=23575
> ==============================================================================
> --- trunk/subversion/libsvn_ra_svn/ra_svn.h (original)
> +++ trunk/subversion/libsvn_ra_svn/ra_svn.h Mon Mar 5 12:20:04 2007
> @@ -175,12 +175,19 @@
> /* Return whether or not there is data pending on STREAM. */
> svn_boolean_t svn_ra_svn__stream_pending(svn_ra_svn__stream_t *stream);
>
> -/* Respond to an auth request and perform authentication. REALM may
> - * be NULL for the initial authentication exchange of protocol version
> - * 1. */
> -svn_error_t *svn_ra_svn__do_auth(svn_ra_svn__session_baton_t *sess,
> - apr_array_header_t *mechlist,
> - const char *realm, apr_pool_t *pool);
> +/* Respond to an auth request and perform authentication. Use the Cyrus
> + * SASL library for mechanism negotiation and for creating authentication
> + * tokens. REALM may be NULL for the initial authentication exchange of
> + * protocol version 1. */
> +svn_error_t *svn_ra_svn__do_sasl_auth(svn_ra_svn__session_baton_t *sess,
> + apr_array_header_t *mechlist,
> + const char *realm, apr_pool_t *pool);
> +
> +/* Same as svn_ra_svn__do_sasl_auth, but uses the built-in implementation of
> + * the CRAM-MD5, ANONYMOUS and EXTERNAL mechanisms. */
> +svn_error_t *svn_ra_svn__do_simple_auth(svn_ra_svn__session_baton_t *sess,
> + apr_array_header_t *mechlist,
> + const char *realm, apr_pool_t *pool);
>
> /* Having picked a mechanism, start authentication by writing out an
> * auth response. If COMPAT is true, also write out a version number
>
> Modified: trunk/subversion/libsvn_ra_svn/sasl_auth.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_ra_svn/sasl_auth.c?pathrev=23575&r1=23574&r2=23575
> ==============================================================================
> --- trunk/subversion/libsvn_ra_svn/sasl_auth.c (original)
> +++ trunk/subversion/libsvn_ra_svn/sasl_auth.c Mon Mar 5 12:20:04 2007
> @@ -386,6 +386,7 @@
> /* Success. */
> break;
> case SASL_NOMECH:
> + return svn_error_create(SVN_ERR_RA_SVN_NO_MECHANISMS, NULL, NULL);
> case SASL_BADPARAM:
> case SASL_NOMEM:
> /* Fatal error. Fail the authentication. */
> @@ -720,9 +721,9 @@
> return SVN_NO_ERROR;
> }
>
> -svn_error_t *svn_ra_svn__do_auth(svn_ra_svn__session_baton_t *sess,
> - apr_array_header_t *mechlist,
> - const char *realm, apr_pool_t *pool)
> +svn_error_t *svn_ra_svn__do_sasl_auth(svn_ra_svn__session_baton_t *sess,
> + apr_array_header_t *mechlist,
> + const char *realm, apr_pool_t *pool)
> {
> apr_pool_t *subpool;
> sasl_conn_t *sasl_ctx;
> @@ -826,7 +827,20 @@
> _("Can't get username or password"));
> }
> if (err)
> - return err;
> + {
> + if (err->apr_err == SVN_ERR_RA_SVN_NO_MECHANISMS)
> + {
> + svn_error_clear(err);
> +
> + /* We could not find a supported mechanism in the list sent by the
> + server. In many cases this happens because the client is missing
> + the CRAM-MD5 or ANONYMOUS plugins, in which case we can simply use
> + the built-in implementation. In all other cases this call will be
> + useless, but hey, at least we'll get consistent error messages. */
> + return svn_ra_svn__do_simple_auth(sess, mechlist, realm, pool);
> + }
> + return err;
> + }
> }
> while (!success);
> svn_pool_destroy(subpool);
>
> Modified: trunk/subversion/libsvn_ra_svn/simple_auth.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_ra_svn/simple_auth.c?pathrev=23575&r1=23574&r2=23575
> ==============================================================================
> --- trunk/subversion/libsvn_ra_svn/simple_auth.c (original)
> +++ trunk/subversion/libsvn_ra_svn/simple_auth.c Mon Mar 5 12:20:04 2007
> @@ -18,7 +18,6 @@
> */
>
> #include "svn_private_config.h"
> -#ifndef SVN_HAVE_SASL
>
> #define APR_WANT_STRFUNC
> #include <apr_want.h>
> @@ -62,9 +61,9 @@
> return SVN_NO_ERROR;
> }
>
> -svn_error_t *svn_ra_svn__do_auth(svn_ra_svn__session_baton_t *sess,
> - apr_array_header_t *mechlist,
> - const char *realm, apr_pool_t *pool)
> +svn_error_t *svn_ra_svn__do_simple_auth(svn_ra_svn__session_baton_t *sess,
> + apr_array_header_t *mechlist,
> + const char *realm, apr_pool_t *pool)
> {
> svn_ra_svn_conn_t *conn = sess->conn;
> const char *realmstring, *user, *password, *msg;
> @@ -113,8 +112,5 @@
> return SVN_NO_ERROR;
> }
> else
> - return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> - _("Cannot negotiate authentication mechanism"));
> + return svn_error_create(SVN_ERR_RA_SVN_NO_MECHANISMS, NULL, NULL);
> }
> -
> -#endif /* SVN_HAVE_SASL */
- application/pgp-signature attachment: stored
Received on Mon Mar 5 21:44:03 2007