potiuk commented on code in PR #70879:
URL: https://github.com/apache/airflow/pull/70879#discussion_r3767607624
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/fs/msgraph.py:
##########
@@ -158,8 +158,14 @@ def get_fs(conn_id: str | None, storage_options: dict[str,
Any] | None = None) -
if param in options:
oauth2_client_params[param] = options[param]
- if "scopes" in options and "scope" not in oauth2_client_params:
- oauth2_client_params["scope"] = " ".join(_get_scopes(options))
+ # authlib expects a singular, space-delimited "scope"; the connection
form only
+ # offers "scopes", which the hook treats as comma-separated, so
translate it and
+ # always default so authlib never authenticates without a scope.
+ if "scope" not in oauth2_client_params:
+ scopes = options.get("scopes")
+ if isinstance(scopes, str):
+ scopes = " ".join(scope.strip() for scope in scopes.split(",")
if scope.strip())
+ oauth2_client_params["scope"] = scopes or DEFAULT_SCOPE
Review Comment:
Small regression here: the code this replaces went through `_get_scopes()`,
which supports `list[str]`, and the caller joined it into a string. This branch
only stringifies when `isinstance(scopes, str)`, so `storage_options={"scopes":
["User.Read", "Files.Read"]}` now assigns a *list* to
`oauth2_client_params["scope"]` and authlib gets something it cannot use.
Moving the comma handling into `_get_scopes()` keeps list support and fixes
the certificate branch at line 137 (which still has the comma bug) at the same
time:
```suggestion
# authlib expects a singular, space-delimited "scope"; the
connection form only
# offers "scopes", which the hook treats as comma-separated, so
translate it and
# always default so authlib never authenticates without a scope.
if "scope" not in oauth2_client_params:
oauth2_client_params["scope"] = " ".join(_get_scopes(options))
```
(with `_get_scopes()` extended to `scopes.replace(",", " ").split()` for the
`str` case).
##########
providers/microsoft/azure/tests/unit/microsoft/azure/fs/test_msgraph.py:
##########
@@ -252,6 +253,36 @@ def test_get_fs_with_certificate_data_from_storage_options(
)
assert result == mock_fs_instance
+ @pytest.mark.parametrize(
Review Comment:
Nice coverage of the string forms. Two cases missing: `scopes` passed as a
list (see the comment on `msgraph.py`), and the certificate-auth branch, which
resolves its scope separately at `msgraph.py:137` and is still comma-broken.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
