darrenjrobinson / 1Pwd

PowerShell Module for 1Password CLI
4 stars 0 forks source link

Module can't use op from system path #3

Closed ghost closed 2 years ago

ghost commented 2 years ago

Even the instructions say Install it in your system path, it won't be able to locate and use it:

image

Am I missing something here?

P.S. Also expression .\\op.exe doesn't make sense - it assumes op is available in current directory, while it is not: https://github.com/darrenjrobinson/1Pwd/blob/5d82c686a38b8274815f8e73f9d1ae1c2166f31e/1Pwd.psm1#L212 https://github.com/darrenjrobinson/1Pwd/blob/5d82c686a38b8274815f8e73f9d1ae1c2166f31e/1Pwd.psm1#L272

ghost commented 2 years ago

Actually, I found a much bigger issue with Invoke-1PasswordExpression - it doesn't work after Powershell restarted. Reason is simple: line https://github.com/darrenjrobinson/1Pwd/blob/5d82c686a38b8274815f8e73f9d1ae1c2166f31e/1Pwd.psm1#L272 expects $1PMasterPasswordDecrypted and $1PSecretKeyDecrypted to be set, but they only set once in Test-1PasswordCredentials. I am confused how it supposed to work?!

darrenjrobinson commented 2 years ago

Try putting OP.exe in the same directory as your script or the directory in which your PS shell is in. It will then use it from the local directory.

For the config after setting it, you need to save it using Set-1PasswordConfiguration. The config is then read in each time the module loads.

ghost commented 2 years ago

Try putting OP.exe in the same directory as your script or the directory in which your PS shell is in. It will then use it from the local directory.

This doesn't make sense, honestly. I am not managing op myself - I got it installed using scoop, it is on the system path and this is sufficient to use it. I would completely drop anything op related in the scripts and just assume/require op to be in path.

For the config after setting it, you need to save it using Set-1PasswordConfiguration. The config is then read in each time the module loads.

Yes, I see it is done when module is loaded, however master password and secret key are loaded in encrypted format, but to retrieve sessionToken they need to be decrypted first and this never happens in the Invoke-1PasswordExpression function. Looks like there is re-use of $Test-1PasswordCredentials method inside, sorry. However somehow code didn't work for me out of the box...

ghost commented 2 years ago

After spending more time with the code I definitely like the idea of this utility, but implementation doesn't looks logical to me. Here is what I think can be done better:

What do you think about these items? I have some time and desire re-work this project. I can fork it, however if you are fine with suggested changes, let me know and I can make a PR to this project instead.

darrenjrobinson commented 2 years ago

One of my original goals was to ensure that the module worked in both Windows PowerShell and PowerShell.
It would be good to validate it on other platforms (linux, mac etc). I have always used it with op.exe in the current path but it could be improved to get the path of op and call it will full path. And do that on other platforms too. Yes the setup is a little involved, but it one off (per host).

I would happily accept enhancements and PRs.

ghost commented 2 years ago

I have always used it with op.exe in the current path but it could be improved to get the path of op and call it will full path. And do that on other platforms too.

There is no need to call it with full path if it is already in path (and otherwise you won't be able to locate it anyway). I would suggest to allow to supply the location of the op binary as a parameter and default it to op (without .exe), which will make it work cross-platform when op is in PATH.

Another question I have here: do we really need to store the secret key? It is needed only once during 1st time setup for the account. And while we can store it, it is not really needed for subsequent signin operations (master password is sufficient). If to re-work the Set-1PasswordConfiguration to be interactive, all we need to store is shorthand (account) and master password (and still query everything it does now, but perform initial login right during setup, so no address/email/secret key are needed after the setup). This will reduce number of items stored in the config a lot.

darrenjrobinson commented 2 years ago

For 'op' location I think having the flexibility of it being in the path and the local (script) directory are valid, especially on systems where you can modify the path or have privileges to add binaries to an existing listed path.

Definitely like the idea of minimising what is stored in the configuration. Even though the config is protected to the profile of the user that created it, having less in it is highly preferred.

ghost commented 2 years ago

Ouch, there was 1Password CLI v2 release recently and it breaks everything... I am currently evaluating new version, but having biometric unlock feature solves lots of issues I wanted 1Pwd for... Playing with new version now.