Copilot commented on code in PR #64422:
URL: https://github.com/apache/airflow/pull/64422#discussion_r3107041116
##########
providers/git/tests/unit/git/hooks/test_git.py:
##########
@@ -352,3 +437,130 @@ def test_passphrase_askpass_cleaned_up(self,
create_connection_without_db):
assert os.path.exists(askpass_path)
# Both the askpass script and the temp key file should be cleaned up
assert not os.path.exists(askpass_path)
+
+ # --- GitHub App auth tests ---
+
+ def test_only_app_id_without_installation_id_raises(self):
+ with pytest.raises(
+ AirflowException, match="Both 'github_app_id' and
'github_installation_id' must be provided"
+ ):
+ GitHook(git_conn_id=CONN_APP_ONLY_APP_ID)
+
+ def test_only_installation_id_without_app_id_raises(self):
+ with pytest.raises(
+ AirflowException,
+ match="Both 'github_app_id' and 'github_installation_id' must be
provided",
+ ):
+ GitHook(git_conn_id=CONN_APP_ONLY_INSTALLATION_ID)
+
+ def test_app_id_and_installation_id_without_key_raises(self):
+ with pytest.raises(
+ AirflowException,
+ match="Missing inline private_key or key_file for GitHub App Auth",
+ ):
+ GitHook(git_conn_id=CONN_APP_NO_KEY)
+
+ def test_invalid_github_app_id_raises(self):
+ with pytest.raises(
+ AirflowException,
+ match="Invalid 'github_app_id' value",
+ ):
+ GitHook(git_conn_id=CONN_APP_INVALID_APP_ID)
+
+ def test_invalid_github_installation_id_raises(self):
+ with pytest.raises(
+ AirflowException,
+ match="Invalid 'github_installation_id' value",
+ ):
+ GitHook(git_conn_id=CONN_APP_INVALID_INSTALLATION_ID)
+
+ def test_app_auth_with_key_file_reads_file(self,
create_connection_without_db, tmp_path):
+ key_file = tmp_path / "app_key.pem"
+ key_file.write_text("file_pem_key_content")
+ create_connection_without_db(
+ Connection(
+ conn_id="git_app_key_file",
+ host=AIRFLOW_HTTPS_URL,
+ conn_type="git",
+ extra={
+ "github_app_id": "12345",
+ "github_installation_id": "67890",
+ "key_file": str(key_file),
+ },
+ )
+ )
+ with pytest.raises(Exception):
+ # PyGithub not available or auth fails, but key should be read
+ hook = GitHook(git_conn_id="git_app_key_file")
+ assert hook.private_key == "file_pem_key_content"
+
Review Comment:
`test_app_auth_with_key_file_reads_file` is currently ineffective/flaky: the
`assert hook.private_key == ...` is inside `with pytest.raises(Exception)`, so
if `GitHook(...)` raises (e.g., missing PyGithub) the assert never executes,
and if it doesn’t raise the test fails. Instead, stub `_get_github_app_token`
(like the other success tests) so initialization succeeds, then assert the key
file content was loaded; if you want to test the ImportError path, add a
dedicated test that specifically asserts the AirflowException message.
```suggestion
with pytest.MonkeyPatch().context() as mp:
mp.setattr(
"airflow.providers.git.hooks.git.GitHook._get_github_app_token",
lambda self: ("x-access-token", "ghs_test_token"),
)
hook = GitHook(git_conn_id="git_app_key_file")
assert hook.private_key == "file_pem_key_content"
```
##########
providers/git/src/airflow/providers/git/hooks/git.py:
##########
@@ -104,10 +110,50 @@ def __init__(
self.host_proxy_cmd = extra.get("host_proxy_cmd")
self.ssh_port: int | None = int(extra["ssh_port"]) if
extra.get("ssh_port") else None
+ # GitHub App Auth Options
+ raw_github_app_id = extra.get("github_app_id")
+ if raw_github_app_id is not None:
+ try:
+ self.github_app_id: int | None = int(raw_github_app_id)
+ except (TypeError, ValueError) as exc:
+ raise AirflowException(
+ f"Invalid 'github_app_id' value {raw_github_app_id!r}. It
must be an integer."
+ ) from exc
+ else:
+ self.github_app_id = None
+
+ raw_github_installation_id = extra.get("github_installation_id")
+ if raw_github_installation_id is not None:
+ try:
+ self.github_installation_id: int | None =
int(raw_github_installation_id)
+ except (TypeError, ValueError) as exc:
+ raise AirflowException(
+ f"Invalid 'github_installation_id' value
{raw_github_installation_id!r}. It must be an integer."
+ ) from exc
+ else:
+ self.github_installation_id = None
self.env: dict[str, str] = {}
if self.key_file and self.private_key:
raise AirflowException("Both 'key_file' and 'private_key' cannot
be provided at the same time")
+ if (self.github_app_id and not self.github_installation_id) or (
+ not self.github_app_id and self.github_installation_id
+ ):
+ raise AirflowException(
+ "Both 'github_app_id' and 'github_installation_id' must be
provided to use GitHub App Authentication"
+ )
+ if self.github_app_id and self.github_installation_id:
+ if not self.key_file and not self.private_key:
+ raise AirflowException("Missing inline private_key or key_file
for GitHub App Auth")
+ if self.key_file and not self.private_key:
+ try:
+ with open(self.key_file, encoding="utf-8") as key_file:
+ self.private_key = key_file.read()
+ except OSError as exc:
+ raise AirflowException(
+ f"Failed to read GitHub App private key file
{self.key_file!r}: {exc}"
+ ) from exc
+ self.user_name, self.auth_token = self._get_github_app_token()
Review Comment:
GitHub App auth reuses `key_file`/`private_key` (SSH fields) to hold the App
PEM key. Since `GitHook.configure_hook_env()` treats these as SSH key material,
any DagBundle clone (see `providers/git/.../bundles/git.py` which wraps
clone/fetch in `configure_hook_env()`) will write the GitHub App private key to
a temp file and set `GIT_SSH_COMMAND` even though GitHub App auth is
HTTPS-based. This is unnecessary key exposure and can also break SSH-based
operations (e.g., submodules) by trying to use the GitHub App PEM as an SSH
key. Consider storing the App key in separate attributes and/or making
`configure_hook_env()` a no-op for HTTPS/GitHub-App-auth connections; also fail
fast if GitHub App auth is configured but `repo_url` isn’t http(s) (e.g.,
`git@...`).
--
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]
