Copilot commented on code in PR #64643:
URL: https://github.com/apache/airflow/pull/64643#discussion_r3066476752
##########
providers/samba/src/airflow/providers/samba/hooks/samba.py:
##########
@@ -43,29 +43,55 @@ class SambaHook(BaseHook):
the connection is used in its place.
:param share_type:
An optional share type name. If this is unset then it will assume a
posix share type.
+ :param auth_protocol:
+ An optional authentication protocol. If this is unset then it defaults
to negotiate.
"""
conn_name_attr = "samba_conn_id"
default_conn_name = "samba_default"
conn_type = "samba"
hook_name = "Samba"
+ VALID_AUTH_PROTOCOLS = {"negotiate", "ntlm", "kerberos"}
+
def __init__(
self,
samba_conn_id: str = default_conn_name,
share: str | None = None,
share_type: Literal["posix", "windows"] | None = None,
+ auth_protocol: Literal["negotiate", "ntlm", "kerberos"] | None = None,
) -> None:
super().__init__()
conn = self.get_connection(samba_conn_id)
+ extra = conn.extra_dejson
+
+ self._auth_protocol: str = (
+ auth_protocol or extra.get("auth_protocol") or extra.get("auth")
or "negotiate"
Review Comment:
`extra.get(\"auth\")` is a very generic key and may be used for other
purposes in existing connections; interpreting it as an auth protocol can cause
unexpected `ValueError`s or behavior changes. To keep backward compatibility
without broadening the API unexpectedly, only treat the legacy `auth` key as an
auth protocol when it’s a string and matches a supported protocol (or
explicitly equals `'kerberos'`), otherwise ignore it.
```suggestion
legacy_auth = extra.get("auth")
if isinstance(legacy_auth, str) and legacy_auth in
self.VALID_AUTH_PROTOCOLS:
legacy_auth_protocol = legacy_auth
else:
legacy_auth_protocol = None
self._auth_protocol: str = (
auth_protocol or extra.get("auth_protocol") or
legacy_auth_protocol or "negotiate"
```
##########
providers/samba/pyproject.toml:
##########
@@ -70,6 +70,9 @@ dependencies = [
"google" = [
"apache-airflow-providers-google"
]
+"kerberos" = [
+ "smbprotocol[kerberos]>=1.5.0",
Review Comment:
The runtime check in `SambaHook` requires the `krb5` module to be
importable, but this optional extra only adds `smbprotocol[kerberos]`. If
`smbprotocol[kerberos]` doesn’t reliably provide an importable `krb5` module
across environments, users can still hit
`AirflowOptionalProviderFeatureException` even after installing
`apache-airflow-providers-samba[kerberos]`. Consider adding an explicit
dependency that provides the `krb5` import (or updating the import/check to
match the package actually installed by the extra), so the error message and
extras are consistent.
```suggestion
"smbprotocol[kerberos]>=1.5.0",
"krb5",
```
##########
providers/samba/tests/unit/samba/hooks/test_samba.py:
##########
@@ -191,6 +196,99 @@ def test__join_path(
hook = SambaHook("samba_default", share_type=path_type)
assert hook._join_path(path) == full_path
+ @mock.patch.dict("sys.modules", {"krb5": mock.MagicMock()})
+ @mock.patch("smbclient.register_session")
+ @mock.patch(f"{BASEHOOK_PATCH_PATH}.get_connection")
+ def test_kerberos_auth_via_extra(self, get_conn_mock, register_session):
+ """Test that auth_protocol='kerberos' from extra is passed to
smbclient."""
+ connection = Connection(
+ host="kerb-host.example.com",
+ schema="share",
+ extra='{"auth_protocol": "kerberos"}',
+ )
+ get_conn_mock.return_value = connection
+ register_session.return_value = None
+ with SambaHook("samba_default"):
+ _, kwargs = tuple(register_session.call_args_list[0])
+ assert kwargs["auth_protocol"] == "kerberos"
+ assert kwargs["username"] is None
+ assert kwargs["password"] is None
+
+ @mock.patch.dict("sys.modules", {"krb5": mock.MagicMock()})
+ @mock.patch("smbclient.register_session")
+ @mock.patch(f"{BASEHOOK_PATCH_PATH}.get_connection")
+ def test_kerberos_auth_via_legacy_auth_key(self, get_conn_mock,
register_session):
+ """Test backward compat: extra {"auth": "kerberos"} is recognized."""
+ connection = Connection(
+ host="kerb-host.example.com",
+ schema="share",
+ extra='{"auth": "kerberos"}',
+ )
+ get_conn_mock.return_value = connection
+ register_session.return_value = None
+ with SambaHook("samba_default"):
+ _, kwargs = tuple(register_session.call_args_list[0])
+ assert kwargs["auth_protocol"] == "kerberos"
+
+ @mock.patch.dict("sys.modules", {"krb5": mock.MagicMock()})
+ @mock.patch("smbclient.register_session")
+ @mock.patch(f"{BASEHOOK_PATCH_PATH}.get_connection")
+ def test_kerberos_auth_via_constructor(self, get_conn_mock,
register_session):
+ """Test that constructor auth_protocol overrides extra."""
+ connection = Connection(
+ host="kerb-host.example.com",
+ schema="share",
+ login="user",
+ password="pass",
+ )
+ get_conn_mock.return_value = connection
+ register_session.return_value = None
+ with SambaHook("samba_default", auth_protocol="kerberos"):
+ _, kwargs = tuple(register_session.call_args_list[0])
+ assert kwargs["auth_protocol"] == "kerberos"
+
+ @mock.patch(f"{BASEHOOK_PATCH_PATH}.get_connection")
+ def test_invalid_auth_protocol_raises(self, get_conn_mock):
+ """Test that an invalid auth_protocol raises ValueError."""
+ connection = Connection(
+ host="host",
+ schema="share",
+ extra='{"auth_protocol": "invalid"}',
+ )
+ get_conn_mock.return_value = connection
+ with pytest.raises(ValueError, match="Invalid auth_protocol
'invalid'"):
+ SambaHook("samba_default")
+
+ @mock.patch(f"{BASEHOOK_PATCH_PATH}.get_connection")
+ def test_kerberos_without_dependency_raises(self, get_conn_mock):
+ """Test that kerberos auth without krb5 installed raises
AirflowOptionalProviderFeatureException."""
+ connection = Connection(
+ host="kerb-host.example.com",
+ schema="share",
+ extra='{"auth_protocol": "kerberos"}',
+ )
+ get_conn_mock.return_value = connection
+ with mock.patch.dict("sys.modules", {"krb5": None}):
+ with pytest.raises(AirflowOptionalProviderFeatureException,
match="krb5"):
+ SambaHook("samba_default")
Review Comment:
This test’s approach to simulating a missing `krb5` dependency can be
brittle: setting `sys.modules['krb5'] = None` may not consistently force
`import krb5` to raise `ImportError` across Python/importlib behaviors, which
can make the test flaky (especially if `krb5` is installed in the test
environment). A more deterministic approach is to patch the import mechanism
for `krb5` (e.g., patch `builtins.__import__` to raise `ImportError` for name
`'krb5'`, or patch a helper you control) so the exception path is guaranteed.
##########
providers/samba/src/airflow/providers/samba/hooks/samba.py:
##########
@@ -325,12 +352,22 @@ def get_ui_field_behaviour(cls) -> dict[str, Any]:
def get_connection_form_widgets(cls) -> dict[str, Any]:
"""Return connection widgets to add to connection form."""
from flask_babel import lazy_gettext
- from wtforms import StringField
+ from wtforms import SelectField, StringField
return {
"share_type": StringField(
label=lazy_gettext("Share Type"),
description="The share OS type (`posix` or `windows`). Used to
determine the formatting of file and folder paths.",
default="posix",
- )
+ ),
+ "auth_protocol": SelectField(
+ label=lazy_gettext("Auth Protocol"),
+ description=(
+ "Authentication protocol: `negotiate` (auto-select,
default), "
+ "`ntlm`, or `kerberos`. When using `kerberos`, the
system's "
+ "Kerberos ticket cache is used and username/password are
optional."
+ ),
+ choices=["negotiate", "ntlm", "kerberos"],
Review Comment:
For WTForms `SelectField`, using `choices` as `(value, label)` pairs is more
explicit and makes it easier to localize labels via `lazy_gettext` while
keeping stable stored values. Consider changing `choices` to tuples like
`[(\"negotiate\", lazy_gettext(\"negotiate\")), ...]` (or non-localized labels
if you prefer) to avoid ambiguity and improve UI/i18n maintainability.
```suggestion
choices=[
("negotiate", lazy_gettext("negotiate")),
("ntlm", lazy_gettext("ntlm")),
("kerberos", lazy_gettext("kerberos")),
],
```
--
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]
