[Spice-devel] [PATCH v2 spice-gtk 1/2] authentication: Handle failed SASL authentication separately

[Christophe Fergeau]

On Wed, Feb 22, 2017 at 11:50:21AM +0200, Snir Sheriber wrote:
> Hi,
> 
> 
> On 02/21/2017 06:37 PM, Christophe Fergeau wrote:
> > On Sun, Feb 19, 2017 at 04:47:17PM +0200, Snir Sheriber wrote:
> > > Remove handling with failures in the SASL authentication
> > > process to separate function
> > > ---
> > >   src/spice-channel.c | 44 +++++++++++++++++++++++++++-----------------
> > >   1 file changed, 27 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/src/spice-channel.c b/src/spice-channel.c
> > > index af67931..cbf1291 100644
> > > --- a/src/spice-channel.c
> > > +++ b/src/spice-channel.c
> > > @@ -1113,28 +1113,38 @@ static int spice_channel_read(SpiceChannel *channel, void *data, size_t length)
> > >       return length;
> > >   }
> > > +#if HAVE_SASL
> > >   /* coroutine context */
> > > -static void spice_channel_failed_authentication(SpiceChannel *channel,
> > > -                                                gboolean invalidPassword)
> > > +static void spice_channel_failed_sasl_authentication(SpiceChannel *channel)
> > >   {
> > >       SpiceChannelPrivate *c = channel->priv;
> > > +    gint err_code; /* Affects the authentication window activated fileds */
> > >       if (c->auth_needs_username && c->auth_needs_password)
> > > -        g_set_error_literal(&c->error,
> > > -                            SPICE_CLIENT_ERROR,
> > > -                            SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
> > > -                            _("Authentication failed: password and username are required"));
> > > +        err_code = SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME;
> > >       else if (c->auth_needs_username)
> > > -        g_set_error_literal(&c->error,
> > > -                            SPICE_CLIENT_ERROR,
> > > -                            SPICE_CLIENT_ERROR_AUTH_NEEDS_USERNAME,
> > > -                            _("Authentication failed: username is required"));
> Is there a mechanism that allows only username ?

I guess in SSO setups, it makes sense to first ask for just a username,
then check for a valid kerberos ticket for that username (or whatever
you use for SSO), and if there is no such ticket, then ask for an
additional authentication token.

> > > -    else if (c->auth_needs_password)
> > > -        g_set_error_literal(&c->error,
> > > -                            SPICE_CLIENT_ERROR,
> > > -                            SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
> > > -                            _("Authentication failed: password is required"));
> > > -    else if (invalidPassword)
> > > +        err_code = SPICE_CLIENT_ERROR_AUTH_NEEDS_USERNAME;
> > > +    else
> > > +        err_code = SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD;
> > > +
> > > +    g_set_error_literal(&c->error,
> > > +                        SPICE_CLIENT_ERROR,
> > > +                        err_code,
> > > +                        _("SASL authentication failed"));
> > I don't understand what you are aiming for with this series. I thought
> > the goal was to get better error messages on autentication failures, but
> > this change merges 3 distinct error messages
> > _("Authentication failed: password and username are required"));
> > _("Authentication failed: username is required"));
> > _("Authentication failed: password is required"));
> > 
> > to just
> > 
> > _("SASL authentication failed"));
> > 
> > Christophe
> It basically similar issue as in rhbz#1365736 but for the sasl
> authentication case..
> 
> The auth_needs_username & auth_needs_password are used for signaling the
> application if the selected
> mechanism requires them, according to that the username\password fields will
> be enabled\disabled,

Ah, right, so yes, library users can now about the need for username
and/or password through the SPICE_CLIENT_ERROR_AUTH_NEEDS_xxx error code
which is set in the GError returned through a channel-event signal. Then
application *should* disable unneeded fields. (I would not say "will" as
an application could do differently there ;)


> imo it's
> enough for letting the user know "password and username are required"
> The authentication type string is just for letting the user know what
> authentication is in use.
> 
> This is not significant , but i guess it somehow more clear (or maybe less
> confusing) to the user that way..

Now I agree, but the commit log really should explain *why* you are
doing the change, and *why* this is not making the error reporting worse
for the user.
In this case, we could even add a few words in the documentation about
the expected behaviour of client applications to the
SPICE_CLIENT_ERROR_AUTH_NEEDS_xxx errors (assuming there is a good place
to document that).

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170222/f897b6d2/attachment.sig>