shahar1 commented on code in PR #74804:
URL: https://github.com/apache/airflow/pull/74804#discussion_r4301965765
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -203,6 +203,9 @@ def get_valid_oauth_token(
"refresh_token": conn_config["refresh_token"],
}
+ if grant_type == "password":
+ data |= {"username": conn_config["oauth_username"], "password":
conn_config["oauth_password"]}
Review Comment:
**minor** — Validate the password grant extras
`_add_authentication_config` sets `oauth_username` and `oauth_password` only
when the extra `grant_type` is `password`. On other connections,
`hook.get_oauth_token(grant_type="password")` fails with `KeyError:
'oauth_username'`. When an extra is empty, `_get_field` returns `None`.
`requests` drops `None` form fields. The IdP then returns a 400 error that does
not name the missing field. Read the two extras next to `scope`. Raise
`ValueError` here when one of them is missing. Add a test for this error.
##########
providers/snowflake/docs/connections/snowflake.rst:
##########
@@ -78,8 +79,10 @@ Extra (optional)
is ``OIDC``. The connector reads the token from this file, which suits
projected or rotated tokens
(for example a Kubernetes service-account token).
* ``token_endpoint``: Specify token endpoint for external OAuth provider.
- * ``grant_type``: Specify grant type for OAuth authentication. Currently
supported: ``refresh_token`` (default), ``client_credentials``.
+ * ``grant_type``: Specify grant type for OAuth authentication. Currently
supported: ``refresh_token`` (default), ``client_credentials``, ``password``.
* ``scope``: Specify OAuth scope to include in the access token request
for any OAuth grant type.
+ * ``oauth_username``: Specify Snowflake user name which is used as trusted
context for OAuth to include in the access token request for password OAuth
grant type.
Review Comment:
**major** — The docs describe the wrong credentials
`oauth_username` and `oauth_password` are not Snowflake credentials. RFC
6749 §4.3.2 defines them as the resource owner credentials. The external IdP,
for example Entra ID or Okta, validates them. Snowflake maps the token to a
Snowflake user through the security integration user-mapping claim. "Trusted
context" is not a Snowflake term. With this text, users send their Snowflake
password to the IdP. Describe both extras as IdP user credentials. Tell users
that the IdP user must map to the Snowflake user. See
https://docs.snowflake.com/en/user-guide/oauth-okta.
##########
providers/snowflake/provider.yaml:
##########
@@ -196,6 +196,8 @@ connection-types:
"token_endpoint": "token endpoint",
"refresh_token": "refresh token",
"scope": "scope",
+ "oauth_username": "oauth_username",
Review Comment:
**nit** — Keep the placeholders in sync
Add `password` to the `grant_type` placeholder here and in
`get_ui_field_behaviour`. Both still show `refresh_token client_credentials`.
The two new placeholders repeat the key name. Use a description, as the other
keys do. For example, use `"oauth_password": "IdP user password (password
grant)"`. Then run the `update-providers-build-files` prek hook to regenerate
`get_provider_info.py`.
##########
providers/snowflake/docs/connections/snowflake.rst:
##########
@@ -78,8 +79,10 @@ Extra (optional)
is ``OIDC``. The connector reads the token from this file, which suits
projected or rotated tokens
(for example a Kubernetes service-account token).
* ``token_endpoint``: Specify token endpoint for external OAuth provider.
- * ``grant_type``: Specify grant type for OAuth authentication. Currently
supported: ``refresh_token`` (default), ``client_credentials``.
+ * ``grant_type``: Specify grant type for OAuth authentication. Currently
supported: ``refresh_token`` (default), ``client_credentials``, ``password``.
Review Comment:
**minor** — Document the limits of the password grant
Add two facts to this line. First, `password` needs an external IdP
`token_endpoint`. Without it, the hook sends the request to
`https://<account>.snowflakecomputing.com/oauth/token-request`. Snowflake
documents only `authorization_code` and `refresh_token` for that endpoint
(https://docs.snowflake.com/en/user-guide/oauth-custom). Also raise
`ValueError` when `grant_type` is `password` and `token_endpoint` is empty.
Second, RFC 9700 §2.4 says the password grant "MUST NOT be used". The grant
also does not work with MFA. Add a `.. warning::` that recommends
`client_credentials`, workload identity federation or key-pair authentication.
##########
providers/snowflake/docs/connections/snowflake.rst:
##########
@@ -1,3 +1,4 @@
+
Review Comment:
Tracking this issue in #74914
##########
providers/snowflake/tests/unit/snowflake/hooks/test_snowflake.py:
##########
@@ -1567,6 +1567,46 @@ def test_get_oauth_token_with_scope(self,
mock_requests_post):
assert called_data["scope"] == "default"
assert called_data["grant_type"] == "client_credentials"
+ @mock.patch("requests.post")
+ def test_get_oauth_token_with_password_grant_type(self,
mock_requests_post):
+ """
+ Verify that `get_oauth_token` returns an access token and includes the
+ provided required parameters for password type of grant in the
outgoing OAuth request payload.
+ """
+
+ mock_requests_post.return_value = Mock(
Review Comment:
**nit** — Use `spec` on the mock
This `Mock()` has no `spec`. The project rules require `spec` or `autospec`
on mocks. Use `Mock(spec=requests.Response, status_code=200, json=...)`. Import
`requests` for this. The other OAuth tests in this file use the same pattern,
so this is not a blocker.
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -628,6 +633,17 @@ def _add_authentication_config(
"scope",
)
+ if conn_config.get("grant_type") == "password":
Review Comment:
**minor** — Remove the IdP credentials after the token request
`oauth_username` and `oauth_password` stay in `conn_config` after the hook
gets the token. `get_conn()` sends them to `snowflake.connector.connect()`. The
connector stores each one as a connection attribute. `get_uri()` writes them as
plain query parameters, for example
`?oauth_password=S3cret&oauth_username=alice`. The connector does not use them.
Pop the two keys in `_get_conn_params` after the token request, next to
`pop("password")`.
--
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]
