Skip to content

Conversation

@arg3t
Copy link

@arg3t arg3t commented Mar 16, 2021

I added support for One Time Passwords, this was the only thing missing in my opinion.

Copy link
Owner

@andykais andykais left a comment

Choose a reason for hiding this comment

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

Requesting some changes, the pr as it stands breaks on my laptop (which selfishly is very important for me to support!). Awesome work adding a feature though. Lets get it working robustly.

bin/cli.js Outdated

const dmenuArgsParsed = dmenuArgs ? dmenuArgs.split(/\s+/) : []
const dmenuPswdArgsParsed = ['-p', 'Password:', '-nf', 'black', '-nb', 'black'].concat(
const dmenuPswdArgsParsed = ['-p', 'Password:', '-P'].concat(
Copy link
Owner

Choose a reason for hiding this comment

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

I ran your changes locally, and it gives an error.

 andrew  bitwarden-dmenu-pr  ./bin/cli.js        
node:events:356
      throw er; // Unhandled 'error' event
      ^

Error: write EPIPE
    at afterWriteDispatched (node:internal/stream_base_commons:160:15)
    at writeGeneric (node:internal/stream_base_commons:151:3)
    at Socket._writeGeneric (node:net:775:11)
    at Socket._write (node:net:787:8)
    at writeOrBuffer (node:internal/streams/writable:400:12)
    at _write (node:internal/streams/writable:341:10)
    at Socket.Writable.write (node:internal/streams/writable:345:10)
    at /home/andrew/Code/development/bitwarden-dmenu-pr/src/executable-wrappers/dmenu.js:16:17
    at new Promise (<anonymous>)
    at /home/andrew/Code/development/bitwarden-dmenu-pr/src/executable-wrappers/dmenu.js:11:3
Emitted 'error' event on Socket instance at:
    at emitErrorNT (node:internal/streams/destroy:188:8)
    at emitErrorCloseNT (node:internal/streams/destroy:153:3)
    at processTicksAndRejections (node:internal/process/task_queues:81:21) {
  errno: -32,
  code: 'EPIPE',
  syscall: 'write'
}

Im not sure what version of dmenu you are using, but mine does not include a -P flag (dmenu-5.0 for reference)

Copy link
Author

Choose a reason for hiding this comment

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

That's because I have the password patch installed. But I reverted that line back to what it was and instead added a command to README for people who want to use that patch.

bitwarden-dmenu --dmenu-args='-i' --clear-clipboard 30 --session-timeout 100 --sync-vault-after 3600 --on-error 'xargs notify-send --urgency=low'
```

If you have the [password patch](https://tools.suckless.org/dmenu/patches/password/) for dmenu, you can run the following command to see your password as you enter it:
Copy link
Owner

Choose a reason for hiding this comment

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

hmm...I dont love this. I think its best to keep out of the readme. If you took the time to install the password patch, you probably already know to use it. As an alternative though, is it possible to programatically detect this? E.g. can we detect that the -P flag is available by calling dmenu -h and doing a text search? That would avoid the necessity for users to manually set things, and I doubt they would ever not want this flag

Copy link
Author

Choose a reason for hiding this comment

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

Text search of dmenu -h might not work for some people. For instance, if a patch fails and I have to manually add it, I just don't bother with editing the help message. This is the case in my current dmenu setup as well. We could just run dmenu -P and check whether that spits an error, but if the command has that option it will wait for user input before returning an exit code and that is not ideal either. We can just leave it as is right now and allow people to manually set it. If I ever find a solution though I will submit another pull request.

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.

2 participants