diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 628ad59f5..bbda5af15 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -35,6 +35,12 @@ list = builtins.list dict = builtins.dict +# AuthResolver has no optional deps (stdlib + internal utils only), so it must +# be imported unconditionally here -- NOT inside the APM_DEPS_AVAILABLE guard. +# If it were gated, a missing optional dep (e.g. GitPython) would cause a +# NameError in install() before the graceful APM_DEPS_AVAILABLE check fires. +from ..core.auth import AuthResolver + # APM Dependencies (conditional import for graceful degradation) APM_DEPS_AVAILABLE = False _APM_IMPORT_ERROR = None @@ -56,7 +62,7 @@ # --------------------------------------------------------------------------- -def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, logger=None): +def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, logger=None, auth_resolver=None): """Validate packages exist and can be accessed, then add to apm.yml dependencies section. Implements normalize-on-write: any input form (HTTPS URL, SSH URL, FQDN, shorthand) @@ -68,6 +74,7 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo dry_run: If True, only show what would be added. dev: If True, write to devDependencies instead of dependencies. logger: InstallLogger for structured output. + auth_resolver: Shared auth resolver for caching credentials. Returns: Tuple of (validated_packages list, _ValidationOutcome). @@ -146,7 +153,7 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo # Validate package exists and is accessible verbose = bool(logger and logger.verbose) - if _validate_package_exists(package, verbose=verbose): + if _validate_package_exists(package, verbose=verbose, auth_resolver=auth_resolver): valid_outcomes.append((canonical, already_in_deps)) if logger: logger.validation_pass(canonical, already_present=already_in_deps) @@ -258,7 +265,7 @@ def _local_path_no_markers_hint(local_dir, verbose_log=None): _rich_echo(f" ... and {len(found) - 5} more", color="dim") -def _validate_package_exists(package, verbose=False): +def _validate_package_exists(package, verbose=False, auth_resolver=None): """Validate that a package exists and is accessible on GitHub, Azure DevOps, or locally.""" import os import subprocess @@ -266,7 +273,9 @@ def _validate_package_exists(package, verbose=False): from apm_cli.core.auth import AuthResolver verbose_log = (lambda msg: _rich_echo(f" {msg}", color="dim")) if verbose else None - auth_resolver = AuthResolver() + # Use provided resolver or create new one if not in a CLI session context + if auth_resolver is None: + auth_resolver = AuthResolver() try: # Parse the package to check if it's a virtual package or ADO @@ -300,8 +309,8 @@ def _validate_package_exists(package, verbose=False): org = dep_ref.repo_url.split('/')[0] if dep_ref.repo_url and '/' in dep_ref.repo_url else None if verbose_log: verbose_log(f"Auth resolved: host={host}, org={org}, source={ctx.source}, type={ctx.token_type}") - downloader = GitHubPackageDownloader(auth_resolver=auth_resolver) - result = downloader.validate_virtual_package_exists(dep_ref) + virtual_downloader = GitHubPackageDownloader(auth_resolver=auth_resolver) + result = virtual_downloader.validate_virtual_package_exists(dep_ref) if not result and verbose_log: try: err_ctx = auth_resolver.build_error_context(host, f"accessing {package}", org=org) @@ -316,26 +325,39 @@ def _validate_package_exists(package, verbose=False): if dep_ref.is_azure_devops() or (dep_ref.host and dep_ref.host != "github.com"): from apm_cli.utils.github_host import is_github_hostname, is_azure_devops_hostname - downloader = GitHubPackageDownloader() + # Determine host type before building the URL so we know whether to + # embed a token. Generic (non-GitHub, non-ADO) hosts are excluded + # from APM-managed auth; they rely on git credential helpers via the + # relaxed validate_env below. + is_generic = not is_github_hostname(dep_ref.host) and not is_azure_devops_hostname(dep_ref.host) + + # For GHES / ADO: resolve per-dependency auth up front so the URL + # carries an embedded token and avoids triggering OS credential + # helper popups during git ls-remote validation. + _url_token = None + if not is_generic: + _dep_ctx = auth_resolver.resolve_for_dep(dep_ref) + _url_token = _dep_ctx.token + + ado_downloader = GitHubPackageDownloader(auth_resolver=auth_resolver) # Set the host if dep_ref.host: - downloader.github_host = dep_ref.host + ado_downloader.github_host = dep_ref.host - # Build authenticated URL using downloader's auth - package_url = downloader._build_repo_url( - dep_ref.repo_url, use_ssh=False, dep_ref=dep_ref + # Build authenticated URL using the resolved per-dep token. + package_url = ado_downloader._build_repo_url( + dep_ref.repo_url, use_ssh=False, dep_ref=dep_ref, token=_url_token ) # For generic hosts (not GitHub, not ADO), relax the env so native # credential helpers (SSH keys, macOS Keychain, etc.) can work. # This mirrors _clone_with_fallback() which does the same relaxation. - is_generic = not is_github_hostname(dep_ref.host) and not is_azure_devops_hostname(dep_ref.host) if is_generic: - validate_env = {k: v for k, v in downloader.git_env.items() + validate_env = {k: v for k, v in ado_downloader.git_env.items() if k not in ('GIT_ASKPASS', 'GIT_CONFIG_GLOBAL', 'GIT_CONFIG_NOSYSTEM')} validate_env['GIT_TERMINAL_PROMPT'] = '0' else: - validate_env = {**os.environ, **downloader.git_env} + validate_env = {**os.environ, **ado_downloader.git_env} if verbose_log: verbose_log(f"Trying git ls-remote for {dep_ref.host}") @@ -550,6 +572,10 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo is_partial = bool(packages) logger = InstallLogger(verbose=verbose, dry_run=dry_run, partial=is_partial) + # Create shared auth resolver for all downloads in this CLI invocation + # to ensure credentials are cached and reused (prevents duplicate auth popups) + auth_resolver = AuthResolver() + # Check if apm.yml exists apm_yml_exists = Path(APM_YML_FILENAME).exists() @@ -571,7 +597,7 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo # If packages are specified, validate and add them to apm.yml first if packages: validated_packages, outcome = _validate_and_add_packages_to_apm_yml( - packages, dry_run, dev=dev, logger=logger, + packages, dry_run, dev=dev, logger=logger, auth_resolver=auth_resolver, ) # Short-circuit: all packages failed validation — nothing to install if outcome.all_failed: @@ -672,6 +698,7 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo apm_package, update, verbose, only_pkgs, force=force, parallel_downloads=parallel_downloads, logger=logger, + auth_resolver=auth_resolver, target=target, ) apm_count = install_result.installed_count @@ -1092,6 +1119,7 @@ def _install_apm_dependencies( force: bool = False, parallel_downloads: int = 4, logger: "InstallLogger" = None, + auth_resolver: "AuthResolver" = None, target: str = None, ): """Install APM package dependencies. @@ -1104,6 +1132,7 @@ def _install_apm_dependencies( force: Whether to overwrite locally-authored files on collision parallel_downloads: Max concurrent downloads (0 disables parallelism) logger: InstallLogger for structured output + auth_resolver: Shared auth resolver for caching credentials target: Explicit target override from --target CLI flag """ if not APM_DEPS_AVAILABLE: @@ -1137,8 +1166,12 @@ def _install_apm_dependencies( apm_modules_dir = project_root / APM_MODULES_DIR apm_modules_dir.mkdir(exist_ok=True) + # Use provided resolver or create new one if not in a CLI session context + if auth_resolver is None: + auth_resolver = AuthResolver() + # Create downloader early so it can be used for transitive dependency resolution - downloader = GitHubPackageDownloader() + downloader = GitHubPackageDownloader(auth_resolver=auth_resolver) # Track direct dependency keys so the download callback can distinguish them from transitive direct_dep_keys = builtins.set(dep.get_unique_key() for dep in apm_deps) @@ -1499,13 +1532,9 @@ def _collect_descendants(node, visited=None): _pre_downloaded_keys = builtins.set(_pre_download_results.keys()) # Create progress display for sequential integration - _auth_resolver = None - if verbose: - try: - from apm_cli.core.auth import AuthResolver - _auth_resolver = AuthResolver() - except Exception: - pass + # Reuse the shared auth_resolver (already created in this invocation) so + # verbose auth logging does not trigger a duplicate credential-helper popup. + _auth_resolver = auth_resolver with Progress( SpinnerColumn(), diff --git a/src/apm_cli/core/auth.py b/src/apm_cli/core/auth.py index e1e56ad77..fc18d4ca8 100644 --- a/src/apm_cli/core/auth.py +++ b/src/apm_cli/core/auth.py @@ -182,25 +182,30 @@ def resolve(self, host: str, org: Optional[str] = None) -> AuthContext: """Resolve auth for *(host, org)*. Cached & thread-safe.""" key = (host.lower() if host else host, org.lower() if org else org) with self._lock: - if key in self._cache: - return self._cache[key] - - host_info = self.classify_host(host) - token, source = self._resolve_token(host_info, org) - token_type = self.detect_token_type(token) if token else "unknown" - git_env = self._build_git_env(token) - - ctx = AuthContext( - token=token, - source=source, - token_type=token_type, - host_info=host_info, - git_env=git_env, - ) - - with self._lock: + cached = self._cache.get(key) + if cached is not None: + return cached + + # Hold lock during entire credential resolution to prevent duplicate + # credential-helper popups when parallel downloads resolve the same + # (host, org) concurrently. The first caller fills the cache; all + # subsequent callers for the same key become O(1) cache hits. + # Bounded by APM_GIT_CREDENTIAL_TIMEOUT (default 60s). No deadlock + # risk: single lock, never nested. + host_info = self.classify_host(host) + token, source = self._resolve_token(host_info, org) + token_type = self.detect_token_type(token) if token else "unknown" + git_env = self._build_git_env(token) + + ctx = AuthContext( + token=token, + source=source, + token_type=token_type, + host_info=host_info, + git_env=git_env, + ) self._cache[key] = ctx - return ctx + return ctx def resolve_for_dep(self, dep_ref: "DependencyReference") -> AuthContext: """Resolve auth from a ``DependencyReference``.""" diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index f5e10e6f8..2de9e70b9 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -186,20 +186,18 @@ def _setup_git_environment(self) -> Dict[str, Any]: else: env['GIT_CONFIG_GLOBAL'] = '/dev/null' - # Resolve default host tokens via AuthResolver (backward compat properties) - default_ctx = self.auth_resolver.resolve(default_host()) - self._default_github_ctx = default_ctx - self.github_token = default_ctx.token - self.has_github_token = default_ctx.token is not None - self._github_token_from_credential_fill = ( - self.has_github_token - and self.token_manager.get_token_for_purpose('modules', env) is None - ) - - # Azure DevOps - ado_ctx = self.auth_resolver.resolve("dev.azure.com") - self.ado_token = ado_ctx.token - self.has_ado_token = ado_ctx.token is not None + # IMPORTANT: Do not resolve credentials via helpers at construction time. + # AuthResolver.resolve(...) can trigger OS credential helper UI. If we do + # this eagerly (host-only key) and later resolve per-dependency (host+org), + # users can see duplicate auth prompts. Keep constructor token state env-only + # and resolve lazily per dependency during clone/validate flows. + self.github_token = self.token_manager.get_token_for_purpose('modules', env) + self.has_github_token = self.github_token is not None + self._github_token_from_credential_fill = False + + # Azure DevOps (env-only at init; lazy auth resolution happens per dep) + self.ado_token = self.token_manager.get_token_for_purpose('ado_modules', env) + self.has_ado_token = self.ado_token is not None # JFrog Artifactory (not host-based, uses dedicated env var) self.artifactory_token = self.token_manager.get_token_for_purpose('artifactory_modules', env) @@ -648,7 +646,9 @@ def _clone_with_fallback(self, repo_url_base: str, target_path: Path, progress_r f"If this package lives on a different server (e.g., github.com), " f"use the full hostname in apm.yml: {suggested}" ) - elif not self.has_github_token: + elif not has_token: + # No auth was resolved (neither env var nor credential helper). + # Guide the user through setting up authentication. host = dep_host or default_host() org = dep_ref.repo_url.split('/')[0] if dep_ref and dep_ref.repo_url else None error_msg += self.auth_resolver.build_error_context(host, "clone", org=org) diff --git a/tests/test_github_downloader.py b/tests/test_github_downloader.py index ec941ddf9..28c47d402 100644 --- a/tests/test_github_downloader.py +++ b/tests/test_github_downloader.py @@ -69,6 +69,15 @@ def test_setup_git_environment_no_token(self): # Should not have GitHub tokens in environment assert 'GITHUB_TOKEN' not in env or not env['GITHUB_TOKEN'] assert 'GH_TOKEN' not in env or not env['GH_TOKEN'] + + def test_setup_git_environment_does_not_eagerly_call_credential_helper(self): + """Constructor should not invoke git credential helper (lazy per-dep auth).""" + with patch.dict(os.environ, {}, clear=True): + with patch( + 'apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git' + ) as mock_cred: + GitHubPackageDownloader() + mock_cred.assert_not_called() @patch('apm_cli.deps.github_downloader.Repo') @patch('tempfile.mkdtemp') diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index 9daa10d09..ab63b3b2f 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -1,6 +1,8 @@ """Unit tests for AuthResolver, HostInfo, and AuthContext.""" import os +import time +from concurrent.futures import ThreadPoolExecutor from unittest.mock import patch import pytest @@ -134,6 +136,25 @@ def test_caching(self): ctx2 = resolver.resolve("github.com", org="microsoft") assert ctx1 is ctx2 + def test_caching_is_singleflight_under_concurrency(self): + """Concurrent resolve() calls for the same key should populate cache once.""" + resolver = AuthResolver() + + def _slow_resolve_token(host_info, org): + time.sleep(0.05) + return ("cred-token", "git-credential-fill") + + with patch.object(AuthResolver, "_resolve_token", side_effect=_slow_resolve_token) as mock_resolve: + with ThreadPoolExecutor(max_workers=8) as pool: + futures = [ + pool.submit(resolver.resolve, "github.com", "microsoft") + for _ in range(8) + ] + contexts = [f.result() for f in futures] + + assert mock_resolve.call_count == 1 + assert all(ctx is contexts[0] for ctx in contexts) + def test_different_orgs_different_cache(self): """Different orgs get different cache entries.""" with patch.dict(os.environ, {