Re: [PR] Fix Microsoft Graph filesystem auth by defaulting OAuth2 scope [airflow]

["potiuk (via GitHub)" <gi...@apache.org>]

potiuk commented on PR #70879:
URL: https://github.com/apache/airflow/pull/70879#issuecomment-5150947075

   Good diagnosis — the connection form does expose a `Scopes` field (`hooks/msgraph.py:258`, defaulting to `DEFAULT_SCOPE`), but `get_fs` built `oauth2_client_params` without a `scope` key, so `MSGDriveFS` skipped its own default and authlib was left with nothing. A UI-configured connection could not authenticate.
   
   The precedence you chose is right — explicit `scope`, then the form's `scopes`, then the Graph default — and parametrizing over all three branches is the right way to pin it.
   
   One thing to fix before this goes in: the two consumers of that same field disagree on how it is delimited.
   
   `MSGraphAsyncOperator`'s hook treats `scopes` as comma-separated and splits it:
   
   ```python
   scopes = config.get("scopes", self.scopes)
   if isinstance(scopes, str):
       scopes = scopes.split(",")
   ```
   
   This change passes the value through verbatim into the OAuth2 `scope` parameter, which is space-delimited by spec. So a connection configured the way the hook expects — `"User.Read,Files.Read"` — reaches authlib as the single literal string `"User.Read,Files.Read"`, which is one malformed scope rather than two. Single-scope values work, which is why the current tests pass; they only ever use one value.
   
   That is the same shape as the bug being fixed here: one field, two consumers, different expectations.
   
   Something like this keeps both readings working:
   
   ```python
   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_SCOPE
   ```
   
   Worth a fourth parametrize case with a comma-separated value so the behaviour is pinned rather than assumed.
   
   Minor, while you're in there: `DEFAULT_SCOPE` is redefined here as a module constant, but `KiotaRequestAdapterHook.DEFAULT_SCOPE` already holds the same literal. Importing it would keep the two from drifting apart later.
   
   ---
   Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org