Merge lp:~mterry/ubuntu-system-settings/same-pass-switch into lp:ubuntu-system-settings

[Michael Terry]

Allow switch from PIN code to passphrase and back using the same password. (LP: #1357043)

The reason this bug exists in the first place is that passwd doesn't let you switch to the same password. So while we could change the AccountsService password display hint just fine, we'd see an error from passwd and bail out. So IF the user gave us the correct password, we can ignore any errors from passwd if the new password is the same.

I also added some more verbose output in some error cases while I was there. It'll be helpful when debugging in the future (and I didn't want to separate it out into its own branch because conflicts).

Allow switch from PIN code to passphrase and back using the same password. (LP: #1357043)

The reason this bug exists in the first place is that passwd doesn't let you switch to the same password. So while we could change the AccountsService password display hint just fine, we'd see an error from passwd and bail out. So IF the user gave us the correct password, we can ignore any errors from passwd if the new password is the same.

I also added some more verbose output in some error cases while I was there. It'll be helpful when debugging in the future (and I didn't want to separate it out into its own branch because conflicts).

== 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?
 Is that me these days?

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1005
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1431/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/4624
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/3424
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-i386-ci/623
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/4403
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5876
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5876/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12903
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/2827
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/3726
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/3726/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Sebastien Bacher (seb128)]

seems ok to me, thanks

review: Approve

=== modified file 'plugins/security-privacy/securityprivacy.cpp'
--- plugins/security-privacy/securityprivacy.cpp	2014-08-29 05:19:03 +0000
+++ plugins/security-privacy/securityprivacy.cpp	2014-09-08 19:37:19 +0000
@@ -18,6 +18,7 @@
  */
 
 #include "securityprivacy.h"
+#include <QtCore/QDebug>
 #include <QtCore/QProcess>
 #include <QtDBus/QDBusConnection>
 #include <QtDBus/QDBusConnectionInterface>
@@ -220,7 +221,12 @@
                          QDBusConnection::systemBus());
 
     QDBusReply<void> success = iface.call("SetPasswordMode", newMode);
-    return success.isValid();
+    if (success.isValid()) {
+        return true;
+    } else {
+        qWarning() << "Could not set password mode:" << success.error().message();
+        return false;
+    }
 }
 
 bool SecurityPrivacy::setPasswordModeWithPolicykit(SecurityType type, QString password)
@@ -242,6 +248,8 @@
     // But first, see if we have cached authentication
     if (setPasswordMode(type))
         return true;
+    else if (password.isEmpty())
+        return false;
 
     QProcess polkitHelper;
     polkitHelper.setProgram(HELPER_EXEC);
@@ -283,7 +291,7 @@
         QString output = QString::fromUtf8(pamHelper.readLine());
         if (output.isEmpty()) {
             return "Internal error: could not run passwd";
-        } else {	
+        } else {
             // Grab everything on first line after the last colon.  This is because
             // passwd will bunch it up like so:
             // "(current) UNIX password: Enter new UNIX password: Retype new UNIX password: You must choose a longer password"
@@ -363,12 +371,22 @@
     } else {
         QString errorText = setPassword(oldValue, value);
         if (!errorText.isEmpty()) {
-            setDisplayHint(oldType);
-            // Special case this common message because the one PAM gives is so awful
-            if (errorText == dgettext("Linux-PAM", "Authentication token manipulation error"))
+            if (errorText == dgettext("Linux-PAM", "Authentication token manipulation error")) {
+                // Special case this common message because the one PAM gives is so awful
+                setDisplayHint(oldType);
                 return badPasswordMessage(oldType);
-            else
+            } else if (oldValue != value) {
+                // Only treat this as an error case if the passwords aren't
+                // the same.  (If they are the same, and we're just switching
+                // display hints, then passwd will give us an error message
+                // like "Password unchanged" but we don't want to rely on
+                // parsing that output.  Instead, if we got past the "bad old
+                // password" part above and oldValue == value, we don't care
+                // about any errors from passwd.)
+                setDisplayHint(oldType);
                 return errorText;
+            }
+            // else fall through to below
         }
         if (!setPasswordModeWithPolicykit(type, value)) {
             setDisplayHint(oldType);

=== modified file 'src/accountsservice.cpp'
--- src/accountsservice.cpp	2014-07-28 09:08:45 +0000
+++ src/accountsservice.cpp	2014-09-08 19:37:19 +0000
@@ -21,6 +21,7 @@
 #include "accountsservice.h"
 
 #include <QDBusReply>
+#include <QDebug>
 
 #include <unistd.h>
 #include <sys/types.h>
@@ -134,6 +135,9 @@
                                   interface,
                                   property,
                                   QVariant::fromValue(QDBusVariant(value)));
+    if (msg.type() == QDBusMessage::ErrorMessage) {
+        qWarning() << "Could not set AccountsService property" << property << "on interface" << interface << "for object" << m_objectPath << "to" << value << ":" << msg.errorMessage();
+    }
     return msg.type() == QDBusMessage::ReplyMessage;
 }
 
@@ -146,5 +150,9 @@
                           m_systemBusConnection,
                           this);
 
-    return iface.call(method, value).type() == QDBusMessage::ReplyMessage;
+    QDBusMessage msg = iface.call(method, value);
+    if (msg.type() == QDBusMessage::ErrorMessage) {
+        qWarning() << "Could not call AccountsService method" << method << "for object" << m_objectPath << "with argument" << value << ":" << msg.errorMessage();
+    }
+    return msg.type() == QDBusMessage::ReplyMessage;
 }

=== modified file 'wizard/qml/main.qml'
--- wizard/qml/main.qml	2014-09-04 00:57:21 +0000
+++ wizard/qml/main.qml	2014-09-08 19:37:19 +0000
@@ -46,10 +46,13 @@
         // Immediately go to black to give quick feedback
         blackCover.visible = true
 
-        // Ignore any errors, since we're past where the user set the
-        // method.  Worst case, we just leave the user with a swipe
-        // security method and they fix it in the system settings.
-        securityPrivacy.setSecurity("", password, passwordMethod)
+        var errorMsg = securityPrivacy.setSecurity("", password, passwordMethod)
+        if (errorMsg !== "") {
+            // Ignore (but log) any errors, since we're past where the user set
+            // the method.  Worst case, we just leave the user with a swipe
+            // security method and they fix it in the system settings.
+            console.log("Error setting security method:", errorMsg)
+        }
 
         Qt.quit()
     }