fix(bing-ads): surface tailored error for tenant missing service principal#60828
Merged
jabahamondes merged 3 commits intoJun 1, 2026
Conversation
…cipal AADSTS650052 from Microsoft Advertising means the customer's Microsoft 365 tenant has not granted admin consent to the bingads OAuth application — reconnecting will not help, an org admin has to issue the consent first. The error arrives wrapped in an "invalid_client" envelope, which currently matches first and surfaces the generic "reconnect your Bing Ads integration" toast, sending users down the wrong path. Add a specific AADSTS650052 entry ahead of invalid_client in the non-retryable map (first match wins in external_data_job.handle_non_retryable), with a message that names the underlying tenant-consent step. Fixes PostHog#54911
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
posthog/temporal/data_imports/sources/bing_ads/source.py:61-62
**AADSTS650052 ordering fix is incomplete — `OAuthTokenRequestException` matches first**
The goal is for `AADSTS650052` to win the first-match race, but `OAuthTokenRequestException` also appears verbatim in the error string (the SDK wraps it as `"Failed to fetch customer ID: OAuthTokenRequestException: invalid_client AADSTS650052: …"`). Since `OAuthTokenRequestException` comes before `AADSTS650052` in the dict, `external_data_job.py`'s list-comprehension picks `auth_friendly` first and users still see the generic reconnect message. The test `test_aadsts650052_message_wins_over_invalid_client` confirms this: `friendly_errors[0]` is `auth_friendly` (from `OAuthTokenRequestException` matching), so `assert "AADSTS650052" in friendly_errors[0]` will fail. Moving `AADSTS650052` to be the very first key — before `OAuthTokenRequestException` — fixes both the production path and unbreaks that test.
### Issue 2 of 2
posthog/temporal/data_imports/sources/bing_ads/tests/test_bing_ads_source.py:309
**Superfluous `is not None` guard**
Dict values in `get_non_retryable_errors` are `str | None`, but the list comprehension collects everything — `None` values included. The real guard needed here is `assert friendly_errors` (or `assert len(friendly_errors) >= 1`) to catch an empty list before indexing; accessing `[0]` on an empty list raises `IndexError` instead of an assertion failure. As written, `friendly_errors[0] is not None` is trivially true whenever the list is non-empty and provides no new safety.
Reviews (1): Last reviewed commit: "fix(bing-ads): surface tailored error fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
61
to
62
| "AADSTS650052": service_principal_friendly, | ||
| "invalid_client": auth_friendly, |
Contributor
There was a problem hiding this comment.
OAuthTokenRequestException matches first
The goal is for AADSTS650052 to win the first-match race, but OAuthTokenRequestException also appears verbatim in the error string (the SDK wraps it as "Failed to fetch customer ID: OAuthTokenRequestException: invalid_client AADSTS650052: …"). Since OAuthTokenRequestException comes before AADSTS650052 in the dict, external_data_job.py's list-comprehension picks auth_friendly first and users still see the generic reconnect message. The test test_aadsts650052_message_wins_over_invalid_client confirms this: friendly_errors[0] is auth_friendly (from OAuthTokenRequestException matching), so assert "AADSTS650052" in friendly_errors[0] will fail. Moving AADSTS650052 to be the very first key — before OAuthTokenRequestException — fixes both the production path and unbreaks that test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/temporal/data_imports/sources/bing_ads/source.py
Line: 61-62
Comment:
**AADSTS650052 ordering fix is incomplete — `OAuthTokenRequestException` matches first**
The goal is for `AADSTS650052` to win the first-match race, but `OAuthTokenRequestException` also appears verbatim in the error string (the SDK wraps it as `"Failed to fetch customer ID: OAuthTokenRequestException: invalid_client AADSTS650052: …"`). Since `OAuthTokenRequestException` comes before `AADSTS650052` in the dict, `external_data_job.py`'s list-comprehension picks `auth_friendly` first and users still see the generic reconnect message. The test `test_aadsts650052_message_wins_over_invalid_client` confirms this: `friendly_errors[0]` is `auth_friendly` (from `OAuthTokenRequestException` matching), so `assert "AADSTS650052" in friendly_errors[0]` will fail. Moving `AADSTS650052` to be the very first key — before `OAuthTokenRequestException` — fixes both the production path and unbreaks that test.
How can I resolve this? If you propose a fix, please make it concise.| "The app is trying to access a service that your organization lacks a service principal for." | ||
| ) | ||
| friendly_errors = [msg for pattern, msg in non_retryable_errors.items() if pattern in error_message] | ||
| assert friendly_errors[0] is not None |
Contributor
There was a problem hiding this comment.
is not None guard
Dict values in get_non_retryable_errors are str | None, but the list comprehension collects everything — None values included. The real guard needed here is assert friendly_errors (or assert len(friendly_errors) >= 1) to catch an empty list before indexing; accessing [0] on an empty list raises IndexError instead of an assertion failure. As written, friendly_errors[0] is not None is trivially true whenever the list is non-empty and provides no new safety.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/temporal/data_imports/sources/bing_ads/tests/test_bing_ads_source.py
Line: 309
Comment:
**Superfluous `is not None` guard**
Dict values in `get_non_retryable_errors` are `str | None`, but the list comprehension collects everything — `None` values included. The real guard needed here is `assert friendly_errors` (or `assert len(friendly_errors) >= 1`) to catch an empty list before indexing; accessing `[0]` on an empty list raises `IndexError` instead of an assertion failure. As written, `friendly_errors[0] is not None` is trivially true whenever the list is non-empty and provides no new safety.
How can I resolve this? If you propose a fix, please make it concise.
jabahamondes
approved these changes
Jun 1, 2026
The SDK wraps the tenant-missing-service-principal error as `OAuthTokenRequestException: invalid_client AADSTS650052: ...`, so the non-retryable error envelope contains all three substrings. The dict in get_non_retryable_errors is matched first-hit-wins by handle_non_retryable, so AADSTS650052 has to come before both generic wrappers — not just before invalid_client — otherwise the user still sees the generic 'reconnect your integration' toast and reconnecting cannot fix it (only an org admin granting tenant consent can). Update the corresponding test to pin ordering against OAuthTokenRequestException as well, so this regression cannot return silently.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Closes #54911.
When a customer connects Bing Ads from a Microsoft 365 tenant that has not granted admin consent to the bingads OAuth application, Microsoft returns:
The error arrives wrapped in an
invalid_clientenvelope. The current non-retryable error map matchesinvalid_clientfirst and shows the generic "reconnect your Bing Ads integration" toast — sending the customer through a reconnect loop that cannot succeed. The fix in the linked ticket required an org admin to grant tenant-wide consent, not a reconnect.Changes
posthog/temporal/data_imports/sources/bing_ads/source.py:AADSTS650052entry toget_non_retryable_errors, ordered beforeinvalid_clientsohandle_non_retryableinexternal_data_job.py(which picks the first substring match) surfaces the new message.How did you test this code?
test_get_non_retryable_errors_pattern_recognisedexercising the AADSTS650052 substring inside a realisticinvalid_client AADSTS650052: …error envelope.test_aadsts650052_message_wins_over_invalid_clientto pin two invariants:AADSTS650052must appear beforeinvalid_clientin the dict (ordering is load-bearing for first-match-wins).test_get_non_retryable_errors_does_not_match_transient_failuresstill passes — the AADSTS650052 substring is specific enough to not match transient transport errors.Publish to changelog?
no
Docs update
No docs change needed — error-surface tweak only.