-
Notifications
You must be signed in to change notification settings - Fork 24
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
Feature: Local cache for op items #16
base: master
Are you sure you want to change the base?
Conversation
…q-filter to disable and customize filtering
New tmux options @1password-enabled-url-filter and @1password-items-j…
To cache op list-items results I must first create a file everytime I get any return from the command itself. On this commit, I am creating the logic to save the return from op list-items into a given file for further usage. Now, I just need to add the logic to fetch from this file instead, if the file is new enought.
Now that I can cache the op list-items result, I need to use the cache instead of getting it online, if it exists. It is important to have a TTL on the cache file too. On this commit I'm going to add the logic to use the cache file (if applicable) and also create a rule to use it only when the cache is recent. Now, I've finished the feature and can start using it.
Feature/cache op results
While I love the idea of caching, putting all of my incredibly sensitive passwords in an unencrypted file in a directory that every program running on my computer has read access to seems like a really bad idea, and not something I would be comfortable with from a security perspective. |
@camspiers the file contains only the title and ID of the account :) it does not contain the password itself. The password is fetched by the plug-in afterwards, when you select which account you want to get |
Just to give more details, the data I'm caching is the return of This is not sensitive data, so there is no need to encrypt it. Even if someone stole your file, they would need your 1Password password to fetch the actual data for your account. So, in my opinion, there is no need for any extra security step here |
Oh right. Excellent! |
@yardnsm can you take a look at this please? :D thanks! |
btw, apologies for the false assumption above. This feature is going to be really valuable. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! But what will happen when:
- The user changes a subdomain (using
@1password-subdomain
)? - The user changes a vault (using
@1password-vault
)? - The user updates his custom JQ filter?
- The user adds passwords to his vault?
To solve all of these problems, I suggest adding a new tmux keybinding that will clear that cache file and run the normal flow. I would like to have prefix + U
, but it's already taken by tpm
😔 Any ideas?
Also, I was originally meant to add a 1pass
support (which have it's own secure caching mechanism even for the passwords). It's documented (very roughly) in #6, but in the meantime I think your solution is awesome!
@yardnsm nice idea! I'm going to search for a proper keybinding to use! About So, this could be a good alternative to those who won't use |
@yardnsm I'm going to take a look on your suggestions on the following days. Also, I've noticed another problem. Since we're just asking for user password during the I must also fix this by asking for the password during afterwards if the session has expired |
During the PR review, some contributors of tmux-1password suggested to bind a key to clear the cache from the local caching. On this commit I've created all the required scripts and configurations to create that binding. Now we should be able to clear our cache with prefix + R
During the PR, I've received some suggestion to improve the code. On this commit, I've added those suggestions.
During the PR, I've received some suggestion to improve the code. On this commit, I've added those suggestions.
@yardnsm I've updated the code :) can you please review it and merge with master? I've also updated the The only thing that I wasn't able to do was add a condition to login the user if the session expired. |
@yardnsm thanks for the review, I'm going to take a look on your comments. |
@yardnsm I've fixed your comments, and also fixed a bug. The cache was being automatically cleared before Waiting for you review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
@yardnsm done :) Waiting for you approval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks. I'm going to revert it in the next few days and get in touch with you ;) |
@yardnsm sorry for the late reply. I've just finished it and you can merge right now ;) Thanks! |
@yardnsm will this branch ever be merged? :) |
This looks good, great work @odelucca! Any chance of this being merged @yardnsm? Also, do you think it's worth updating the permissions of the temporary files so they can't be read by other system users? |
☕ Purpose
After I started using #13, I've noticed that it takes too long if every time I fetch passwords my computer needs to go to the 1Password server to fetch them.
To fix this, I've created this PR, which saves a local cache containing the result of
op list-items
. The cache is available for 6 hours :)This PR is based on #13, so some of the code here may looks different but it is actually the implementation of #13. As soon as that PR is merged, it would be more clear to see the diffs.
🧐 Checklist