Thanks for the feedback! I'm not sure exactly what you mean by a "callback" here: a specific implementation of how exactly to do the copy and paste (which the current code seems to *almost* support, in the form of "copy_cmd" and "paste_cmd", all that's missing is a means for changing them less intrusively than by adding more if cases in the code itself)? Or remiplementing the entire clip() function as a whole?
IIUC, the latter can already be achieved by providing a platform-specific implementation of clip()? The problem with reimplementation, though (and why I ultimately preferred to use hooks), is that there is non-trivial logic going on in the function, which might be lost by the reimplementations. For example, the sample snippet you provide above doesn't deal with the subtlety of avoiding a second call to 'pass -c' restoring -- after the timeout -- the value copied there by the first 'pass -c' (it took me a long time to realize that the current code is avoiding that using the pkill...). Although, perhaps for a platform-specific implementation it makes more sense to expect a high-quality implementation, than for a clipboard-manager-specific implementation -- assuming that there is a wider variety of clipboard managers than there are platforms... I appreciate, though, that hooks do not solve the problem you are trying to solve :/ On Sun, Jul 5, 2020 at 6:28 AM Allan Odgaard <[email protected]> wrote: > > On macOS the way for password managers to temporarily put sensitive data > on the clipboard is by tagging the data. > > Therefore a pre/post hook will not be a good solution for macOS, instead > it should be a callback that places the content on the clipboard. > > A simple interface would be a function that accepts $password and > $duration, the default implementation on macOS (using `pbcopy` and > `pbpaste`) would be something like this: > > function user_function { > password="$1" > duration="$2" > > old="$(pbpaste)" > printf "%s" "$password" | pbcopy > > sleep "$duration" > > if [[ "$(pbpaste)" == "$password" ]]; then > printf "%s" "$old" | pbcopy > fi > } > > The advantage of using pre/post hooks is that it does not become the job > of the callback to sleep nor check if content has changed. > > However, for the latter, a callback might do better than `pass` itself, > for example on macOS the clipboard has a change counter, so the callback > can check that to learn about changes. > > Furthermore for the `sleep`: Having the callback perform the sleep, we > avoid having to introduce state that is shared between the pre and post > hook, if we also want to improve restoring content (e.g. on macOS `pass` > will currently not restore non-text content or text with multiple > representations). > > So all in all, I think the interface should be a single function that > does all the work, instead of introducing hooks, as I don’t think the > complexity of implementing a callback is higher than writing hooks, and > a function is much less limited, e.g. on macOS we will be able both to > tag the content (for clipboard managers) and we will be able to do a > better job at restoring the old clipboard data, neither of which is > possible with the hooks. > > For the records though, in order to improve things on macOS, we will > need to write a compiled helper and use that instead of > `pbcopy`/`pbpaste`. > > > On 5 Jul 2020, at 8:20, Dov Feldstern wrote: > > > # HG changeset patch > > # User Dov Feldstern <[email protected]> > > # Date 1593908715 -10800 > > # Sun Jul 05 03:25:15 2020 +0300 > > # Node ID d4af3f470f3f04fb81bbcb079556ea56e8e43898 > > # Parent dcfa9a1c9c827206f899e7ba604f42366fbd60f3 > > RFC: hooks for clipboard manager integration > > > > I've been using password-store for a few months now, and am very happy > > with it > > -- thanks to all involved! > > > > One thing that I'm still grappling with is how to avoid having the > > passwords > > get stored by my clipboard manager (currently copyq). I see that > > discussions of > > this come up in the mailing list every once in a while, for various > > clipboard > > managers; and also from the clipboard manager side, I see that many > > clipboard > > managers discuss how to deal with the issue of passwords. But there > > doesn't > > seem to be any standard way, which obviously makes dealing with this > > in a > > generic way a bit difficult... > > > > In this patch, I propose to add hooks, which can then be implemented > > as > > appropriate for the various clipboard managers. I moved the handling > > for > > klipper into the "default" hook, and in contrib/ I added a reference > > implementation for how I'd like passwords to be handled for copyq -- > > so this > > demonstrates that the method works for at least two password managers > > ;) . > > > > I am offering this here as an RFC: is this kind of approach acceptable > > to the > > project? If so, probably the hooking mechanism should be designed a > > bit more > > throughly than what I've done here (I am also not very proficient in > > bash > > programming, so there's probably a nicer way to add hooks than what > > I've > > done...). But once such a hooking mechanism is in place, I think it > > offers a > > more scalable solution for dealing with the variety of clipboard > > managers, and > > will probably also find a bunch of other use cases, as well... > > > > I'd be glad to hear any thoughts on this... > > > > Thanks! > > Dov > > > > diff -r dcfa9a1c9c82 -r d4af3f470f3f contrib/hooks/copyq.sh > > --- /dev/null Thu Jan 01 00:00:00 1970 +0000 > > +++ b/contrib/hooks/copyq.sh Sun Jul 05 03:25:15 2020 +0300 > > @@ -0,0 +1,11 @@ > > +hook_clip_pre_copy_pw() { > > + copyq disable > > +} > > + > > +hook_clip_post_copy_pw() { > > + copyq enable > > +} > > + > > +hook_clip_pre_restore() { > > + return > > +} > > diff -r dcfa9a1c9c82 -r d4af3f470f3f src/password-store.sh > > --- a/src/password-store.sh Thu Jun 25 23:41:11 2020 +0200 > > +++ b/src/password-store.sh Sun Jul 05 03:25:15 2020 +0300 > > @@ -14,6 +14,7 @@ > > > > PREFIX="${PASSWORD_STORE_DIR:-$HOME/.password-store}" > > EXTENSIONS="${PASSWORD_STORE_EXTENSIONS_DIR:-$PREFIX/.extensions}" > > +HOOKS="${PASSWORD_STORE_HOOKS_DIR:-$PREFIX/.hooks}" > > X_SELECTION="${PASSWORD_STORE_X_SELECTION:-clipboard}" > > CLIP_TIME="${PASSWORD_STORE_CLIP_TIME:-45}" > > GENERATED_LENGTH="${PASSWORD_STORE_GENERATED_LENGTH:-25}" > > @@ -152,6 +153,25 @@ > > # BEGIN platform definable > > # > > > > +hook_clip_pre_copy_pw() { > > + return > > +} > > + > > +hook_clip_post_copy_pw() { > > + return > > +} > > + > > +hook_clip_pre_restore() { > > + # It might be nice to programatically check to see if klipper > > exists, > > + # as well as checking for other common clipboard managers. > > But for > > now, > > + # this works fine -- if qdbus isn't there or if klipper isn't > > running, > > + # this essentially becomes a no-op. > > + # > > + # Clipboard managers frequently write their history out in > > plaintext, > > + # so we axe it here: > > + qdbus org.kde.klipper /klipper > > org.kde.klipper.klipper.clearClipboardHistory &>/dev/null > > +} > > + > > clip() { > > if [[ -n $WAYLAND_DISPLAY ]]; then > > local copy_cmd=( wl-copy ) > > @@ -175,21 +195,15 @@ > > # trailing new lines. > > pkill -f "^$sleep_argv0" 2>/dev/null && sleep 0.5 > > local before="$("${paste_cmd[@]}" 2>/dev/null | $BASE64)" > > + hook_clip_pre_copy_pw > > echo -n "$1" | "${copy_cmd[@]}" || die "Error: Could not copy data > > to the clipboard" > > + hook_clip_post_copy_pw > > ( > > ( exec -a "$sleep_argv0" bash <<<"trap 'kill %1' TERM; sleep > > '$CLIP_TIME' & wait" ) > > local now="$("${paste_cmd[@]}" | $BASE64)" > > [[ $now != $(echo -n "$1" | $BASE64) ]] && before="$now" > > > > - # It might be nice to programatically check to see if klipper > > exists, > > - # as well as checking for other common clipboard managers. > > But for > > now, > > - # this works fine -- if qdbus isn't there or if klipper isn't > > running, > > - # this essentially becomes a no-op. > > - # > > - # Clipboard managers frequently write their history out in > > plaintext, > > - # so we axe it here: > > - qdbus org.kde.klipper /klipper > > org.kde.klipper.klipper.clearClipboardHistory &>/dev/null > > - > > + hook_clip_pre_restore > > echo "$before" | $BASE64 -d | "${copy_cmd[@]}" > > ) >/dev/null 2>&1 & disown > > echo "Copied $2 to clipboard. Will clear in $CLIP_TIME seconds." > > @@ -246,6 +260,10 @@ > > > > source "$(dirname "$0")/platform/$(uname | cut -d _ -f 1 | tr > > '[:upper:]' '[:lower:]').sh" 2>/dev/null # PLATFORM_FUNCTION_FILE > > > > +for hook in $HOOKS/*.sh; do > > + source $hook > > +done > > + > > # > > # END platform definable > > #
