Skip to content

fix(data-warehouse): validate nested Stripe resources via parent endpoint#57215

Merged
danielcarletti merged 30 commits intomasterfrom
claude/stripe-nested-resource-validation
May 6, 2026
Merged

fix(data-warehouse): validate nested Stripe resources via parent endpoint#57215
danielcarletti merged 30 commits intomasterfrom
claude/stripe-nested-resource-validation

Conversation

@danielcarletti
Copy link
Copy Markdown
Contributor

@danielcarletti danielcarletti commented Apr 30, 2026

Problem

Follow-up to #56795. Customers report seeing the toast

Stripe credentials lack permissions for CustomerPaymentMethod
Stripe credentials lack permissions for CustomerBalanceTransaction

every time they toggle the sync method on one of those tables, even when their restricted key has the correct scopes. The actual sync also doesn't kick off — the validation error fires before the worker is dispatched.

validate_credentials filters its resources_to_check list by the table name and falls through to a " does not exist" branch when nothing matches. The two Customer* tables are nested under /v1/customers/:id and were never added to that list (they can't be listed without a parent customer ID), so the filter always produces an empty list and the fake permission error fires every single time.

Changes

posthog/temporal/data_imports/sources/stripe/stripe.py

  • Add a NESTED_RESOURCE_PROXIES map (CustomerBalanceTransaction → Customer, CustomerPaymentMethod → Customer) and resolve the table name through it before filtering. Validation now hits the parent's list endpoint, which covers the customer scope they all share.
  • The nested resources may still need additional scopes at actual sync time (e.g. rak_payment_method_read for /v1/customers/:id/payment_methods); those will surface with a real 403 from the sync run rather than a misleading toast at setup.

How did you test this code?

I'm an agent. I ran the following automated tests:

New tests:

  • test_validate_credentials_nested_resource_validates_via_parent — parametrized over both CustomerBalanceTransaction and CustomerPaymentMethod. Asserts that validation hits customers.list and returns True instead of raising.
  • test_validate_credentials_nested_resource_surfaces_parent_permission_error — parametrized similarly. Asserts that a 403 on the parent Customer endpoint is reported as a Customer permission gap rather than the nested table name, so the customer sees the actual scope they need to grant.

Regression coverage:

  • posthog/temporal/tests/data_imports/stripe/test_stripe_source.py — full file: 19 passed.

Manual tests:

  • It works now for both schemas:
image

Publish to changelog?

no

🤖 Agent context

Authored by Claude (Opus 4.7) via Claude Code. Direct follow-up to #56795 surfaced by user feedback after that PR landed. Human review required before merge.

danielcarletti and others added 21 commits April 28, 2026 15:29
Two related UX bugs caused customers with valid Stripe permissions to see
misleading error messages when connecting Stripe as a data source:

1. validate_credentials() lumped every per-resource exception into
   StripePermissionError. An invalid/expired API key (401) thus surfaced
   as "lacks permissions for ALL 13 resources" — pushing users to grant
   permissions they already had. Now distinguishes 401 (raises new
   StripeAuthenticationError, short-circuits the loop) from 403
   (still per-resource permission gap).

2. OAuth token-exchange failures raised a bare Exception, which DRF
   turned into a 500 with no `detail` field — the frontend toast then
   fell back to a generic "Something went wrong". Now raises
   ValidationError with the provider's `error_description` extracted
   from the response body, and tolerates non-JSON error bodies.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…er resource

Following up on the previous Stripe credentials fix:

1. The pre-filled API key creation link and the in-app permissions doc
   omitted Invoice items. Stripe treats `rak_invoice_item_read` as a
   distinct scope from `rak_invoice_read`, so following our docs
   verbatim still produced a 403 on /v1/invoiceitems. Added the scope
   to the prefill list and to the help caption.

2. The per-resource permission failure now includes the underlying
   Stripe error message, not just the resource name. A customer hitting
   a missing scope now sees the literal Stripe response (e.g. "this
   account is not currently set up for Connect"), which makes the gap
   actionable instead of guesswork.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When all failing resources are clean stripe.PermissionError (403), the
resource name alone tells the customer which scope to enable on Stripe.
Including Stripe's full error string (request id, status, headers, body)
just bloats the toast — a customer missing one scope was getting a
multi-line dump for a problem with a one-word answer.

Now: clean 403s render as a plain resource list. Only fall back to the
verbose underlying error when a resource failed with a non-permission
exception (network, rate limit, schema mismatch) where the cause isn't
obvious from the resource name.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A permission denial (403) and an unknown failure (network, schema drift,
rate limit) are different problems with different fixes, but both were
being raised as StripePermissionError — which forced the user-facing
message to either dump every Stripe error verbatim or hide the cause of
non-permission failures behind a misleading "missing permissions" toast.

Now: validate_credentials tracks the two classes in separate dicts and
raises a distinct exception for each. StripePermissionError carries
clean 403s and renders as a plain resource list. New StripeValidationError
carries unknown errors and surfaces the underlying Stripe message,
folding any collected 403s into the same toast for one-shot reporting.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Extract _raise_oauth_validation_error helper to dedupe the
  ValidationError construction that was repeated across the Salesforce
  fallback and the general OAuth error branches.
- Parameterize the two OAuth token-exchange failure tests into a single
  case-driven test covering both JSON and non-JSON error bodies.
- Hoist `import stripe as stripe_lib` to module level in the Stripe
  source tests instead of repeating it inside three test bodies.
- Update Stripe sandbox-retry tests to match the new ValidationError
  message ("OAuth failed") that supersedes the bare "Oauth error"
  Exception they previously asserted on.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Guard the StripeValidationError formatter against empty / whitespace-only
  error strings so we can never crash the response path while reporting a
  different error.
- _extract_oauth_error_message now falls back to a truncated raw body
  snippet when the provider returns a JSON shape with no recognised
  error fields, instead of returning None and letting the caller render
  a status-code-only message.
- Drop the rak_invoice_item_read prefill / docs change from this PR; it
  is a separate concern from the credential-error surfacing work and
  will land in its own change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nto claude/amazing-mcnulty-ec3e1d

# Conflicts:
#	posthog/hogql_queries/web_analytics/test/__snapshots__/test_web_stats_table.ambr
…oint

CustomerBalanceTransaction and CustomerPaymentMethod are nested under
/v1/customers/:id, so they cannot be listed without a parent customer
ID and weren't part of validate_credentials's resources_to_check list.
Filtering by table name produced an empty list and fell through to the
"<resource> does not exist" branch — which the source layer renders as
"Stripe credentials lack permissions for CustomerPaymentMethod" every
time the user toggles the sync method on one of these tables.

Map the two nested resources to the Customer endpoint for validation.
That covers the parent scope they all share and avoids the fake
permission-error toast at setup. The nested resources may still need
additional scopes at sync time (rak_payment_method_read for example),
but those will surface with a real 403 from the actual sync run rather
than a misleading message before the run even starts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@assign-reviewers-posthog assign-reviewers-posthog Bot requested a review from a team April 30, 2026 20:14
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 30, 2026

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
posthog/temporal/data_imports/sources/stripe/stripe.py:367-370
**Module-level constant**

`NESTED_RESOURCE_PROXIES` is a static dict with no dependency on the function arguments or the `client` object, so it is reconstructed on every call to `validate_credentials`. Moving it to module scope is the conventional Python approach and keeps the function body focused on logic.

```suggestion
NESTED_RESOURCE_PROXIES: dict[str, str] = {
    CUSTOMER_BALANCE_TRANSACTION_RESOURCE_NAME: CUSTOMER_RESOURCE_NAME,
    CUSTOMER_PAYMENT_METHOD_RESOURCE_NAME: CUSTOMER_RESOURCE_NAME,
}
```

Reviews (1): Last reviewed commit: "fix(data-warehouse): validate nested Str..." | Re-trigger Greptile

Comment thread posthog/temporal/data_imports/sources/stripe/stripe.py Outdated
danielcarletti and others added 3 commits April 30, 2026 17:20
Move the nested→parent map alongside the existing resource-name maps
in constants.py instead of redeclaring it inside validate_credentials.
Single source of truth for the relationship — get_rows already encodes
it live via StripeNestedResource.parent, but validate_credentials
needs it before instantiating the StripeClient, so a module-level
constant is the right shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the standalone NESTED_RESOURCE_TO_PARENT constant in favour of
deriving the linkage from the StripeNestedResource.parent reference
that already exists in the resources dict — same source of truth used
by get_rows. Both call sites now go through a new _build_resources
factory; validate_credentials walks parent.method via an identity
reverse-index to recover the parent's name.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
def _resolve_to_flat(name: str) -> tuple[str, StripeResource]:
entry = all_resources[name]
if isinstance(entry, StripeNestedResource):
parent_name = flat_method_to_name.get(id(entry.parent.method), name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: if we get an error, this is gonna fall back to the nested resource instead of the parent. wouldn't this produce the error we are trying to avoid:
Stripe credentials lack permissions for CustomerPaymentMethod
?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea. I've updated the error message to include the parent as well, which is the actionable path (the permission that users can actually grant). Now it looks like this: Stripe credentials lack permissions for CustomerPaymentMethod (Customer).

Tried to keep it concise, do you think it is clear enough for the user to figure it out?

danielcarletti and others added 2 commits May 4, 2026 10:14
Previous fallback `flat_method_to_name.get(id(entry.parent.method), name)`
silently used the nested resource name when the reverse-index missed —
which would have reproduced the very "lacks permissions for
CustomerPaymentMethod" toast this whole code path was added to avoid.

Replace with a direct dict access (KeyError on miss) and add a CI test
that asserts every StripeNestedResource's parent.method is registered
as a top-level StripeResource. The misconfiguration is a programming
error and should fail in CI rather than at runtime.

Also format the nested resource error as `Nested (Parent)` (e.g.
"CustomerPaymentMethod (Customer)") so the customer sees both the
table they toggled and the actionable scope they need to grant.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Reverse index from parent method → flat resource name so we can recover a nested
# resource's parent name from its `parent.method` reference, without restating the
# relationship in a separate constant.
flat_method_to_name = {id(r.method): name for name, r in all_resources.items() if isinstance(r, StripeResource)}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the feeling this is not gonna work, the build_resources method does client.customer.list, and if you execute this locally:

import stripe
c = stripe.StripeClient('sk_test_fake')
a = c.customers.list
b = c.customers.list
id(a) == id(b)
False
a is b
False

you'll see that since you create a new object (same as you are doing for each nested resource) is not gonna match, so the parent_name = flat_method_to_name[id(entry.parent.method)] is gonna keyerror at runtime.

ask claude why tests are passing and:
Tests pass because MagicMock caches child attribute access (mock.customers.list always returns the same object), masking the issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fixed and retested. This lookup is simpler now and it works. Tested locally, I see actionable messages and if the permissions are correct, then the sync actually runs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example:
Screenshot 2026-05-05 at 13 14 17

danielcarletti and others added 4 commits May 5, 2026 11:26
Stripe's SDK exposes endpoints via property descriptors that return a
fresh bound method on every attribute access — so
`id(client.customers.list) == id(client.customers.list)` is False at
runtime. The previous reverse index built `{id(r.method): name}` and
then KeyError'd when validating a nested resource because the method
identity at lookup time never matched the one captured at registration
time. Tests passed because MagicMock caches child attribute access and
makes identity equality look True, masking the bug.

Carry the parent linkage explicitly on the StripeNestedResource via a
new `parent_name: str` field. validate_credentials now does a direct
dict lookup on the name. The CI invariant test no longer compares
method identity; it asserts each nested resource's parent_name is a
registered top-level StripeResource — which is the actual property
the runtime depends on.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stripe's PermissionError carries the exact missing scope ("Having the
'rak_payment_method_read' permission would allow this request to
continue"), which is far more actionable than the generic "check your
configuration and permissions in Stripe" toast we used to show. The raw
message also contains an ~80-char redacted run from the restricted key
that bloats the toast — collapse it before surfacing.

- Add `_clean_stripe_error_message` to collapse 5+ consecutive `*` chars
  (the redacted middle of a restricted key) to `***`. The visible prefix
  and suffix Stripe leaves are preserved for support escalations.
- Add `_call_stripe` wrapper around all SDK list/iteration calls in
  get_rows so any StripeError raised during sync is re-raised with a
  cleaned message under the same exception class — keeps the framework's
  non-retryable substring matching working.
- Apply the cleaner in validate_credentials's per-resource error
  capture as well, so setup-time toasts are also tight.
- Change get_non_retryable_errors's "PermissionError" mapping from a
  static generic message to None, so the now-cleaned underlying Stripe
  message flows through to the user.

Drop a redundant comment on StripeNestedResource.parent_name.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The CI test test_validate_credentials_nested_resources_have_registered_parents
already enforces that every StripeNestedResource.parent_name resolves to a
registered top-level StripeResource. Running the same check on every
validate_credentials call is wasted work — strengthen the test to cover the
type invariant too and let the validator do a plain dict access with a cast.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The PERMISSIONS list already includes `rak_payment_method_read` (so the
prefill URL ticks the box automatically), but the in-app permissions
caption omits Payment methods from the Core list. A user creating a
restricted key manually — by following the visible instructions instead
of clicking the prefill link — would miss the scope and then hit a 403
on /v1/customers/:id/payment_methods when CustomerPaymentMethod syncs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
return method(*args, **kwargs)
except stripe_lib.StripeError as e:
cleaned = _clean_stripe_error_message(str(e))
raise type(e)(message=cleaned) from e
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit pick: we could add the rest of the stuf, like json_body and the rest of stuff

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exception shows up in the toast notification, I'd rather keep it small. It shows the clear error anyway, so I believe this is good



def _clean_stripe_error_message(msg: str) -> str:
"""Collapse the long redacted middle of a restricted API key ('rk_live_********...****gbeftZ')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit pick: repo's CLAUDE.md says "default to writing no comments" and "explain why, not what.", there are a bunch of descriptive comments around

@danielcarletti danielcarletti merged commit 61a6e00 into master May 6, 2026
239 of 240 checks passed
@danielcarletti danielcarletti deleted the claude/stripe-nested-resource-validation branch May 6, 2026 13:09
@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented May 6, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-05-06 13:36 UTC Run
prod-us ✅ Deployed 2026-05-06 14:02 UTC Run
prod-eu ✅ Deployed 2026-05-06 14:01 UTC Run

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.

2 participants