Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix op signin for v2 #1961

Merged
merged 3 commits into from
Mar 24, 2022
Merged

Conversation

halostatue
Copy link
Collaborator

The release of op 2.0.0 introduced a breaking change to op signin so that the previous version invocation op signin account no longer works. Now, it is required to use the option --account, as in op signin --account account.

Note that there is likely another issue hidden by this when not using biometric authentication for op, because the session environment variable is no longer OP_SESSION_<account> but OP_SESSION_<accountUUID>. If the account is specified as the account UUID, everything will probably work. I’m using biometric authentication, so I cannot easily verify this.

Also note that in op v1, you could specify --vault ''; this is no longer legal.

https://1password.community/discussion/128083/problems-with-op-v2-when-integrating-with-chezmoi#latest

The release of op 2.0.0 introduced a breaking change to `op signin` so
that the previous version invocation `op signin account` no longer
works. Now, it is required to use the option `--account`, as in `op
signin --account account`.

Note that there is likely another issue hidden by this when not using
biometric authentication for `op`, because the session environment
variable is no longer `OP_SESSION_<account>` but
`OP_SESSION_<accountUUID>`. If the account is specified as the account
UUID, everything will probably work. I’m using biometric authentication,
so I cannot easily verify this.

Also note that in `op` v1, you could specify `--vault ''`; this is no
longer legal.

https://1password.community/discussion/128083/problems-with-op-v2-when-integrating-with-chezmoi#latest
Adjust the 1Password CLI 2.0 documentation to cover changes made to the
protocols used by chezmoi (preferring the account UUID over the account
name because of the change in `OP_SESSION_*` variables and not
specifying an empty vault because of errors).

https://1password.community/discussion/128083/problems-with-op-v2-when-integrating-with-chezmoi#latest
@halostatue
Copy link
Collaborator Author

Note that I am trying to fix failures with test scripts on Windows, but I think that the better option would be to modify the logic in newOnepasswordArgs so that it’s something more like

	if len(userArgs) > 1 && userArgs[1] != "" {
		a.vault = userArgs[1]
		a.args = append(a.args, "--vault", a.vault)
	}
	if len(userArgs) > 2 && userArgs[2] != "" {
		a.account = userArgs[2]
		a.args = append(a.args, "--account", a.account)
	}

This would prevent adding --vault '' or --account '' if the values are empty. I can make that change and revert some of the tests if you’d like.

@twpayne
Copy link
Owner

twpayne commented Mar 21, 2022

Thank you for such a high quality pull request - including tests and documentation, thank you!

I think that the better option would be to modify the logic in newOnepasswordArgs so that it’s something more like
...
This would prevent adding --vault '' or --account '' if the values are empty. I can make that change and revert some of the tests if you’d like.

This indeed does sound like the better option. Please do implement this.

@twpayne
Copy link
Owner

twpayne commented Mar 21, 2022

For Windows, I'll happily still accept this PR even if you don't get the tests passing on Windows. Windows is quite a peculiar beast, and it can be extremely tiresome debugging through GitHub Action failure round-trips. I've got a local Windows machine for chezmoi development and will happily implement the Windows fixes if you don't have access to a Windows box.

@halostatue
Copy link
Collaborator Author

@twpayne You’re going to have more documentation to review, because I decided to really finalize the the documentation for 1Password CLI v2 as well as implementing the changes discussed above.

fix: Finalize 1Password CLI 2.x support

  • Ensure that --vault '' and --account '' will not be passed when given empty strings. This did not cause problems for 1Password 1.x, but errors out on 1Password 2.x.
  • Finalize documentation, emphasizing 1Password 2.x formatting first.
  • Changed some announcement box types to reflect the changes that are caused by using 1Password 2.x.

Hopefully, the tests will entirely pass now.

- Ensure that `--vault ''` and `--account ''` will not be passed when
  given empty strings. This did not cause problems for 1Password 1.x,
  but errors out on 1Password 2.x.

- Finalize documentation, emphasizing 1Password 2.x formatting first.

- Changed some announcement box types to reflect the changes that are
  caused by using 1Password 2.x.
@twpayne twpayne merged commit 7e6ce39 into twpayne:master Mar 24, 2022
@twpayne twpayne mentioned this pull request Mar 24, 2022
@twpayne
Copy link
Owner

twpayne commented Mar 24, 2022

Thanks very much for this, I made a couple of minor tweaks in #1966.

@halostatue halostatue deleted the fix-op-signin-for-v2 branch March 24, 2022 14:32
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants