Handle autologin auth attempt errors

[SgAkErRu]

If the autologin auth attempt failed for any reason, then start socket server and greeter (launch sddm theme, like as if there was no autologin at all).

[Vogtinator]

IMO this is the wrong place for such a check. If the auth attempt fails for any reason, it should be handled gracefully.

[SgAkErRu]

It is attempt to autologin, why we need to try to auth, if we know right away that it won't work? A little higher in this function, session is checked in a similar way.

It seems to me there is no point in making such an attempt. But anyway I see problem with handling auth attempt after autologin in that m_socketServer (which, as it seemed to me, is engaged in handling the failed authorization signal) is not even launched before autologin attempt (but I can be wrong).

Also, proposed patch is similar to this case (there we don't even try to log in if it doesn't make sense).

[SgAkErRu]

But I think I can trying to move that check into Display::startAuth, following the example of sanity session checks there.

What do you think about this idea?

[Vogtinator]

It is attempt to autologin, why we need to try to auth, if we know right away that it won't work? A little higher in this function, session is checked in a similar way.

That's actually for a different reason: The session has to be found to set sessionType correctly.

It seems to me there is no point in making such an attempt. But anyway I see problem with handling auth attempt after autologin in that m_socketServer (which, as it seemed to me, is engaged in handling the failed authorization signal) is not even launched before autologin attempt (but I can be wrong).

If that's the case, that should probably be changed. IMO autologin should be as close to regular login as possible.

Also, proposed patch is similar to this case (there we don't even try to log in if it doesn't make sense).

Yeah, but that's also a case of doing an early check. Even if you remove this condition, it'll behave the same way. Login will just be refused by the PAM backend instead of the daemon.

But I think I can trying to move that check into Display::startAuth, following the example of sanity session checks there.

What do you think about this idea?

Not wrong, but IMO it's still important to handle autologin auth failures in any case, not just for wrong usernames.

[Vogtinator]

The logic right now looks good, but what I don't really like is that there are now two ways autologin can fail: Synchronosly (returns false) and asynchronously (auth failure signal). Any idea how this could be unified? That avoids subtle errors like somehow ending up without m_auth->setAutologin(false); in some weird circumstance.

[SgAkErRu]

...two ways autologin can fail: Synchronosly (returns false) and asynchronously (auth failure signal). Any idea how this could be unified?

I think, still, they have a different nature: synchronous failure is a failure of early checks, and async failure is the authorization failure.

But, I think we can try to reduce the difference between them, if we put failure handling in a separate function (i mean that handling):

qWarning() << "Autologin failed!";
m_auth->setAutologin(false);
startSocketServerAndGreeter();

And will use that function in Display::slotAuthError and Display::attemptAutologin() directly, without returning boolean from Display::attemptAutologin() in Display::displayServerStarted(), so it will be just:

attemptAutologin();
return;

instead of

bool success = attemptAutologin();
if (success) {
    return;
} else {
    qWarning() << "Autologin failed!";
}

in Display::displayServerStarted() function.

[Vogtinator]

...two ways autologin can fail: Synchronosly (returns false) and asynchronously (auth failure signal). Any idea how this could be unified?

I think, still, they have a different nature: synchronous failure is a failure of early checks, and async failure is the authorization failure.

IMO that's an "implementation detail" (as far as that is possible inside the same class). The result is the same. I'm thinking along the lines of emitting slotAuthError within attemptAutologin for instance, though that can be misleading in other ways.

But, I think we can try to reduce the difference between them, if we put failure handling in a separate function (i mean that handling):

qWarning() << "Autologin failed!";
m_auth->setAutologin(false);
startSocketServerAndGreeter();

And will use that function in Display::slotAuthError and Display::attemptAutologin() directly, without returning boolean from Display::attemptAutologin() in Display::displayServerStarted(), so it will be just:

attemptAutologin();
return;

instead of

bool success = attemptAutologin();
if (success) {
    return;
} else {
    qWarning() << "Autologin failed!";
}

in Display::displayServerStarted() function.

Ok.

[SgAkErRu]

I'm thinking along the lines of emitting slotAuthError within attemptAutologin for instance, though that can be misleading in other ways.

Yes, I agree, because a failure of early checks didn't perform authorization process (through m_auth), so it can be misleading indeed.

I have put handling in separate function, but I have decided to leave Display::attemptAutologin() returning boolean, because it can return false if unable to find autologin session entry. So, true value of Display::attemptAutologin() just means that autologin has been started and this is expressed in the name of the variable autologinStarted instead of success. I think this will emphasized that the autologin can be failed asynchronously.

[SgAkErRu]

@Vogtinator can you check, please?

[Vogtinator]

Except for that nitpick it looks good