jscarle / OnePassword.NET

A 1Password CLI Wrapper for .NET.
https://jscarle.github.io/OnePassword.NET/
MIT License
23 stars 8 forks source link

Add support for 1Password CLI app integration #26

Closed thedavecarroll closed 1 year ago

thedavecarroll commented 1 year ago

First, I'm not sure if I'm creating the pull request correctly. Feel free to close it if not and provide some guidelines.

I've only done minimal testing within my PowerShell module, but I think the revisions should not require any existing code changes for any consumers.

Here is my OPassCliPS repo if you wanted to check it out.

Connect-OPAccount -Verbose
Get-OPSession | Format-List
VERBOSE: Attempting to discover the path to 1Password CLI.
VERBOSE: Setting 1Password CLI Path session variable.
VERBOSE: Creating 1Password session with app integration.
VERBOSE: 1Password CLI Version: 2.12.0
VERBOSE: Signing into 1Password.
VERBOSE: Retrieving connected account.
VERBOSE: Creating module cache for vaults, items, and accounts.
VERBOSE: Cache creation time: 00:03:876

CliPath              : C:\Program Files\1Password CLI\op.exe
IsAppIntegrated      : True
IsAuthenticated      : True
AuthenticatedAccount : <my account>
Connect-OPAccount -Domain $Domain -Email $Email -SecretKey $SecretKey -Password $Password -Verbose
Get-OPSession | Format-List
VERBOSE: Attempting to discover the path to 1Password CLI.
VERBOSE: Setting 1Password CLI Path session variable.
VERBOSE: Creating 1Password session without app integration.
VERBOSE: Adding account for <email address>.
VERBOSE: 1Password CLI Version: 2.12.0
VERBOSE: Signing into 1Password.
VERBOSE: Retrieving connected account.
VERBOSE: Creating module cache for vaults, items, and accounts.
VERBOSE: Cache creation time: 00:01:964

CliPath              : C:\Program Files\1Password CLI\op.exe
IsAppIntegrated      : False
IsAuthenticated      : True
AuthenticatedAccount : <my account>
jscarle commented 1 year ago

I'll review the code when I have a free moment. There's a few things to address. First, the tests fail. Also, according to the documentation, I'm not sure that --force is required. Though I understand it could be useful to have in certain scenarios.

thedavecarroll commented 1 year ago

Definitely no rush and no pressure.

I agree on the --force for SignIn() and I don't recall why I thought I needed it.

If the tests are trying to run against the new SignIn(), I'm not sure how that can be accomplished.

Not that it really matters, but I just ran the same commands dotnet build --configuration Release --no-restore and dotnet test --configuration Release --no-build --verbosity normal. The latter completed, but skipped all tests. :shrug:

Enjoy your weekend. I probably won't be back to my module until Tuesday.

jscarle commented 1 year ago

Because tests are run against a real account, they will only run if enabled via the environment variables declared in TestBase.cs.