Merge lp:~mterry/ubuntu-system-settings/revoke-auth into lp:ubuntu-system-settings

[Michael Terry]

Revoke any cached authorizations before trying to change password mode. This makes sure that we authenticate the user again even if we just did so (for UX consistency).

Note that this is kind of an involved change:

- I took out the trySetSecurity call which doesn't make sense anymore if we don't allow cached authorization.

- I changed the policykit agent to register as a session-wide agent (not just for the system-settings process). This is because polkitd does not yet allow you to revoke cached authorizations for processes. Registering and revoking as a session-wide agent should be fine.

- I also had to use the call to revoke all authorizations. There is a more targeted call, to only remove one authorization, by ID. But polkitd didn't let me find the authorization ID when I queried for it. So... I figured just revoking all of them was fine. We are changing the password after all. Either the user changes it and it makes sense to revoke all auths from the old password or the user fails to change it and it makes sense to revoke all auths as a security measure (some non-owner is holding the phone). Between the problems revoking-by-process and revoking-by-ID, I get the sense that the revokation API isn't often used.

- Adding more calls to the policykit agent made it more noticeable that we were using a blocking call to wait for the agent to finish. That's bad UX. So I changed system-settings to call QCoreApplication::processEvents every now and then while waiting.

- Now that Qt was processing events while waiting, I had introduced a new problem: the user can interact with the main page (even pressing the "back" button) while the dialog is waiting -- you can see this by going to landscape mode). So I made sure that the page and back button are disabled while the dialog is up. (And to get access to the back button, I had to bump the Ubuntu.Components API version to 1.1 to get the 'head' property on a Page.)

Revoke any cached authorizations before trying to change password mode. This makes sure that we authenticate the user again even if we just did so (for UX consistency).

Note that this is kind of an involved change:

- I took out the trySetSecurity call which doesn't make sense anymore if we don't allow cached authorization.

- I changed the policykit agent to register as a session-wide agent (not just for the system-settings process). This is because polkitd does not yet allow you to revoke cached authorizations for processes. Registering and revoking as a session-wide agent should be fine.

- I also had to use the call to revoke all authorizations. There is a more targeted call, to only remove one authorization, by ID. But polkitd didn't let me find the authorization ID when I queried for it. So... I figured just revoking all of them was fine. We are changing the password after all. Either the user changes it and it makes sense to revoke all auths from the old password or the user fails to change it and it makes sense to revoke all auths as a security measure (some non-owner is holding the phone). Between the problems revoking-by-process and revoking-by-ID, I get the sense that the revokation API isn't often used.

- Adding more calls to the policykit agent made it more noticeable that we were using a blocking call to wait for the agent to finish. That's bad UX. So I changed system-settings to call QCoreApplication::processEvents every now and then while waiting.

- Now that Qt was processing events while waiting, I had introduced a new problem: the user can interact with the main page (even pressing the "back" button) while the dialog is waiting -- you can see this by going to landscape mode). So I made sure that the page and back button are disabled while the dialog is up. (And to get access to the back button, I had to bump the Ubuntu.Components API version to 1.1 to get the 'head' property on a Page.)

[Ken VanDine (ken-vandine)]

I love all the code comments, thanks! I did ask a question inline, but I don't think it's anything that needs fixing. I think the code looks good, but we'll need to do some manual testing once we get debs from CI.

[Michael Terry (mterry)]

Inline comments replied!

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1265
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1930/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/987/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/200/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/985/console

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

review: Needs Fixing (continuous-integration)

[lp:~mterry/ubuntu-system-settings/revoke-auth]

1266. By Michael Terry on 2015-01-26

Fix header

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1265
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1933/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1001
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/203/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/886
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/999
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/999/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17471

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1266
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1936/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1010
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/206
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/892
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1008
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1008/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17482

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

review: Approve (continuous-integration)

[Ken VanDine (ken-vandine)]

In testing I've confirmed it does require the old password in each attempt. However, a couple times in the change password dialog, I ran in to cases where using an incorrect current password silently failed to change the password. It's good that it didn't actually change the password, but the UI didn't show the incorrect password error. I can't reproduce it reliably, not sure if it is a regression or not. I'll do the same test without this branch now.

[Ken VanDine (ken-vandine)]

I did manage to reproduce the silent failure to change a password without this branch. So this isn't a regression.

review: Approve

[Ken VanDine (ken-vandine)]

I actually don't think what I ran into was a failure case, I think this happened when I used the incorrect current password and the current password as the new password. So it was just a no-op. I just needed a sample case of more than two different passwords.

=== modified file 'plugins/security-privacy/LockSecurity.qml'
--- plugins/security-privacy/LockSecurity.qml	2015-01-12 13:31:14 +0000
+++ plugins/security-privacy/LockSecurity.qml	2015-01-26 20:32:19 +0000
@@ -31,6 +31,24 @@
     id: page
     title: i18n.tr("Lock security")
 
+    // The user can still press the main "back" button or other buttons on the
+    // page while the "change password" dialog is up.  This is because the
+    // dialog is not guaranteed to cover the whole screen; consider the case of
+    // turning a phone to landscape mode.  We'd rather not have the password
+    // changing operation interrupted by destroying the dialog out from under
+    // it.  So we make sure the whole page and header back button are disabled
+    // while the dialog is working.
+    enabled: dialog === null
+    head.backAction: Action {
+        iconName: "back"
+        enabled: page.enabled
+        onTriggered: {
+            pageStack.pop();
+        }
+    }
+
+    property var dialog: null
+
     UbuntuSecurityPrivacyPanel {
         id: securityPrivacy
     }
@@ -58,16 +76,12 @@
     }
 
     function openDialog() {
-        var newMethod = indexToMethod(unlockMethod.selectedIndex)
-        if (securityPrivacy.trySetSecurity(newMethod))
-            return // Oh, that was easy!  No need to prompt
-
-        var dlg = PopupUtils.open(dialogComponent)
+        dialog = PopupUtils.open(dialogComponent, page)
         // Set manually rather than have these be dynamically bound, since
         // the security type can change out from under us, but we don't
         // want dialog to change in that case.
-        dlg.oldMethod = securityPrivacy.securityType
-        dlg.newMethod = newMethod
+        dialog.oldMethod = securityPrivacy.securityType
+        dialog.newMethod = indexToMethod(unlockMethod.selectedIndex)
     }
 
     RegExpValidator {
@@ -341,6 +355,7 @@
                     enabled: newInput.acceptableInput
                     onClicked: {
                         changeSecurityDialog.enabled = false
+                        incorrect.text = ""
 
                         var match = (newInput.text == confirmInput.text)
                         notMatching.visible = !match
@@ -355,16 +370,15 @@
                             currentInput.visible ? currentInput.text : "",
                             newInput.text,
                             changeSecurityDialog.newMethod)
-                        incorrect.text = errorText
+
                         if (errorText !== "") {
-                            changeSecurityDialog.enabled = true
+                            incorrect.text = errorText
                             currentInput.forceActiveFocus()
                             currentInput.selectAll()
-                            return
+                            changeSecurityDialog.enabled = true
+                        } else {
+                            PopupUtils.close(changeSecurityDialog)
                         }
-
-                        changeSecurityDialog.enabled = true
-                        PopupUtils.close(changeSecurityDialog)
                     }
                 }
             }

=== modified file 'plugins/security-privacy/polkitlistener.cpp'
--- plugins/security-privacy/polkitlistener.cpp	2014-09-03 13:28:48 +0000
+++ plugins/security-privacy/polkitlistener.cpp	2015-01-26 20:32:19 +0000
@@ -18,6 +18,7 @@
  */
 
 #include "polkitlistener.h"
+#include <stdlib.h>
 
 struct _UssPolkitListenerPrivate
 {
@@ -104,7 +105,35 @@
 {
     UssPolkitListenerPrivate *priv = listener->priv;
 
-    PolkitSubject *subject = polkit_unix_process_new_for_owner(priv->pid, 0, getuid());
+    // Use session subject rather than process subject because polkitd doesn't
+    // yet support revoking process subject authorizations yet.  Note that this
+    // means for a brief moment we will be answering authorization requests for
+    // everyone.  But that's OK.  It also means when we revoke authorization,
+    // we clear the whole session's cached auths.  But that's also OK because
+    // we are changing passwords here.  Not unreasonable to do so.  And they're
+    // only cached auths.  It's not critical that they are preserved.
+    PolkitSubject *subject = polkit_unix_session_new(getenv("XDG_SESSION_ID"));
+    if (!subject) {
+        return false;
+    }
+
+    // Revoke any authentication.  This is to ensure that policykit actually
+    // verifies the password we took from the user.  If policykit has a cached
+    // auth token, the user could have entered the wrong password and wonder
+    // why we asked for it if we don't check it.  We value a consistent
+    // interface over the rare times a user will be pleasantly surprised we
+    // kept track of the authorization (for only the swipe option really...).
+    // There will still be a tiny race between revokation and asking policykit,
+    // where the user could be granted authorization again.  But that seems
+    // vanishingly unlikely and to fix it, we'd need to pass the password
+    // prompt signal up to UI and back down again.  Let's just not worry
+    // about it.
+    PolkitAuthority *authority = polkit_authority_get_sync(nullptr, nullptr);
+    polkit_authority_revoke_temporary_authorizations_sync(authority, subject,
+                                                          nullptr, nullptr);
+    g_object_unref(authority);
+
+    // Now actually register ourselves
     priv->registration = polkit_agent_listener_register(POLKIT_AGENT_LISTENER(listener),
                                                         POLKIT_AGENT_REGISTER_FLAGS_RUN_IN_THREAD,
                                                         subject, nullptr, nullptr, nullptr);

=== modified file 'plugins/security-privacy/securityprivacy.cpp'
--- plugins/security-privacy/securityprivacy.cpp	2014-10-16 15:53:41 +0000
+++ plugins/security-privacy/securityprivacy.cpp	2015-01-26 20:32:19 +0000
@@ -18,6 +18,8 @@
  */
 
 #include "securityprivacy.h"
+#include <QtCore/QCoreApplication>
+#include <QtCore/QDateTime>
 #include <QtCore/QDebug>
 #include <QtCore/QProcess>
 #include <QtDBus/QDBusConnection>
@@ -256,7 +258,7 @@
     // SetPasswordMode will involve a check with policykit to see
     // if we have admin authorization.  Since Touch doesn't have a general
     // policykit agent yet (and the design for this panel involves asking for
-    // the password up from anyway), we will spawn our own agent just for this
+    // the password directly anyway), we will spawn our own agent just for this
     // call.  It will only authorize one request for this pid and it will use
     // the password we pass it via stdin.  We can drop this helper code when
     // Touch has a real policykit agent and/or the design for this panel
@@ -267,10 +269,7 @@
     // and QProcess's signal handling conflict.  They seem to get in each
     // other's way for the same signals.  So we just do this out-of-process.
 
-    // But first, see if we have cached authentication
-    if (setPasswordMode(type))
-        return true;
-    else if (password.isEmpty())
+    if (password.isEmpty())
         return false;
 
     QProcess polkitHelper;
@@ -279,15 +278,32 @@
     polkitHelper.write(password.toUtf8() + "\n");
     polkitHelper.closeWriteChannel();
 
-    while (polkitHelper.canReadLine() || polkitHelper.waitForReadyRead()) {
-        QString output = polkitHelper.readLine();
-        if (output == "ready\n")
-            break;
+    qint64 endTime = QDateTime::currentMSecsSinceEpoch() + 10000;
+
+    while (polkitHelper.state() != QProcess::NotRunning) {
+        if (polkitHelper.canReadLine()) {
+            QString output = polkitHelper.readLine();
+            if (output == "ready\n")
+                break;
+        }
+        qint64 waitTime = endTime - QDateTime::currentMSecsSinceEpoch();
+        if (waitTime <= 0) {
+            polkitHelper.kill();
+            return false;
+        }
+        QCoreApplication::processEvents(QEventLoop::AllEvents, waitTime);
     }
 
     bool success = setPasswordMode(type);
 
-    polkitHelper.waitForFinished();
+    while (polkitHelper.state() != QProcess::NotRunning) {
+        qint64 waitTime = endTime - QDateTime::currentMSecsSinceEpoch();
+        if (waitTime <= 0) {
+            polkitHelper.kill();
+            return false;
+        }
+        QCoreApplication::processEvents(QEventLoop::AllEvents, waitTime);
+    }
 
     return success;
 }
@@ -337,30 +353,6 @@
     }
 }
 
-bool SecurityPrivacy::trySetSecurity(SecurityType type)
-{
-    if (m_user == NULL || !act_user_is_loaded(m_user))
-        return false;
-
-    // We only support setting swipe without more information
-    if (type != SecurityPrivacy::Swipe)
-        return false;
-
-    SecurityType oldType = getSecurityType();
-    if (type == oldType)
-        return true; // nothing to do
-
-    if (!setDisplayHint(type))
-        return false;
-
-    if (!setPasswordMode(type)) {
-        setDisplayHint(oldType);
-        return false;
-    }
-
-    return true;
-}
-
 QString SecurityPrivacy::setSecurity(QString oldValue, QString value, SecurityType type)
 {
     if (m_user == nullptr || !act_user_is_loaded(m_user))

=== modified file 'plugins/security-privacy/securityprivacy.h'
--- plugins/security-privacy/securityprivacy.h	2014-10-16 13:43:16 +0000
+++ plugins/security-privacy/securityprivacy.h	2015-01-26 20:32:19 +0000
@@ -93,7 +93,6 @@
 
     // Returns error text, if an error occurred
     Q_INVOKABLE QString setSecurity(QString oldValue, QString value, SecurityType type);
-    Q_INVOKABLE bool trySetSecurity(SecurityType type);
 
 public Q_SLOTS:
     void slotChanged(QString, QString);

=== modified file 'src/SystemSettings/ItemPage.qml'
--- src/SystemSettings/ItemPage.qml	2013-07-30 14:09:08 +0000
+++ src/SystemSettings/ItemPage.qml	2015-01-26 20:32:19 +0000
@@ -19,7 +19,7 @@
  */
 
 import QtQuick 2.0
-import Ubuntu.Components 0.1
+import Ubuntu.Components 1.1
 
 Page {
     id: root