fix(auth): fall back to git credentials for ADO - #2610
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates Azure DevOps (ADO) git-fetch behavior to use AuthResolver.try_with_fallback, adding a final fallback to repository-scoped git credential fill when bearer auth is rejected.
Changes:
- Route ADO marketplace
_fetch_gitclones throughAuthResolver.try_with_fallbackwith a repopathhint. - Add an ADO credential-helper fallback after bearer fallback inside
AuthResolver.try_with_fallback. - Update/add unit tests to cover the new ADO fallback routing and credential-helper fallback behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/unit/marketplace/test_client_git.py | Updates the ADO git-fetch unit test to validate auth-fallback routing and path propagation. |
| tests/unit/core/test_auth_phase3.py | Adds coverage ensuring ADO bearer rejection falls back to repository-scoped credential helper. |
| src/apm_cli/marketplace/client.py | Switches ADO _fetch_git to try_with_fallback, passing path derived from the ADO URL. |
| src/apm_cli/core/auth.py | Implements an additional ADO fallback step using git credential fill after bearer fallback fails. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| try: | ||
| if getattr(host_info, "kind", "") == "ado": | ||
| fallback_kwargs = { | ||
| "org": source.owner or None, | ||
| "path": urlsplit(source.url).path.lstrip("/"), | ||
| "unauth_first": False, | ||
| } | ||
| if source.port is not None: | ||
| fallback_kwargs["port"] = source.port | ||
| checkout_dir = auth_resolver.try_with_fallback( | ||
| host_info.host, | ||
| _checkout, | ||
| **fallback_kwargs, | ||
| ) |
There was a problem hiding this comment.
Addressed in 4bee5c3. The ADO branch now builds hardened_git_base_env() and passes it as base_env= to try_with_fallback, so PAT, bearer, and credential-helper attempts share the hardened environment.
| return _try_ado_bearer_fallback(exc) | ||
| try: | ||
| result = _try_ado_bearer_fallback(exc) | ||
| except Exception as auth_exc: |
There was a problem hiding this comment.
Addressed in 4bee5c3. The ADO credential-helper fallback now calls is_ado_auth_failure_signal() and skips git credential fill for network, TLS, 404, and other non-auth failures. Added regression coverage.
| if getattr(host_info, "kind", "") == "ado": | ||
| fallback_kwargs = { | ||
| "org": source.owner or None, | ||
| "path": urlsplit(source.url).path.lstrip("/"), | ||
| "unauth_first": False, | ||
| } |
There was a problem hiding this comment.
Addressed in 4bee5c3. Updated _fetch_git documentation to describe the ADO try_with_fallback path, repository path hint, scoped credential fill, and hardened base environment.
|
@microsoft-github-policy-service agree company="AXA UK" |
Sergio Sisternes (sergio-sisternes-epam)
added
the
panel-review
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/apm_cli/core/auth.py:759
_try_ado_credential_fallbackgates onis_ado_auth_failure_signal(str(exc)), but many ADO auth failures from Git subprocesses surface as aRuntimeErrorwhose__cause__is asubprocess.CalledProcessError(where the auth signal lives in.stderr, notstr(...)). As a result, the credential-helper fallback can be skipped even on real 401/auth failures from Git.
from apm_cli.utils.github_host import is_ado_auth_failure_signal
if not is_ado_auth_failure_signal(str(exc)):
raise exc
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 2 | Sound fallback chain extension; nested closure is idiomatic but client.py's getattr duck-typing and ADO branch warrant minor tightening. |
| CLI Logging Expert | 0 | 1 | 2 | Fallback logs are clean but silent failure on credential-not-found leaves users stranded without actionable guidance. |
| DevX UX Expert | 0 | 2 | 1 | Fallback is correctly silent-on-success but the final error when ALL fallbacks fail lacks actionable ADO-specific guidance. |
| Supply Chain Security Expert | 0 | 0 | 2 | Fallback is well-guarded: path sanitization prevents injection, credentials flow via extraheader not URL, gate predicates sound. No blocking issues. |
| OSS Growth Hacker | 0 | 1 | 1 | Fixes a silent auth cliff for Windows+GCM+ADO users -- high-value unlock for enterprise adoption, needs CHANGELOG and a story beat. |
| Auth Expert | 0 | 1 | 2 | ADO GCM fallback chain is correctly ordered and secure; one signal-matching concern and a minor logging nit. |
| Doc Writer | 0 | 5 | 0 | PR introduces a git-credential-fill fallback for ADO that is undocumented in three places; CHANGELOG entry is missing; two doc pages state the ADO chain terminates at az-cli. |
| Test Coverage Expert | 0 | 1 | 1 | Unit tests cover the new GCM fallback chain well; is_ado_auth_failure_signal has dedicated predicate tests. Integration-tier gap exists for the bearer-to-GCM path. |
| Performance Expert | 0 | 1 | 4 | ADO fallback chain can incur up to 3 git network round-trips on failure, but only on the error path for a single marketplace fetch; happy path is cost-neutral. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Add integration-with-fixtures test for bearer->git-credential-fill fallback path -- outcome:missing on a secure-by-default auth surface; unit mocks don't prove the real subprocess pipeline works end-to-end.
- [Auth Expert] Anchor _ADO_AUTH_FAILURE_SIGNALS with regex word boundaries instead of bare substring match -- Substring '401' can false-positive on port numbers or unrelated error text; cheap fix, prevents latent bug.
- [Doc Writer] Update CHANGELOG [Unreleased] and three stale doc pages (install-failures.md, authentication.md, security.md) -- User-facing auth behavior change with no CHANGELOG entry violates ship-fast-communicate-clearly principle.
- [DevX UX Expert] Wrap final raise in AuthError summarising all three failed attempts with ADO-specific guidance -- When all fallbacks fail the user sees only the original 401 with no actionable next step.
- [CLI Logging Expert] Emit _log message before re-raising when git credential fill returns no credential -- Silent failure on credential-not-found leaves users stranded without diagnostic breadcrumb.
Architecture
classDiagram
direction LR
class AuthResolver {
<<Strategy>>
+resolve(host, org, port) AuthContext
+try_with_fallback(host, operation, **kw) T
-_try_ado_credential_fallback(exc) T
-_try_ado_bearer_fallback(exc) T
-_allow_external_fallback bool
}
class TokenManager {
+resolve_credential_from_git(host, port, path) Credential
}
class HostInfo {
<<ValueObject>>
+host str
+port int
+kind str
+display_name str
}
class MarketplaceClient {
+_fetch_git(source, file_path) Path
}
class GitCache {
+get_checkout(url, ref, env, sparse_paths) Path
}
AuthResolver *-- TokenManager : delegates credential resolution
AuthResolver ..> HostInfo : reads
MarketplaceClient ..> AuthResolver : calls try_with_fallback
MarketplaceClient ..> GitCache : checkout
note for AuthResolver "Chain of Responsibility: PAT -> bearer -> git-credential-fill"
class AuthResolver:::touched
class MarketplaceClient:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["MarketplaceClient._fetch_git"] -->|host_info.kind==ado| B["AuthResolver.try_with_fallback"]
A -->|other hosts| C["AuthResolver.resolve"]
B --> D{"PAT / env token?"}
D -->|yes| E["operation(token, env)"]
D -->|no / rejected| F{"Azure CLI bearer?"}
F -->|success| E
F -->|ADO auth failure signal| G["[NET] TokenManager.resolve_credential_from_git"]
G -->|credential found| H["operation(credential, extraheader env)"]
G -->|no credential| I["raise original exc"]
C --> J["_checkout(None, hardened_env)"]
E --> K["[FS] GitCache.get_checkout"]
sequenceDiagram
participant MC as MarketplaceClient
participant AR as AuthResolver
participant TM as TokenManager
participant GC as GitCache
MC->>AR: try_with_fallback(host, _checkout, path=...)
AR->>AR: resolve PAT/bearer
AR--xAR: ADO rejects bearer (401)
AR->>TM: resolve_credential_from_git(host, port, path)
TM-->>AR: Credential
AR->>MC: _checkout(credential, extraheader_env)
MC->>GC: get_checkout(url, ref, env)
GC-->>MC: checkout_dir
Recommendation
Ship. The auth chain is correctly ordered, security is clean, happy path is cost-neutral, and this unblocks a high-value enterprise corridor. The five followups (integration test, signal anchoring, doc updates, error UX, logging) are all post-merge safe -- none changes the auth contract or introduces regression risk. CHANGELOG entry should ideally land in-PR (one line, low cost) but is not a gate.
Full per-persona findings
Python Architect
-
[recommended] getattr(host_info, "kind", "") duck-types a typed object at
src/apm_cli/marketplace/client.py
HostInfo is a known dataclass with akindattribute. Using getattr signals the caller is unsure of the type, which weakens static analysis and hides bugs if the attribute is ever renamed.
Suggested: Replacegetattr(host_info, "kind", "") == "ado"withhost_info.kind == "ado"(HostInfo always has.kind). -
[recommended] ADO-specific branch in _fetch_git duplicates the non-ADO path's checkout call at
src/apm_cli/marketplace/client.py
The if/else block has two paths that both end in _checkout(token, env). Long-term: converge both paths through try_with_fallback. -
[nit] Bare
except Exceptionin auth.py swallows non-auth errors into credential fallback atsrc/apm_cli/core/auth.py
Catches all exceptions from _try_ado_bearer_fallback, including programming errors. Inner guard filters most, but a tighter outer catch would be more defensive. -
[nit] Nested closure depth is acceptable but approaching the readability ceiling
Three nested helpers share the same captured scope. If a fourth fallback appears, refactor to a list-of-strategies dispatch.
CLI Logging Expert
-
[recommended] Silent re-raise when git credential fill returns None gives user no hint why auth failed or what to do next at
src/apm_cli/core/auth.py
When resolve_credential_from_git returns None, the original exception is re-raised with zero additional context.
Suggested: Add_log("git credential fill returned no credential for {host_info.display_name} -- ensure Git Credential Manager is configured")before re-raising. -
[nit] First _log message includes raw path which may expose internal registry paths in verbose output
Not a credential leak, but verbose output may end up in CI logs. Acceptable for --verbose; noting it is intentional. -
[nit] Log messages consider prefixing with STATUS_SYMBOLS for consistency
Raw_log()without a symbol breaks visual scanning in verbose output.
Suggested: Use_log(f"[>] trying git credential fill for {host_info.display_name}...").
DevX UX Expert
-
[recommended] When git credential fill also fails, user sees original 401 with no hint about GCM at
src/apm_cli/core/auth.py
The nested re-raise surfaces the bearer-fallback exception, not a user-friendly message explaining all three attempts failed.
Suggested: Wrap the final raise in an AuthError summarising all three attempts with ADO-specific guidance. -
[recommended] Success path is silent -- user cannot tell why auth now works (discoverability) at
src/apm_cli/core/auth.py
_log calls use internal debug logger, invisible at default verbosity.
Suggested: Emit a single _rich_info note on successful credential-fill. -
[nit] ADO-only branch uses getattr -- defensive but opaque at
src/apm_cli/marketplace/client.py
If host_info ever lacks kind, silently falls through to non-ADO path.
Suggested: Usehost_info.kind == "ado"directly if the type guarantees the attribute.
Supply Chain Security Expert
-
[nit] Auth failure signals include bare '401'/'403' substrings which could false-positive on unrelated error text at
src/apm_cli/utils/github_host.py
Substring matching can hit port numbers or unrelated message fragments.
Suggested: Anchor to 'http 401', 'http/2 403', 'returned error: 401' to reduce false-positive matches. -
[nit] Log message includes the path value; consider omitting in non-verbose mode at
src/apm_cli/core/auth.py
Exposes ADO repo path in standard output. Low risk since _log is typically debug-level.
OSS Growth Hacker
-
[recommended] Missing CHANGELOG entry under [Unreleased] for a user-facing auth fix at
CHANGELOG.md
Windows+ADO+GCM is the dominant enterprise dev setup. Users upgrading won't know APM fixed this.
Suggested: Add to [Unreleased] Fixed:apm installnow falls back to Git Credential Manager for private ADO packages when PAT and Azure CLI bearer tokens fail (fix(auth): fall back to git credentials for ADO #2610). -
[nit] Opportunity for a troubleshooting FAQ entry in docs
Users searching '401 ADO apm' should land on a docs page confirming GCM fallback works.
Suggested: Add a short FAQ entry in docs/src/content/docs/guides/auth.md.
Auth Expert
-
[recommended] "401" and "403" in _ADO_AUTH_FAILURE_SIGNALS may false-positive on non-auth error bodies at
src/apm_cli/utils/github_host.py
Substring matching can hit port numbers or any message containing those digits. Use anchored patterns liker'\b40[13]\b'.
Suggested: Tighten signals to ('http 401', 'http 403', 'authentication failed', 'unauthorized', 'could not read username') or user'\b40[13]\b'. -
[nit] Credential fallback re-raises the original exc, hiding the GCM failure reason at
src/apm_cli/core/auth.py
When resolve_credential_from_git returns None, the original exc is re-raised, losing context about why GCM returned nothing.
Suggested: Add_log(f'git credential fill returned no credential for {host_info.display_name}')beforeraise exc. -
[nit] scheme='basic' with base64(':token') is correct for GCM-sourced ADO PATs -- no issue
GCM returns a PAT as the password field. ADO accepts Basic auth with empty username and PAT as password. Confirming correctness for panel record.
Doc Writer
-
[recommended] CHANGELOG [Unreleased] has no entry for this fix at
CHANGELOG.md
Every auth behavior change of this magnitude has a [Unreleased] entry. Without one, the fix is invisible to users upgrading.
Suggested: Add to [Unreleased] > Fixed: Private ADO marketplace installs no longer fail with HTTP 401 when the Azure CLI tenant does not own the ADO org. (fix(auth): fall back to git credentials for ADO #2610) -
[recommended] ADO credential chain in install-failures.md is now stale -- missing git credential fill step at
docs/src/content/docs/troubleshooting/install-failures.md
Line 71 showsADO_APM_PAT -> AAD bearer (via az cli) -> none. After this PR the chain ends withgit credential fill, notnone.
Suggested: Change line 71 to include git credential fill. Add a note explaining the GCM/OS-keychain path. -
[recommended] authentication.md ADO section (apm-guide skill) states the chain terminates at auth-failed error at
packages/apm-guide/.apm/skills/apm-usage/authentication.md
The enumeration has no step for git credential fill. After this PR that enumeration is incorrect.
Suggested: Insert a new step before the 'Otherwise' entry: git credential fill (GCM, OS keychain, or any configured credential helper). -
[recommended] enterprise/security.md ADO bearer section does not mention git credential fill as a fallback at
docs/src/content/docs/enterprise/security.md
The 'Azure DevOps Services AAD bearer tokens' section describes two-step ADO auth but not the GCM fallback.
Suggested: Add after the last bullet: 'If both ADO_APM_PAT and the az CLI bearer fail, APM falls back to git credential fill so Windows GCM and other OS credential helpers work without configuration.' -
[recommended] ADO troubleshooting table in authentication.md is missing a row for the tenant-mismatch/GCM scenario at
packages/apm-guide/.apm/skills/apm-usage/authentication.md
The troubleshooting table omits the GCM option for the tenant-mismatch case.
Suggested: Update the tenant-mismatch row's Fix cell to include: 'or rely on git credential fill (GCM / OS keychain) if a credential helper is configured for the ADO host.'
Test Coverage Expert
-
[recommended] No integration-with-fixtures test exercises the new bearer->git-credential-fill fallback path at
tests/integration/test_ado_preflight_bearer_fallback_e2e.py
Tier floor for auth is integration-with-fixtures. Unit tests mock at boundary. Integration test file exists but has no GCM/credential-fill test.
Proof (missing):tests/integration/test_ado_preflight_bearer_fallback_e2e.py::test_ado_bearer_rejected_falls_back_to_git_credential_fill-- proves: bearer rejected -> GCM succeeds -> Basic auth header used [secure-by-default] -
[nit] is_ado_auth_failure_signal already has dedicated parametrized unit tests -- no gap at
tests/unit/utils/test_github_host_predicate.py
Proof (passed):tests/unit/utils/test_github_host_predicate.py::multiple parametrized cases-- proves: is_ado_auth_failure_signal correctly classifies ADO 401 stderr patterns
Performance Expert
-
[recommended] Worst-case 3 sequential git clone/fetch attempts on ADO auth failure path
Each fallback tier invokes cache.get_checkout (git network op). Only on failure path, acceptable for correctness. Consider logging elapsed time per fallback tier at DEBUG level for future profiling. -
[nit] hardened_git_base_env() called unconditionally -- O(|env|) but less than 0.1ms, negligible
-
[nit] getattr(host_info, 'kind', '') is negligible overhead -- single attribute lookup, ~50ns
-
[nit] Happy path has no regression vs before -- same number of network round-trips (1 resolve + 1 clone)
-
[nit] AuthResolver cache amortizes repeated credential resolution; fallback tiers don't cache but marketplace fetches one file per source
Suggested: If ADO marketplace sources become common with multi-file fetches, consider caching a successful credential-fill result into AuthResolver._cache.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Sergio Sisternes (sergio-sisternes-epam)
removed
the
panel-review
Summary
git credential fillwhen Azure DevOps PAT/Azure CLI bearer authentication is rejectedAuthResolver.try_with_fallbackorg/project/_git/repopath so Git Credential Manager can select the same credential as native GitProblem
On Windows with a private Azure DevOps marketplace, native
git ls-remote <url>succeeds through Git Credential Manager whileapm marketplace add <url>fails. APM resolves an Azure CLI bearer token first; when ADO rejects that token (for example, because the active Azure CLI tenant does not own the ADO organization), ADO authentication explicitly excludesgit credential fill. The marketplace REST-to-Git fallback also reused the rejected auth context instead of entering the resolver fallback chain.This is related to #2526, but distinct from draft PR #2594: that PR preserves credential helpers for generic Git hosts and intentionally keeps ADO on its hardened path. This PR adds the missing ADO fallback without weakening that path: PAT and bearer remain first, then the repository-scoped helper is tried, and the resulting credential is injected through
http.extraheaderrather than the URL.Reproduction evidence
git ls-remoteagainst the private ADO URL succeeds401 Unauthorized/Authentication failedgit credential fillresolves a credential for the exact escaped ADO clone pathValidation
Focused suites:
Also ran
ruff check,ruff format --check, andgit diff --checkon the changed files.Related: #2526
Companion to: #2594