Merge lp:~michaelaquilina/synapse-project/pass-plugin into lp:synapse-project

[Michael Aquilina]

Add pass (http://www.passwordstore.org/) plugin to synapse. Allows the user to place the contents of a password in their store in the clipboard as an action. Matching is simply performed based on the name of the password.

See this launchpad question for more detail: https://answers.launchpad.net/synapse-project/+question/287038

[Michael Aquilina (michaelaquilina)]

Any comments on this? Is there an active maintainer to merge this in?

[Rico Tzschichholz (ricotz)]

It looks straight forward.

There are bunch of indentation problems, codestyle issues with missing space before "(" and some TODOs.

Better hold the GFile reference to ".password_store" instead of creating it every time.

review: Needs Fixing

[Michael Aquilina (michaelaquilina)]

Is there a style guide I could follow? I mainly code in python nowadays so I stuck to the conventions I'm used to.

[Michael Aquilina (michaelaquilina)]

Looking at the diff, I now see what you're referring to in terms of indents. It looks fine in my editor funnily enough (atom). So I'm not sure what the issue is.

[Michael Aquilina (michaelaquilina)]

The indentation issues should be solved. The issue was that piece of code was copied from somewhere to start off with and modified. It contained hard tabs which my editor tried to maintain. I changed it so that it uses "soft" tabs instead which seems to have worked.

[Rico Tzschichholz (ricotz)]

Still a bunch of missing spaces before "(" like "printf(this.title)" >> "printf (this.title)"

Avoid removing a line from Makefile.am.

Review your strings. Use lower-cases as usual in english.
"Copy encrypted GPG password to clipboard" < this is OK.
"Copied %s Password to Clipboard" < this is not OK.

Not sure what you mean with "soft" tabs, but the synapse source doesn't use tabs only spaces.

Redo you branch in the end to make it consist of only one commit again.

review: Needs Fixing

[Michael Aquilina (michaelaquilina)]

That extra line being removed is completely extra (it's automatically removed in all modern editors). I feel like that should be left there unless you see a strong reason to keep it? Most people contributing will experience it being removed automatically otherwise - which is minor but still annoying.

As for squashing the commits, I am completely aware of how to do this in git but I cant find any reference for how to do it in bzr. Is there anything you could point me at?

[Rico Tzschichholz (ricotz)]

Still some "(" :\

Better call list_passwords() when it would provide actual changes. E.g. on start up and on changes while using a GFileMonitor on the folder.

Regarding "standard::*" only retrieve the information you need, so standard::name and stardard::type.

You will need to use bzr uncommit multiple times and bzr commit again afterwards.
I really dislike when unrelated code-portions are touched for no reason. So please keep the line.

review: Needs Fixing

[Michael Aquilina (michaelaquilina)]

* Passwords only listed on activation and on changed event in password directory
* Only 'name' 'type' and 'content-type' retrieved by enumerate_children
* Fixed more styling issues
* Reverted removal of extra line at the end of MakeFile.am

Ready for review!

[Michael Aquilina (michaelaquilina)]

Hi, is there anything I need to do/fix?

[Rico Tzschichholz (ricotz)]

No need for password_monitor and password_store to be public properties, private fields are sufficient.

file.resolve_relative_path (info.get_name ()); -> file.get_child (info.get_name ());

if the ordering of file-list is not important then GLib.List.prepend() is sufficient and faster.

Decide between "... password %s" or "... %s password".

review: Needs Fixing

[Michael Aquilina (michaelaquilina)]

Fixed according to your review comments

[Rico Tzschichholz (ricotz)]

No need for password_monitor and password_store to be *properties*, make them *fields*.

review: Needs Fixing

[Michael Aquilina (michaelaquilina)]

Sorry about that, didnt realise you also meant I should convert the properties to fields. Fixed.

[Michael Aquilina (michaelaquilina)]

I seem to have found a bug using FileMonitor, it (obviously now thinking about it) does not recursively track changes in children directories and for that reasons changes to for example "Web/SomeSite/ABC" will not be reflected in the code. Is there a standard approach for dealing with tracking of folders recursively or will I have to write this myself?

[Rico Tzschichholz (ricotz)]

Look into using http://valadoc.org/#!api=gio-2.0/GLib.File.monitor_directory

[Michael Aquilina (michaelaquilina)]

Fixed the file monitoring bug. Should be good for merging now.

[Michael Aquilina (michaelaquilina)]

@Rico is there anything that needs fixing?

[Rico Tzschichholz (ricotz)]

https://paste.debian.net/plain/405740

I really don't like the plain re-invocation of update_passwords() you updating all password that is fine, but recreating all monitors without stopping/destroying them is a not good.

Also the infinite/unrestricted recursion might bite you, so better limit it to a depth of 2 or 3.

And use explicitly use GLib.File.monitor_directory() as suggested. While not caring about updating existing data and just recreating things from the bottom make use of the monitor'S rate-limit.

review: Needs Fixing

[Michael Aquilina (michaelaquilina)]

Limiting the recursion depth to something as low as 2 or 3 wont really work since its common to have passwords organised at that depth. Why would recursing directories cause a problem though? You cant really have an infinite directory structure since following symbolic links is disabled.

I'll add disabling of previous file monitors if that's the main issue.

[Michael Aquilina (michaelaquilina)]

I've adding code to cancel previous FileMonitors when they're no longer being used and the other requests you've made. I tried updating to code to avoid re-generating the monitors each time a change is detected. I attempted this by adding/removing a FileMoinitor on a changed event on a directory but this immediately became overly complicated.

If we believe using recursive FileMonitors is the best approach, then I feel like this is as good as it can get unless you know of an alternative?

[Rico Tzschichholz (ricotz)]

Alright one more clean up and it should be fine.

https://paste.debian.net/plain/411306

[lp:~michaelaquilina/synapse-project/pass-plugin]

637. By Michael Aquilina on 2016-03-03

Add pass plugin

[Michael Aquilina (michaelaquilina)]

Updated according to your diff

[Michael Aquilina (michaelaquilina)]

@Rico, does everything look good?

[Michael Aquilina (michaelaquilina)]

Sorry to annoy you @Rico, but any chance you could give this a look some time?

[Rico Tzschichholz (ricotz)]

review: Approve

=== modified file 'po/POTFILES.in'
--- po/POTFILES.in	2016-02-26 16:57:32 +0000
+++ po/POTFILES.in	2016-03-03 11:50:29 +0000
@@ -33,6 +33,7 @@
 src/plugins/launchpad-plugin.vala
 src/plugins/locate-plugin.vala
 src/plugins/opensearch.vala
+src/plugins/pass-plugin.vala
 src/plugins/pastebin-plugin.vala
 src/plugins/pidgin-plugin.vala
 src/plugins/rhythmbox-plugin.vala

=== modified file 'po/POTFILES.skip'
--- po/POTFILES.skip	2016-02-26 16:57:32 +0000
+++ po/POTFILES.skip	2016-03-03 11:50:29 +0000
@@ -34,6 +34,7 @@
 src/plugins/launchpad-plugin.c
 src/plugins/locate-plugin.c
 src/plugins/opensearch.c
+src/plugins/pass-plugin.c
 src/plugins/pastebin-plugin.c
 src/plugins/pidgin-plugin.c
 src/plugins/rhythmbox-plugin.c

=== modified file 'src/plugins/Makefile.am'
--- src/plugins/Makefile.am	2016-02-26 16:57:32 +0000
+++ src/plugins/Makefile.am	2016-03-03 11:50:29 +0000
@@ -45,6 +45,7 @@
 	launchpad-plugin.vala \
 	locate-plugin.vala \
 	opensearch.vala \
+	pass-plugin.vala \
 	pastebin-plugin.vala \
 	pidgin-plugin.vala \
 	rhythmbox-plugin.vala \

=== added file 'src/plugins/pass-plugin.vala'
--- src/plugins/pass-plugin.vala	1970-01-01 00:00:00 +0000
+++ src/plugins/pass-plugin.vala	2016-03-03 11:50:29 +0000
@@ -0,0 +1,228 @@
+/*
+ * Copyright (C) 2015 Michael Aquilina <[e-mail]>
+
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY 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, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301  USA.
+ *
+ * Authored by Michael Aquilina <[e-mail]>
+ *
+ */
+
+namespace Synapse
+{
+  public class PassPlugin : Object, Activatable, ItemProvider
+  {
+    private class PassMatch : ActionMatch
+    {
+      public PassMatch (string password_name)
+      {
+        Object (title: password_name,
+                description: _("Copy decrypted PGP password to clipboard"),
+                has_thumbnail: false, icon_name: "dialog-password");
+      }
+
+      public override void do_action ()
+      {
+        Pid child_pid;
+        int standard_output;
+        int standard_error;
+
+        try
+        {
+          Process.spawn_async_with_pipes (null,
+              {"pass", "-c", this.title},
+              null, SpawnFlags.SEARCH_PATH | SpawnFlags.DO_NOT_REAP_CHILD,
+              null, out child_pid,
+              null, out standard_output, out standard_error
+          );
+          ChildWatch.add (child_pid, (pid, status) => {
+            Process.close_pid (pid);
+
+            try
+            {
+              string message, icon_name;
+              if (status == 0) {
+                message = _("Copied %s password to clipboard").printf (this.title);
+                icon_name = "dialog-password";
+              } else {
+                message = _("Unable to decrypt %s password").printf (this.title);
+                icon_name = "dialog-error";
+              }
+
+              var notification = (Notify.Notification) Object.new (
+                  typeof (Notify.Notification),
+                  summary: _("Synapse - Pass"),
+                  body: message,
+                  icon_name: icon_name,
+                  null
+              );
+              notification.show ();
+            }
+            catch (Error err) {
+              warning ("%s", err.message);
+            }
+          });
+        }
+        catch (SpawnError err) {
+          warning ("%s", err.message);
+        }
+      }
+    }
+
+    public bool enabled { get; set; default = true; }
+
+    private File password_store;
+    private List<string> passwords;
+    private List<FileMonitor> monitors;
+
+    public void activate ()
+    {
+      password_store = File.new_for_path (
+        "%s/.password-store".printf (Environment.get_home_dir ())
+      );
+      update_passwords ();
+    }
+
+    public void deactivate ()
+    {
+    }
+
+    static void register_plugin ()
+    {
+      PluginRegistry.get_default ().register_plugin (
+        typeof (PassPlugin),
+        _("Pass Integration"),
+        _("Quickly place passwords from your password store in the clipboard."),
+        "dialog-password",
+        register_plugin,
+        Environment.find_program_in_path ("pass") != null,
+        _("pass is not installed")
+      );
+    }
+
+    static construct
+    {
+      register_plugin ();
+    }
+
+    public bool handles_query (Query query)
+    {
+      return (QueryFlags.ACTIONS in query.query_type);
+    }
+
+    private void update_passwords () {
+      foreach (unowned FileMonitor monitor in monitors) {
+        monitor.cancel ();
+      }
+      monitors = null;
+
+      try {
+        monitors = activate_monitors (password_store);
+      } catch (Error err) {
+        warning ("Unable to monitor password directory: %s", err.message);
+      }
+      try {
+        passwords = list_passwords (password_store, password_store);
+      } catch (Error err) {
+        warning ("Unable to list passwords: %s", err.message);
+      }
+    }
+
+    private List<FileMonitor> activate_monitors (File directory) throws Error {
+      List<FileMonitor> result = new List<FileMonitor> ();
+
+      FileEnumerator enumerator = directory.enumerate_children (
+        FileAttribute.STANDARD_NAME + "," +
+        FileAttribute.STANDARD_TYPE + "," +
+        FileAttribute.STANDARD_IS_HIDDEN,
+        FileQueryInfoFlags.NOFOLLOW_SYMLINKS,
+        null
+      );
+
+      FileMonitor monitor = directory.monitor_directory (FileMonitorFlags.NONE, null);
+      monitor.set_rate_limit (500);
+      monitor.changed.connect ((src, dest, event) => {
+        message ("Detected a change (%s) in password store. Reloading", event.to_string ());
+        update_passwords ();
+      });
+      result.append (monitor);
+
+      FileInfo? info = null;
+      while ((info = enumerator.next_file (null)) != null) {
+        if (info.get_is_hidden ()) continue;
+
+        var target_name = info.get_name ();
+        File target_file = directory.get_child (target_name);
+        if (info.get_file_type () == FileType.DIRECTORY) {
+          result.concat (activate_monitors (target_file));
+        }
+      }
+
+      return result;
+    }
+
+    private List<string> list_passwords (File root, File directory) throws Error {
+      List<string> result = new List<string> ();
+
+      FileEnumerator enumerator = directory.enumerate_children (
+        FileAttribute.STANDARD_NAME + "," +
+        FileAttribute.STANDARD_TYPE + "," +
+        FileAttribute.STANDARD_CONTENT_TYPE,
+        FileQueryInfoFlags.NOFOLLOW_SYMLINKS,
+        null
+      );
+
+      FileInfo? info = null;
+      while ((info = enumerator.next_file (null)) != null) {
+        File target_file = directory.get_child (info.get_name ());
+        if (info.get_file_type () == FileType.DIRECTORY) {
+          result.concat (list_passwords (root, target_file));
+        }
+        else if (info.get_content_type () == "application/pgp-encrypted") {
+          var path = root.get_relative_path (target_file);
+          result.prepend (path.replace (".gpg", ""));
+        }
+      }
+      return result;
+    }
+
+    public async ResultSet? search (Query query) throws SearchError
+    {
+      var matchers = Query.get_matchers_for_query (
+        query.query_string, 0,
+        RegexCompileFlags.OPTIMIZE | RegexCompileFlags.CASELESS
+      );
+
+      var results = new ResultSet ();
+      foreach (unowned string password in passwords)
+      {
+        foreach (var matcher in matchers)
+        {
+          if (matcher.key.match (password)) {
+            results.add (new PassMatch (password), MatchScore.GOOD);
+            break;
+          }
+        }
+      }
+
+      // make sure this method is called before returning any results
+      query.check_cancellable ();
+      if (results.size > 0) {
+        return results;
+      } else {
+        return null;
+      }
+    }
+  }
+}

=== modified file 'src/ui/synapse-main.vala'
--- src/ui/synapse-main.vala	2016-02-27 15:19:21 +0000
+++ src/ui/synapse-main.vala	2016-03-03 11:50:29 +0000
@@ -174,6 +174,7 @@
         typeof (ChromiumPlugin),
         typeof (FileOpPlugin),
         typeof (PidginPlugin),
+        typeof (PassPlugin),
         typeof (ChatActions),
         typeof (ZealPlugin),
 #if HAVE_ZEITGEIST