Add function to process the passcmd for API key retrieval. #866

Gopinath-Mahendiran · 2025-04-24T10:47:15Z

This commit is a continuation of PR #1581 in the zulip-terminal repository.
It adds functionality to retrieve the API key by executing a passcmd specified in the config file, rather than directly reading the key from the config. Once the passcmd is executed and the API key is obtained, the usual authentication flow proceeds as before.

How did you test this PR?

Self-review checklist

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

neiljp

@Gopinath-Mahendiran Thanks for giving the porting of the other PR across to this repo a try! 👍

I've left some comments inline, but the primary issue from my point of view is that we need to retain backwards compatibility, rather than just looking for passcmd instead of key.

The challenge with this repo (and module) is that there are few automated tests to start with, so it's important to check your changes carefully, or potentially add more tests first.

Much like the ZT PR has a documentation update, we'll want a similar update here too, for the new functionality.

neiljp · 2025-04-26T04:42:08Z

zulip/zulip/__init__.py

-                api_key = config.get("api", "key")
+                passcmd = config.get("api", "passcmd")
+                api_key = self.get_api_key(passcmd)


This does not provide passcmd as a fallback to key, in the configuration file, but only appears to replace it? Running this PR/branch with current zulip-term main causes a crash, since my default zuliprc does not contain a passcmd, but only a key. You can reproduce this by entering the zulip-term virtual environment and installing the /zulip/ package folder in editable (-e) mode.

Also see my comment here: zulip/zulip-terminal#1581 (comment)
(while that comment applied to the documentation, this applies just as much to the code)

neiljp · 2025-04-26T05:07:06Z

zulip/zulip/__init__.py

+    def get_api_key(self, passcmd: str) -> Optional[str]:
+        # run the passcmd command and get the API key
+        result = subprocess.run(passcmd.split(), capture_output=True, check=False)
+        if result.returncode == 0:
+            return result.stdout.decode().strip()
+        else:
+            raise RuntimeError("Error: Unable to retrieve API key.")


I'd suggest we don't need this to be a method that looks developer-facing, and it could likely be placed inline directly in the code above where it would be otherwise called.

That said:

You're currently returning Optional[str], but I don't see where that can happen; it's unclear to me why the type-checker isn't complaining about that

RuntimeError is a very broad exception; if we're to use an exception here, or differently if this is inlined, I'd suggest it should be more specific

The returncode appears not to be portable across platforms; run can provide another solution, I believe?

While we rely on a user to know what they are doing when using passcmd, I would suggest that the simpler commands we accept, and/or the more checking of the command we can do, the better.

now look at the changes and give the feedback

neiljp · 2025-04-26T06:23:26Z

@Gopinath-Mahendiran It would help to know how you tested this - not everything is applicable here, but you didn't fill out the self-review checklist, for example.

Gopinath-Mahendiran · 2025-04-26T06:27:04Z

@neiljp I made the changes directly in the package on my local machine by replicating the modifications from this PR.

neiljp

@Gopinath-Mahendiran This is an improvement, and works for me with expected functionality, with Terminal 👍

I've left some notes inline, not all for you, though it also seems that the formatting is not correct based on the ruff CI error? The other CI error seems systemic to this repo.

neiljp · 2025-04-27T22:46:37Z

zulip/zulip/__init__.py

@@ -366,12 +368,15 @@ class MissingURLError(ZulipError):
 class UnrecoverableNetworkError(ZulipError):
    pass

+class APIKeyRetrievalError(ZulipError):


Minor: This seems used only for the passcmd case, not all API key retrieval? If so, the name could be improved.

neiljp · 2025-04-27T22:53:17Z

zulip/zulip/__init__.py

                api_key = config.get("api", "key")
+            if passcmd is None and config.has_option("api", "passcmd"):


Note for later review: I'm unsure what behavior we want here, ie. whether we should allow passcmd to override key in the config file.

have a look at this zulip/zulip-terminal#1581 (comment) ,If either passcmd or api is present in the configuration file, the application will use whichever is available. This line specifically checks whether passcmd is present in the configuration.

neiljp · 2025-04-27T22:55:24Z

zulip/zulip/__init__.py


 class Client:
    def __init__(
        self,
        email: Optional[str] = None,
        api_key: Optional[str] = None,
+        passcmd: Optional[str] = None,


I'll defer to Tim as to if we'll want a passcmd option to the initializer.

However, with the current code below, the behavior appears incorrect, if we intend to support this behavior.

neiljp · 2025-04-27T23:26:31Z

zulip/zulip/__init__.py

+                        f'Failed to retrieve API key using passcmd "{passcmd}".'
+                        f"Command exited with return code {err.returncode}."


Nit: This formatting looks strange and long.

the first block handles parsing the passcmd using shlex.split(), raising an APIKeyRetrievalError if parsing fails.

It then runs the parsed command securely using subprocess.run() with error checking.

If the command fails (non-zero return code), a descriptive APIKeyRetrievalError is raised with details.

neiljp · 2025-04-27T23:32:11Z

zulip/zulip/__init__.py

+                malicious_chars = {";", "|", "&", ">", "<", "`", "$", "\\", "\n", "\r"}
+                if any(char in passcmd for char in malicious_chars):
+                    raise APIKeyRetrievalError(
+                        f"Invalid characters detected in passcmd: {passcmd!r}"
+                    )


What's the basis for this approach? Do you have a reference?

If passcmd contains any of these characters, it may lead to command injection, potentially resulting in unexpected behavior.

neiljp · 2025-04-27T23:45:44Z

The other outstanding update is the zuliprc docs; fine details will depend on a few final decisions re the behavior, but a provisional adjustment to the README would be a good starting point.

zulipbot added the size: S label Apr 24, 2025

Gopinath-Mahendiran force-pushed the modifing_client branch 3 times, most recently from 8454aee to 0e6f7af Compare April 24, 2025 13:30

Gopinath-Mahendiran mentioned this pull request Apr 24, 2025

pass command instead of password in zuliprc zulip/zulip-terminal#1581

Open

18 tasks

neiljp reviewed Apr 26, 2025

View reviewed changes

Gopinath-Mahendiran force-pushed the modifing_client branch from 0e6f7af to a1acb67 Compare April 26, 2025 08:51

zulipbot added size: M and removed size: S labels Apr 26, 2025

Gopinath-Mahendiran force-pushed the modifing_client branch 4 times, most recently from f448f7a to c388e71 Compare April 26, 2025 09:22

neiljp reviewed Apr 27, 2025

View reviewed changes

feat: Add function to process the passcmd for API key retrieval.

9bd6cbd

Gopinath-Mahendiran force-pushed the modifing_client branch from c388e71 to 9bd6cbd Compare April 28, 2025 05:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add function to process the passcmd for API key retrieval. #866

Add function to process the passcmd for API key retrieval. #866

Gopinath-Mahendiran commented Apr 24, 2025

neiljp left a comment

neiljp Apr 26, 2025

neiljp Apr 26, 2025

Gopinath-Mahendiran Apr 26, 2025

neiljp commented Apr 26, 2025

Gopinath-Mahendiran commented Apr 26, 2025 •

edited

Loading

neiljp left a comment

neiljp Apr 27, 2025

neiljp Apr 27, 2025

Gopinath-Mahendiran Apr 28, 2025 •

edited

Loading

neiljp Apr 27, 2025

neiljp Apr 27, 2025

Gopinath-Mahendiran Apr 28, 2025 •

edited

Loading

neiljp Apr 27, 2025

Gopinath-Mahendiran Apr 28, 2025

neiljp commented Apr 27, 2025

		api_key = config.get("api", "key")
		if passcmd is None and config.has_option("api", "passcmd"):

		f'Failed to retrieve API key using passcmd "{passcmd}".'
		f"Command exited with return code {err.returncode}."

Add function to process the passcmd for API key retrieval. #866

Are you sure you want to change the base?

Add function to process the passcmd for API key retrieval. #866

Conversation

Gopinath-Mahendiran commented Apr 24, 2025

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented Apr 26, 2025

Gopinath-Mahendiran commented Apr 26, 2025 • edited Loading

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gopinath-Mahendiran Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gopinath-Mahendiran Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented Apr 27, 2025

Gopinath-Mahendiran commented Apr 26, 2025 •

edited

Loading

Gopinath-Mahendiran Apr 28, 2025 •

edited

Loading

Gopinath-Mahendiran Apr 28, 2025 •

edited

Loading