Re: [STUMP] [PATCH] Initial interface to pwsafe password keyring. Two new commands: `pwsafe-menu', `pwsafe-entry'.

[Ben Spencer]

Hi Wojciech,

Thanks for the patch.  Comments below.


On Fri, Jun 24, 2011 at 01:37:20AM +0100, Wojciech Meyer wrote:
> +(defun lines-from-string (string)
> +  "Boiler plate for splitting STRING buffer to lines.  Rewritten
> +thousands of times already"
> +  (loop for i = 0 then (1+ j)
> +        as j = (position #\Newline string :start i)
> +        collect (subseq string i j)
> +        while j))

Stumpwm already has a split-string function you could use for this.
To split on newlines you'd call it as follows:

(split-string str '(#\Newline))


> +(defun pair-split (entry separator)
> +  "Split two ENTRY sections separated by SEPARATOR into pair"
> +  (let ((lst (cl-ppcre:split separator entry)))
> +    (cons (remove #\Newline (car lst)) (remove #\Newline (cadr lst)))))

This function is a bit confusing.  The newline-removing behaviour
isn't described by the name or the docstring, and doesn't seem to be
required by at least two of the callers.  Seems like you could just
call cl-ppcre:split directly for those callers and not bother with
this function.


> +(defun command-options (options &optional argument) 
> +  "Create command OPTIONS with optional ARGUMENTS.  BUGS: Need to
> +insert space after each switch, should interleave space."
> +  (format nil "~a ~a" 
> +          (if (listp options) 
> +              (apply #'concat options) options) 
> +          (or argument "") t))

You can do what you want with format and the ~{ directive here,
something like:

(format nil "~{~a~^ ~}" options)

I'm not really sure why this function takes two separate arguments
rather than just a simple list of options though.

Should the strings be shell escaped?


> +(defun pwsafe-command (password options &optional argument) 
> +  "Execute pwsafe command using master PASSWORD with OPTIONS and
> +additional ARGUMENT. Not safe."
> +  (format nil "echo \"~a\" | pwsafe ~a" password (command-options options 
> argument)))

Again, the password should probably be shell escaped, and I'm not sure
why options are argument are separate.  It seems like this function
couild be merged with command-options since it's the only thing that
calls it.  The resulting function could just take a password and a
list of options.


> +(defun with-xsel (command &optional options)
> +  "Pipe COMMAND with OPTIONS to xsel."
> +  (format nil "~a | xsel ~a" command (or options "")))

You could perhaps do this using clx.  Stump already has a
set-x-selection function, it could be useful for it to have
set-x-clipboard too.

Otherwise, shell escaping again.  It might also be clearer to make
options default to "" rather than using or.


> +(defun run-pwsafe-command (password options &optional argument)
> +  (let ((output (run-shell-command (pwsafe-command password options 
> argument) t)))
> +    (when (cl-ppcre::scan "Passphrase is incorrect" output)
> +      (throw 'error "Passphrase is incorrect"))
> +    output))

Is this error on stdout?  run-shell-command doesn't collect output
from stderr, so if it goes there this won't work without a 2>&1 in the
command.


> +(defun assoc-entries (entries)
> +  "Create assoc from ENTRIES keyed by the name"
> +  (mapcar (lambda (entry) (cons (pwsafe-entry-name entry) entry)) entries))

This seems to happen every time pwsafe-entries is called, so why not
do it as part of that rather than iterating over the list twice?


> +(defun pwsafe-password-to-clipboard (entry)
> +  "Main function that will perform side action on ENTRY with all the
> +associated side effects like priting message and putting password into
> +xclipboard"
> +  (let* ((pwsafe-entry (pwsafe-entry-name entry))
> +         (output 
> +          (run-pwsafe-command (pwsafe-entry-password entry) 
> +                           ;; FIXME: dirty hack, attempted to be nice
> +                           ;; and keep the options in list, however
> +                           ;; `pwsafe-command' is not interleaving
> +                           ;; them with space
> +                           '("-q " "-p " "--echo " "-E ") pwsafe-entry))
> +         ;; FIXME: this one is a bit buggy, it should be really not
> +         ;; calling pair-split it might not work in all cases
> +         (entry-password (cdr (pair-split output "passphrase for.*: "))))

Not really sure what's going on here, if you give me an example of the
output I might be able to help with parsing it.


> +    (set-x-selection entry-password)
> +    (run-shell-command (with-xsel (format nil "echo \"~a\"" entry-password) 
> "-ib") t)
> +    (message 
> +     (format nil "Username: ~a (password copied to clipboard)" 
> +             (pwsafe-entry-user-name entry)))))

message takes a format string and args so there should be no need for
a separate call to format.


> +(defcommand pwsafe-menu (password) ((:password "Pwsafe password: "))
> +  "Prompt for PASSWORD. Show menu with pwsafe database entries. Let
> +the user choose entry, put password to clipboard and notify user about
> +associated username"
> +  (let* ((entries (pwsafe-entries password))
> +         (entry (select-from-menu (current-screen) 
> +                                  (assoc-entries entries))))

select-from-menu is not currently exported, you'll need to either
export it or call it with stumpwm:: here (exporting it seems
reasonable).


> +(define-stumpwm-type :pwsafe-entry (input prompt)
> +  "This is our type for prompting entry, and performing completion."
> +  (or (argument-pop input)

This looks iffy, won't it return a string rather than the expected
pwsafe-entry?



Ben