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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel