Fix Microsoft Graph filesystem auth by defaulting OAuth2 scope - #70879
Fix Microsoft Graph filesystem auth by defaulting OAuth2 scope#70879haseebmalik18 wants to merge 3 commits into
Conversation
eaa30b7 to
764a9de
Compare
|
Good diagnosis — the connection form does expose a The precedence you chose is right — explicit One thing to fix before this goes in: the two consumers of that same field disagree on how it is delimited.
scopes = config.get("scopes", self.scopes)
if isinstance(scopes, str):
scopes = scopes.split(",")This change passes the value through verbatim into the OAuth2 That is the same shape as the bug being fixed here: one field, two consumers, different expectations. Something like this keeps both readings working: scopes = get_field(conn_id=conn_id, conn_type=conn_type, extras=extras, field_name="scopes")
if isinstance(scopes, str):
scopes = " ".join(s.strip() for s in scopes.split(",") if s.strip())
oauth2_client_params["scope"] = scopes or DEFAULT_SCOPEWorth a fourth parametrize case with a comma-separated value so the behaviour is pinned rather than assumed. Minor, while you're in there: Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
764a9de to
431b39f
Compare
|
@potiuk Following up |
|
Due to the merge of your other PR, merge conflicts must be resolved. |
508c707 to
bd63061
Compare
|
Merge conflicts resolved |
potiuk
left a comment
There was a problem hiding this comment.
Thanks for the follow-up — the premise checks out: hooks/msgraph.py splits the form's scopes on commas and the connection form only exposes scopes, so a UI-configured connection really did end up scope-less. The fix solves that case. Two edges I'd like tightened before it lands, plus a test gap — none of them a problem with the approach.
scopes given as a list now reaches authlib as a list (fs/msgraph.py:168)
The code this replaces went through _get_scopes(), which explicitly supports list[str], and the caller joined the result into a string. The new block only stringifies when isinstance(scopes, str), so a list — reachable via storage_options={"scopes": ["User.Read", "Files.Read"]}, and a shape _get_scopes() documents as supported — is now assigned to oauth2_client_params["scope"] unchanged. That is a regression on an input that worked before this PR.
The certificate-auth branch keeps the bug this PR fixes (fs/msgraph.py:137)
"scope": " ".join(_get_scopes(options)) is unchanged, and _get_scopes() splits on whitespace only. The very same UI connection with scopes = "User.Read,Files.Read" still yields the single bogus scope User.Read,Files.Read when certificate auth is used. Same root cause, same user-visible failure — worth fixing in the same PR rather than leaving half the code path broken.
Both fall out if the comma handling moves into _get_scopes() instead of being duplicated at the call site:
def _get_scopes(options: dict[str, Any]) -> list[str]:
scopes = options.get("scope") or options.get("scopes") or DEFAULT_SCOPE
if isinstance(scopes, str):
return [scope.strip() for scope in scopes.replace(",", " ").split() if scope.strip()]
return scopes…and the new block collapses to:
if "scope" not in oauth2_client_params:
oauth2_client_params["scope"] = " ".join(_get_scopes(options))That keeps list support, fixes the certificate branch and _get_certificate_token()'s credential.get_token(*_get_scopes(options)) at the same time, and leaves one place where scope resolution lives.
Smaller observations
- The new parametrized test covers the string forms nicely, but not the list-valued
scopescase above, and not the certificate branch. If you take the_get_scopes()route, a case per branch would keep both covered.
This review was drafted by an AI-assisted tool and
confirmed by an Airflow maintainer. The findings
below are observations, not blockers; an Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Fixes the Microsoft Graph filesystem failing to authenticate when the connection is set up through the UI.
The connection form only exposes a "Scopes" field, but get_fs() hands MSGDriveFS a fully built oauth2_client_params dict. That makes the library skip its own scope default, so authlib ends up with no scope and the connection fails. This falls back to the connection's scopes value, then to the Graph default, so a UI-configured connection works without hand-adding a scope key to extras.
closes: #70822