[virt-tools-list] [virt-viewer: PATCH 1/3] Ask for username when connecting with SASL

[Christophe Fergeau]

On Wed, Oct 01, 2014 at 04:41:28PM +0200, Fabiano Fidêncio wrote:
> When connecting with SASL for authentication, some authentication
> mechanisms need a username (the plain text and md5 ones, for example).
> ---
> For testing the patch, please, apply:
> http://lists.freedesktop.org/archives/spice-devel/2014-October/017505.html
> ---
>  src/virt-viewer-session-spice.c | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-session-spice.c
> index 885399c..41a9a49 100644
> --- a/src/virt-viewer-session-spice.c
> +++ b/src/virt-viewer-session-spice.c
> @@ -483,6 +483,7 @@ virt_viewer_session_spice_main_channel_event(SpiceChannel *channel G_GNUC_UNUSED
>      VirtViewerSessionSpice *self = VIRT_VIEWER_SESSION_SPICE(session);
>      gchar *password = NULL, *user = NULL;
>      gboolean ret;
> +    static gboolean username_required = FALSE;
>  
>      g_return_if_fail(self != NULL);
>  
> @@ -502,22 +503,52 @@ virt_viewer_session_spice_main_channel_event(SpiceChannel *channel G_GNUC_UNUSED
>          g_debug("main channel: switching host");
>          break;
>      case SPICE_CHANNEL_ERROR_AUTH:
> -        g_debug("main channel: auth failure (wrong password?)");
> +        g_debug("main channel: auth failure (wrong username/password?)");
> +#if SPICE_GTK_CHECK_VERSION(0, 23, 21)

You could change that to the version when we return an error for missing
username, but this one is fine too as this is when
spice_channel_get_error() was introduced.

> +        {
> +            const GError *error = spice_channel_get_error(channel);
> +            if (error != NULL) {
> +                /* When the password is invalid, SPICE_CHANNEL_ERROR_AUTH is
> +                 * returned with no GError associated. It means we just want
> +                 * to change the 'user_required' according to the first try
> +                 * to connect to the server and where a GError will be set to
> +                 * indicate if the authentication needs password and username
> +                 * (SALS case when using authentication mechanisms like
"SASL" not "SALS"
> +                 * md5-digest or plain-text) or if the authencation needs the
"authentication"
> +                 * password only. */

I'm not sure I can read this comment correctly :( The first sentence is
fine, but I'm not sure the rest of the comment adds much to the
g_error_matches() call just below. Or is there some gotcha here that I'm
missing and that this comment explains?

> +                username_required = g_error_matches(error,
> +                                                    SPICE_CHANNEL_ERROR,
> +                                                    SPICE_CHANNEL_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME);
> +            }
> +        }
> +#endif
>  
>          if (self->priv->pass_try > 0)
>              g_signal_emit_by_name(session, "session-auth-failed",
>                                    _("invalid password"));

Maybe this 'invalid password' needs to be changed?

> +

I don't think we need to add a blank line here

>          self->priv->pass_try++;
>  
> +        /* A username is *only* pre-filled in case where some authentication
> +         * error happened. Unfortunately, we don't have a clear way to
> +         * differantiate bewteen invalid username and invalid password.

'differentiate between'

> +         * So, in both cases the username entry will be pre-filled with the
> +         * username used in the previous attempt. */
> +        if (username_required)
> +            g_object_get(self->priv->session, "username", &user, NULL);
> +
>          ret = virt_viewer_auth_collect_credentials(self->priv->main_window,
>                                                     "SPICE",
>                                                     NULL,
> -                                                   NULL, &password);
> +                                                   username_required ? &user : NULL,
> +                                                   &password);
>          if (!ret) {
>              g_signal_emit_by_name(session, "session-cancelled");
>          } else {
>              gboolean openfd;
>  
> +            if (username_required)
> +                g_object_set(self->priv->session, "username", user, NULL);

I guess we could always set 'username' ? This would set it to NULL in
caes the session is reused (dunno if this can happen).

>              g_object_set(self->priv->session, "password", password, NULL);
>              g_object_get(self->priv->session, "client-sockets", &openfd, NULL);
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20141003/02900eff/attachment.sig>