Adds an OAuthIntegration #52

dbkegley · 2024-02-23T22:35:09Z

Adds a client.oauth resource for interacting the the v1/oauth/integrations/credentials endpoint to perform a token exchange with Connect.

github-actions · 2024-02-23T22:47:18Z

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
544	508	93%	80%	🟢

New Files

File	Coverage	Status
src/posit/connect/external/init.py	100%	🟢
src/posit/connect/external/databricks.py	0%	🟢
src/posit/connect/external/databricks_test.py	100%	🟢
src/posit/connect/oauth.py	100%	🟢
src/posit/connect/oauth_test.py	100%	🟢
TOTAL	80%	🟢

Modified Files

File	Coverage	Status
src/posit/connect/client.py	100%	🟢
src/posit/connect/client_test.py	100%	🟢
TOTAL	100%	🟢

updated for commit: ec35b17 by action🐍

nealrichardson

A few notes. LMK if you want to talk through using responses to set up some tests that stub out the server side.

nealrichardson · 2024-02-26T14:10:57Z

src/posit/connect/client.py

+        if self._oauth is None:
+            self._oauth = OAuthIntegration(config=self.config, session=self.session)
+        return self._oauth


AFAICT there's nothing in OAuthIntegration that needs to be cached, so we just need to be able to access it here, as we do (now) for Users below.

Suggested change

if self._oauth is None:

self._oauth = OAuthIntegration(config=self.config, session=self.session)

return self._oauth

return OAuthIntegration(config=self.config, session=self.session)

nealrichardson · 2024-02-26T14:14:19Z

src/posit/connect/oauth.py

+        self.session = session
+
+
+    def get_credentials(self, user_identity: Optional[str]) -> Response:


Does this need to return Response or can it return the response body or something? I don't see this being used anywhere yet, so I can't tell.

I think we're trying to avoid having the request/response details leaking outside of the get/put/post/etc. methods.

for reference, here's the place it gets used:

posit-sdk-py/src/posit/connect/external/databricks.py

Line 35 in ad3dba8

access_token = self.posit_oauth.get_credentials(self.user_identity).json()['access_token']

So we could definitely make the return type more specific and not expose the response object.

Ah sorry, I was using github's symbol lookup and it didn't find it, should have stuck with good ol' cmd-F.

Yeah in that case, I'd have this method return response.json()

A defined type would be even better so that the consumer gets type hints.

@dataclass class OAuthResponse: access_token: str

With the expectation that this endpoint might eventually return something other than an access_token, I like the idea of returning a defined type.

nealrichardson · 2024-02-26T14:16:26Z

src/posit/connect/client.py

 from .users import Users


+# Connect sets the value of the environment variable RSTUDIO_PRODUCT = CONNECT


😭 it's 2024, we should start setting POSIT_PRODUCT in the env too, yeah?

Can we use the presence of CONNECT_SERVER instead and sidestep branding issues?

…client

tdstein · 2024-02-26T15:15:09Z

src/posit/connect/external/databricks.py

+
+    @abc.abstractmethod
+    def auth_type(self) -> str:
+        ...


Should these abstract method's raise NotImplementedError()?

These are directly copied from the databricks client(s) so it's unclear if they are intentionally not raising an error

https://github.com/databricks/databricks-sdk-py/blob/525576073da90de0b98ff652ad5b6f2d2bb77871/databricks/sdk/credentials_provider.py#L30-L40
https://github.com/databricks/databricks-sql-python/blob/50489346440cdf9e547b597254643ee4ceb28c19/src/databricks/sql/auth/authenticators.py#L22-L33

tdstein · 2024-02-26T15:42:03Z

src/posit/connect/client.py

+def is_local() -> bool:
+    return not os.getenv("RSTUDIO_PRODUCT") == "CONNECT"


It looks like this is only being used src/posit/connect/external/databricks.py.

Should it be moved there?

This could also go in Config if there is reuse. But I'd rename it to something like run_mode, runtime_environment, etc.

Currently yes it's only used by the databricks module. I could see this being useful elsewhere though. No preference from me on where this lives

👍🏻 Place it where it's used for now.

tdstein · 2024-02-26T15:43:40Z

src/posit/connect/external/databricks.py

What does usage of this file look like? I think the implementation can be reduced, but I'm not convinced since it seems dependent on the consuming code.

Here's our example from last weeks demo.

posit-sdk-py/connect-content/sample-content.py

Lines 27 to 30 in 62fcc8e

with Client() as connect_client:

credentials_provider = viewer_credentials_provider(

connect_client.oauth, content_identity=CONTENT_IDENTITY)

cfg = Config(host=DB_HOST_URL, credentials_provider=credentials_provider)

This usage has changed a little bit since then. In this PR we accept a client as an optional parameter so that the Content doesn't require a Connect server when running locally. The SDK client is initialized only when the content is running on Connect by default.

tdstein · 2024-02-26T16:04:22Z

src/posit/connect/external/databricks.py

+# https://github.com/databricks/databricks-sdk-py/blob/v0.20.0/databricks/sdk/credentials_provider.py
+# https://github.com/databricks/databricks-sql-python/blob/v3.1.0/src/databricks/sql/auth/authenticators.py
+# In order to keep compatibility with the Databricks SDK
+class CredentialsProvider(abc.ABC):


Why not import https://github.com/databricks/databricks-sdk-py/blob/main/databricks/sdk/credentials_provider.py#L30 directly?

I wanted to avoid adding their SDK as a dependency. I'll test and see if we can remove this abstract definition altogether.

Got it. See if you can utilize optional dependencies: https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#dependencies-optional-dependencies

Let's leave this for now. I think we will have a similar problem for our content types. It would be nice to provide helpers for each framework when obtaining the identity token from the headers but it will vary by content type.

src/posit/connect/external/databricks.py

tdstein · 2024-02-26T16:11:33Z

src/posit/connect/external/databricks.py

+        return inner
+
+
+def viewer_credentials_provider(client: Optional[Client], user_identity: Optional[str]) -> Optional[CredentialsProvider]:


Is the value of user_identity ever something other than headers.get('Posit-Connect-Content-Identity')?

The value is always contained in a header called Posit-Connect-Content-Identity. But the method for obtaining the header value varies by content type:

https://docs.posit.co/connect/user/shiny/#user-meta-data
https://docs.posit.co/connect/user/flask/#user-meta-data

…tabricks-oauth-2

nealrichardson · 2024-02-27T03:16:20Z

src/posit/connect/external/databricks.py

+# Use these environment varariables to determine if the
+# client SDK was initialized from a piece of content running on a Connect server.
+def is_local() -> bool:
+    return not os.getenv("CONNECT_SERVER") and not os.getenv("CONNECT_CONTENT_GUID")


I don't think CONNECT_SERVER is right here: that's what we recommend folks set locally in all of our API examples (e.g. https://docs.posit.co/connect/cookbook/).

I'd rather use the accurate variable even if it says RSTUDIO; we can fix that on the server going forward and (eventually) purge it here.

…tabricks-oauth-2

dbkegley requested review from atheriel, nealrichardson, tdstein and zackverham February 23, 2024 22:35

dbkegley force-pushed the kegs/databricks-oauth-2 branch from f82a67f to 93bf580 Compare February 23, 2024 22:46

Add Databricks OAuthIntegration credentials provider

ad3dba8

dbkegley force-pushed the kegs/databricks-oauth-2 branch from 93bf580 to ad3dba8 Compare February 23, 2024 22:48

nealrichardson reviewed Feb 26, 2024

View reviewed changes

dbkegley added 2 commits February 26, 2024 09:32

Return Credentials response. Dont cache oauthIntegration resource in …

325ba30

…client

Add oauth integration tests

29dfd54

tdstein reviewed Feb 26, 2024

View reviewed changes

src/posit/connect/external/databricks.py Outdated Show resolved Hide resolved

tdstein reviewed Feb 26, 2024

View reviewed changes

dbkegley added 6 commits February 26, 2024 11:33

Test databricks content on Connect

d74f221

Merge branch 'main' of github.com:posit-dev/posit-sdk-py into kegs/da…

d3ec91f

…tabricks-oauth-2

Move is_local; check CONNECT_SERVER and CONNECT_CONTENT_GUID

0b13600

Use post body temporarily for oauth/integration/credentials endpoints

7690348

use form xml post in credentials request

ea91a37

Add sample databricks content

57508a6

dbkegley force-pushed the kegs/databricks-oauth-2 branch from 40a3830 to 57508a6 Compare February 26, 2024 21:33

dbkegley added 2 commits February 26, 2024 16:40

Fix linter

24ad714

update sample content requirements

34f7449

nealrichardson reviewed Feb 27, 2024

View reviewed changes

dbkegley added 2 commits February 27, 2024 10:06

Revert env var change

8091ddc

Use user-session-token instead of content-identity-token

8e3a865

dbkegley force-pushed the kegs/databricks-oauth-2 branch from 109a55b to 8e3a865 Compare February 27, 2024 17:27

Merge branch 'main' of github.com:posit-dev/posit-sdk-py into kegs/da…

ec35b17

…tabricks-oauth-2

dbkegley merged commit 19b6cab into main Feb 27, 2024
7 checks passed

dbkegley deleted the kegs/databricks-oauth-2 branch February 27, 2024 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds an OAuthIntegration #52

Adds an OAuthIntegration #52

dbkegley commented Feb 23, 2024

github-actions bot commented Feb 23, 2024 •

edited

Loading

nealrichardson left a comment

nealrichardson Feb 26, 2024

nealrichardson Feb 26, 2024

zackverham Feb 26, 2024

nealrichardson Feb 26, 2024

tdstein Feb 26, 2024

atheriel Feb 26, 2024

nealrichardson Feb 26, 2024

atheriel Feb 26, 2024

tdstein Feb 26, 2024

dbkegley Feb 26, 2024

tdstein Feb 26, 2024

dbkegley Feb 26, 2024

tdstein Feb 26, 2024

tdstein Feb 26, 2024

dbkegley Feb 26, 2024

tdstein Feb 26, 2024

dbkegley Feb 26, 2024

tdstein Feb 26, 2024

dbkegley Feb 26, 2024

tdstein Feb 26, 2024

dbkegley Feb 26, 2024

nealrichardson Feb 27, 2024

		self.session = session


		def get_credentials(self, user_identity: Optional[str]) -> Response:

		from .users import Users


		# Connect sets the value of the environment variable RSTUDIO_PRODUCT = CONNECT

		def is_local() -> bool:
		return not os.getenv("RSTUDIO_PRODUCT") == "CONNECT"

	with Client() as connect_client:
	credentials_provider = viewer_credentials_provider(
	connect_client.oauth, content_identity=CONTENT_IDENTITY)
	cfg = Config(host=DB_HOST_URL, credentials_provider=credentials_provider)

		return inner


		def viewer_credentials_provider(client: Optional[Client], user_identity: Optional[str]) -> Optional[CredentialsProvider]:

Adds an OAuthIntegration #52

Adds an OAuthIntegration #52

Conversation

dbkegley commented Feb 23, 2024

github-actions bot commented Feb 23, 2024 • edited Loading

☂️ Python Coverage

Overall Coverage

New Files

Modified Files

nealrichardson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2024 •

edited

Loading