Skip to content

Docs: shared mailbox (M365/Exchange) spec for review (#31) - #59

Open
CityDweller wants to merge 3 commits into
mainfrom
shared-mailbox-spec
Open

Docs: shared mailbox (M365/Exchange) spec for review (#31)#59
CityDweller wants to merge 3 commits into
mainfrom
shared-mailbox-spec

Conversation

@CityDweller

Copy link
Copy Markdown
Collaborator

Design spec for Issue #31 — shared mailbox support with auto-detection, submitted for review.

📋 Spec only — no code, and intentionally parked. Per our plan, implementation won't start until the Graph backend PRs (dev-spec PRs 4–8) and the rest of the in-flight roadmap are done. Reviewing the design now means it's agreed and de-risked before we build.

Key findings baked into the spec

  • Access needs no Graph — a shared mailbox opens on today's IMAP/OAuth stack: delegate user's token + shared address in the XOAUTH2 user= field. IMAP.AccessAsUser.All (already requested) suffices.
  • Auto-detection = Exchange Autodiscover parsing AlternativeMailbox — which needs the EWS.AccessAsUser.All scope for discovery only, plus enabling it on the Azure app registration.
  • Graph can't auto-detect mailboxes, so this feature is independent of the Graph backend and won't wait on it.
  • Recommends a linked-account model (shared mailbox = a parent-linked, first-class account entity) that reuses the existing per-account sync/pool/router machinery and avoids a schema migration — versus the issue's cache-key mailbox-dimension approach, which is a much wider change.

Decisions I'd like your call on (spec §14)

  1. Azure app registration (blocking for detection): can you add EWS.AccessAsUser.All (O365 Exchange Online, delegated) to the shared client ID, and does your tenant need admin consent? Access works without it; only auto-detection depends on it.
  2. Model approach: OK with the linked-account model (no schema migration) over the cache-key mailbox dimension from the issue? This is the biggest design fork.
  3. Send-As: is sending from a shared mailbox in scope for v1, or read-first with send as a fast-follow?

No rush given it's parked — review whenever; I'll fold your answers in before implementation begins.

Proposed design for Issue #31 — shared mailbox support with auto-detection.
Parked behind the Graph backend PRs; submitted now for review so the design is
agreed before any code. Covers: IMAP XOAUTH2 access with the shared address in
the user= field (no Graph needed), Autodiscover detection (+ the new
EWS.AccessAsUser.All scope and the Azure app-registration dependency), a
recommended linked-account model that avoids a schema migration, RFC 2342
NAMESPACE for generic IMAP, UX/accessibility, a PR breakdown, edge cases,
tests, and open questions for review.
The "maintainer adds a scope" step read as if it were per-mailbox or
per-user-tenant. Adds shared-mailboxes spec section 6.2-6.3 and mirrors a
concise version into the Graph PM spec 9.3:

- one app registration = QuickMail's own identity (maintainer's tenant), NOT
  per mailbox or per user-tenant; declaration (maintainer, once) vs. consent
  (each user's tenant, at sign-in); end users never create a registration.
- Microsoft-only concept; Gmail / generic IMAP are unaffected.
- the real friction is tenant policy: admin-consent handling (AADSTS65001 /
  90094 + an admin-consent URL), publisher verification for the "unverified"
  warning, and a per-account custom client-ID escape hatch for locked tenants.
- multi-tenant + personal-MSA registration is a prerequisite to verify.
@CityDweller

Copy link
Copy Markdown
Collaborator Author

Added a "Microsoft consent model & app registration" section (§6.2–6.3) after a good question about how the scope/registration actually works. It clears up a common misconception:

  • The app registration is QuickMail's own identity — one registration, in the maintainer's tenant, not per-mailbox or per-user-tenant. Adding a scope is declaration (maintainer, once); each user's tenant just consents at sign-in. End users never create or edit an app registration. App registrations are Microsoft-only, so Gmail / generic IMAP are unaffected.
  • The real friction is tenant policy, not user capability: orgs that require admin consent (handled gracefully via the AADSTS65001/90094 error + an admin-consent URL), the unverified-publisher warning (publisher verification), and the per-account custom client ID escape hatch for locked-down tenants. Plus a prerequisite to confirm: the registration is multi-tenant + personal MSAs.

Since the identical model governs the Graph Mail.* scopes, I also mirrored a concise version into graph-backend-pm-spec.md §9.3 (so this PR now touches that file too — one logical change: document the consent model across both M365 specs).

- Graph PM spec 9.3: add a complete delegated-permission table (User.Read,
  Mail.ReadWrite, Mail.Send, MailboxSettings.Read, User.ReadBasic.All,
  offline_access, openid/profile) with the reason + consent level for each.
- Add User.Read and User.ReadBasic.All to GraphMailScopes (dev spec 6.8) for
  the /me identity probe and directory/GAL user search (recipient autocomplete
  / finding a user). Note the People.Read alternative and that directory-read
  is the permission most likely gated behind admin consent.
- Clarify that EWS.AccessAsUser.All (shared mailboxes) is a separate Exchange
  Online permission, not a Graph one.
@kellylford

Copy link
Copy Markdown
Owner

@CityDweller — this has been open since Jun 9 with no activity, and a lot has shipped since. Is it still live, or superseded?

Asking rather than closing it, because I can't tell from the outside. Since this was opened, the Graph/M365 backend landed end to end (read path, send, mutations, folder nesting, change notifier), M365/Exchange went default-on in 0.8.35, and the whole server-rules stack (#334, #335, #337, #339, #364, #367) merged. A shared-mailbox spec written before any of that may describe a design the implementation has since diverged from — or it may still be the plan of record for work nobody has started.

Three ways this can go:

  1. Still the plan — rebase it onto main (it's BEHIND) and I'll review it properly against what actually shipped.
  2. Superseded — close it, and if any of the thinking is still worth keeping, say which parts so they can move into docs/planning/ rather than being lost with the PR.
  3. Deferred — close the PR but open an issue so shared-mailbox support stays on the list instead of living in a stale branch.

@kellylford — flagging for you too, since #31 is the underlying request and whether shared mailboxes are still on the roadmap is your call, not something I should infer from the branch going quiet.

@CityDweller

Copy link
Copy Markdown
Collaborator Author

Still live — option 1. Shared mailboxes are still on my list; the branch went quiet because I deliberately parked this behind the Graph/M365 and server-rules work, which is exactly what's been shipping. That stack has now largely landed (the Graph backend end-to-end, default-on M365 in 0.8.35, and the server-rules chain #334/#335/#337/#339/#364/#367#367 merged just now), so this is next up after the one server-rules piece still in flight: the unified single-list rules manager (§20 of the server-rules spec).

Your divergence worry is fair, but I think what shipped has de-risked the design rather than invalidated it:

  • The spec models each shared mailbox as a parent-linked account that reuses the per-account pipeline. Since I wrote it, Rules: retire "All accounts" — per-account migration + scoping (#333 D1) #364 landed that per-account pipeline for real (rules are now per-account; "All accounts" is retired). So the core structural assumption is now backed by shipped code, not speculative.
  • Access is still IMAP/OAuth (shared address in the XOAUTH2 user= field) — the Graph backend is additive and didn't disturb that path. Auto-detection via Autodiscover still needs the EWS.AccessAsUser.All scope (the §14 decision that's yours), and manual "add by address" still works without it.

So the plan of record holds; it needs a refresh pass, not a rewrite. I'll rebase #59 onto main and update the spec to reference what actually shipped (per-account pipeline, the consent model as it now stands), then it's ready for a real review — sequenced right after the unified rules list lands. Whether it stays at that spot on the roadmap is your call; I'm willing to build it.

@kellylford

Copy link
Copy Markdown
Owner

Reviewed against main as of #393 (account setup), which landed today and moves the ground under §3. The data-model reasoning still looks right to me; the access mechanism needs revisiting. Everything below I checked in the code rather than inferred.

1. The central assumption has changed — §3.1 and §3.4

The spec's headline is that this rides the existing IMAP/OAuth stack and "does not wait on, or benefit from, the Graph work". That was true when written. It isn't now.

#393 routes work-or-school Microsoft 365 accounts on a custom domain onto the Graph backend by default. The IMAP path requests outlook.office.com/IMAP.AccessAsUser.All + SMTP.Send, which most tenants have never consented to — sign-in was ending at "your administrator needs to make a change" on a real tenant.

That is precisely the user in §2: a blind or low-vision worker on a corporate M365 tenant. Their parent account is now a Graph account, which means:

  • there is no IMAP connection to hang a linked shared mailbox off, and
  • they never receive IMAP.AccessAsUser.All at all. OAuthService.DefaultScopesFor returns the IMAP scopes only when BackendKind != MicrosoftGraph; a Graph account gets Graph scopes.

So §3.1's "The scope QuickMail already requests — IMAP.AccessAsUser.All — is sufficient for access, no new scope is needed" no longer holds for the target user.

Two ways out, and I think the spec should pick one explicitly rather than inherit the old assumption:

Worth being precise about what §3.4 still gets right: Graph cannot return the automapped list, so detection still has to go through Autodiscover regardless. What has changed is access. The clean split is probably "detect via Autodiscover, access via whatever backend the parent account uses" — which is a bigger change than the spec currently contemplates.

This likely reopens §5.1's Approach A/B analysis, since the linked-account model's main appeal was reusing the per-account IMAP pool. I still think Approach B is right — but for the "no schema migration, first-class in the UI" reasons rather than the connection-reuse one.

2. §6.1 is stale

OAuthService requests a single static Scopes array.

That hasn't been true since #208 / #218 / #242. There are five arrays now — ImapSmtpScopes, GraphMailScopes, GraphMailScopesPersonal, GraphContactScopes, GraphCalendarScopes — selected per account type by DefaultScopesFor. The patch in §6.1 doesn't apply, and it matters which array gains the scope: adding it to the IMAP set means Graph-backend accounts never get it, which is the population that needs it.

Also worth noting the consent-impact line understates things now. Per Microsoft's docs, once a grant exists .default does no incremental consent — it returns the old grant silently rather than re-prompting. So "existing users see a re-consent prompt when their refresh token expires" may not happen at all; they may just silently lack the scope. (This overlaps with what #392 is wrestling with.)

3. Resource identifier mismatch — worth pinning down before the registration edit

The spec writes https://outlook.office365.com/EWS.AccessAsUser.All. The codebase uses the https://outlook.office.com/ identifier for its Exchange Online scopes. Both are valid identifier URIs for that service principal, so this may be harmless — but a token request cannot span resources, and I haven't verified how MSAL treats two different host forms in one array. Cheap to settle, expensive to discover during the registration change. §14 Q1 should probably name the exact string.

4. What the new Autodiscover client does and does not give you — §7

#393 added QuickMail/Services/AutoDiscoverService.cs. Reusable:

  • HTTPS-only with host-restricted redirect following (max 3 hops), and outlook.com is already on the allowlist, so autodiscover-s.outlook.com is permitted
  • 301/302/303 downgrade to a bodyless GET, response size capped

Not reusable, and each is real work:

  • Different protocol. It speaks POX v1 — POST .../autodiscover/autodiscover.xml with the outlook/requestschema/2006 envelope. §3.2 needs the SOAP service at autodiscover.svc with GetUserSettings. Different endpoint, envelope, and response schema.
  • It is anonymous. No Authorization header anywhere, and the constructor takes IProviderCatalog + IConfigService — no IOAuthService. Automapping data is only returned to an authenticated caller.
  • AlternativeMailbox is not parsed — zero occurrences in the file, and the parser only enumerates Protocol elements.
  • DiscoveredSettings cannot carry it. It is a flat imap/smtp/provider record, and DiscoverAsync returns the first non-null tier and stops. Shared-mailbox discovery needs its own method and result type on IAutoDiscoverService.
  • A 401 is swallowed as "found nothing." Any non-success status returns null so the next tier runs. Fine for settings discovery; for shared mailboxes it means a missing scope or permission is silent and undiagnosable. That path needs to distinguish "no mailboxes" from "not allowed to ask".

Also a lifecycle difference worth designing around: settings discovery runs before sign-in, from the add-account dialog, on an address with no token. Shared-mailbox discovery runs after sign-in, per account, with a token, and wants periodic refresh.

5. §14 Q4 — the parking condition is satisfied but inverted

"Confirmed parked behind the Graph backend PRs." Graph has landed and is on by default, so the gate is lifted — but the reason for parking has flipped. The spec parked this because Graph was incomplete; now Graph being complete and default is what invalidates §3.1.

What I would keep unchanged

The linked-account recommendation, the manual "add shared mailbox by address" fallback for automapping-disabled grants, and the RFC 2342 NAMESPACE path for generic IMAP — that last one is genuinely low marginal cost and is unaffected by any of the above, since non-Microsoft IMAP accounts still use the IMAP backend.

The §2 problem statement is also worth restating: Outlook surfaces these automatically and no accessible client does it well. That is still the reason to build it.

Suggested amendments

  1. Rewrite §3.1 and §3.4 for a Graph-backend parent account, and say explicitly which access mechanism v1 uses.
  2. Revisit §5.1 in that light — I think the conclusion holds, but the stated reason doesn't.
  3. Replace §6.1's patch with the current per-account-type scope selection, naming which array gains EWS.AccessAsUser.All and what happens for Graph-backend accounts.
  4. Reconcile the EWS resource identifier with the codebase, and put the exact string in §14 Q1.
  5. Expand §7 to note that the new client shares HTTP hardening but not protocol, auth, parsing, or result shape — and that discovery needs to distinguish "none" from "not permitted".
  6. Add an open question: does a Graph-backend parent change the answer to Q2?

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