Merge lp:~attente/ubuntu-system-settings/session-settings-service into lp:ubuntu-system-settings

[William Hua]

Add a session-settings-service.

Providing confined apps with direct access to AccountsService is too dangerous. There's too much sensitive information such as LoginHistory, PasswordHint, etc. Also, because object paths for users are dynamic (e.g. /org/freedesktop/Accounts/User1000), we cannot write AppArmor policy to restrict calls for other users' data.

We propose a service that allows confined apps to read-only access for a limited subset of non-sensitive AccountsService data (language and locale). The service lives on the session bus and provides explicit getters and change notification signals. The service does not provide D-Bus properties since that requires AppArmor policy that opens up the org.freedesktop.DBus.Properties interface.

Compare the following policies. The first is what's needed for direct AccountsService access, the second is what's needed using the proposed session service:

dbus bus=system
     path=/org/freedesktop/Accounts
     interface=org.freedesktop.DBus.Properties
     member=GetAll,
dbus bus=system
     path=/org/freedesktop/Accounts
     interface=org.freedesktop.Accounts
     member=ListCachedUsers,
dbus bus=system
     path=/org/freedesktop/Accounts
     interface=org.freedesktop.Accounts
     member=FindUserByName,
dbus bus=system
     interface=org.freedesktop.DBus.Properties
     member=GetAll,
dbus bus=system
     interface=org.freedesktop.Accounts.User
     member=Changed,

vs.

dbus (send)
     bus=session
     path=/com/canonical/UbuntuSystemSettings
     interface=com.ubuntu.Settings
     member=GetLanguage,
dbus (send)
     bus=session
     path=/com/canonical/UbuntuSystemSettings
     interface=com.ubuntu.Settings
     member=GetLocale,
dbus (receive)
     bus=session
     path=/com/canonical/UbuntuSystemSettings
     interface=com.ubuntu.Settings
     member=LanguageChanged,
dbus (receive)
     bus=session
     path=/com/canonical/UbuntuSystemSettings
     interface=com.ubuntu.Settings
     member=LocaleChanged,

Add a session-settings-service.

Providing confined apps with direct access to AccountsService is too dangerous. There's too much sensitive information such as LoginHistory, PasswordHint, etc. Also, because object paths for users are dynamic (e.g. /org/freedesktop/Accounts/User1000), we cannot write AppArmor policy to restrict calls for other users' data.

We propose a service that allows confined apps to read-only access for a limited subset of non-sensitive AccountsService data (language and locale). The service lives on the session bus and provides explicit getters and change notification signals. The service does not provide D-Bus properties since that requires AppArmor policy that opens up the org.freedesktop.DBus.Properties interface.

Compare the following policies. The first is what's needed for direct AccountsService access, the second is what's needed using the proposed session service:

dbus bus=system
     path=/org/freedesktop/Accounts
     interface=org.freedesktop.DBus.Properties
     member=GetAll,
dbus bus=system
     path=/org/freedesktop/Accounts
     interface=org.freedesktop.Accounts
     member=ListCachedUsers,
dbus bus=system
     path=/org/freedesktop/Accounts
     interface=org.freedesktop.Accounts
     member=FindUserByName,
dbus bus=system
     interface=org.freedesktop.DBus.Properties
     member=GetAll,
dbus bus=system
     interface=org.freedesktop.Accounts.User
     member=Changed,

vs.

dbus (send)
     bus=session
     path=/com/canonical/UbuntuSystemSettings
     interface=com.ubuntu.Settings
     member=GetLanguage,
dbus (send)
     bus=session
     path=/com/canonical/UbuntuSystemSettings
     interface=com.ubuntu.Settings
     member=GetLocale,
dbus (receive)
     bus=session
     path=/com/canonical/UbuntuSystemSettings
     interface=com.ubuntu.Settings
     member=LanguageChanged,
dbus (receive)
     bus=session
     path=/com/canonical/UbuntuSystemSettings
     interface=com.ubuntu.Settings
     member=LocaleChanged,

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:815
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1053/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/2449/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/1989/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-amd64-ci/245/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-armhf-ci/245/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-i386-ci/245/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3685/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/2239/console

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

review: Needs Fixing (continuous-integration)

[lp:~attente/ubuntu-system-settings/session-settings-service]

816. By William Hua on 2014-07-24

Merge trunk.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:816
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1054/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/2458
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/1993
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-amd64-ci/246
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-armhf-ci/246
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-armhf-ci/246/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-i386-ci/246
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/2620
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3699
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3699/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10375
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/1666
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/2243
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/2243/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Thomas Voß (thomas-voss)]

First of all thanks for the changes. Two things, though:

  * Our general policy for any new code is to avoid Vala. C, C++/QML or Go is fine, please consider C++ as it is the implementation language of Ubuntu System Settings.

  * It would be quite helpful if you could provide reviewers with an arch document detailing the rationale behind your proposed changes. I appreciate your summary on the MP, but would like to have all stakeholders involved here. Exposing certain types of settings via a service is a feature requested across the board and we should make sure that we have requirements captured properly.

Thanks again for the changes, but I'm still disapproving for the reasons mentioned before.

review: Disapprove

Unmerged revisions

816. By William Hua on 2014-07-24

Merge trunk.

815. By William Hua on 2014-07-23

Update com.ubuntu.Settings interface.

814. By William Hua on 2014-07-23

Use explicit getters and change signals.

813. By William Hua on 2014-07-23

Rename system-settings-service to session-settings-service.

812. By William Hua on 2014-07-22

Use dash instead of underscore.

811. By William Hua on 2014-07-22

Capitalize property names.

810. By William Hua on 2014-07-22

Add missing build dependencies.

809. By William Hua on 2014-07-21

Add copyright header.

808. By William Hua on 2014-07-21

Remove Service header.

807. By William Hua on 2014-07-21

System settings service.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2014-06-19 13:32:47 +0000
+++ CMakeLists.txt	2014-07-24 17:28:32 +0000
@@ -56,9 +56,15 @@
 set(PLUGIN_PRIVATE_MODULE_DIR "${CMAKE_INSTALL_PREFIX}/${LIBDIR}/${PLUGIN_PRIVATE_MODULE_DIR_BASE}")
 set(SETTINGS_SHARE_DIR "${CMAKE_INSTALL_PREFIX}/${PLUGIN_MANIFEST_DIR_BASE}")
 
+set(SERVICE_INSTALL_DIR_BASE ubuntu-system-settings)
+set(SERVICE_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/${LIBDIR}/${SERVICE_INSTALL_DIR_BASE}")
+set(UPSTART_INSTALL_DIR_BASE share/upstart/sessions)
+set(UPSTART_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/${UPSTART_INSTALL_DIR_BASE}")
+
 SET(CMAKE_INSTALL_RPATH "${PLUGIN_MODULE_DIR}")
 
 add_subdirectory(po)
+add_subdirectory(data)
 add_subdirectory(schema)
 add_subdirectory(lib)
 include_directories(lib)

=== added directory 'data'
=== added file 'data/CMakeLists.txt'
--- data/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ data/CMakeLists.txt	2014-07-24 17:28:32 +0000
@@ -0,0 +1,2 @@
+configure_file(session-settings-service.conf.in session-settings-service.conf @ONLY)
+install(FILES ${CMAKE_CURRENT_BINARY_DIR}/session-settings-service.conf DESTINATION ${UPSTART_INSTALL_DIR})

=== added file 'data/session-settings-service.conf.in'
--- data/session-settings-service.conf.in	1970-01-01 00:00:00 +0000
+++ data/session-settings-service.conf.in	2014-07-24 17:28:32 +0000
@@ -0,0 +1,9 @@
+description "Session settings service"
+author "William Hua <[e-mail]>"
+
+start on started dbus
+stop on stopping dbus
+
+respawn
+
+exec @SERVICE_INSTALL_DIR@/session-settings-service

=== modified file 'debian/control'
--- debian/control	2014-07-21 14:50:32 +0000
+++ debian/control	2014-07-24 17:28:32 +0000
@@ -32,6 +32,9 @@
                pep8,
                python3-pep8,
                pyflakes,
+               dh-apparmor,
+               libgirepository1.0-dev,
+               valac,
 Standards-Version: 3.9.4
 Homepage: https://launchpad.net/ubuntu-system-settings
 # If you aren't a member of ~system-settings-touch but need to upload packaging

=== modified file 'debian/ubuntu-system-settings.install'
--- debian/ubuntu-system-settings.install	2014-07-17 16:25:10 +0000
+++ debian/ubuntu-system-settings.install	2014-07-24 17:28:32 +0000
@@ -4,4 +4,5 @@
 usr/share/glib-2.0
 usr/share/locale
 usr/share/ubuntu/settings/system
+usr/share/upstart/sessions/session-settings-service.conf
 usr/share/url-dispatcher

=== modified file 'lib/SystemSettings/CMakeLists.txt'
--- lib/SystemSettings/CMakeLists.txt	2013-09-30 11:11:20 +0000
+++ lib/SystemSettings/CMakeLists.txt	2014-07-24 17:28:32 +0000
@@ -1,4 +1,8 @@
-add_library(SystemSettings SHARED item-base.cpp item-base.h)
+include_directories(SYSTEM ${GIO_INCLUDE_DIRS})
+link_directories(${GIO_LIBRARY_DIRS})
+
+add_library(SystemSettings SHARED item-base.cpp item-base.h service.c)
+target_link_libraries(SystemSettings ${GIO_LIBRARIES})
 set_target_properties(SystemSettings PROPERTIES
 VERSION 1.0.0
 SOVERSION 1
@@ -7,9 +11,11 @@
 qt5_use_modules(SystemSettings Core Gui Quick Qml)
 
 install(TARGETS SystemSettings LIBRARY DESTINATION ${LIBDIR})
-install(FILES item-base.h ItemBase plugin-interface.h PluginInterface
+install(FILES item-base.h ItemBase plugin-interface.h PluginInterface ${CMAKE_CURRENT_BINARY_DIR}/service.h
 DESTINATION include/SystemSettings)
 
 set(SYSTEMSETTINGS_LIB SystemSettings)
 configure_file(SystemSettings.pc.in SystemSettings.pc @ONLY)
 install(FILES ${CMAKE_CURRENT_BINARY_DIR}/SystemSettings.pc DESTINATION ${LIBDIR}/pkgconfig)
+
+add_custom_command(OUTPUT service.h service.c COMMAND gdbus-codegen --interface-prefix com.ubuntu --generate-c-code service --c-namespace USS ${CMAKE_CURRENT_SOURCE_DIR}/com.ubuntu.Settings.xml DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/com.ubuntu.Settings.xml)

=== added file 'lib/SystemSettings/com.ubuntu.Settings.xml'
--- lib/SystemSettings/com.ubuntu.Settings.xml	1970-01-01 00:00:00 +0000
+++ lib/SystemSettings/com.ubuntu.Settings.xml	2014-07-24 17:28:32 +0000
@@ -0,0 +1,17 @@
+<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN" "http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd">
+<node>
+  <interface name="com.ubuntu.Settings">
+    <method name="GetLanguage">
+      <arg name="language" type="s" direction="out" />
+    </method>
+    <method name="GetLocale">
+      <arg name="locale" type="s" direction="out" />
+    </method>
+    <signal name="LanguageChanged">
+      <arg name="language" type="s" />
+    </signal>
+    <signal name="LocaleChanged">
+      <arg name="locale" type="s" />
+    </signal>
+  </interface>
+</node>

=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2014-02-13 15:27:47 +0000
+++ src/CMakeLists.txt	2014-07-24 17:28:32 +0000
@@ -6,6 +6,7 @@
 add_definitions(-DPLUGIN_MODULE_DIR="${PLUGIN_MODULE_DIR}")
 
 add_subdirectory(SystemSettings)
+add_subdirectory(service)
 
 set(USS_SOURCES
     debug.cpp

=== added directory 'src/service'
=== added file 'src/service/CMakeLists.txt'
--- src/service/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ src/service/CMakeLists.txt	2014-07-24 17:28:32 +0000
@@ -0,0 +1,10 @@
+include_directories(SYSTEM ${GIO_INCLUDE_DIRS} ${ACCOUNTSSERVICE_INCLUDE_DIRS})
+link_directories(${GIO_LIBRARY_DIRS} ${ACCOUNTSSERVICE_LIBRARY_DIRS})
+
+add_executable(service-bin server.c)
+set_target_properties(service-bin PROPERTIES OUTPUT_NAME session-settings-service)
+target_link_libraries(service-bin ${GIO_LIBRARIES} ${ACCOUNTSSERVICE_LIBRARIES})
+install(TARGETS service-bin RUNTIME DESTINATION ${SERVICE_INSTALL_DIR})
+
+add_custom_command(OUTPUT server.c COMMAND valac -C --enable-experimental-non-null --vapidir=. --pkg gio-2.0 --pkg posix --pkg accountsservice ${CMAKE_CURRENT_SOURCE_DIR}/server.vala DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/server.vala accountsservice.vapi)
+add_custom_command(OUTPUT accountsservice.vapi COMMAND vapigen --pkg gio-2.0 --library accountsservice /usr/share/gir-1.0/AccountsService-1.0.gir)

=== added file 'src/service/server.vala'
--- src/service/server.vala	1970-01-01 00:00:00 +0000
+++ src/service/server.vala	2014-07-24 17:28:32 +0000
@@ -0,0 +1,144 @@
+/*
+ * Copyright (C) 2014 Canonical Ltd.
+ *
+ * Contact: William Hua <[e-mail]>
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+[DBus (name = "com.ubuntu.Settings")]
+public class Server : Object {
+
+	private string? _language;
+	private string? _locale;
+
+	private DBusConnection _connection;
+
+	private Act.UserManager? _accountsservice_manager;
+	private ulong _accountsservice_manager_id;
+	private Act.User? _accountsservice_user;
+	private ulong _accountsservice_user_id[2];
+
+	public Server (DBusConnection connection) {
+		_connection = connection;
+
+		load_accountsservice_manager ();
+	}
+
+	~Server () {
+		if (_accountsservice_user != null) {
+			if (_accountsservice_user_id[1] > 0) {
+				((!) _accountsservice_user).disconnect (_accountsservice_user_id[1]);
+			}
+
+			if (_accountsservice_user_id[0] > 0) {
+				((!) _accountsservice_user).disconnect (_accountsservice_user_id[0]);
+			}
+		}
+
+		if (_accountsservice_manager != null && _accountsservice_manager_id > 0) {
+			((!) _accountsservice_manager).disconnect (_accountsservice_manager_id);
+		}
+	}
+
+	public string get_language () {
+		return _language != null ? (!) _language : "";
+	}
+
+	public string get_locale () {
+		return _locale != null ? (!) _locale : "";
+	}
+
+	public signal void language_changed (string language);
+	public signal void locale_changed (string locale);
+
+	private void set_language (string language) {
+		if (language != _language) {
+			_language = language;
+			language_changed (language);
+		}
+	}
+
+	private void set_locale (string locale) {
+		if (locale != _locale) {
+			_locale = locale;
+			locale_changed (locale);
+		}
+	}
+
+	private void load_accountsservice_manager () {
+		_accountsservice_manager = Act.UserManager.get_default ();
+
+		if (((!) _accountsservice_manager).is_loaded) {
+			load_accountsservice_user ((!) _accountsservice_manager);
+		} else {
+			_accountsservice_manager_id = ((!) _accountsservice_manager).notify["is-loaded"].connect (() => {
+			        if (((!) _accountsservice_manager).is_loaded) {
+			                ((!) _accountsservice_manager).disconnect (_accountsservice_manager_id);
+			                _accountsservice_manager_id = 0;
+			                load_accountsservice_user ((!) _accountsservice_manager);
+			        }
+			});
+		}
+	}
+
+	private void load_accountsservice_user (Act.UserManager manager) {
+		_accountsservice_user = manager.get_user (Environment.get_user_name ());
+
+		if (_accountsservice_user != null) {
+			if (((!) _accountsservice_user).is_loaded) {
+				watch_accountsservice_user ((!) _accountsservice_user);
+			} else {
+				_accountsservice_user_id[0] = ((!) _accountsservice_user).notify["is-loaded"].connect (() => {
+				        if (((!) _accountsservice_user).is_loaded) {
+				                ((!) _accountsservice_user).disconnect (_accountsservice_user_id[0]);
+				                _accountsservice_user_id[0] = 0;
+				                watch_accountsservice_user ((!) _accountsservice_user);
+				        }
+				});
+			}
+		}
+	}
+
+	private void watch_accountsservice_user (Act.User user) {
+		_accountsservice_user_id[0] = user.notify["language"].connect (() => { set_language (user.language); });
+		_accountsservice_user_id[1] = user.notify["formats-locale"].connect (() => { set_locale (user.formats_locale); });
+		_language = user.language;
+		_locale = user.formats_locale;
+	}
+}
+
+private static MainLoop loop;
+
+private static int main (string[] args) {
+	Bus.own_name (BusType.SESSION,
+	              "com.canonical.UbuntuSystemSettings",
+	              BusNameOwnerFlags.ALLOW_REPLACEMENT | BusNameOwnerFlags.REPLACE,
+	              (connection, name) => {
+	                      try {
+	                              connection.register_object ("/com/canonical/UbuntuSystemSettings", new Server (connection));
+	                      } catch (IOError error) {
+	                              warning ("Object registration failed: %s", error.message);
+	                      }
+	              },
+	              null,
+	              null);
+
+	loop = new MainLoop ();
+	Posix.sigaction_t action = { () => { loop.quit (); } };
+	Posix.sigaction (Posix.SIGINT, action, null);
+	Posix.sigaction (Posix.SIGTERM, action, null);
+	loop.run ();
+
+	return 0;
+}