Merge lp:~mterry/ubuntu-system-settings/allow-some-failures into lp:ubuntu-system-settings

[Michael Terry]

Fix false-negatives from AccountsService when switching from swipe to password (LP: #1363405)

When AccountsService switches from swipe to password (or back), it does several things (delete user password if moving to swipe, then adjust some group memberships like nopasswdlogin, then adjust some internal state).

But on Touch, the group membership change will fail since /etc/group is readonly. (But it doesn't matter, since AccountsService will note from its inotify monitor on /var/lib/extrausers/shadow that it needs to update its internal state.)

However! USS will see the failure and try to rollback to swipe instead of changing away from swipe. Except even then it will mess up. It won't make the same call again to AS to delete the password again, so we'll end up with the password changed but left in place. Producing the symptoms in bug 1363405.

Fix false-negatives from AccountsService when switching from swipe to password (LP: #1363405)

When AccountsService switches from swipe to password (or back), it does several things (delete user password if moving to swipe, then adjust some group memberships like nopasswdlogin, then adjust some internal state).

But on Touch, the group membership change will fail since /etc/group is readonly. (But it doesn't matter, since AccountsService will note from its inotify monitor on /var/lib/extrausers/shadow that it needs to update its internal state.)

However! USS will see the failure and try to rollback to swipe instead of changing away from swipe. Except even then it will mess up. It won't make the same call again to AS to delete the password again, so we'll end up with the password changed but left in place. Producing the symptoms in bug 1363405.

== Checklist ==

 * Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)
 Yes

 * Did you build your software in a clean sbuild/pbuilder chroot or ppa?
 Yes

 * Did you build your software in a clean sbuild/pbuilder armhf chroot or ppa?
 Yes

 * Has your component "TestPlan” been executed successfully on emulator, N4?
 Yes

 * Has a 5 minute exploratory testing run been executed on N4?
 Yes

 * If you changed the packaging (debian), did you subscribe a core-dev to this MP?
 NA

 * If you changed the UI, did you subscribe the design-reviewers to this MP?
 NA

 * What components might get impacted by your changes?
 Security/Privacy

 * Have you requested review by the teams of these owning components?
 That's me I guess.

[Michael Terry (mterry)]

BTW, this change got introduced when I moved from the act_set_password_mode() call to directly calling SetPasswordMode over DBus. The libact version returns no error information and we had to indirectly determine whether it succeeded. When calling directly, we apparently get too much error information. :)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1006
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1433/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/4628
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/3427
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-i386-ci/625
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/4410
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5880
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5880/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12912
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/2830
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/3730
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/3730/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1433/rebuild

review: Needs Fixing (continuous-integration)

[Sebastien Bacher (seb128)]

Thanks (wouldn't it be nicer to have the comment in /* */ rather than having to add // on each line? ;-)

review: Approve

[Michael Terry (mterry)]

I've always liked // blocks... That's a personal quirk though. :)

=== modified file 'plugins/security-privacy/securityprivacy.cpp'
--- plugins/security-privacy/securityprivacy.cpp	2014-09-08 21:09:12 +0000
+++ plugins/security-privacy/securityprivacy.cpp	2014-09-08 21:09:12 +0000
@@ -220,11 +220,20 @@
                          "org.freedesktop.Accounts.User",
                          QDBusConnection::systemBus());
 
-    QDBusReply<void> success = iface.call("SetPasswordMode", newMode);
-    if (success.isValid()) {
+    QDBusReply<void> reply = iface.call("SetPasswordMode", newMode);
+    if (reply.isValid() || reply.error().name() == "org.freedesktop.Accounts.Error.Failed") {
+        // We allow "org.freedesktop.Accounts.Error.Failed" because we actually
+        // expect that error in some cases.  In Ubuntu Touch, group memberships
+        // are not allowed to be changed (/etc/group is read-only).  So when
+        // AccountsService tries to add/remove the user from the nopasswdlogin
+        // group, it will fail.  Thankfully, this will be after it does what we
+        // actually care about it doing (deleting user password).  But it will
+        // return an error in this case, with a message about gpasswd failing
+        // and the above error name.  In other cases (like bad authentication),
+        // it will return something else (like Error.PermissionDenied).
         return true;
     } else {
-        qWarning() << "Could not set password mode:" << success.error().message();
+        qWarning() << "Could not set password mode:" << reply.error().message();
         return false;
     }
 }
@@ -391,6 +400,7 @@
         if (!setPasswordModeWithPolicykit(type, value)) {
             setDisplayHint(oldType);
             setPassword(value, oldValue);
+            setPasswordModeWithPolicykit(oldType, oldValue); // needed to revert to swipe
             return badPasswordMessage(oldType);
         }
     }