Skip to content

fix(outlook): skip mailbox-less accounts and stop SSL misconfig from aborting sync - #4085

Merged
Jan-Kazlouski-elastic merged 9 commits into
mainfrom
jan-kazlouski/outlook-skip-missing-mailbox-and-ssl-cert
Jun 23, 2026
Merged

Jan-Kazlouski-elastic merged 9 commits into
mainfrom
jan-kazlouski/outlook-skip-missing-mailbox-and-ssl-cert

Conversation

@Jan-Kazlouski-elastic

@Jan-Kazlouski-elastic Jan-Kazlouski-elastic commented Jun 19, 2026 •

Copy link
Copy Markdown
Contributor

Relates to https://github.com/elastic/sdh-search/issues/1898

Fixes two independent failure modes in the Outlook Server connector that caused an entire sync job to abort when a single account or the SSL configuration was misconfigured. The guiding principle of this branch: per-account problems skip that account and continue; connection-wide problems fail the sync loudly (so they cannot silently empty the index).

Issue A — ErrorNonExistentMailbox kills the whole sync

When iterating over AD user accounts, get_mails() accesses account.inbox, which lazily resolves account.root via an EWS GetFolder call. For AD users whose SMTP address has no associated Exchange mailbox (a valid LDAP entry but no mailbox provisioned), exchangelib raises ErrorNonExistentMailbox. With no exception handling around the per-account block, this propagated up and terminated the sync, losing all remaining accounts. This is the still-open part of #2931.

Fix: the per-account block in get_docs() is wrapped in try/except ErrorNonExistentMailbox. A missing mailbox is specific to a single account, so the account is skipped with a warning and the sync continues with the remaining accounts.

Issue B — NO_CERTIFICATE_OR_CRL_FOUND SSL crash

When ssl_enabled=True but no certificate is provided, ssl_ca is set to "". The old code wrote that empty string to the cert file and still selected RootCAAdapter. When urllib3 later called context.load_verify_locations() on the empty file it raised NO_CERTIFICATE_OR_CRL_FOUND, aborting the sync. The error occurs inside the HTTP layer (not inside cert_verify()), so RootCAAdapter's own try/except never caught it.

Fix: in ExchangeUsers.get_user_accounts(), the cert file is written and RootCAAdapter is selected only when ssl_ca is actually populated. When SSL is enabled but no certificate is supplied, the connector falls back to NoVerifyHTTPAdapter and logs a clear warning instead of crashing.

Why connection-wide SSL errors are NOT skipped per account

An earlier iteration of this branch also caught a custom SSLFailed exception inside the per-account loop to "skip" accounts on SSL problems. That approach was removed because it was both ineffective and dangerous:

  • Ineffective: SSLFailed is only raised inside RootCAAdapter.cert_verify, and requests' cert_verify does not actually load the certificate — it only records the CA path. A genuinely bad/expired cert fails later, during the TLS handshake, where exchangelib catches the underlying requests.exceptions.SSLError and re-raises it as exchangelib.errors.TransportError (and explicitly does not retry it). So SSLFailed was never raised in practice and the except SSLFailed clause was dead code.
  • Dangerous: an SSL/connection failure affects every account, not one. Silently skipping all accounts would produce an empty but "successful" sync, which the framework interprets as "all documents deleted" — wiping previously indexed data.

Therefore only ErrorNonExistentMailbox (genuinely per-account) is skipped. Connection-wide failures — TLS errors surfaced as TransportError, and any other unexpected error — propagate and abort the sync loudly, so the misconfiguration is surfaced to the operator and existing indexed data is preserved.

Checklists

Pre-Review Checklist

  • this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check config.yml.example)
  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)
  • For bugfixes: backport safely to all minor branches still receiving patch releases
  • Considered corresponding documentation changes
  • Contributed any configuration settings changes to the configuration reference
  • if you added or changed Rich Configurable Fields for a Native Connector, you made a corresponding PR in Kibana

Changes Requiring Extra Attention

  • Security-related changes (encryption, TLS, SSRF, etc) — the SSL cert fallback behaviour changes: ssl_enabled=True with no cert now falls back to NoVerifyHTTPAdapter (unverified connections) instead of crashing. A genuinely invalid/expired certificate now fails the sync loudly rather than being silently skipped. Reviewers should confirm the no-cert fallback posture is desired.

Related Pull Requests

Release Note

Outlook Server connector: sync jobs no longer abort when an Active Directory user has a valid SMTP address but no associated Exchange mailbox (ErrorNonExistentMailbox). The affected account is now skipped with a warning and the sync continues. Additionally, configuring SSL without providing a certificate no longer crashes with NO_CERTIFICATE_OR_CRL_FOUND; the connector falls back to unverified connections and logs a clear warning. Genuine certificate/connection errors still fail the sync loudly rather than silently emptying the index.

Made with Cursor

Jan-Kazlouski-elastic and others added 2 commits June 19, 2026 18:06
…ad of aborting sync

Issue A: wrap per-account processing in get_docs() with try/except so that
ErrorNonExistentMailbox (raised by exchangelib when an SMTP address has no
associated mailbox) only skips that account instead of killing the entire sync.
SSLFailed is also caught per-account as a safety net.

Issue B: in ExchangeUsers.get_user_accounts(), only write the cert file and
use RootCAAdapter when ssl_ca is actually populated. Previously, ssl_enabled=True
with an empty certificate string would write an empty file and still pass it to
RootCAAdapter, causing urllib3 to fail with NO_CERTIFICATE_OR_CRL_FOUND. Now the
code falls back to NoVerifyHTTPAdapter and logs a clear warning in that case.

Co-authored-by: Cursor <cursoragent@cursor.com>
Removed an unnecessary blank line in datasource.py. This change improves code readability without affecting functionality.
…ection

Issue A: add tests asserting get_docs() skips an account that raises
ErrorNonExistentMailbox or SSLFailed (logging a warning and continuing with
healthy accounts) while still re-raising unexpected errors.

Issue B: add a parametrized test asserting ExchangeUsers.get_user_accounts()
selects RootCAAdapter only when a certificate is present, falls back to
NoVerifyHTTPAdapter with a warning when SSL is enabled without a cert, and uses
NoVerifyHTTPAdapter silently when SSL is disabled.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Jan-Kazlouski-elastic
Jan-Kazlouski-elastic marked this pull request as draft June 19, 2026 15:52
…ilently skipping

The previous SSL safeguard caught a custom SSLFailed exception per account,
but exchangelib wraps TLS/cert failures as TransportError (and explicitly
does not retry them), so SSLFailed was never raised in practice. Worse,
catching a connection-wide failure per account would yield an empty but
"successful" sync that deletes previously indexed documents.

Only ErrorNonExistentMailbox (genuinely per-account) is skipped now;
connection-wide errors propagate and abort the sync. Tests updated to cover
the real behavior.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Jan-Kazlouski-elastic Jan-Kazlouski-elastic changed the title fix(outlook): skip accounts with no mailbox or invalid SSL cert instead of aborting sync fix(outlook): skip mailbox-less accounts and stop SSL misconfig from aborting sync Jun 19, 2026
@Jan-Kazlouski-elastic
Jan-Kazlouski-elastic marked this pull request as ready for review June 19, 2026 16:11
account=account, timezone=timezone
):
yield child_calendar
except ErrorNonExistentMailbox:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would non-existent mailbox mean that there are no contacts, tasks, calendars and so on?

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.

Yes. In exchangelib, inbox, contacts, tasks, and calendar all resolve through the same account.root (Root.get_distinguished) which is what raises ErrorNonExistentMailbox. So a missing mailbox means none of them exist.
It's an account-level condition, not folder-level, so skipping the whole account is correct and nothing syncable is dropped.

@artem-shelkovnikov artem-shelkovnikov left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BaseProtocol.HTTP_ADAPTER_CLS = RootCAAdapter
else:
if self.ssl_enabled and not self.ssl_ca:
logger.warning(

@Jan-Kazlouski-elastic Jan-Kazlouski-elastic Jun 22, 2026 •

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.

Hi @artem-shelkovnikov
Last minute check regarding this unverified SSL connection fallback. It happens here if SSL is enabled, but there is no certificate. Customer gets a warning and the operation proceeds unverified. We might want to throw an error here to be safe. Warning is easy to miss and fallback might give customer false sense of security.

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.

Current approach fixes the reported issue, but I just thought that it might be a better solution to be a bit more restrictive here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

True so. Let's revert this code?

Why would the user want to run the connector with SSL enabled but without a certificate?

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.

Reverted this and moved check to validate_config method in order to have a clean understandable error message.

@artem-shelkovnikov
artem-shelkovnikov self-requested a review June 22, 2026 09:02

@artem-shelkovnikov artem-shelkovnikov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As Jan mentioned, there's a problem with SSL logic in the connector

Jan-Kazlouski-elastic and others added 4 commits June 22, 2026 12:13
…ing verification

Enabling SSL but supplying no certificate is a misconfiguration. The
previous fallback to NoVerifyHTTPAdapter silently downgraded the
connection to unverified TLS, weakening the guarantee the user explicitly
asked for. The framework already rejects this case up front: ssl_ca is a
required dependent field of ssl_enabled, so check_valid() raises a clear
"SSL certificate cannot be empty" error before any sync starts.

Restore the strict adapter selection in get_user_accounts and replace the
adapter-fallback test with regression tests asserting validate_config
rejects SSL-enabled-without-certificate and accepts it with a certificate.

Co-authored-by: Cursor <cursoragent@cursor.com>
The required-field check only verifies the SSL certificate field is filled
in; a present-but-malformed certificate still passed validation and then
crashed mid-sync with an opaque "NO_CERTIFICATE_OR_CRL_FOUND" X509 error.

Override validate_config to load the certificate through the exact same
transformation (get_pem_format) and loader (load_verify_locations) used at
sync time. A certificate that passes validation is therefore guaranteed not
to fail loading during the sync. Loading happens in-memory via cadata, so it
is fast and has no side effects.

Add tests covering missing, malformed, and valid certificates.

Co-authored-by: Cursor <cursoragent@cursor.com>
get_pem_format reflows a single-line value by replacing every space with a
newline, then repairing the BEGIN/END markers. When given input that is
already multi-line (e.g. a certificate copied verbatim from a .pem file), the
trailing newline caused the repair step to miss the space inside
"-----END CERTIFICATE-----", splitting the marker across two lines and
producing an unloadable certificate.

Detect already multi-line input (after stripping) and only normalize
per-line whitespace instead of reflowing it, so both the single-line and the
standard multi-line formats are accepted. Single-line handling is unchanged.

Add tests for multi-line certificates and private keys, surrounding
whitespace, and single-line values with a trailing newline.

Co-authored-by: Cursor <cursoragent@cursor.com>
Make the docstrings/comments added for SSL certificate validation and the
multi-line get_pem_format handling more concise, and extract the repeated
expected PEM certificate in test_utils into a single MULTILINE_PEM_CERTIFICATE
constant.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Jan-Kazlouski-elastic

Copy link
Copy Markdown
Contributor Author

Fix is applied, @artem-shelkovnikov
Could you give this PR another look?

@@ -847,6 +847,12 @@ def test_evaluate_timedelta():
assert expected_response == "2023-02-19T14:25:05.158843"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The way I read the tests below it looks like the change is not breaking existing logic, only fixing a bug when multiline PEM certificate is provided, is this correct?

Is there a chance this would break existing customers' setup for other connectors?

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.

Single-line certs/keys still take the original reflow path unchanged; the new branch only triggers on already-multiline input, which the old replace(" ", "\n") corrupted (e.g. real .pem with a trailing newline → broken -----END marker → NO_CERTIFICATE_OR_CRL_FOUND).
No regression for other connectors: GCS only calls it for newline-free keys, and Redis/MSSQL/Mongo/PostgreSQL/ssl_context get byte-identical output for single-line input and a correct result for multiline input the old code would have broken.
Verified by reverting just this function and re-running: unchanged-behavior tests pass on old & new, only the bug-fix cases fail on old — and the Redis/MSSQL/Mongo/PostgreSQL/GCS suites (154 tests) stay green.

@artem-shelkovnikov artem-shelkovnikov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🚀 🌓

@Jan-Kazlouski-elastic
Jan-Kazlouski-elastic enabled auto-merge (squash) June 23, 2026 12:28
@Jan-Kazlouski-elastic
Jan-Kazlouski-elastic enabled auto-merge (squash) June 23, 2026 12:32
@Jan-Kazlouski-elastic
Jan-Kazlouski-elastic merged commit cba96af into main Jun 23, 2026
2 checks passed
@Jan-Kazlouski-elastic
Jan-Kazlouski-elastic deleted the jan-kazlouski/outlook-skip-missing-mailbox-and-ssl-cert branch June 23, 2026 13:04
@github-actions

Copy link
Copy Markdown

💔 Failed to create backport PR(s)

Status Branch Result
✅ 9.3 #4091
✅ 9.4 #4092
❌ 8.19 Commit could not be cherrypicked due to conflicts

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
backport --pr 4085 --autoMerge --autoMergeMethod squash

Jan-Kazlouski-elastic added a commit that referenced this pull request Jun 23, 2026
… from aborting sync (#4085) (#4091)

Backports the following commits to 9.3:
- fix(outlook): skip mailbox-less accounts and stop SSL misconfig from
aborting sync (#4085)

Co-authored-by: Jan-Kazlouski-elastic <jan.kazlouski@elastic.co>
Co-authored-by: Cursor <cursoragent@cursor.com>
Jan-Kazlouski-elastic added a commit that referenced this pull request Jun 23, 2026
… from aborting sync (#4085) (#4092)

Backports the following commits to 9.4:
- fix(outlook): skip mailbox-less accounts and stop SSL misconfig from
aborting sync (#4085)

Co-authored-by: Jan-Kazlouski-elastic <jan.kazlouski@elastic.co>
Co-authored-by: Cursor <cursoragent@cursor.com>
Jan-Kazlouski-elastic added a commit that referenced this pull request Jun 24, 2026
…g from aborting sync (#4085) (#4093)

Backports the following commits to 8.19:
- fix(outlook): skip mailbox-less accounts and stop SSL misconfig from
aborting sync (#4085)

Made with [Cursor](https://cursor.com)

Co-authored-by: Cursor <cursoragent@cursor.com>
Jan-Kazlouski-elastic added a commit that referenced this pull request Jun 29, 2026
…cert-file race (#4094)

## Relates to elastic/sdh-search#1898

Follow-up to #4085. That PR fixed the case where SSL was enabled with an
empty
certificate. This PR addresses the remaining, **intermittent** failure
the
customer reported: `SSLError: [X509] no certificate or crl found
(_ssl.c:4279) (NO_CERTIFICATE_OR_CRL_FOUND)` that appears "once in a
while"
with no configuration change between runs — a classic race condition.

### Root cause — shared certificate file race

The Outlook Server connector verified the Exchange server's TLS
certificate by
writing the configured CA to a **fixed file on disk**
(`outlook_cert.cer` in the
process working directory) and pointing `requests`/`urllib3` at that
path:

- `ManageCertificate.store_certificate()` opened the file in `"w"` mode,
which
  **truncates it to zero bytes** before the new contents are written.
- `RootCAAdapter.cert_verify()` handed that single shared path to the
TLS layer.
- `ExchangeUsers.close()` deleted the file at the end of a sync.

Because the filename is process-global and shared by every sync,
concurrent or
overlapping Outlook syncs race on it. The failure window is small but
real:

1. Sync A is mid-handshake and the TLS layer reads the CA file.
2. Sync B starts and opens the same file in `"w"` mode, truncating it to
empty.
3. Sync A's read now sees an **empty file** →
`NO_CERTIFICATE_OR_CRL_FOUND`.

A second variant: Sync A finishes and deletes the file while Sync B
still
expects it to exist for a new connection. Either way the error is
timing-
dependent, non-reproducible, and unrelated to the (unchanged)
configuration —
exactly matching the customer's description.

### Fix — keep the CA in memory, never touch disk

The certificate is now loaded straight from the configured PEM string
into an
`ssl.SSLContext` via `ssl.create_default_context(cadata=self.ssl_ca)`,
and a
small `requests` adapter (`InMemoryCAAdapter`) injects that context into
`urllib3` through `init_poolmanager`/`proxy_manager_for`. No file is
created,
read, or deleted, so the shared-file race is structurally impossible.

Behavioural notes:

- **TLS verification is genuinely enforced.** `create_default_context()`
  returns a context with `verify_mode = CERT_REQUIRED` and
`check_hostname = True`, so a bad/expired/mismatched server certificate
still
  fails the handshake.
- **No-certificate fallback is preserved.** SSL enabled with no
certificate
still falls back to `NoVerifyHTTPAdapter` and logs a clear warning,
matching
  the behaviour introduced in #4085.
- `ManageCertificate`, `RootCAAdapter`, the `SSLFailed` exception and
the
`CERT_FILE` constant are removed; `close()` no longer needs to clean up
a file.

## Testing

Because there is no containerizable Microsoft Exchange/EWS + Active
Directory
image, this connector cannot use the repo's Docker-based `ftest`
harness. Instead
the change was validated locally in four layers — from static checks up
to **real
TLS handshakes** and **concurrency**, using freshly generated
self-signed
certificates and the production code (no Exchange server required).
**All checks
passed.**

### Layer 1 — Static checks + automated tests
- No remaining references to the removed `ManageCertificate`,
`RootCAAdapter`, `SSLFailed`, `CERT_FILE`, or `outlook_cert.cer` symbols
anywhere in the repo.
- Unit/integration suites green: `tests/sources/test_outlook.py` (48) +
`tests/test_utils.py` (110) = **158 passed**.

### Layer 2 — Real TLS handshake through the production adapter
Drove `InMemoryCAAdapter` against a local HTTPS server using a
self-signed certificate over real sockets (no mocks):
- Correct in-memory CA → connection **verified** (HTTP 200).
- Context enforces `CERT_REQUIRED` + `check_hostname`.
- Wrong in-memory CA → handshake **rejected** with `SSLError`.
- No `outlook_cert.cer` written to disk.

### Layer 3 — Connector code path with the real `ssl` module
Exercised `ExchangeUsers.get_user_accounts()` end-to-end with the real
`ssl.create_default_context` (only AD/LDAP lookups and
`exchangelib.Account` mocked):
- Connector selects `InMemoryCAAdapter`, and the context it builds
**verifies a live TLS server**.
- Marker-less junk cert (reduced to empty by `get_pem_format`) →
`SSLCertificateError`, with **no silent fallback to system CAs**.
- Non-empty but unloadable PEM → `SSLCertificateError`.
- SSL disabled → `NoVerifyHTTPAdapter` selected.
- No cert file written.

### Layer 4 — Concurrency & filesystem independence (the root-cause
scenario)
- SSL setup succeeds in a **read-only working directory** and writes
nothing — proving the disk dependency is gone.
- 4 concurrent SSL syncs (different CAs) complete without error, and
**no shared `outlook_cert.cer` ever appears** during the run — the file
race behind `NO_CERTIFICATE_OR_CRL_FOUND` is structurally eliminated.

<details>
<summary>Raw output of the local verification run (Layers 2–4)</summary>

```
=== Layer 2: production InMemoryCAAdapter over real TLS ===
  [PASS] correct in-memory CA -> TLS verified (HTTP 200)
  [PASS] context enforces verification (CERT_REQUIRED + check_hostname)
  [PASS] wrong in-memory CA -> handshake rejected (SSLError)
  [PASS] no outlook_cert.cer written to disk

=== Layer 3: connector code path with REAL ssl module ===
  [PASS] connector selected InMemoryCAAdapter
  [PASS] connector-built context verifies live server
  [PASS] markerless cert -> SSLCertificateError (no system-CA fallback)
  [PASS] unloadable PEM -> SSLCertificateError
  [PASS] SSL disabled -> NoVerifyHTTPAdapter selected
  [PASS] no outlook_cert.cer written to disk

=== Layer 4: concurrency + read-only working directory ===
  [PASS] SSL setup succeeds in read-only working directory
  [PASS] no cert file created in read-only dir
  [PASS] 4 concurrent SSL syncs complete without error
  [PASS] no shared cert file ever appears during concurrent syncs

ALL CHECKS PASSED
```
</details>

> A full content sync against a **live** Microsoft Exchange Server
remains a manual staging step (with a self-signed CA in a non-production
tenant). The layers above cover SSL adapter selection, real TLS
verification semantics, validation/rejection of bad certificates, and
the concurrency/file-race fix without it.

## Checklists

#### Pre-Review Checklist
- [x] this PR does NOT contain credentials of any kind, such as API keys
or username/passwords (double check `config.yml.example`)
- [x] this PR has a meaningful title
- [x] this PR links to all relevant github issues that it fixes or
partially addresses
- [x] if there is no GH issue, please create it. Each PR should have a
link to an issue
- [x] this PR has a thorough description
- [x] Covered the changes with automated tests
- [x] Tested the changes locally
- [ ] Added a label for each target release version (example: `v7.13.2`,
`v7.14.0`, `v8.0.0`)
- [ ] For bugfixes: backport safely to all minor branches still
receiving patch releases
- [ ] Considered corresponding documentation changes
- [ ] Contributed any configuration settings changes to the
configuration reference
- [ ] if you added or changed Rich Configurable Fields for a Native
Connector, you made a corresponding PR in
[Kibana](https://github.com/elastic/kibana/blob/main/packages/kbn-search-connectors/types/native_connectors.ts)

#### Changes Requiring Extra Attention

- [x] Security-related changes (encryption, TLS, SSRF, etc) — the CA is
now held in process memory instead of written to a file on disk, and TLS
verification is performed via an explicit `ssl.SSLContext`. Reviewers
should confirm verification is still enforced (`CERT_REQUIRED` +
hostname check) and that the no-certificate fallback to unverified
connections remains the desired posture.

## Related Pull Requests

* Builds on #4085

## Release Note

**Outlook Server connector**: fixed an intermittent
`NO_CERTIFICATE_OR_CRL_FOUND` SSL error that could abort syncs when
multiple syncs ran close together. The configured CA certificate is now
verified entirely in memory instead of through a shared temporary file,
removing the race condition. TLS verification behaviour is otherwise
unchanged.

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Jan-Kazlouski-elastic added a commit that referenced this pull request Jul 10, 2026
…assumptions (#4147)

Related to elastic/sdh-search#1898

Follow-up to #4123. The last month has been a series of "the next
malformed item aborts the whole sync" reports, most recently
`'DistributionList' object has no attribute 'email_addresses'`. This PR
fixes that class of failure with explicit, visible handling, and audits
the connector for the remaining assumptions of the same shape.

## Root cause

The Contacts folder returns two item models — `Contact` and
`DistributionList` — but the formatter assumed every item was a
`Contact` and read per-contact fields off it, so a single contact group
aborted the entire sync. The same "unhandled shape aborts everything"
gap existed elsewhere: Calendar/Tasks folders that shared and resource
mailboxes can legitimately lack, birthday events without a start, and an
optional LDAP `type` key.

## Changes

**Contacts formatted by item type.** `_fetch_contacts` now dispatches on
type: a `Contact` goes through the strict `contact_doc_formatter` and a
contact group through a dedicated `distribution_list_doc_formatter` that
indexes it by name and its members' email addresses (typed
`"Distribution List"`). The folder query fetches the union of both
models' fields (incl. `members`). The Contacts folder contractually
returns only these two models, so any other type raises a `TypeError`
rather than being silently skipped — an unknown type is a broken
assumption we want surfaced, not a whole category of items quietly
dropped. (Thanks @artem-shelkovnikov for the review — explicit type
handling over defensive `getattr`, and failing on unexpected types.)

**Note on approach.** An earlier revision of this PR wrapped all
formatting in a broad `_format_item` guard that caught item-shape errors
(`AttributeError`/`KeyError`/`ValueError`/`TypeError`) and skipped the
item. That was dropped: swallowing those errors around every item can
turn a systematic bug into a "successful" sync that returns 0 items and
deletes previously indexed documents. Instead, known shapes are handled
explicitly (type dispatch + targeted field guards) and genuine errors
still fail the sync loudly — consistent with the principle established
in #4085/#4123.

**Remaining assumptions found in the audit, now fixed:**

* **Calendar & Tasks folders** are now skipped on `ErrorFolderNotFound`,
mirroring the existing Contacts/mail-folder handling. Shared and
resource mailboxes can legitimately lack these folders; previously that
aborted the sync (same class as #4065).
* **Birthdays calendar branch** called `.split("T")` on the formatted
datetime, which is `None` when `calendar.start` is missing →
`AttributeError`. Now guarded.
* **Optional LDAP `type` key** is read with `.get(...)` instead of
`user["type"]`.

## Test plan

* `pytest tests/sources/test_outlook.py` (65 tests), incl. coverage for:
   * contact / `DistributionList` type dispatch and the group formatter
   * unexpected Contacts item type → raises `TypeError`
   * `ErrorFolderNotFound` skips for Calendar / child calendars / Tasks
   * birthday without a start
   * missing LDAP `type` key
* `ruff check` / `ruff format --check`

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Jan-Kazlouski-elastic added a commit that referenced this pull request Jul 16, 2026
…ror handling (#4158)

Related to elastic/sdh-search#1898

Follow-up to #4147. That PR fixed the Contacts folder's
single-item-model assumption; this one applies the same "one malformed
item/folder must not abort the whole sync" principle to every remaining
folder-iteration and error-handling site the audit surfaced, most
recently the `'CalendarItem' object has no attribute 'sender'` /
localized-`Archive` class of reports.

## Root cause

The connector iterated Exchange folders assuming each folder returns
exactly one item model, that every distinguished folder resolves, and
that the only recoverable per-folder fault is `ErrorFolderNotFound`.
Real mailboxes violate all three: a Calendar/Tasks/mail folder can
contain stray item types, a child folder under Calendar may not be a
calendar folder (so the `CALENDAR_FIELDS` projection raises
`ValueError`), folder resolution can fail with
`ErrorManagedFolderNotFound`, an inaccessible mailbox raises
`ErrorAccessDenied`, an attachment can be an embedded `ItemAttachment`
(no `content`) or lack a `size`, and the timezone fallback was a bare
string that the datetime formatter can never use. Each of these aborted
the entire sync (or mis-typed an item) instead of being handled visibly.

## Changes

**Type allowlists on every folder iteration.** `_fetch_mails`,
`_fetch_tasks`, `_enqueue_calendars`, and `_fetch_contacts` now
route/skip by type: mail folders accept
`Message`/`MeetingRequest`/`MeetingResponse`/`MeetingCancellation`,
Tasks accept `Task`, Calendars accept `CalendarItem`, and Contacts route
`Contact` vs `DistributionList`. A stray type is logged and skipped
rather than crashing on a missing field. Contacts now skip-and-warn on
an unexpected type (rather than raising `TypeError` as in #4147) for
consistency with the other folders — the warning keeps it visible, and a
single odd item no longer aborts the sync.

**Broadened per-folder skips.**
`get_mails`/`get_calendars`/`get_child_calendars`/`get_tasks`/`get_contacts`
skip on `FOLDER_SKIP_ERRORS` (`ErrorFolderNotFound` +
`ErrorManagedFolderNotFound`) instead of only `ErrorFolderNotFound`.

**Child-calendar projection guard.** A non-calendar child folder under
Calendar can't be projected with `CALENDAR_FIELDS` and raises
`ValueError`; that child is now skipped (`CHILD_CALENDAR_SKIP_ERRORS`)
while healthy sibling calendars still sync.

**Account-level access errors.** `get_docs` now skips an account on
`ErrorAccessDenied` in addition to `ErrorNonExistentMailbox`.
Connection-wide `TransportError`s (e.g. TLS failures) still propagate —
silently swallowing them across all accounts would yield a doc-less
"successful" sync that deletes previously indexed documents (the
principle from #4085/#4123).

**Attachment guards.** `get_content` only extracts `FileAttachment`
content (embedded `ItemAttachment` has no `content`) and guards the
optional `size`, which may be `None`.

**Timezone fallback.** Replaced the dead `DEFAULT_TIMEZONE = "UTC"`
string fallback with the real `exchangelib.UTC` (`EWSTimeZone`), so the
`account.default_timezone or UTC` path actually formats datetimes if a
mailbox ever lacks a default timezone.

## Test plan

* `pytest tests/sources/test_outlook.py` (92 tests), incl. coverage for:
* `FOLDER_SKIP_ERRORS` (`ErrorFolderNotFound` +
`ErrorManagedFolderNotFound`) skips across mails / calendars / child
calendars / tasks / contacts
* child-calendar `ValueError` / managed-folder skip, with a healthy
sibling still syncing
* account skip on `ErrorAccessDenied`, and connection-wide errors still
re-raised
* every type in `MAIL_ITEM_TYPES` accepted, and stray non-mail /
non-task / non-calendar / unexpected-contact items skipped-and-warned
  * `get_content` skips `ItemAttachment` and `None` / `0` size
  * UTC timezone fallback when `default_timezone` is missing
* `ruff check` / `ruff format --check`
* 97% line coverage on `client.py` and `datasource.py`; every changed
line exercised

## Release Note

Harden the Outlook connector so unexpected Exchange item types,
unresolvable or inaccessible folders, embedded/size-less attachments,
and missing mailbox timezones are skipped with a warning instead of
aborting the sync.

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Jan-Kazlouski-elastic added a commit that referenced this pull request Jul 28, 2026
…sync (#4287)

## Part of elastic/sdh-search#1898

Follow-up to #4158. The reporter's Exchange server aborts every sync
with:

```
ValueError: Item type {http://schemas.microsoft.com/exchange/services/2006/types}EndTimeZone was unexpected in a BaseFolder folder
```

### Root cause

`CALENDAR_FIELDS` asks for `start` and `end`, and exchangelib silently
appends two more fields to that projection — `calendar:StartTimeZone`
and `calendar:EndTimeZone` (`BaseFolder.normalize_fields`). The server
then returns the `EndTimeZone` element as a sibling of the
`CalendarItem` it belongs to, inside the response's item container,
rather than nested within it.

exchangelib treats every element of that container as an item, looks its
tag up in `BaseFolder.ITEM_MODEL_MAP`, and raises `ValueError` when it
is absent. This is the same failure shape as
ecederstrand/exchangelib#877 (`Booking` items on servers with Microsoft
Bookings installed).

Two things make it fatal rather than merely noisy:

* The exception is raised while
`list(folder.all().only(*CALENDAR_FIELDS))` is materialised in
`get_calendars`, so the per-folder type allowlists added in #4158 never
see any items — the entire folder is lost, not one element.
* `get_docs` skips an account only on `ErrorNonExistentMailbox` /
`ErrorAccessDenied`, so a bare `ValueError` propagates and aborts the
whole sync.

### Change

`BaseFolder.ITEM_MODEL_MAP` is replaced with a `dict` subclass whose
`__missing__` returns the generic `Item` and logs a warning naming the
exact tag — once per tag, since a single folder can hold many copies of
the same stray element. Extending that map is the extension point
exchangelib's maintainer recommends for this class of problem.

The degraded `Item` then reaches the existing allowlists from #4158,
which skip it with a warning while healthy siblings keep syncing. All
three exchangelib call sites (`GetItem`, `FindItem`, `SyncFolderItems`)
resolve tags through this one map, so mails, contacts, tasks and
calendars are covered by the single change.

Genuine errors still fail loudly. Only the unmappable element is
dropped, so this cannot turn a systematic failure into a doc-less
"successful" sync that deletes previously indexed documents — the
principle established in #4085 / #4123 / #4147.

### Testing

* `pytest tests/sources/test_outlook.py` — 103 tests, including the four
added here: known tags still resolve to their own models, an unknown tag
degrades to `Item` and warns once, the reporter's exact response shape
parses into `[CalendarItem, Item]` instead of raising, and the degraded
item is skipped by the calendar allowlist.
* Full service suite: 2302 passed, total coverage 92.19% (gate 90%).
`client.py` and `datasource.py` at 97%; every line added here is
exercised.
* `ruff check` / `ruff format --check` clean on `connectors` and
`tests`; `pyright tests` clean, `pyright connectors` unchanged at the 13
pre-existing errors present on `main`.
* Before/after check against the reporter's response shape through
exchangelib's real parsing path: `ValueError` as reported without the
change, `['CalendarItem', 'Item']` plus the new warning with it.
* No E2E coverage: Outlook has no ftest fixture (no emulated backend),
and the failure only reproduces on the reporter's server. Verification
is the parsing check above against the exact XML shape from the SDH.

## Checklists

#### Pre-Review Checklist
- [x] this PR does NOT contain credentials of any kind, such as API keys
or username/passwords (double check `config.yml.example`)
- [x] this PR has a meaningful title
- [x] this PR links to all relevant github issues that it fixes or
partially addresses
- [x] if there is no GH issue, please create it. Each PR should have a
link to an issue
- [x] this PR has a thorough description
- [x] Covered the changes with automated tests
- [x] Tested the changes locally
- [x] Added a label for each target release version (example: `v7.13.2`,
`v7.14.0`, `v8.0.0`)
- [x] Considered corresponding documentation changes
- [ ] Contributed any configuration settings changes to the
configuration reference
- [ ] if you added or changed Rich Configurable Fields for a Native
Connector, you made a corresponding PR in
[Kibana](https://github.com/elastic/kibana/blob/main/packages/kbn-search-connectors/types/native_connectors.ts)

#### Changes Requiring Extra Attention

- [ ] Security-related changes (encryption, TLS, SSRF, etc)
- [ ] New external service dependencies added.
- [x] `BaseFolder.ITEM_MODEL_MAP` is mutated process-wide at import
time. Outlook is the only consumer of exchangelib, and this mirrors the
existing process-global `BaseProtocol.HTTP_ADAPTER_CLS` assignment from
#4094. A test asserts that every model exchangelib already knows still
resolves to itself, so the map only ever adds a fallback.

## Related Pull Requests

* #4158 — per-folder item type
allowlists this change feeds into
* #4147 — Contacts item type
dispatch
* #4123 — nullable field
hardening
* #4085 — per-account skips
vs. connection-wide failures
* #4065 — localized Exchange
folder names

The 8.19 backport will need to be done by hand: that branch still has
the single-file `connectors/sources/outlook.py` layout, so the
cherry-pick conflicts (as it did for #4158).

## Release Note

Fixed an issue where the Outlook connector aborted an entire sync when
an Exchange server returned an element it did not recognise as an item,
such as a stray `EndTimeZone` alongside a calendar item. Such elements
are now skipped with a warning and the rest of the mailbox continues to
sync.

Made with [Cursor](https://cursor.com)

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Jan-Kazlouski-elastic added a commit that referenced this pull request Sep 1, 2026
Part of elastic/sdh-search#1898
Part of #2931

## Problem

On large on-prem Exchange deployments the connector enumerates users
from Active Directory and opens each mailbox via EWS impersonation using
the LDAP `mail` attribute. When that value is a proxy or alias address
rather than the account's primary SMTP address, Exchange raises
`ErrorNonPrimarySmtpAddress` during folder resolution (`account.inbox` →
`account.root`).

This is a distinct failure mode from the cases already handled in #4085:

| Already handled | This PR |
|-----------------|---------|
| No `mail` attribute (#4078) | Has `mail`, but it is not the primary
SMTP |
| `ErrorNonExistentMailbox` — no mailbox provisioned (#4085) |
`ErrorNonPrimarySmtpAddress` — mailbox exists, wrong address used |

Because `ErrorNonPrimarySmtpAddress` was not in the per-account skip
handler, a single bad AD entry aborted the entire sync. A customer on
SDH #1898 hit this after indexing 1.6M+ documents over ~15 hours.

## Fix

Add `ErrorNonPrimarySmtpAddress` to the existing per-account
`try/except` in `get_docs()`. The affected account is skipped with a
warning; the sync continues with remaining accounts. Connection-wide
errors still propagate.

## Checklists

#### Pre-Review Checklist
- [x] this PR does NOT contain credentials of any kind, such as API keys
or username/passwords (double check `config.yml.example`)
- [x] this PR has a meaningful title
- [x] this PR links to all relevant github issues that it fixes or
partially addresses
- [x] if there is no GH issue, please create it. Each PR should have a
link to an issue
- [x] this PR has a thorough description
- [x] Covered the changes with automated tests
- [x] Tested the changes locally
- [ ] Added a label for each target release version (example: `v7.13.2`,
`v7.14.0`, `v8.0.0`)
- [ ] For bugfixes: backport safely to all minor branches still
receiving patch releases
- [x] Considered corresponding documentation changes
- [x] Contributed any configuration settings changes to the
configuration reference
- [x] if you added or changed Rich Configurable Fields for a Native
Connector, you made a corresponding PR in
[Kibana](https://github.com/elastic/kibana/blob/main/packages/kbn-search-connectors/types/native_connectors.ts)

#### Changes Requiring Extra Attention

- [ ] Security-related changes (encryption, TLS, SSRF, etc)
- [ ] New external service dependencies added.

## Related Pull Requests

* #4085 — established the
per-account skip pattern for `ErrorNonExistentMailbox`
* #4078 — skip AD users
without a valid `mail` attribute

## Release Note

**Outlook Server connector**: sync jobs no longer abort when an Active
Directory user has a valid `mail` attribute that is not the primary SMTP
address (`ErrorNonPrimarySmtpAddress`). The affected account is skipped
with a warning and the sync continues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants