Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
gopidesupavan commented on PR #47191: URL: https://github.com/apache/airflow/pull/47191#issuecomment-2816342503 @Sharashchandra cant we merge `get_oauth_token` into single method ? this is being used in `SnowflakeSqlApiHook` and `SnowflakeHook` the olnly difference i could see here is `self.account_identifier` and `conn_config['account']` ? -- 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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
bugraoz93 commented on PR #47191: URL: https://github.com/apache/airflow/pull/47191#issuecomment-2816242619 Congrats @Sharashchandra! :tada: -- 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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
boring-cyborg[bot] commented on PR #47191: URL: https://github.com/apache/airflow/pull/47191#issuecomment-2816240553 Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions. -- 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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
bugraoz93 merged PR #47191: URL: https://github.com/apache/airflow/pull/47191 -- 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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
Sharashchandra commented on PR #47191: URL: https://github.com/apache/airflow/pull/47191#issuecomment-2813251417 @potiuk rebased! -- 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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
potiuk commented on PR #47191: URL: https://github.com/apache/airflow/pull/47191#issuecomment-2813036740 Needs to be rebased again. Can you please rebase? @Sharashchandra -- 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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
potiuk commented on PR #47191: URL: https://github.com/apache/airflow/pull/47191#issuecomment-2769914605 Rebased. This one looks good to merge if the questions are resolved @sunank200 ? -- 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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
Sharashchandra commented on code in PR #47191:
URL: https://github.com/apache/airflow/pull/47191#discussion_r1975084560
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
+"redirect_uri": conn_config.get("redirect_uri",
"https://localhost.com";),
Review Comment:
Redirect uri is required to fetch access_token using a refresh_token. This
redirect_uri should match whatever is mentioned while creating a security
inegration on Snowflake.
We're defaulting to https://localhost.com because that's what is mentioned
in the Snowflake documentation.
[Snowflake
OAuth](https://docs.snowflake.com/en/user-guide/oauth-custom#integration-example)
--
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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
Sharashchandra commented on code in PR #47191:
URL: https://github.com/apache/airflow/pull/47191#discussion_r1975093489
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
+"redirect_uri": conn_config.get("redirect_uri",
"https://localhost.com";),
+}
+response = requests.post(
+url,
+data=data,
+headers={
+"Content-Type": "application/x-www-form-urlencoded",
+},
+auth=HTTPBasicAuth(conn_config["client_id"],
conn_config["client_secret"]), # type: ignore[arg-type]
+)
+
+try:
+response.raise_for_status()
Review Comment:
``` response.raise_for_status() ``` raises a
```requests.exceptions.HTTPError```
We want to raise an ``` AirflowException ``` instead
--
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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
Sharashchandra commented on code in PR #47191:
URL: https://github.com/apache/airflow/pull/47191#discussion_r1975074629
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
Review Comment:
We're calling the get_oauth_token function only if we find "refresh_token"
in the extra. This would not throw an exception
--
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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
Sharashchandra commented on code in PR #47191:
URL: https://github.com/apache/airflow/pull/47191#discussion_r1975084560
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
+"redirect_uri": conn_config.get("redirect_uri",
"https://localhost.com";),
Review Comment:
We're defaulting to https://localhost.com because that's what is being used
in the Snowflake documentation.
[Snowflake
OAuth](https://docs.snowflake.com/en/user-guide/oauth-custom#integration-example)
--
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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
Sharashchandra commented on code in PR #47191:
URL: https://github.com/apache/airflow/pull/47191#discussion_r1975084560
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
+"redirect_uri": conn_config.get("redirect_uri",
"https://localhost.com";),
Review Comment:
We're defaulting to https://localhost.com because that's what is mentioned
in the Snowflake documentation.
[Snowflake
OAuth](https://docs.snowflake.com/en/user-guide/oauth-custom#integration-example)
--
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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
Sharashchandra commented on code in PR #47191:
URL: https://github.com/apache/airflow/pull/47191#discussion_r1975084560
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
+"redirect_uri": conn_config.get("redirect_uri",
"https://localhost.com";),
Review Comment:
Redirect uri is required to fetch access_token using a refresh_token. This
redirect_uri should match whatever is mentioned while creating a security
inegration on Snowflake.
We're defaulting to https://localhost.com because that's what is mentioned
in the Snowflake documentation.
[https://docs.snowflake.com/en/user-guide/oauth-custom#integration-example](Snowflake
OAuth)
--
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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
Sharashchandra commented on code in PR #47191:
URL: https://github.com/apache/airflow/pull/47191#discussion_r1975079154
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
+"redirect_uri": conn_config.get("redirect_uri",
"https://localhost.com";),
+}
+response = requests.post(
+url,
+data=data,
+headers={
+"Content-Type": "application/x-www-form-urlencoded",
+},
+auth=HTTPBasicAuth(conn_config["client_id"],
conn_config["client_secret"]), # type: ignore[arg-type]
Review Comment:
client_id is being picked up from the login field and client_secret is
picked up from the password field.
Login is a required field so that's always going to be there.
Missing password in already handled above so we'll always have either the
password or empty for the password key
--
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]
Re: [PR] Adding OAuth support for SnowflakeHook [airflow]
sunank200 commented on code in PR #47191:
URL: https://github.com/apache/airflow/pull/47191#discussion_r1975031486
##
providers/snowflake/docs/connections/snowflake.rst:
##
@@ -39,10 +39,11 @@ Configuring the Connection
--
Login
-Specify the snowflake username.
+Specify the snowflake username. For OAuth, the OAuth Client ID.
Review Comment:
Should we also create a separate section for OAuth?
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
Review Comment:
Do we need to retry for the request?
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
+"redirect_uri": conn_config.get("redirect_uri",
"https://localhost.com";),
+}
+response = requests.post(
+url,
+data=data,
+headers={
+"Content-Type": "application/x-www-form-urlencoded",
+},
+auth=HTTPBasicAuth(conn_config["client_id"],
conn_config["client_secret"]), # type: ignore[arg-type]
Review Comment:
Is `client_id` and `client_id` always defined? Should there be a check
before getting OAuth token?
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
Review Comment:
what if `refresh_token` is not there? `conn_config["refresh_token"]` would
throw an error.
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
+"redirect_uri": conn_config.get("redirect_uri",
"https://localhost.com";),
Review Comment:
why default to localhost?
##
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##
@@ -185,6 +187,30 @@ def _get_field(self, extra_dict, field_name):
return extra_dict[field_name] or None
return extra_dict.get(backcompat_key) or None
+def get_oauth_token(self, conn_config: dict) -> str:
+"""Generate temporary OAuth access token using refresh token in
connection details."""
+url =
f"https://{conn_config['account']}.snowflakecomputing.com/oauth/token-request"
+data = {
+"grant_type": "refresh_token",
+"refresh_token": conn_config["refresh_token"],
+"redirect_uri": conn_config.get("redirect_uri",
"https://localhost.com";),
+}
+response = requests.post(
+url,
+data=data,
+headers={
+"Content-Type": "application/x-www-form-urlencoded",
+},
+auth=HTTPBasicAuth(conn_config["client_id"],
conn_config["client_secret"]), # type: ignore[arg-type]
+)
+
+try:
+response.raise_for_status()
Review Comment:
Do we need to try except and raise here? We already have
`response.raise_for_status()`
--
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]
