Merge lp:~pete-woods/unity-notifications/clients-own-their-notifications into lp:unity-notifications

[Pete Woods]

Notifications are now owned by the client that opened them

Notifications are now owned by the client that opened them

[Charles Kerr (charlesk)]

Looks great!

review: Approve

[lp:~pete-woods/unity-notifications/clients-own-their-notifications]

232. By Pete Woods on 2015-06-23

Fix parallel build issues

234. By Pete Woods on 2015-06-23

Actually pass in notification server

235. By Pete Woods on 2015-06-23

Notifications are now owned by the client that opened them

236. By Pete Woods on 2015-06-23

Handle local notifications

=== modified file 'include/Notification.h'
--- include/Notification.h	2014-07-18 14:21:41 +0000
+++ include/Notification.h	2015-06-23 15:48:50 +0000
@@ -109,6 +109,8 @@
     void setType(Type type);
     ActionModel* getActions() const;
     void setActions(const QStringList &actions);
+    QString getClientId() const;
+    void setClientId(const QString& clientId);
     void detachFromServer();
 
     QVariantMap getHints() const;

=== modified file 'include/NotificationServer.h'
--- include/NotificationServer.h	2015-06-23 15:48:50 +0000
+++ include/NotificationServer.h	2015-06-23 15:48:50 +0000
@@ -77,6 +77,7 @@
 
 private:
     void incrementCounter();
+    QString messageSender();
 
     QSharedPointer<Notification> buildNotification(NotificationID id, const QVariantMap &hints);
     NotificationModel &model;

=== modified file 'src/Notification.cpp'
--- src/Notification.cpp	2014-09-22 12:43:29 +0000
+++ src/Notification.cpp	2015-06-23 15:48:50 +0000
@@ -39,6 +39,7 @@
     ActionModel* actionsModel;
     QVariantMap hints;
     int displayTime;
+    QString clientId;
 };
 
 /*
@@ -205,6 +206,14 @@
     }
 }
 
+QString Notification::getClientId() const {
+    return p->clientId;
+}
+
+void Notification::setClientId(const QString& clientId) {
+    p->clientId = clientId;
+}
+
 void Notification::detachFromServer() {
     /*
      * FIXME. This function should not exist. In fact

=== modified file 'src/NotificationServer.cpp'
--- src/NotificationServer.cpp	2015-06-23 15:48:50 +0000
+++ src/NotificationServer.cpp	2015-06-23 15:48:50 +0000
@@ -26,6 +26,13 @@
 #include <QDBusConnectionInterface>
 #include <QSharedPointer>
 
+static const char* LOCAL_OWNER = "local";
+
+static bool isAuthorised(const QString& clientId, QSharedPointer<Notification> notification)
+{
+    return (clientId == LOCAL_OWNER) || (notification->getClientId() == clientId);
+}
+
 NotificationServer::NotificationServer(const QDBusConnection& connection, NotificationModel &m, QObject *parent) :
     QObject(parent), model(m), idCounter(0), m_connection(connection) {
 
@@ -146,6 +153,14 @@
         idCounter = 1;
 }
 
+QString NotificationServer::messageSender() {
+    QString sender(LOCAL_OWNER);
+        if (calledFromDBus()) {
+                sender = message().service();
+        }
+        return sender;
+}
+
 unsigned int NotificationServer::Notify(const QString &app_name, uint replaces_id,
                            const QString &app_icon, const QString &summary,
                            const QString &body, const QStringList &actions,
@@ -157,12 +172,22 @@
     int currentId = 0;
 
     bool newNotification = true;
+    QString clientId = messageSender();
 
     QSharedPointer<Notification> notification;
     if (replaces_id != 0) {
         if (model.hasNotification(replaces_id)) {
             newNotification = false;
             notification = model.getNotification(replaces_id);
+            if (!isAuthorised(clientId, notification)) {
+                auto message =
+                        QString::fromUtf8(
+                                "Client '%1' tried to update notification %2, which it does not own.").arg(
+                                clientId).arg(replaces_id);
+                qWarning() << message;
+                sendErrorReply(QDBusError::InvalidArgs, message);
+                return FAILURE;
+            }
         } else {
             notification = buildNotification(replaces_id, hints);
         }
@@ -240,7 +265,9 @@
     QVariant value = hints[VALUE_HINT];
     notification->setValue(value.toInt());
 
-    if(newNotification) {
+    notification->setClientId(clientId);
+
+    if (newNotification) {
         model.insertNotification(notification);
     } else {
         model.notificationUpdated(currentId);
@@ -249,8 +276,22 @@
 }
 
 void NotificationServer::CloseNotification (unsigned int id) {
-    Q_EMIT NotificationClosed(id, 1);
-    model.removeNotification(id);
+    if (model.hasNotification(id)) {
+        auto clientId = messageSender();
+        auto notification = model.getNotification(id);
+        if (!isAuthorised(clientId, notification))
+        {
+            auto message =
+                    QString::fromUtf8(
+                            "Client '%1' tried to close notification %2, which it does not own.").arg(
+                            clientId).arg(id);
+            qWarning() << message;
+            sendErrorReply(QDBusError::InvalidArgs, message);
+            return;
+        }
+        Q_EMIT NotificationClosed(id, 1);
+        model.removeNotification(id);
+    }
 }
 
 QString NotificationServer::GetServerInformation (QString &vendor, QString &version, QString &specVersion) const {

=== modified file 'test/notificationservertest.cpp'
--- test/notificationservertest.cpp	2015-06-23 15:48:50 +0000
+++ test/notificationservertest.cpp	2015-06-23 15:48:50 +0000
@@ -16,6 +16,10 @@
 
     QSharedPointer<OrgFreedesktopNotificationsInterface> notificationsInterface;
 
+    QSharedPointer<QDBusConnection> secondConnection;
+
+    QSharedPointer<OrgFreedesktopNotificationsInterface> notificationsInterfaceSecond;
+
 private Q_SLOTS:
 
 void init() {
@@ -31,20 +35,40 @@
                                             QStringList{"--no-gui"})));
     dbus->startServices();
 
+    // Create a connection to the server
     notificationsInterface.reset(
                 new OrgFreedesktopNotificationsInterface(DBUS_SERVICE_NAME,
                                                          DBUS_PATH,
                                                          dbus->sessionConnection()));
+
+    secondConnection.reset(
+            new QDBusConnection(
+                    QDBusConnection::connectToBus(
+                            dbus->sessionBus(),
+                            dbus->sessionBus() + "_second")));
+
+    // Create a second connection to the server over a different dbus connection
+    notificationsInterfaceSecond.reset(
+                new OrgFreedesktopNotificationsInterface(
+                        DBUS_SERVICE_NAME,
+                        DBUS_PATH,
+                        *secondConnection));
 }
 
 void cleanup() {
+    notificationsInterfaceSecond.reset();
+
+    secondConnection.reset();
+
     notificationsInterface.reset();
 
     dbus.reset();
 }
 
-int notify(const QString& name, int replacesId = 0) {
-    auto reply = notificationsInterface->Notify("my app", replacesId, "icon " + name,
+int _notify(
+            QSharedPointer<OrgFreedesktopNotificationsInterface> interface,
+            const QString& name, int replacesId = 0) {
+    auto reply = interface->Notify("my app", replacesId, "icon " + name,
                                                    "summary " + name,
                                                    "body " + name, QStringList(),
                                                    QVariantMap(), 0);
@@ -55,6 +79,14 @@
     return reply;
 }
 
+int notifyFirst(const QString& name, int replacesId = 0) {
+    return _notify(notificationsInterface, name, replacesId);
+}
+
+int notifySecond(const QString& name, int replacesId = 0, bool secondInterface = false) {
+    return _notify(notificationsInterfaceSecond, name, replacesId);
+}
+
 void expectNotification(const NotificationDataList& notifications, int index, int id, const QString& name) {
     QCOMPARE(
         notifications.at(index),
@@ -68,11 +100,27 @@
         );
 }
 
+void close(QSharedPointer<OrgFreedesktopNotificationsInterface> interface, int id) {
+    auto reply = interface->CloseNotification(id);
+    reply.waitForFinished();
+    if (reply.isError()) {
+        throw std::domain_error(reply.error().message().toStdString());
+    }
+}
+
+void closeFirst(int id) {
+    close(notificationsInterface, id);
+}
+
+void closeSecond(int id) {
+    close(notificationsInterfaceSecond, id);
+}
+
 void testNotify() {
-    uint id1 = notify("1");
+    uint id1 = notifyFirst("1");
     QCOMPARE(id1, uint(1));
 
-    uint id2 = notify("2");
+    uint id2 = notifyFirst("2");
     QCOMPARE(id2, uint(2));
 
     NotificationDataList notifications = notificationsInterface->GetNotifications("my app");
@@ -83,10 +131,10 @@
 }
 
 void testClose() {
-    uint id1 = notify("1");
+    uint id1 = notifyFirst("1");
     QCOMPARE(id1, uint(1));
 
-    uint id2 = notify("2");
+    uint id2 = notifyFirst("2");
     QCOMPARE(id2, uint(2));
 
     {
@@ -97,7 +145,7 @@
         expectNotification(notifications, 1, id2, "2");
     }
 
-    notificationsInterface->CloseNotification(id1).waitForFinished();
+    closeFirst(id1);
 
     {
         NotificationDataList notifications = notificationsInterface->GetNotifications("my app");
@@ -108,7 +156,7 @@
 }
 
 void testNotifyCloseNotify() {
-    uint id1 = notify("1");
+    uint id1 = notifyFirst("1");
     QCOMPARE(id1, uint(1));
 
     {
@@ -118,14 +166,14 @@
         expectNotification(notifications, 0, id1, "1");
     }
 
-    notificationsInterface->CloseNotification(id1).waitForFinished();
+    closeFirst(id1);
 
     {
         NotificationDataList notifications = notificationsInterface->GetNotifications("my app");
         QCOMPARE(notifications.size(), 0);
     }
 
-    id1 = notify("1", id1);
+    id1 = notifyFirst("1", id1);
     QCOMPARE(id1, uint(1));
 
     {
@@ -137,22 +185,22 @@
 }
 
 void testNotifyCounterIncrement() {
-    uint id1 = notify("1", 1);
+    uint id1 = notifyFirst("1", 1);
     QCOMPARE(id1, uint(1));
 
-    uint id2 = notify("2", 2);
+    uint id2 = notifyFirst("2", 2);
     QCOMPARE(id2, uint(2));
 
-    uint id4 = notify("4", 4);
+    uint id4 = notifyFirst("4", 4);
     QCOMPARE(id4, uint(4));
 
-    uint id5 = notify("5", 5);
+    uint id5 = notifyFirst("5", 5);
     QCOMPARE(id5, uint(5));
 
-    uint id3 = notify("3");
+    uint id3 = notifyFirst("3");
     QCOMPARE(id3, uint(3));
 
-    uint id6 = notify("6");
+    uint id6 = notifyFirst("6");
     QCOMPARE(id6, uint(6));
 
     {
@@ -168,6 +216,18 @@
     }
 }
 
+void testNotifyTwoClientsOneNaughty() {
+    // Create notification from one client
+    uint id1 = notifySecond("1");
+    QCOMPARE(id1, uint(1));
+
+    // Try and update notification from another client
+    QVERIFY_EXCEPTION_THROWN(notifyFirst("2", id1), std::domain_error);
+
+    // Try and close notification from another client
+    QVERIFY_EXCEPTION_THROWN(closeFirst(id1), std::domain_error);
+}
+
 void testGetCapabilities() {
     QStringList capabilities = notificationsInterface->GetCapabilities();
     QStringList expected {"actions",