Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pass plugin #822

Closed

Conversation

MichaelAquilina
Copy link
Contributor

@MichaelAquilina MichaelAquilina commented Jan 12, 2021

I've started working on a plugin which integrates with password store and thought it would be best to open this PR early to get some feedback quickly to make sure:

  • it would even be considered for merging
  • is using the right conventions and best practices considered by the team
  • is overall in the right direction

The plugin populates the shell launcher with the available user passwords and on submission will copy the password into the users clipboard (as password store usually does)

I've based most of the work from some of your existing plugins along with another vala plugin I originally wrote for (the now defunct) synapse project here: https://github.com/MichaelAquilina/synapse-project/blob/master/src/plugins/pass-plugin.vala

Keep in mind that its still work in progress and will require quite a bit of cleanup before its actually merged in (there are still some debug log messages lying around for example)

@MichaelAquilina MichaelAquilina force-pushed the feat/pass_plugin branch 2 times, most recently from 8e76090 to a0e99f6 Compare January 12, 2021 17:51
@mmstick
Copy link
Member

mmstick commented Jan 12, 2021

If there are any features you need, feel free to make suggestions on ways to improve our plugin API

@MichaelAquilina MichaelAquilina changed the title (WIP) Implement pass plugin Add pass plugin Jan 13, 2021
@MichaelAquilina
Copy link
Contributor Author

Hi @mmstick I think the plugin is now ready to review for the most part :)

Here's some things I noted would be useful while working on this:

The ability for a plugin to remain persistent

Right now it seems like the plugin is only called after the user typed something and is closed immediately after the launcher is closed. This means that expensive operations that ideally only need to be performed once need to be run multiple times (in my case this was enumerating the password files).

The ability for users to configure plugin trigger in shell prefernces

Right now the plugin is the thing that determines what triggers it running. For example the calc plugin runs with =. In this plugin's case I have the personal preference that this would always be run (i.e. no prefix), however I can imagine this not being something that every user wants.

Some kind of test system for plugins

It probably isnt too hard to make an extra effort on my part and add some form of tests. However it would be good to provide an easy to use pattern/framework for writing behavioral and functional tests. Right now I've had to resort to manual testing after every change I make which can be error prone especially over a longer period of time.

Other than that, it's been a pretty seemless experience thanks!

@mmstick
Copy link
Member

mmstick commented Jan 13, 2021

This looks good, but I'm not sure about including it in Pop by default. I would be willing to make a running list of plugins for Pop Shell if you make a repository for this. Then you wouldn't be forced to use JavaScript for your plugin, and we wouldn't have to include the pass application with Pop by default.

@MichaelAquilina
Copy link
Contributor Author

@mmstick that sounds reasonable to me. I wouldnt expect this plugin to be enabled by default since I assume the majority of users would not be using password store (and as a result we wouldnt want to the additional needless overhead of running this each time).

I guess it would be helpful to document on the README how to install external plugins in that case though? I'm assuming it involves copying the plugin to the correct location where pop os shell is installed?

As an aside for the future, it might be helpful to have a way to enable/disable and configure plugins for the launcher in the future. Although I assume and appreciate of course that this is a large amount of work!

@varac
Copy link
Contributor

varac commented Jan 15, 2021

Nice, looking forward to it! Will this work with gopass as well ?

@MichaelAquilina
Copy link
Contributor Author

Glad to hear @varac. This is the first time I've seen gopass before - but it looks quite similar to standard pass so I suspect it would not be hard to add support for it and/or creating a similar plugin for it.

class App {
constructor() {
let home_dir = GLib.get_home_dir();
let password_store = Gio.File.new_for_path(`${home_dir}/.password-store`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this first try to read PASSWORD_STORE_DIR environment variable and fallback to this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest - I was not aware this environment variable option existed. Do you have any links to documentation for this? I couldnt find it on https://www.passwordstore.org/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see it here for example https://wiki.archlinux.org/index.php/Pass#Advanced_usage or you can look at man pass and then search for "ENVIRONMENT VARIABLES" there is a list of all supported options (the first one being the store dir.

description: null,
icon: 'dialog-password',
})
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing manual iteration over a collection of items, it might be better to let the compiler figure it out :)
Also, this way, you get both the item and its index for free, as they are the 1st and 2nd args in Array.prototype.forEach(). You can also name them however you want, including ways that can take advantage of object shorthand notation for a little bit of code golf

this.passwords.forEach((name, id) => {
            this.selections.push({
                    id,
                    name,
                    description: null,
                    icon: 'dialog-password'
            })
})

@hverlin
Copy link

hverlin commented Aug 17, 2021

(Sorry, adding a comment here, since I have noticed that a discussion started around plugins' feedback.)

I have been using raycast on macOS, and they have a large collection of scripts:
https://github.com/raycast/script-commands

I would be quite interested in re-using/adapting some of them (e.g. to get Spotify, clipboard, etc.)

What's the process for testing plugins?
I have seen that I can add a script to /usr/lib/pop-shell/scripts and then reload gnome with alt+F2 and then r.
Is it the recommended way to test them?

Thanks for the great work! Seems that this launcher has a lot of potential.

@mmstick
Copy link
Member

mmstick commented Aug 17, 2021

The launcher was rewritten in Rust, so this will be closed. https://github.com/pop-os/launcher

@mmstick mmstick closed this Aug 17, 2021
@MichaelAquilina MichaelAquilina deleted the feat/pass_plugin branch August 18, 2021 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants