feat(microsoft): withhold tenancy instead of failing sign-in on directory error - #90
Merged
Merged
Conversation
…tory error The Graph organization lookup only enriches an identity that the userinfo call has already established, but any failure of it — a 403 from a tenant that has not consented, a transient outage, an unparseable body — raised IdentityFetchError and blocked the sign-in outright. Degrade every one of those paths to tenancies=() with a logged warning. This is fail-closed for authorization: withholding the assertion makes domain_owning_tenancy() return None, which refuses a domain-gated grant rather than opening one, and it joins the existing "no assertion" return already used for personal accounts, B2B guests, and failed chain-of-trust checks. Only the userinfo call, which establishes identity itself, still raises. The exception value is never logged. The status code and exception class are the actionable facts for separating a permanent consent problem from a transient one; a test asserts the bearer token stays out of the logs. Retrieval moves to _fetch_organization so the degradation logging stays separate from tenant binding and domain mapping. Closes #74
…siently The assertion resolves from a live directory call, so a consumer can see it absent for reasons unrelated to the tenant's actual domains. Record that it is withheld rather than fabricated on failure, and that a consumer persisting the result should re-resolve rather than treat one absent assertion as durable. Also record that the lookup needs no consent beyond the User.Read scope already in BASE_SCOPES, resolving the open scope question from #74.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesMicrosoft organisation lookup failures now degrade to empty tenancy assertions with warnings instead of failing identity retrieval. The package also configures a default Identity and logging behaviour
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the Microsoft identity provider to gracefully degrade when the Microsoft Graph /v1.0/organization lookup fails, so sign-in succeeds but domain-ownership tenancy assertions are withheld (tenancies=()), with a warning logged instead of raising.
Changes:
- Refactors Graph organization retrieval into
_fetch_organization()that never raises and logs actionable warnings without leaking bearer tokens. - Updates tenancy resolution to treat any organization-lookup failure/untrusted response as no asserted tenancies rather than a sign-in failure.
- Replaces the prior “organization call failure raises” test with a degradation-focused suite covering HTTP status errors, transport errors, non-JSON responses, and empty collections; README is updated to document the behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/apron_auth/providers/microsoft.py |
Introduces non-raising organization fetch + warning logs; _fetch_verified_tenancies() now degrades to () on directory failures. |
tests/providers/test_microsoft.py |
Replaces the raising test with TestMicrosoftOrganizationDegradation asserting usable identity + tenancies=() + warning logs without token leakage. |
README.md |
Documents that Microsoft’s domain-ownership assertion may be transiently withheld and should be re-resolved if persisted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The package attached no handler anywhere on its logger chain, so logging.lastResort wrote WARNING and above straight to the consumer's stderr whenever the application had configured no logging. A library has no standing to emit that output uninvited, and the preceding commit made it materially worse by taking the Microsoft handler from one warning path to five — one of which, an unconsented tenant returning 403, fires on every sign-in for that deployment. Attach a NullHandler to the apron_auth logger. Records then find a handler and the fallback stays out of it, while any handler the application configures continues to receive them. Document the contract in the README, since none of it was discoverable before: the logger names, the two levels in use and what each means, and that configuring the apron_auth logger reaches the whole library. State outright that there is no logger parameter to inject, because the hierarchy is the injection point and a second configuration surface would only compete with it. Also state what is never logged — credentials, token bytes, and exception values — as a contract consumers can rely on and report against, rather than a property they have to infer by reading the source.
Comment on lines
225
to
+229
| body = response.json() | ||
| except ValueError as exc: | ||
| raise IdentityFetchError(f"Failed to parse Microsoft organization response: {exc}") from exc | ||
| except ValueError: | ||
| logger.warning( | ||
| "microsoft directory lookup could not parse the tenant's verified domains; " | ||
| "not asserting domain ownership" |
Contributor
Author
There was a problem hiding this comment.
Same reasoning as the HTTP branch, and also not changing.
response.json() raises json.JSONDecodeError essentially universally here, so type(exc).__name__ would be a near-constant. The message already says the parse failed; "(JSONDecodeError)" adds no information a reader does not already have.
The README wording that implied otherwise is corrected in 8a972f4.
…main Every path that withholds domain ownership logged a warning except this one: an organization whose verifiedDomains is absent, malformed, or holds no entry with a non-empty name returned an empty tuple silently. The consumer-visible symptom is identical to a failed lookup — domain-gated access refuses — so leaving it unobservable gave an operator nothing to search for in the one case the response looked superficially fine. Every Entra tenant carries at least an *.onmicrosoft.com domain, so an organization naming none is anomalous rather than a tenant legitimately owning nothing, and is worth surfacing. Fold the shape check into the name extraction so both the malformed and the empty case reach one warning, rather than two branches that must be kept in step. Also correct the README's logging contract, which claimed the exception class is logged alongside the status code for HTTP failures. It is not, deliberately: httpx.HTTPStatusError has no subclasses, so its class name is a constant that would add noise to every degraded sign-in, and response.json() raises JSONDecodeError near-universally. The class is logged for RequestError, where the subclass distinguishes a connect failure from a timeout. State the guarantee — never the exception value — and let the diagnostic carried vary, rather than specifying a format the code has no reason to follow uniformly.
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
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.
Closes #74
What
Two related changes.
The Graph organization lookup no longer fails the sign-in when it errors. Every failure mode degrades to
tenancies=()plus a logged warning.The library no longer writes to a consumer's stderr uninvited, and its logging contract is documented for the first time.
Why the degradation
The lookup only enriches an identity that the userinfo call has already established. Raising
IdentityFetchErrorescalated a third-party problem — a tenant that never consented, a throttle, a transient outage — into a total sign-in failure for a user whose identity was never in doubt.Degrading is fail-closed for authorization: withholding the assertion makes
owns_domain()returnFalse, which refuses a domain-gated grant rather than opening one.()was already the value for personal accounts, B2B guests, and failed chain-of-trust checks, so consumers necessarily handled it. Only the userinfo call, which establishes identity itself, still raises.Why the logging fix
The package attached no handler anywhere on its logger chain, so
logging.lastResortwrote WARNING and above straight to the consumer's stderr whenever the application had configured no logging. That was pre-existing, but the degradation work made it materially worse — the Microsoft handler went from one warning path to five, and one of them (an unconsented tenant returning 403) fires on every sign-in for that deployment.A
NullHandleron theapron_authlogger fixes it: records find a handler, the fallback stays out of it, and any handler the application configures still receives them.The README now documents what was previously undiscoverable — logger names, the two levels and what each means, and that configuring
apron_authreaches the whole library. It also states there is deliberately nologgerparameter to inject: the logging hierarchy is the injection point, and a second configuration surface would only compete with it.It further states what is never logged — credentials, token bytes, and exception values — as a contract consumers can rely on and report against, rather than a property they had to infer from the source.
Notes on #74's acceptance criteria
Most of #74 had already landed via #80 (which closed #79) — chain-of-trust validation, per-verified-domain
TenancyContextemission,can_assert_domain_ownership=True, and the capability table. Two criteria were outstanding:Graceful degradation — this PR.
test_organization_call_failure_raiseshad asserted the opposite behavior; it is replaced byTestMicrosoftOrganizationDegradation.The open scope question — resolved, and the answer is better than the issue assumed. #74 proposed
GET /v1.0/domains, whose least-privileged delegated permission isDomain.Read.All(admin consent). #80 instead usedGET /v1.0/organization, whose least-privileged delegated permission isUser.Read— already inBASE_SCOPES. Microsoft's docs state an app with onlyUser.Readreads exactlyid,displayName, andverifiedDomains, which is precisely what the check needs. No additional consent is required. Verified against Microsoft Learngraph/api/organization-getandgraph/api/domain-list, 2026-07.Structure
Retrieval moves to
_fetch_organization, so degradation logging stays separate from tenant binding and domain mapping. Both functions document that they never raise.The exception value is never logged — the status code and exception class are the actionable facts, and a test asserts the bearer token stays out of the logs.
Testing
tyclean. Verified per-commit, not just at the tip.IdentityProfile,tenancies == (), a warning, and thatowns_domain()refuses the user's own email domain.NullHandlerpresent, no other configuration applied, and a subprocess check that an unconfigured application sees empty stdout and stderr. Confirmed these fail without the fix.Not verified live. The degradation paths are exercised against mocks only.
tests/integration/test_microsoft.pycovers token refresh, not identity fetch. That an unconsented tenant returns403specifically is reasoned from documentation rather than observed — the handler degrades on any status, so the behavior does not depend on that guess being right.Rebase note
Rebased onto #89, which removed
domain_owning_tenancy(). The degradation test was adapted toowns_domain()/domain_owning_tenancies(), and now asserts the gate refuses rather than merely that the old helper returnedNone.Summary by CodeRabbit
Documentation
Bug Fixes
Logging