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

Proposed by Andrej Shadura on 2019-01-29
Status: Needs review
Proposed branch: ~andrew.sh/unity:fix-auth-failure-lp1733557
Merge into: unity:master
Diff against target: 13 lines (+3/-0)
1 file modified
lockscreen/UserAuthenticatorPam.cpp (+3/-0)
Reviewer Review Type Date Requested Status
Andrea Azzarone 2019-02-01 Disapprove on 2019-02-13
Unity Team 2019-01-29 Pending
Review via email: mp+362414@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Andrej Shadura (andrew.sh) wrote :

Any comments, suggestions?

Andrea Azzarone (azzar1) :
review: Needs Fixing
Andrej Shadura (andrew.sh) wrote :

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

Andrej Shadura (andrew.sh) wrote :

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

Andrea Azzarone (azzar1) wrote :

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) wrote :

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) wrote :

> 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) wrote :

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) wrote :

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) wrote :

Ping :)

Andrea Azzarone (azzar1) :
review: Disapprove
Andrea Azzarone (azzar1) wrote :

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

Do you mind testing it?

Andrej Shadura (andrew.sh) wrote :

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

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>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lockscreen/UserAuthenticatorPam.cpp b/lockscreen/UserAuthenticatorPam.cpp
2index 463d8f9..e8a41b9 100644
3--- a/lockscreen/UserAuthenticatorPam.cpp
4+++ b/lockscreen/UserAuthenticatorPam.cpp
5@@ -46,6 +46,9 @@ bool UserAuthenticatorPam::AuthenticateStart(std::string const& username,
6 if (pam_handle_)
7 {
8 LOG_ERROR(logger) << "Unable to start authentication because another one has already been started";
9+ pam_end(pam_handle_, status_);
10+ pam_handle_ = nullptr;
11+ source_manager_.AddTimeout(0, [this] { start_failed.emit(); return false; });
12 return false;
13 }
14

Subscribers

People subscribed via source and target branches