Skip to content

pass command instead of password in zuliprc #1581

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,12 @@ to get the hang of things.

## Configuration
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last commit is not just a docs update. If there are changes to tidy other commits, they belong combined with those commits. That will ensure that all the linting and tests passes on each commit individually, rather than just over the branch.

If you have other changes, they belong in new commits specific to those improvements.


configuration conssist of two file:
- zulip_key, file contains the api_key
- zuliprc, file consist of login configurations

The `zulip_key`contains only the api_key.

Comment on lines +210 to +215
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zulip_key (or similar) file is intended to be optional, so that zuliprc files in a previous format (and current, from the web app) work correctly.

For that reason anything like this part should likely be further down.

Since this documents the way the zuliprc file works, whether only for the Terminal app, or for all apps using the python zulip package (when we use the updated package, if so), it should be clear that it refers to the [api] part of the config.

The `zuliprc` file contains two sections:
- an `[api]` section with information required to connect to your Zulip server
- a `[zterm]` section with configuration specific to `zulip-term`
Expand All @@ -216,13 +222,15 @@ A file with only the first section can be auto-generated in some cases by
above). Parts of the second section can be added and adjusted in stages when
you wish to customize the behavior of `zulip-term`.

If you’re downloading the config file from your Zulip account, you should replace the `key` field with `passcmd`, setting its value to a command that outputs the api_key (e.g., cat zulip_key). If you’re not downloading it manually, zulip-term will configure this for you automatically, though it’s recommended to update the passcmd value afterward for better security.

The example below, with dummy `[api]` section contents, represents a working
configuration file with all the default compatible `[zterm]` values uncommented
and with accompanying notes:
```
[api]
[email protected]
key=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
passcmd=cat zulip_key
Comment on lines -225 to +233
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this needs to be optional, and if in Terminal only will be specific to us, it needs to be briefly documented in the sample file, and commented out - see how we document the Terminal options further below.

site=https://example.zulipchat.com

[zterm]
Expand Down Expand Up @@ -257,6 +265,7 @@ transparency=disabled
# editor: nano
```


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check for unnecessary additions.

> **NOTE:** Most of these configuration settings may be specified on the
command line when `zulip-term` is started; `zulip-term -h` or `zulip-term --help`
will give the full list of options.
Expand Down
60 changes: 46 additions & 14 deletions tests/cli/test_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,7 @@ def test_main_cannot_write_zuliprc_given_good_credentials(

# This is default base path to use
zuliprc_path = os.path.join(str(tmp_path), path_to_use)
zuliprc_file = os.path.join(zuliprc_path, "zuliprc")
monkeypatch.setenv("HOME", zuliprc_path)

# Give some arbitrary input and fake that it's always valid
Expand All @@ -412,12 +413,18 @@ def test_main_cannot_write_zuliprc_given_good_credentials(
captured = capsys.readouterr()
lines = captured.out.strip().split("\n")

expected_line = (
"\x1b[91m"
f"{expected_exception}: zuliprc could not be created "
f"at {os.path.join(zuliprc_path, 'zuliprc')}"
"\x1b[0m"
)
if expected_exception == "FileNotFoundError":
expected_error = (
f"could not create {zuliprc_file} "
f"([Errno 2] No such file or directory: '{zuliprc_file}')"
)
else: # PermissionError
expected_error = (
f"could not create {zuliprc_file} "
f"([Errno 13] Permission denied: '{zuliprc_file}')"
)

expected_line = f"\x1b[91m{expected_exception}: {expected_error}\x1b[0m"
assert lines[-1] == expected_line


Expand Down Expand Up @@ -573,17 +580,29 @@ def test_exit_with_error(
def test__write_zuliprc__success(
tmp_path: Path, id: str = "id", key: str = "key", url: str = "url"
) -> None:
path = os.path.join(str(tmp_path), "zuliprc")

error_message = _write_zuliprc(path, api_key=key, server_url=url, login_id=id)
"""Test successful creation of zuliprc and zulip_key files."""
path = tmp_path / "zuliprc"
key_path = tmp_path / "zulip_key"

error_message = _write_zuliprc(
to_path=str(path),
key_path=str(key_path),
login_id=id,
api_key=key,
server_url=url,
)

assert error_message == ""

expected_contents = f"[api]\nemail={id}\nkey={key}\nsite={url}"
expected_contents = f"[api]\nemail={id}\npasscmd=cat zulip_key\nsite={url}"
with open(path) as f:
assert f.read() == expected_contents

with open(key_path) as f:
assert f.read() == key

assert stat.filemode(os.stat(path).st_mode)[-6:] == 6 * "-"
assert stat.filemode(os.stat(key_path).st_mode)[-6:] == 6 * "-"


def test__write_zuliprc__fail_file_exists(
Expand All @@ -593,11 +612,24 @@ def test__write_zuliprc__fail_file_exists(
key: str = "key",
url: str = "url",
) -> None:
path = os.path.join(str(tmp_path), "zuliprc")

error_message = _write_zuliprc(path, api_key=key, server_url=url, login_id=id)
"""Test that _write_zuliprc fails when files already exist."""
path = tmp_path / "zuliprc"
key_path = tmp_path / "zulip_key"

# Create the files first to simulate they already exist
with open(path, "w") as f:
f.write("existing content")

error_message = _write_zuliprc(
to_path=str(path),
key_path=str(key_path),
login_id=id,
api_key=key,
server_url=url,
)

assert error_message == "zuliprc already exists at " + path
assert error_message == f"FileExistsError: {path} already exists"
assert not Path(key_path).exists() # key_path should not be created


@pytest.mark.parametrize(
Expand Down
55 changes: 40 additions & 15 deletions zulipterminal/cli/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import argparse
import configparser
import contextlib
import logging
import os
import stat
Expand Down Expand Up @@ -311,36 +312,60 @@ def fetch_zuliprc(zuliprc_path: str) -> None:
print(in_color("red", "\nIncorrect Email(or Username) or Password!\n"))
login_data = get_api_key(realm_url)

zulip_key_path = os.path.join(
os.path.dirname(os.path.abspath(zuliprc_path)), "zulip_key"
)

preferred_realm_url, login_id, api_key = login_data
save_zuliprc_failure = _write_zuliprc(
zuliprc_path,
login_id=login_id,
to_path=zuliprc_path,
key_path=zulip_key_path,
api_key=api_key,
login_id=login_id,
server_url=preferred_realm_url,
)
if not save_zuliprc_failure:
print(f"Generated API key saved at {zuliprc_path}")
print(f"Generated config file saved at {zuliprc_path}")
else:
exit_with_error(save_zuliprc_failure)


def _write_zuliprc(
to_path: str, *, login_id: str, api_key: str, server_url: str
to_path: str, *, key_path: str, login_id: str, api_key: str, server_url: str
) -> str:
"""
Writes a zuliprc file, returning a non-empty error string on failure
Only creates new private files; errors if file already exists
Writes both zuliprc and zulip_key files securely.
Ensures atomicity: if one file fails to write, cleans up the other.
Returns an empty string on success, or a descriptive error message on failure.
"""
zuliprc_created = False

try:
# Write zuliprc
with open(
os.open(to_path, os.O_CREAT | os.O_WRONLY | os.O_EXCL, 0o600), "w"
) as f:
f.write(f"[api]\nemail={login_id}\nkey={api_key}\nsite={server_url}")
f.write(
f"[api]\nemail={login_id}\npasscmd=cat zulip_key\nsite={server_url}"
)
zuliprc_created = True
# Write zulip_key
with open(
os.open(key_path, os.O_CREAT | os.O_WRONLY | os.O_EXCL, 0o600), "w"
) as f:
f.write(api_key)

return ""

except FileExistsError:
return f"zuliprc already exists at {to_path}"
filename = to_path if not zuliprc_created else key_path
return f"FileExistsError: {filename} already exists"
except OSError as ex:
return f"{ex.__class__.__name__}: zuliprc could not be created at {to_path}"
if zuliprc_created:
with contextlib.suppress(Exception):
os.remove(to_path)
filename = key_path if zuliprc_created else to_path
return f"{ex.__class__.__name__}: could not create {filename} ({ex})"


def parse_zuliprc(zuliprc_str: str) -> Dict[str, SettingData]:
Expand All @@ -366,12 +391,12 @@ def parse_zuliprc(zuliprc_str: str) -> Dict[str, SettingData]:
in_color(
"red",
"ERROR: Please ensure your zuliprc is NOT publicly accessible:\n"
" {0}\n"
"(it currently has permissions '{1}')\n"
f" {zuliprc_path}\n"
f"(it currently has permissions '{stat.filemode(mode)}')\n"
"This can often be achieved with a command such as:\n"
" chmod og-rwx {0}\n"
f" chmod og-rwx {zuliprc_path}\n"
"Consider regenerating the [api] part of your zuliprc to ensure "
"your account is secure.".format(zuliprc_path, stat.filemode(mode)),
"your account is secure.",
)
)
sys.exit(1)
Expand Down Expand Up @@ -687,8 +712,8 @@ def print_setting(setting: str, data: SettingData, suffix: str = "") -> None:
# Dump stats only after temporary file is closed (for Win NT+ case)
prof.dump_stats(profile_path)
print(
"Profile data saved to {0}.\n"
"You can visualize it using e.g. `snakeviz {0}`".format(profile_path)
f"Profile data saved to {profile_path}.\n"
f"You can visualize it using e.g. `snakeviz {profile_path}`"
)

sys.exit(1)
Expand Down
Loading