Merge ~andrew.sh/unity:fix-auth-failure-lp1733557 into unity:master

[Andrej Shadura]

This change fixes the regression introduced in 44e6022, which causes the lock screen to start failing when session switching is involved.

I have tested the change on my machine with two logged in users; the session running the build with the change proposed was working normally, while the session running unpatched Unity would fail after a few session switches.

LP: #1733557

[Andrej Shadura (andrew.sh)]

Any comments, suggestions?

[Andrea Azzarone (azzar1)]

review: Needs Fixing

[Andrej Shadura (andrew.sh)]

@azzar1, I have resubmitted the patch. During the testing it seemed to work equally fine.

[Andrej Shadura (andrew.sh)]

Ha, for some reason my yesterday’s comment did go anywhere.

[Andrea Azzarone (azzar1)]

I took a look at the code and I think the best way to fix this bug is to make sure that we deal properly with failures in pam_authenticate. This function can:
- succeed because the user entered the correct password
- fail because the user entered the wrong password
- fail because the module failed to start

Right now the third case is not considered. We should emit start_failed signal in this case. Let me know if you want to rewrite the patch otherwise I'll try to do it by the end of the week.

[Andrej Shadura (andrew.sh)]

How about:

- merging this fix as is and get it into packages asap to make the lockscreen work properly for the users
- rewrite it in the proper way while the hacky fix is making lives easier :)

I’m willing to look into the proper fix myself, but I’d like to unbreak this sooner rather than later.

[Andrea Azzarone (azzar1)]

> How about:
>
> - merging this fix as is and get it into packages asap to make the lockscreen
> work properly for the users
> - rewrite it in the proper way while the hacky fix is making lives easier :)
>
> I’m willing to look into the proper fix myself, but I’d like to unbreak this
> sooner rather than later.

That would make more sense if unity was default in Disco. The main target for this fix is Xenial, that means that we need to SRU a fix and it usually requires a couple of weeks. I would prefer to SRU the fix is one single iteration.

[Andrej Shadura (andrew.sh)]

On Tue, 5 Feb 2019 at 19:04, Andrea Azzarone
<email address hidden> wrote:
> That would make more sense if unity was default in Disco. The main target for this fix is Xenial, that means that we need to SRU a fix and it usually requires a couple of weeks. I would prefer to SRU the fix is one single iteration.

It’s true it’s not the default in Disco, but I believe Unity still has
a lot of users who find this bug annoying; SRU to Xenial could go with
the proper fix when we have it :)

--
Cheers,
  Andrej

[Andrej Shadura (andrew.sh)]

On Tue, 5 Feb 2019 at 19:07, Andrej Shadura <email address hidden> wrote:
> It’s true it’s not the default in Disco, but I believe Unity still has
> a lot of users who find this bug annoying; SRU to Xenial could go with
> the proper fix when we have it :)

Regardless of the Unity status, do I understand this correctly that
the proper fix would be basically adding and adapting
"self->source_manager_.AddTimeout(0, [self] {
self->start_failed.emit(); return false; });" after we call pam_end?

--
Cheers,
  Andrej

[Andrej Shadura (andrew.sh)]

Ping :)

[Andrea Azzarone (azzar1)]

review: Disapprove

[Andrea Azzarone (azzar1)]

I proposed a fix here: https://code.launchpad.net/~azzar1/unity/+git/unity/+merge/363124

Do you mind testing it?

[Andrej Shadura (andrew.sh)]

The code looks good at the first glance, I’ll test it a bit later today.

Thanks a lot!

--
Cheers,
  Andrej

On Wed, 13 Feb 2019, 12:24 Andrea Azzarone <<email address hidden>
wrote:

> I proposed a fix here:
> https://code.launchpad.net/~azzar1/unity/+git/unity/+merge/363124
>
> Do you mind testing it?
> --
> https://code.launchpad.net/~andrew.sh/unity/+git/unity/+merge/362414
> You are the owner of ~andrew.sh/unity:fix-auth-failure-lp1733557.
>

[Sebastien Bacher (seb128)]

The other changes were merged, did you have a chance to test those? Should we close that one?

[Andrej Shadura (andrew.sh)]

Oh, yes, sure, the other changes work perfectly!

[Sebastien Bacher (seb128)]

thanks, let's close the merge request here then!

Unmerged commits

481bacb... by c4pp4 on 2019-01-16

Fix authentication failure when switching user sessions

Terminate already started PAM transaction and let it try to init the new
one as it's unable to start authentication.

Bug-Ubuntu: https://launchpad.net/bugs/1733557
Signed-off-by: c4pp4 <email address hidden>
Tested-by: Andrej Shadura <email address hidden>
[ Also emit start_failed signal when terminating the PAM transaction ]
Signed-off-by: Andrej Shadura <email address hidden>

diff --git a/lockscreen/UserAuthenticatorPam.cpp b/lockscreen/UserAuthenticatorPam.cpp
index 463d8f9..e8a41b9 100644
--- a/lockscreen/UserAuthenticatorPam.cpp
+++ b/lockscreen/UserAuthenticatorPam.cpp
@@ -46,6 +46,9 @@ bool UserAuthenticatorPam::AuthenticateStart(std::string const& username,
   if (pam_handle_)
   {
     LOG_ERROR(logger) << "Unable to start authentication because another one has already been started";
+    pam_end(pam_handle_, status_);
+    pam_handle_ = nullptr;
+    source_manager_.AddTimeout(0, [this] { start_failed.emit(); return false; });
     return false;
   }