diff --git a/docs/src/content/docs/getting-started/authentication.md b/docs/src/content/docs/getting-started/authentication.md index 559c757a..8215a982 100644 --- a/docs/src/content/docs/getting-started/authentication.md +++ b/docs/src/content/docs/getting-started/authentication.md @@ -21,6 +21,10 @@ When APM has a token for a recognized host (GitHub.com, GitHub Enterprise under For single-file downloads from GitHub (which use the GitHub API rather than `git clone`), APM also queries `git credential fill` as a last-resort fallback when no token environment variable is set. This means credentials stored by `gh auth login` or your OS keychain work for both folder-level and file-level dependencies. +`git credential fill` can trigger interactive account selection on some systems. APM waits up to 60 seconds by default for this step. If your credential helper is slower, set `APM_GIT_CREDENTIAL_TIMEOUT` (seconds, max 180). + +Within a single APM command, credential-helper lookups are cached per host. This avoids repeated account-selection prompts when multiple install phases need authentication in the same run. + ### Object-style `git:` references The `git:` object form in `apm.yml` lets you reference any git URL explicitly — HTTPS, SSH, or any host: diff --git a/src/apm_cli/core/token_manager.py b/src/apm_cli/core/token_manager.py index 4da53b79..10837948 100644 --- a/src/apm_cli/core/token_manager.py +++ b/src/apm_cli/core/token_manager.py @@ -20,11 +20,22 @@ import os import subprocess +import threading from typing import Dict, Optional, Tuple class GitHubTokenManager: """Manages GitHub token environment setup for different AI runtimes.""" + + # `git credential fill` may invoke OS credential helpers (and account pickers + # on Windows). Give it enough time by default to avoid false negatives. + DEFAULT_GIT_CREDENTIAL_TIMEOUT_SECONDS = 60 + MAX_GIT_CREDENTIAL_TIMEOUT_SECONDS = 180 + + # Shared process-wide cache so multiple downloader/token-manager instances + # in a single command do not repeatedly invoke credential helpers. + _SHARED_CREDENTIAL_CACHE: Dict[str, Optional[str]] = {} + _SHARED_CREDENTIAL_CACHE_LOCK = threading.Lock() # Define token precedence for different use cases TOKEN_PRECEDENCE = { @@ -49,7 +60,7 @@ def __init__(self, preserve_existing: bool = True): preserve_existing: If True, never overwrite existing environment variables """ self.preserve_existing = preserve_existing - self._credential_cache: Dict[str, Optional[str]] = {} + self._credential_cache = self._SHARED_CREDENTIAL_CACHE @staticmethod def _is_valid_credential_token(token: str) -> bool: @@ -69,6 +80,28 @@ def _is_valid_credential_token(token: str) -> bool: return False return True + @classmethod + def _get_git_credential_timeout_seconds(cls) -> int: + """Get timeout for `git credential fill` from env or default. + + Environment override: + - APM_GIT_CREDENTIAL_TIMEOUT: integer seconds (1..180) + """ + raw = os.environ.get("APM_GIT_CREDENTIAL_TIMEOUT", "").strip() + if not raw: + return cls.DEFAULT_GIT_CREDENTIAL_TIMEOUT_SECONDS + + try: + parsed = int(raw) + except ValueError: + return cls.DEFAULT_GIT_CREDENTIAL_TIMEOUT_SECONDS + + if parsed < 1: + return cls.DEFAULT_GIT_CREDENTIAL_TIMEOUT_SECONDS + if parsed > cls.MAX_GIT_CREDENTIAL_TIMEOUT_SECONDS: + return cls.MAX_GIT_CREDENTIAL_TIMEOUT_SECONDS + return parsed + @staticmethod def resolve_credential_from_git(host: str) -> Optional[str]: """Resolve a credential from the git credential store. @@ -83,13 +116,15 @@ def resolve_credential_from_git(host: str) -> Optional[str]: Returns: The password/token from the credential store, or None if unavailable """ + timeout_seconds = GitHubTokenManager._get_git_credential_timeout_seconds() + try: result = subprocess.run( ['git', 'credential', 'fill'], input=f"protocol=https\nhost={host}\n\n", capture_output=True, text=True, - timeout=5, + timeout=timeout_seconds, env={**os.environ, 'GIT_TERMINAL_PROMPT': '0', 'GIT_ASKPASS': ''}, ) if result.returncode != 0: @@ -167,13 +202,18 @@ def get_token_with_credential_fallback(self, purpose: str, host: str, env: Optio token = self.get_token_for_purpose(purpose, env) if token: return token - - if host in self._credential_cache: - return self._credential_cache[host] - + + with self._SHARED_CREDENTIAL_CACHE_LOCK: + if host in self._credential_cache: + return self._credential_cache[host] + credential = self.resolve_credential_from_git(host) - self._credential_cache[host] = credential - return credential + + with self._SHARED_CREDENTIAL_CACHE_LOCK: + # Keep first computed value if another thread filled cache first. + if host not in self._credential_cache: + self._credential_cache[host] = credential + return self._credential_cache[host] def validate_tokens(self, env: Optional[Dict[str, str]] = None) -> Tuple[bool, str]: """Validate that required tokens are available. diff --git a/tests/test_token_manager.py b/tests/test_token_manager.py index 3b21a2e2..af5ad299 100644 --- a/tests/test_token_manager.py +++ b/tests/test_token_manager.py @@ -9,6 +9,16 @@ from src.apm_cli.core.token_manager import GitHubTokenManager +@pytest.fixture(autouse=True) +def clear_shared_credential_cache(): + """Ensure shared credential cache is isolated between tests.""" + with GitHubTokenManager._SHARED_CREDENTIAL_CACHE_LOCK: + GitHubTokenManager._SHARED_CREDENTIAL_CACHE.clear() + yield + with GitHubTokenManager._SHARED_CREDENTIAL_CACHE_LOCK: + GitHubTokenManager._SHARED_CREDENTIAL_CACHE.clear() + + class TestModulesTokenPrecedence: """Test GH_TOKEN addition to the modules token precedence chain.""" @@ -204,6 +214,42 @@ def test_accepts_valid_gho_token(self): token = GitHubTokenManager.resolve_credential_from_git('github.com') assert token == 'gho_abc123def456' + def test_uses_default_credential_timeout(self): + """Uses default timeout when APM_GIT_CREDENTIAL_TIMEOUT is not set.""" + mock_result = MagicMock(returncode=0, stdout="password=tok\n") + with patch.dict(os.environ, {}, clear=True), patch( + 'subprocess.run', return_value=mock_result + ) as mock_run: + GitHubTokenManager.resolve_credential_from_git('github.com') + assert mock_run.call_args.kwargs['timeout'] == 60 + + def test_uses_custom_credential_timeout_from_env(self): + """Uses configured timeout from APM_GIT_CREDENTIAL_TIMEOUT.""" + mock_result = MagicMock(returncode=0, stdout="password=tok\n") + with patch.dict(os.environ, {'APM_GIT_CREDENTIAL_TIMEOUT': '42'}, clear=True), patch( + 'subprocess.run', return_value=mock_result + ) as mock_run: + GitHubTokenManager.resolve_credential_from_git('github.com') + assert mock_run.call_args.kwargs['timeout'] == 42 + + def test_invalid_timeout_env_falls_back_to_default(self): + """Invalid timeout env values should not break credential resolution.""" + mock_result = MagicMock(returncode=0, stdout="password=tok\n") + with patch.dict(os.environ, {'APM_GIT_CREDENTIAL_TIMEOUT': 'not-a-number'}, clear=True), patch( + 'subprocess.run', return_value=mock_result + ) as mock_run: + GitHubTokenManager.resolve_credential_from_git('github.com') + assert mock_run.call_args.kwargs['timeout'] == 60 + + def test_timeout_env_is_clamped_to_max(self): + """Very large timeout values are clamped for safety.""" + mock_result = MagicMock(returncode=0, stdout="password=tok\n") + with patch.dict(os.environ, {'APM_GIT_CREDENTIAL_TIMEOUT': '999'}, clear=True), patch( + 'subprocess.run', return_value=mock_result + ) as mock_run: + GitHubTokenManager.resolve_credential_from_git('github.com') + assert mock_run.call_args.kwargs['timeout'] == 180 + class TestIsValidCredentialToken: """Test _is_valid_credential_token validation.""" @@ -306,3 +352,20 @@ def test_different_hosts_separate_cache(self): assert tok1 == 'tok-github.com' assert tok2 == 'tok-gitlab.com' assert mock_cred.call_count == 2 + + def test_cache_is_shared_across_instances(self): + """Credential helper should be queried once per host per process.""" + with patch.dict(os.environ, {}, clear=True): + first = GitHubTokenManager() + second = GitHubTokenManager() + + with patch.object( + GitHubTokenManager, + 'resolve_credential_from_git', + return_value='shared-token', + ) as mock_cred: + tok1 = first.get_token_with_credential_fallback('modules', 'github.com') + tok2 = second.get_token_with_credential_fallback('modules', 'github.com') + assert tok1 == 'shared-token' + assert tok2 == 'shared-token' + mock_cred.assert_called_once()