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

[Snir Sheriber]

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 ?
>> -    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, 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..

Snir.