dheerajturaga commented on code in PR #62217:
URL: https://github.com/apache/airflow/pull/62217#discussion_r2836421920
##########
airflow-ctl/src/airflowctl/ctl/commands/auth_command.py:
##########
@@ -38,7 +38,9 @@ def login(args, api_client=NEW_API_CLIENT) -> None:
success_message = "[green]Login successful! Welcome to airflowctl![/green]"
# Check is username and password are passed
if args.username and args.password:
- # Generate empty credentials with the api_url and env
+ if args.skip_keyring:
Review Comment:
This is counterproductive. A headless user who wants to authenticate via
username/password but has no keyring is the exact target use case. The
save(skip_keyring=True) path already handles this correctly (gets the token,
saves config without storing in keyring). This restriction should be removed so
the flow works end-to-end: authenticate with credentials → get token → save
config without touching keyring
##########
airflow-ctl/docs/installation/prerequisites.rst:
##########
@@ -36,6 +36,8 @@ Recommended keyring backends are:
* `KDE4 & KDE5 KWallet <https://en.wikipedia.org/wiki/KWallet>`_ (requires
`dbus <https://pypi.python.org/pypi/dbus-python>`_)
* `Windows Credential Locker
<https://docs.microsoft.com/en-us/windows/uwp/security/credential-locker>`_
+In case there's no keyring available (common in headless environments) you can
provide the token to each command. See :doc:`/security` and for more
information.
Review Comment:
```suggestion
In case there's no keyring available (common in headless environments) you
can provide the token to each command. See :doc:`/security` for more
information.
```
##########
airflow-ctl/tests/airflow_ctl/api/test_client.py:
##########
@@ -126,6 +126,36 @@ def test_save(self, mock_keyring):
"api_url": credentials.api_url,
}
+ @patch.dict(os.environ, {"AIRFLOW_CLI_ENVIRONMENT":
"TEST_SAVE_NO_KEYRING"})
+ @patch("airflowctl.api.client.keyring")
+ def test_save_no_keyring(self, mock_keyring):
+ from keyring.errors import NoKeyringError
+
+ cli_client = ClientKind.CLI
+ mock_keyring.set_password.side_effect = NoKeyringError("no backend")
+
+ with pytest.raises(AirflowCtlKeyringException, match="Keyring backend
is not available"):
+ Credentials(client_kind=cli_client).save()
+
+ @patch.dict(os.environ, {"AIRFLOW_CLI_ENVIRONMENT":
"TEST_SAVE_SKIP_KEYRING"})
+ @patch("airflowctl.api.client.keyring")
+ def test_save_no_keyring_backend_skip_keyring(self, mock_keyring):
+
+ env = "TEST_SAVE_SKIP_KEYRING"
+ cli_client = ClientKind.CLI
+ mock_keyring.set_password = MagicMock()
+
+ Credentials(client_kind=cli_client).save(skip_keyring=True)
Review Comment:
After calling save(skip_keyring=True), the test calls
Credentials(client_kind=cli_client).load(). The load() will have api_token=None
(not passed in), AIRFLOW_CLI_DEBUG_MODE not set, and AIRFLOW_CLI_TOKEN not set,
so it would attempt mock_keyring.get_password(...), which returns an
auto-created MagicMock. This means credentials.api_token is a MagicMock object,
not a real token. The test passes by luck. The test should either set
AIRFLOW_CLI_TOKEN or use `Credentials(client_kind=cli_client,
api_token="TEST_TOKEN").load()`
##########
airflow-ctl/tests/airflow_ctl/ctl/commands/test_auth_command.py:
##########
@@ -70,6 +70,52 @@ def test_login(self, mock_keyring, api_client_maker,
monkeypatch):
"airflowctl", "api_token_TEST_AUTH_LOGIN", "TEST_TOKEN"
)
+ @patch.dict(os.environ, {"AIRFLOW_CLI_TOKEN": "TEST_TOKEN"})
+ @patch("airflowctl.api.client.keyring")
+ def test_login_with_api_token_no_keyring(self, mock_keyring,
api_client_maker):
+ from keyring.errors import NoKeyringError
+
+ api_client = api_client_maker(
+ path="/auth/token/cli",
+ response_json=self.login_response.model_dump(),
+ expected_http_status_code=201,
+ kind=ClientKind.AUTH,
+ )
+
+ mock_keyring.set_password.side_effect = NoKeyringError("no backend")
+ with (
+ patch("sys.stdin", io.StringIO("test_password")),
+ patch("airflowctl.ctl.cli_config.getpass.getpass",
return_value="test_password"),
+ ):
+ auth_command.login(
+ self.parser.parse_args(
+ ["auth", "login", "--skip-keyring", "--api-url",
"http://localhost:8080";]
+ ),
+ api_client=api_client,
+ )
+
+ @patch("airflowctl.api.client.keyring")
+ def test_login_with_api_token_no_keyring_fails(self, mock_keyring,
api_client_maker):
Review Comment:
The test name says "with_api_token" but no --api-token is involved - it's
testing that auth login without --skip-keyring propagates NoKeyringError. A
more accurate name would be
`test_login_without_skip_keyring_raises_on_no_keyring`
##########
airflow-ctl-tests/tests/airflowctl_tests/conftest.py:
##########
@@ -35,16 +36,32 @@
from tests_common.test_utils.fernet import generate_fernet_key_string
[email protected](scope="module")
+def api_token():
+ url = "http://localhost:8080/auth/token";
+ payload = {"username": "airflow", "password": "airflow"}
+ try:
+ response = requests.post(url, json=payload)
+ response.raise_for_status()
+ token = response.json().get("access_token")
+ if not token:
+ raise ValueError("Response did not contain an access_token")
+ return token
+ except requests.exceptions.RequestException as e:
+ print(f"Error obtaining token: {e}")
+ return None
Review Comment:
This returns None on failure. Any test receiving api_token=None will
silently pass or produce a cryptic failure later (e.g.,
env_vars["AIRFLOW_CLI_TOKEN"] = None sets the env var to the string "None"). It
should use pytest.skip() or pytest.fail() instead.
##########
airflow-ctl/src/airflowctl/api/client.py:
##########
@@ -412,7 +422,11 @@ def decorator(func: Callable[PS, RT]) -> Callable[PS, RT]:
@wraps(func)
def wrapper(*args, **kwargs) -> RT:
if "api_client" not in kwargs:
- with get_client(kind=kind) as api_client:
+ if len(args) > 0 and isinstance(args[0], Namespace):
Review Comment:
If an internal caller passes something other than a Namespace as the first
positional arg, api_token is silently dropped. A safer approach: `api_token =
getattr(args[0], 'api_token', None) if args else None`. This also removes the
dependency on importing argparse.Namespace into client.py, which is an odd
coupling between the API layer and the CLI layer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
