zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

pass command instead of password in zuliprc

Open Gopinath-Mahendiran opened this issue 7 months ago • 8 comments

What does this PR do, and why?

This PR updates the default config generation logic in zulip-term to support a more secure way of managing Zulip API keys. Instead of writing the API key directly into the zuliprc file, we now generate: • a zuliprc file with a passcmd field • a zulip_key file containing just the API key • zulip_key: A temporary file that contains only the API key. • zuliprc: [api]: contains Zulip login configuration (email, server URL, and now passcmd)

Here’s how the new auto-generated zuliprc file looks:

[api]
[email protected]
# Fill the passcmd field with a command that outputs the API key.
# The API key is temporarily stored in the 'zulip_key' file.
# After storing the key in a password manager, please delete the 'zulip_key' file.
passcmd=cat zulip_key
site=https://chat.zulip.org

If you’re downloading a zuliprc file from your Zulip account (which includes the key field), you’re encouraged to manually replace the key line with a passcmd, like this:

passcmd=cat zulip_key

You can (and should) later replace cat zulip_key with a command tied to your preferred secret storage tool

Outstanding aspect(s)

  • [ ]

External discussion & connections

  • [x] Discussed in #zulip-terminal in topic
  • [x] Fully fixes #1502
  • [ ] Partially fixes issue #
  • [ ] Builds upon previous unmerged work in PR #
  • [ ] Is a follow-up to work in PR #
  • [ ] Requires merge of PR #
  • [ ] Merge will enable work on #

How did you test this?

  • [x] Manually - Behavioral changes
  • [ ] Manually - Visual changes
  • [ ] Adapting existing automated tests
  • [x] Adding automated tests for new behavior (or missing tests)
  • [ ] Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • [x] It is a minimal coherent idea
  • [ ] It has a commit summary following the documented style (title & body)
  • [ ] It has a commit summary describing the motivation and reasoning for the change
  • [x] It individually passes linting and tests
  • [x] It contains test additions for any new behavior
  • [ ] It flows clearly from a previous branch commit, and/or prepares for the next commit

Gopinath-Mahendiran avatar Apr 10 '25 11:04 Gopinath-Mahendiran

Hello @Gopinath-Mahendiran, it seems like you have referenced #1502 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #1502..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

zulipbot avatar Apr 10 '25 11:04 zulipbot

@neiljp,@dkwo review the pr and kindly give the feedback

Gopinath-Mahendiran avatar Apr 13 '25 19:04 Gopinath-Mahendiran

I will work towards the changes

Gopinath-Mahendiran avatar Apr 16 '25 06:04 Gopinath-Mahendiran

@neiljp, updated the code as discussed. Let me know if it looks good.

Gopinath-Mahendiran avatar Apr 25 '25 06:04 Gopinath-Mahendiran

@Gopinath-Mahendiran I reviewed zulip/python-zulip-api#866, and of course this PR will depend on those changes to at least some extent.

Generally this PR would be easier to follow if you can rework the commits from what in the README are termed 'developmental-style commits', into commits that focus on the changes more cleanly. Specifically there are still changes that seem unrelated, as well as changes within the PR that fix earlier errors within the PR? The latter is fine if it lays out how you change things step by step, but not so much if something was missing in an earlier commit - it's clearer to correct the original commit.

neiljp avatar Apr 26 '25 07:04 neiljp

@neiljp now the commits are tied up and have a look at zulip/python-zulip-api#866

Gopinath-Mahendiran avatar Apr 26 '25 15:04 Gopinath-Mahendiran

I believe it is no longer necessary to specify whether the user will utilize the key or passcmd. Based on the recent changes in the python-zulip-api, either method is now accepted seamlessly. Users can either rely on the default zuliprc or use passcmd; as long as one of them is present, the application will function correctly without any disruption.

Gopinath-Mahendiran avatar Apr 26 '25 16:04 Gopinath-Mahendiran

Since the README.md requires some modifications, I will update it based on your feedback

Gopinath-Mahendiran avatar Apr 26 '25 16:04 Gopinath-Mahendiran