Add support for viewer-based credentials for Databricks & Cortex #183
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit brings support for Posit Connect's "viewer-based credentials" feature [0] to the Databricks and Cortex chatbots. Similar to the recent work in `odbc` [1], the way this is exposed to R users is to require them to pass a Shiny session argument to `chat_databricks()` or `chat_cortex()`. Checks for viewer-based credentials are designed to fall back gracefully to existing authentication methods. This is intended to allow users to -- for example -- develop and test a Shiny app that uses Databricks or Snowflake credentials in desktop Positron/RStudio or Posit Workbench and deploy it with no code changes to Connect. Most of the actual work is outsourced to a new shared package, `connectcreds` [2]. Note that this commit also brings the internal auth handling for Snowflake much closer to Databricks, notably by making `cortex_credentials()` internal and analogous to `databricks_token()`. Unit tests are included. [0]: https://docs.posit.co/connect/user/oauth-integrations/ [1]: r-dbi/odbc#853 [2]: https://github.com/posit-dev/connectcreds/ Signed-off-by: Aaron Jacobs <[email protected]>
hadley
reviewed
Nov 29, 2024
| @@ -20,6 +20,16 @@ NULL | |||
| #' previous messages. Nor does it support registering tools, and attempting to | |||
| #' do so will result in an error. | |||
| #' | |||
| #' `chat_cortex()` picks up the following ambient Snowflake credentials: | |||
There was a problem hiding this comment.
I wonder if it's worth having a standard ## Auth header?
hadley
reviewed
Nov 29, 2024
| if (!is.null(session)) { | ||
| check_installed("connectcreds", "for viewer-based authentication") | ||
| if (!connectcreds::has_viewer_token(session, snowflake_url(account))) { | ||
| session <- NULL |
There was a problem hiding this comment.
Should this just null session out with no warning/error?
There was a problem hiding this comment.
has_viewer_token() emits a standard warning. I don't love this design, since I think it's non-obvious for the caller, but it does ensure that the warning is consistent across packages and we can limit how often it is shown in one place.
Does that approach make sense to you? Or should I add some sort of connectcreds::show_ignore_message() API for this and have has_viewer_token() keep quiet?
There was a problem hiding this comment.
What about something more like session <- connectcreds::update_session_token(...)? (and drop the if)
atheriel
added a commit
to atheriel/elmer
that referenced
this pull request
Dec 12, 2024
This commit parcels out the refactoring portion of tidyverse#183 so that we don't need to make breaking changes to the CRAN release. Signed-off-by: Aaron Jacobs <[email protected]>
atheriel
added a commit
to atheriel/elmer
that referenced
this pull request
Dec 12, 2024
This commit parcels out the refactoring portion of tidyverse#183 so that we don't need to make breaking changes to the CRAN release. Signed-off-by: Aaron Jacobs <[email protected]>
atheriel
added a commit
to atheriel/elmer
that referenced
this pull request
Dec 12, 2024
This commit parcels out the refactoring portion of tidyverse#183 so that we don't need to make breaking changes to the CRAN release. Signed-off-by: Aaron Jacobs <[email protected]>
hadley
pushed a commit
that referenced
this pull request
Dec 13, 2024
This commit parcels out the refactoring portion of #183 so that we don't need to make breaking changes to the CRAN release. Signed-off-by: Aaron Jacobs <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit brings support for Posit Connect's "viewer-based credentials" feature to the Databricks and Cortex chatbots. Similar to the recent work in
odbc, the way this is exposed to R users is to require them to pass a Shiny session argument tochat_databricks()orchat_cortex().Checks for viewer-based credentials are designed to fall back gracefully to existing authentication methods. This is intended to allow users to -- for example -- develop and test a Shiny app that uses Databricks or Snowflake credentials in desktop Positron/RStudio or Posit Workbench and deploy it with no code changes to Connect.
Most of the actual work is outsourced to a new shared package,
connectcreds.Note that this commit also brings the internal auth handling for Snowflake much closer to Databricks, notably by making
cortex_credentials()internal and analogous todatabricks_token().Unit tests are included.