feat(sqlalchemy): Add Support for externalAuthentication #344
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
CLA should be good to go (= |
| @@ -106,6 +111,10 @@ def create_connect_args(self, url: URL) -> Tuple[Sequence[Any], Mapping[str, Any | |||
| kwargs["http_scheme"] = "https" | |||
| kwargs["auth"] = CertificateAuthentication(unquote_plus(url.query['cert']), unquote_plus(url.query['key'])) | |||
|
|
|||
| if "externalAuthentication" in url.query: | |||
There was a problem hiding this comment.
I understand that it's aligning with JDBC driver, but you can simply pass "auth": OAuth2Authentication() in connect_args.
There was a problem hiding this comment.
That's what I've been doing when using the client directly. But I've been running into an issue passing it as a class when trying to use it from some other projects such as ipython-sql.
Eventually I found out that ipython-sql allows you to actually pass the -creator parameter to bypass the configuration by sending a connection straight away but it'd be great if it would work without this workaround.
There was a problem hiding this comment.
Do we have good behaviour when someone has externalAuthentication in URI and has explicitly set a different auth mechanism? What takes precedence? I'd like to avoid this situation of having multiple ways of configuring something since it leads to more combination of things to test and edge cases to think about.
There was a problem hiding this comment.
Hey @hashhar, I have similar feelings to be honest. I just extended the way it's being done for SQLAlchemy URIs (It's already done this way for Basic Authentication, JWT Authentication and Cert Authentication).
I think it makes sense to align with the JDBC driver and have this for SQLAlchemy... If the user passes multiple Auth Methods I think it's on the user side to be honest, but I guess it could be improved by validating if they did this somehow (checking the query args and connect_args)
There was a problem hiding this comment.
This validation seems useful but out of scope of this PR because it needs to apply to other auth methods as outlined above.
There was a problem hiding this comment.
You mean validating if the user passes multiple auth methods?
So you think it'd be fine to keep that responsibility on the user side so far? I'd be fine with it (=
| @@ -106,6 +111,10 @@ def create_connect_args(self, url: URL) -> Tuple[Sequence[Any], Mapping[str, Any | |||
| kwargs["http_scheme"] = "https" | |||
| kwargs["auth"] = CertificateAuthentication(unquote_plus(url.query['cert']), unquote_plus(url.query['key'])) | |||
|
|
|||
| if "externalAuthentication" in url.query: | |||
There was a problem hiding this comment.
This validation seems useful but out of scope of this PR because it needs to apply to other auth methods as outlined above.
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
merge? |
|
Sorry for forgetting to come back to this. I'll just squash the commits and then merge this. Thanks for your work @esemeniuc. |
After this change if 'externalAuthentication' is passed as a parameter on the connection url we automatically set `http_schema` to `http` and use `OAuth2Authentication`.
|
Squashed the commits + reworded commit msg, will merge once CI is finished. |
|
Sorry this ended up waiting so long @IceS2. We plan a release today/tomorrow and will announce on the #python-client on the Trino Slack. |
Hey! Don't worry (= |
Description
Adds support for the 'externalAuthentication' Trino query URL parameter.
If 'externalAuthentication' is passed on the query arguments:
Non-technical explanation
It allows to use SQLAlchemy with OAuth2Authentication.
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( x ) Release notes are required, with the following suggested text:
closes #343