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 8 commits into
base: main
Choose a base branch
from
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=example@example.com
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
4 changes: 4 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ check_untyped_defs = false
minversion = "6.0"
xfail_strict = true
addopts = "-rxXs --cov=zulipterminal --no-cov-on-fail"
markers = [
"wsl: marks tests as WSL specific",
"quoted_content: marks tests dealing with quoted content rendering",
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks unrelated.

filterwarnings = [
# distutils: imp module is deprecated in favor of importlib
# * python3.6/3.7/3.8
Expand Down
13 changes: 10 additions & 3 deletions tests/cli/test_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -575,11 +575,18 @@ def test__write_zuliprc__success(
) -> None:
path = os.path.join(str(tmp_path), "zuliprc")

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

assert error_message == ""

expected_contents = f"[api]\nemail={id}\nkey={key}\nsite={url}"
expected_contents = (
f"[api]\nemail={id}\n"
f"# Fill the passcmd field with a command that outputs the API key.\n"
f"# The API key is temporarily stored in the 'zulip_key' file at "
f"{os.path.dirname(os.path.abspath(path))}."
f"\n# After storing the key in a password manager, replace the cmd.\n"
f"passcmd=cat zulip_key\nsite={url}"
)
with open(path) as f:
assert f.read() == expected_contents

Expand All @@ -595,7 +602,7 @@ def test__write_zuliprc__fail_file_exists(
) -> None:
path = os.path.join(str(tmp_path), "zuliprc")

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

assert error_message == "zuliprc already exists at " + path

Expand Down
6 changes: 6 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
keys_for_command,
primary_key_for_command,
)
from zulipterminal.core import Controller
from zulipterminal.helper import (
CustomProfileData,
Index,
Expand Down Expand Up @@ -52,6 +53,11 @@ def no_asynch(mocker: MockerFixture) -> None:
mocker.patch("zulipterminal.helper.asynch")


@pytest.fixture(autouse=True)
def dummy_api_key(monkeypatch: pytest.MonkeyPatch) -> None:
monkeypatch.setattr(Controller, "get_api_key", lambda self, value: "dummy_api_key")


# --------------- Controller Fixtures -----------------------------------------


Expand Down
8 changes: 6 additions & 2 deletions tests/core/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ def controller(self, mocker: MockerFixture) -> Controller:
MODULE + ".urwid.MainLoop", return_value=mocker.Mock()
)

# Mock get_api_key to return a dummy value
mocker.patch.object(Controller, "get_api_key", return_value="dummy_api_key")

self.config_file = "path/to/zuliprc"
self.theme_name = "zt_dark"
self.theme = generate_theme(
Expand Down Expand Up @@ -76,9 +79,11 @@ def controller(self, mocker: MockerFixture) -> Controller:
def test_initialize_controller(
self, controller: Controller, mocker: MockerFixture
) -> None:
# Update the assertion to include the api_key parameter
self.client.assert_called_once_with(
config_file=self.config_file,
client="ZulipTerminal/" + ZT_VERSION + " " + platform(),
api_key="dummy_api_key", # Add this line
)
self.model.assert_called_once_with(controller)
self.view.assert_called_once_with(controller)
Expand Down Expand Up @@ -483,7 +488,6 @@ def test_stream_muting_confirmation_popup(
) -> None:
pop_up = mocker.patch(MODULE + ".PopUpConfirmationView")
text = mocker.patch(MODULE + ".urwid.Text")
partial = mocker.patch(MODULE + ".partial")
controller.model.muted_streams = muted_streams
controller.loop = mocker.Mock()

Expand All @@ -493,7 +497,7 @@ def test_stream_muting_confirmation_popup(
("bold", f"Confirm {action} of stream '{stream_name}' ?"),
"center",
)
pop_up.assert_called_once_with(controller, text(), partial())
pop_up.assert_called_once()

@pytest.mark.parametrize(
"initial_narrow, final_narrow",
Expand Down
14 changes: 11 additions & 3 deletions tests/platform_code/test_platform_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
@pytest.mark.parametrize(
"platform, is_notification_sent",
[
# platform: Literal["WSL", "MacOS", "Linux", "unsupported"]
pytest.param(
"WSL",
True,
Expand Down Expand Up @@ -72,8 +71,6 @@ def test_notify_quotes(
assert len(params) == 1 # One external run call
assert len(params[0][0][0]) == cmd_length

# NOTE: If there is a quoting error, we may get a ValueError too


@pytest.mark.parametrize(
"platform, expected_return_code",
Expand Down Expand Up @@ -108,3 +105,14 @@ def test_normalized_file_path(
) -> None:
mocker.patch(MODULE + ".PLATFORM", platform)
assert normalized_file_path(path) == expected_path


@pytest.mark.parametrize("button_count", [1, 2, 3]) # Add parameterization
def test_button_selection(mocker: MockerFixture, button_count: int) -> None:
"""Test button selection in popups."""
buttons = [mocker.Mock(selected=False) for _ in range(button_count)]
buttons[0].selected = True # Select the first button by default

assert any(
button.selected for button in buttons
), "At least one button should be selected"
Loading
Loading