Skip to content

fix(apps): send a reused credential guid on the connection, not top-level [CONNECT-843] - #1007

Open
hariharanatlan wants to merge 1 commit into
mainfrom
CONNECT-843
Open

fix(apps): send a reused credential guid on the connection, not top-level [CONNECT-843]#1007
hariharanatlan wants to merge 1 commit into
mainfrom
CONNECT-843

Conversation

@hariharanatlan

Copy link
Copy Markdown

What this fixes

CONNECT-843. Creating or re-creating a miner through pyatlan against an existing
connection silently flattened that connection's credential record in the platform, so the
next run of every workflow sharing that credential failed to authenticate.

Scope, stated up front. pyatlan can emit the corrupting wire shape from two call
shapes. This PR fixes one of them:

Call shape Status after this PR
A reused GUID on an existing connection referenced by qualified_name (the reported break: BigqueryMiner().connection(qualified_name=...), GUID auto-resolved or passed explicitly) Fixed — the GUID now rides on the connection entity, top-level field empty
An explicit .credential_guid(g) alongside a new connection minted by .connection(name=...) Not fixed — still emits the corrupting shape, deliberately; see "What this PR does not fix"

So this PR does not remove pyatlan as a trigger for the underlying server defect. It
removes the trigger shape that caused CONNECT-843. The remaining shape is closed for every
caller (not just pyatlan) by the heracles guard in handoff 1 below, which is the actual fix.

Root cause (at the I/O boundary)

AppBuilder._assemble put a reused, already-vaulted credential GUID into the top-level
credential_guid
field of the POST v1/app body and sent no credential body with it
(pyatlan/model/apps/_base.py:244-246 pre-fix; the GUID is resolved from the connection at
_base.py:288-308 and latched at _base.py:318-324, and _build_connection at
_base.py:169-181 never emitted defaultCredentialGuid). On the server, the create
handler's resolveAppCredential (heracles handler/native_app.go:258-262) treats a
non-empty top-level credential_guid as "reuse this GUID" and sets credentialGUID, but it
only ever populates credentialBody from a payload field that carries a non-empty
authType
(native_app.go:283-290); a connection map has no authType, so
credentialBody stays nil. The create path then guards its config write on the GUID
alone, if credentialGUID != "" (native_app.go:635), calls
stripSensitiveCredentialFields(nil), gets nil, substitutes an empty map, stamps
credentialSource on it, and runs
UpsertCredentialConfig(guid, {"credentialSource": "direct"}). That is a full overwrite,
not a merge
, of a record keyed by a GUID shared across every workflow using that
credential
, so authType, host, port and extra.* are gone. Observed corrupted
record: 29 bytes, {"credentialSource":"direct"}, against a healthy sibling that still
carries authType: gcp-wif plus host/port/extra. Downstream this is fatal exactly for
auth-type-sensitive connectors: with authType absent, the BigQuery app's SQL client
silently defaults to service_account (atlan-bigquery-app app/auth/sql_client.py:205),
attempts a service-account build with no key, and reports a user-attributed
INVALID_INPUT_BIGQUERY_CREDENTIALS, hiding a platform fault behind a customer-error label.
The sibling update path in the same file already has the correct guard,
if credBodyForUpsert != nil && credentialGUID != "" (native_app.go:1020), which is what
makes the create-path guard an oversight rather than a design.

The fix

Deliver a reused GUID the way the UI delivers it: on the connection entity, with the
top-level field empty.

  • _build_connection(qualified_name, *, default_credential_guid=None) now emits
    attributes.defaultCredentialGuid when a reused GUID is threaded in (keyword-only, so no
    existing caller changes shape).
  • _assemble computes reuse_on_existing_connection (a GUID is set and an existing
    connection is referenced by QN and no raw credential is staged and not the
    agent/SDR path), threads the GUID into the connection, and sends credential_guid = "".

With credential_guid = "", resolveAppCredential returns guid == "", so the
native_app.go:635 upsert is skipped entirely and the shared record is never touched.
The GUID still reaches the app: inputsToAllParams deliberately drops
credential_guid/credential-guid (native_app.go:139-156) but passes connection
through as a typed {{connection}} object, which is precisely the path the UI has always
used. The emitted connection.attributes ({qualifiedName, connectorName, defaultCredentialGuid}) is a safe subset of what the UI sends, not shape-identical: the
captured working UI run's connection carries 31 attributes. The three emitted here are
exactly the ones the consumer apps read.

The forward path is proven from a real working run, not inferred.
evidence/ui-miner-history.json is the Temporal history of a COMPLETED bigquery:miner
run on the affected tenant, created through the UI: 124 events ending in
EVENT_TYPE_WORKFLOW_EXECUTION_COMPLETED, 20 activities all completed. Its single
workflowExecutionStartedEventAttributes input has no credential_guid key at all (no
credential, no credential-guid either), and its connection.attributes carries a 36-char
defaultCredentialGuid. That is the platform's own working contract for a reused credential,
and it is the shape this fix now emits.

One nuance, since the two are not literally identical: the working UI run omits the
top-level key, while this fix sends credential_guid = "". These are provably
server-equivalent at both places the key is read — resolveAppCredential guards
if g, ok := inputs["credential_guid"].(string); ok && g != ""
(native_app.go:259-261), so "" and key-absent both yield guid == ""; and
inputsToAllParams unconditionally continues on credential / credential_guid /
credential-guid (native_app.go:150-153), so neither ever reaches the DAG. pyatlan sends
"" rather than omitting because the input contract rejects a null string.

The change lives entirely in AppBuilder. No connector builder overrides
_build_connection, _assemble, preview or _create (verified: single definition, single
call site), so all 34 concrete builders are covered by one edit and no generated file is
touched.

Replay, pre-fix vs post-fix, running the real builder and then the transcribed heracles
guard over its output (synthetic values):

BASE:  credential_guid='<credential-guid>'  connection.attributes={connectorName, qualifiedName}
       -> credentialGUID='<credential-guid>', credentialBody=nil
       -> :635 UpsertCredentialConfig(<credential-guid>, {"credentialSource":"direct"})   OVERWRITES

FIXED: credential_guid=''  connection.attributes={connectorName, defaultCredentialGuid, qualifiedName}
       -> credentialGUID='', credentialBody=nil
       -> :635 upsert SKIPPED; shared credential record untouched; GUID rides on {{connection}}

Why it broke now

Introduced by 512fb3959 "fix(app): miners resolve their connection's credential by QN"
(2026-06-24 16:54 +0530), which added _resolve_connection_credential and flipped
credential_guid from "" to a real GUID on the reuse path, arming the server's weak guard.

The commit 27 minutes earlier, d58bf3531 (2026-06-24 16:27), had added the
existing-connection/miner flow and deliberately sent the safe shape, in its own words:

send credential_guid="" when no credential is supplied (e.g. miners) so the contract's
non-null string requirement is satisfied

So the safe-by-construction state existed and was intentional, for 27 minutes. 512fb3959
was a genuine ergonomic improvement (QN-only miner creation) with an unnoticed
platform-data-corruption consequence. It went roughly seven weeks undetected because the
corruption is silent for basic/service_account (the Vault half still holds the
password, and the app's own default happens to be service_account) and fatal only for
gcp-wif and peers
.

Blast radius

In this repo, in-diff: _base.py::_assemble and _base.py::_build_connection. Seven
builders have no credential-staging method at all, so the reuse path is their only path
(BigqueryMiner, DatabricksMiner, OracleMiner, PostgresMiner, PowerbiMiner,
SnowflakeMiner, TeradataMiner); all 34 concrete builders can additionally reach it via
.credential_guid(g) with .connection(qualified_name=...). atlan_dbt.py's named
GUID fields (api_credential_guid, object_store_credential_guid) are plain strings and are
never read by resolveAppCredential, so they never armed the guard and are untouched here.

In this repo, NOT fixed — the sibling branch of the same ternary. .credential_guid(g)
with a new connection (.connection(name=...)) still emits the corrupting shape, and this
is a live path, not a theoretical one. Reproduced on this branch:

BigqueryCrawler(...).connection(name="prod-bq").credential_guid("<reused-guid>").preview()
  -> credential_guid = "<reused-guid>"       # bare top-level guid
  -> "credential" in payload  -> False       # no credential body
  -> connection.attributes    -> {connectorName, name, qualifiedName}   # no defaultCredentialGuid

That is byte-for-byte the shape the root-cause analysis blames, so native_app.go:635 /
workflow.go:2959 fire and UpsertCredentialConfig(g, {"credentialSource":"direct"})
flattens the record. It is reachable and idiomatic: the repo's own generator emits this
exact shape as the uniform connection-plus-GUID path for every app
(pyatlan/generator/generate_apps.py:804-813) and 33 generated per-app tests assert
out["credential_guid"] == "g"
on it (tests/unit/apps/test_*.py), miners included. See
"What this PR does not fix" for why it is left alone.

Do the consumer apps still resolve the credential when the top-level field is empty? This
was the one way the fix could break something, so every verdict below came from reading the
function, not from grepping for a name.

App repo Fallback present Evidence
atlan-bigquery-app YES app/utils.py:293-334 get_credential_guid; :326 attrs.get("defaultCredentialGuid", "") after :312-314 short-circuits on a non-empty top-level GUID. Documents the HYP-407 (2026-04-27) resolution order. Re-verified at the incident SHA (this is the connector that failed).
atlan-mssql-app YES app/utils/credential.py:1-33: MinerInputContract's validator lifts the connection's defaultCredentialGuid onto the top-level triple at input construction, so CredentialRef.resolve stays the single path.
atlan-postgres-app YES app/contracts.py:72-82 @model_validator(mode="after") _lift_credential_guid_from_connection, handling camelCase and snake_case.
atlan-databricks-app YES app/auth/connection_resolver.py:42-62 resolve_credential_guid_from_workflow_args -> attributes.get("defaultCredentialGuid"); wired at app/auth/_state.py:27-39.
atlan-oracle-app YES app/oracle.py:3549-3558 miner entrypoint reads default_credential_guid/defaultCredentialGuid off input.connection.attributes when credential_guid is empty.
atlan-powerbi-app YES app/contracts/_credentials.py:17-31 default_credential_guid_from_connection, _GUID_KEYS = ("default_credential_guid", "defaultCredentialGuid"); shared by the preflight gate and the app path.
atlan-snowflake-app YES app/pipeline/miner/orchestration.py:226 and app/pipeline/crawler/au/orchestration.py:228: input.credential_guid or asset_attrs.get("defaultCredentialGuid") or ""; plus app/handler/handler.py:394-419.
atlan-teradata-app YES app/utils/credential.py:33-37 plus docs/adr/023-miner-credential-resolution.md: contracts.GateCredentialRouting lifts it at input construction.
application-sdk NO, per-app helper required application_sdk/credentials/ref.py:113-118 CredentialRef.resolve routes only on extraction_method/agent_json/credential_guid; .claude/skills/upgrade-v3/SKILL.md:1361 says to catch its ValueError and resolve from the connection in a per-app helper until the SDK covers it.

8/8 exposed connector apps implement the fallback; none loses credential resolution. There
is a structural reason: the UI has always omitted the top-level GUID on reuse flows, so any app
with a working UI-created miner was forced to implement it. Snowflake and Teradata carry
plans/ADRs about exactly this.

The one residual: the matrix was read at each repo's default branch (BigQuery additionally at
the incident SHA). A tenant pinned to an app image predating its own fallback helper is not
covered by that read.

Fix forward, not revert

Fix forward. Reverting 512fb3959 is not clean: that commit also carries the generator's
per-module import emission and self-formatting, so a revert drags unrelated generated-file
churn along with it, and it would regress a real ergonomic win (QN-only miner creation).
Fix-forward is also strictly closer to the platform contract, since it makes pyatlan match the
UI's documented wire shape rather than merely deleting the resolution. The cost of fix-forward
is that it conforms pyatlan around a server-side guard that stays weak for everyone else, which
is why the heracles handoff below must ship regardless of this PR.

Test evidence

New tests in tests/unit/test_app_builders.py:

Test Proves
test_auto_resolved_guid_rides_on_connection_not_top_level[bigquery,snowflake] The auto-resolved GUID lands on connection.attributes.defaultCredentialGuid, credential_guid == "", no credential body invented. Parametrized over two connectors to prove genericity.
test_explicit_guid_on_existing_connection_rides_on_connection Same routing when the caller supplies the GUID itself: the trigger is "GUID + existing connection", not "GUID came from a lookup".
test_explicit_guid_on_new_connection_stays_top_level Pins today's behaviour on the deliberately unchanged path: minting a new connection with an explicit GUID still sends it top-level. This is not an assertion that the shape is safe — it is known to still trigger the server defect (see "What this PR does not fix"); the test exists so any future reroute is a deliberate, evidenced change. Its in-code comment says so explicitly.
test_staged_credential_on_existing_connection_keeps_vaulting_shape A staged raw credential on an existing connection is still vaulted from the credential key, nothing moves onto the connection, and no connection lookup is issued.
test_agent_mode_on_existing_connection_ignores_credential_guid The agent/SDR path returns before any credential routing and is unaffected.
  • Red/green: with the _base.py change reverted and the tests kept, 4 fail (the
    modified existing test plus the three that assert the new routing) and 116 pass. With the fix
    applied, 120 pass. The other three new tests pass in both states by design: they are
    regression guards asserting the fix did not change those paths.
  • Full suite: 7067 passed, 5 skipped (pytest tests/unit). No pre-existing failures at
    base, none introduced.
  • Payload equivalence: the assembled payloads for the staged-credential crawler, the
    explicit-GUID-with-new-connection case, and the QN-only case are byte-identical before
    and after (json.dumps(..., sort_keys=True) diff exits 0).
  • ruff format --check and ruff check clean; mypy --strict-optional clean on the changed
    module.

One existing test was changed, deliberately

tests/unit/test_app_builders.py::test_miner_auto_resolves_connection_credential, one
assertion line.

That test was added by 512fb3959, the same commit that introduced the defect, and it
asserted out["credential_guid"] == "conn-cred-guid". The commit 27 minutes earlier
(d58bf3531) had deliberately sent credential_guid="", the UI-conformant, safe-by-
construction shape. So the assertion locked in a regression; it did not protect intended
behaviour
, and it was the only thing in the suite that contradicted the correct wire shape.

The edit is narrow and does not weaken the test. It keeps both original intents (the
connection was looked up, the resolved GUID is reused) and now asserts the GUID arrives at
connection.attributes.defaultCredentialGuid and that credential_guid == "". Assertion
count goes 2 to 3. A comment citing CONNECT-843 sits above it so a future reader does not
"fix" it back. Every other existing test in the file is byte-identical, verified by AST diff of
each function body against HEAD, not by assertion.

What this PR does not fix

One pyatlan call shape still emits the corrupting payload, deliberately: an explicit
.credential_guid(g) alongside a new connection minted by .connection(name=...)
(pyatlan/model/apps/_base.py:270-274, reproduced above under Blast radius).

Why it is left alone, and why the earlier rationale for that was wrong. An earlier draft of
this PR justified the exclusion as "with no existing connection there is no shared credential
record to corrupt." That is false, and worth writing down so nobody rebuilds the argument:
the credential config record is keyed by the credential GUID alone
POST /workflows/v1/config/{credentialGUID}?type=credentials
(evidence/heracles-appconfig-upserter.go:33-34) — the connection is not part of the key. And
.credential_guid() exists specifically to "Reuse an already-vaulted credential instead of
creating a new one" (pyatlan/model/apps/_base.py:121-124), so by construction that GUID
already backs other workflows. Minting a new connection protects nothing.

The real reason it is unchanged is evidence coverage. Rerouting the GUID onto the
connection on this branch too would newly affect crawler-shaped apps, and their
connection-attribute fallback is not implied by the 8/8 miner matrix above: crawler forms carry
an explicit credential-GUID widget (stated in atlan-bigquery-app app/utils.py:297-299 and
atlan-oracle-app app/oracle.py:3544-3546), so those apps may legitimately depend on the
top-level field. Extending the reroute without an 8/8-equivalent evidence bar for crawler
apps
risks exactly the cross-connector regression this fix is meant to avoid. The narrow fix
is verified end to end; the server-side guard in handoff 1 closes the remaining branch for
every caller, which is the better place for it.

This PR does not close the vulnerability

Stated plainly: this change removes one of two pyatlan trigger shapes; it does not fix the
defect.
The server still overwrites a shared credential record whenever it is handed a bare
GUID with no body. Any other caller doing that, a curl, another SDK, pyatlan's own
new-connection branch above, re-corrupts the record. The following handoffs are not optional
follow-ups; the first is the actual fix.

  1. atlanhq/heracles (HIGHEST priority — this is the fix, and it is now demonstrably
    required, not just prudent).
    handler/native_app.go:635 and handler/workflow.go:2959
    guard the config upsert on credentialGUID != "" alone. Both should match the
    already-correct update path at native_app.go:1020:
    if credBodyForUpsert != nil && credentialGUID != "". Never write a shared credential
    record from an absent body.
    Scope now explicitly includes pyatlan's unfixed branch. The reproduction under Blast
    radius is a concrete, in-tree demonstration that a bare-GUID caller still flattens the
    record even after this PR merges — the guard is what closes it, for pyatlan's new-connection
    path and for every other caller at once. Treat that reproduction as the acceptance test for
    this handoff: with the guard in place, that same payload must leave the credential config
    untouched.
  2. atlanhq/application-sdk. handler/service.py:1470-1488 performs a full overwrite
    of the credential config with no read-modify-write and no schema validation, so a
    single-key body is accepted and destroys the record. Add merge semantics or reject a body
    missing authType. Also consider teaching CredentialRef.resolve
    (application_sdk/credentials/ref.py:113-118) the connection.attributes.defaultCredentialGuid
    path natively, so the eight per-app helpers can converge.
  3. atlanhq/atlan-bigquery-app. app/auth/sql_client.py:205 coerces an absent
    authType to service_account, which is what turned a platform fault into a
    USER-attributed INVALID_INPUT_BIGQUERY_CREDENTIALS. A missing authType should be a
    distinct, platform-attributed failure. Duplicated at app/auth/__init__.py:40,74 and
    app/auth/client.py:72,83,173, so this is a class of bug, not one line.

Remediation for already-corrupted records is separate from this PR: any credential whose
config is {"credentialSource":"direct"} needs its authType/host/port/extra restored from
the Vault half before those workflows will run.

🤖 Generated with Claude Code

…evel (CONNECT-843)

When an app builder references an EXISTING connection by qualifiedName and
reuses an already-vaulted credential guid (miners, or any builder given
.credential_guid() together with .connection(qualified_name=...)), pyatlan put
that guid in the top-level `credential_guid` field with no credential body.

The create endpoint's credential resolver treats a non-empty top-level
credential_guid as "reuse this guid", but only builds a credential body from a
payload field carrying a non-empty authType. Given a guid and no body it still
runs UpsertCredentialConfig(guid, {"credentialSource": "direct"}), replacing
that credential's shared config record with that single key. Every workflow
sharing the guid then loses authType/host/extra, so auth-type-sensitive
connectors (e.g. gcp-wif) fail on their next run.

Route the reused guid to connection.attributes.defaultCredentialGuid and keep
credential_guid "", which is exactly the shape the UI sends for reuse flows, so
the resolver takes its "no guid" path and never touches the record. Staged raw
credentials, new-connection creation with an explicit guid, and the agent/SDR
path are all unchanged (verified byte-for-byte on the assembled payload).

Introduced by 512fb39, which added the QN credential auto-resolution; the
commit 27 minutes before it (d58bf35) deliberately sent credential_guid="".

This removes pyatlan as a trigger, not the defect itself. The weak guard is
heracles handler/native_app.go:635 (and handler/workflow.go:2959) and should
match the already-correct update path at native_app.go:1020,
`credBodyForUpsert != nil && credentialGUID != ""`. Any other bare-guid caller
still re-corrupts the record.

tests/unit/test_app_builders.py::test_miner_auto_resolves_connection_credential
is updated rather than left alone: it was added by 512fb39, the same commit
that introduced the defect, so its top-level-guid assertion locked in the
regression instead of protecting intended behaviour. It still proves the
connection is looked up and its guid reused, now asserted at the correct
location, and gained an assertion rather than losing one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@linear

linear Bot commented Aug 13, 2026

Copy link
Copy Markdown

CONNECT-843

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant