feat: Implements OAuth association API resource #286

zackverham · 2024-09-11T19:53:10Z

Adds support for requesting and updating oauth associations through the Connect API.

github-actions · 2024-09-11T19:53:42Z

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
1097	1051	96%	0%	🟢

New Files

File	Coverage	Status
src/posit/connect/oauth/associations.py	100%	🟢
TOTAL	100%	🟢

Modified Files

File	Coverage	Status
src/posit/connect/client.py	99%	🟢
src/posit/connect/content.py	100%	🟢
src/posit/connect/oauth/integrations.py	100%	🟢
TOTAL	100%	🟢

updated for commit: 808ca7c by action🐍

zackverham · 2024-09-11T20:00:09Z

src/posit/connect/oauth/associations.py

+        ]
+
+
+    def update(self, integration_guid: None | str) -> None:


@dbkegley @tdstein would love thoughts on the best way to represent this API interaction through the SDK. I don't think this is precisely what we want, but its a little ambiguous.

The actual API endpoint accepts a list of json objects - this is designed with future flexibility in mind, but for now, the only valid inputs are:

[{"oauth_integration_guid": }] to associate a content item with an integration
[] to dissociate the content item with all integrations

Given that currently you can only set one association per content item, I think it makes sense to support a function signature that accepts a single integration guid outside of a list. This breaks the semantics of PUTting an empty list to dissociate the content from integrations, though.

I put None in as a placeholder for now to facilitate discussion, but I don't really like it all that much. I'm not sure what a better alternative would be.

It feels like the correct expression is content.associations.create(integration_guid), which should return a simple object that maintains the integration_guid and provides a delete method. The delete method would need to remove the association from the list.

class Association(Resource): def delete(self) -> None: # call `v1/content/{self.content_guid}/oauth/integrations/association` to remove the association.

fwiw, here is how I am thinking about the REST API implementation in my head, even though I know this doesn't represent the actual implementation.

Create an association

POST .../oauth/integrations/associations { 'integration_guid': 'whatever..." } Response: { 'guid': 'the-association-guid', 'integration_guid': 'whatever...', 'content_guid': 'the-content-guid' }

Delete an association

DELETE .../the-content-guid/oauth/integrations/associations/the-association-guid

in the current API implementation, you can't create / delete singular oauth associations - there aren't public CRUD operations that let you use an association in isolation. So I don't think your proposed pattern would be possible.

You can only PUT a list of oauth integration guids to the<content_guid>oauth/integrations/associations endpoint - any PUTs completely wipe the current set of associations and replace it with a new set of associations.

I see. When support for multiple associations is added, will the PUT operation still be an overwrite? If so, how should multiple people coordinate the modification when they each want to add a single association to the list simultaneously?

its a good question. I don't know if we have an answer to this - currently, the asnwer is "whoever PUTs last wins."

Really it should only be publishers / admins who use this endpoint to interact with content that they own, so I wouldn't expect there to be many use cases where there's parallel requests coming from many clients. We really built the API with the expected client primarily being the dashboard.

src/posit/connect/content.py

tdstein · 2024-09-11T20:56:42Z

src/posit/connect/content.py

@@ -27,6 +29,13 @@ def __getitem__(self, key: Any) -> Any:
            return ContentItemOwner(params=self.params, **v)
        return v

+    @property
+    def oauth_associations(self) -> ContentItemAssociations:


Maybe this should be expressed as content.oauth.associations?

I like that alot better, yeah. Would the oauth property just be a passthrough class that we hang the associations resource off of?

Yep, that sounds like the right approach.

zackverham · 2024-09-12T19:08:02Z

Test Notes

Here's a test script illustrating / manually validating the behavior of the associations resources:

from posit.connect.client import Client
import json


c = Client("http://localhost:3939", "<redacted>")

integration_a_guid = "22644575-a27b-4118-ad06-e24459b05126"
integration_b_guid = "9dd7ef9d-ccd4-46f0-b76c-12b00f1ff594"
content_guid = "9dfa33ee-62ea-4a69-8b1a-51735c70dba9"

# get content item
content_item = c.content.get(content_guid)

# set initial state for testing
content_item.oauth.associations.update(integration_a_guid)

# associated with integration_a
association_list = content_item.oauth.associations.find()
print(json.dumps(association_list))

# Output:
# [{"app_guid": "9dfa33ee-62ea-4a69-8b1a-51735c70dba9", "oauth_integration_guid": "22644575-a27b-4118-ad06-e24459b05126", "oauth_integration_name": "keycloak", "oauth_integration_description": "", "oauth_integration_template": "custom", "created_time": "2024-09-12T19:05:11Z"}]

# clear associations
content_item.oauth.associations.delete()
association_list = content_item.oauth.associations.find()
print(json.dumps(association_list))

# Output: 
# []

# associated with integration_b
content_item.oauth.associations.update(integration_b_guid)
association_list = content_item.oauth.associations.find()
print(json.dumps(association_list))

# Output: 
# [{"app_guid": "9dfa33ee-62ea-4a69-8b1a-51735c70dba9", "oauth_integration_guid": "9dd7ef9d-ccd4-46f0-b76c-12b00f1ff594", "oauth_integration_name": "default-test", "oauth_integration_description": "desc", "oauth_integration_template": "custom", "created_time": "2024-09-12T19:05:11Z"}]

# integration_b also sees the association
integration = c.oauth.integrations.get(integration_b_guid)
association_list = integration.associations.find()
print(json.dumps(association_list))

# Output: 
# [{"app_guid": "9dfa33ee-62ea-4a69-8b1a-51735c70dba9", "oauth_integration_guid": "9dd7ef9d-ccd4-46f0-b76c-12b00f1ff594", "oauth_integration_name": "default-test", "oauth_integration_description": "desc", "oauth_integration_template": "custom", "created_time": "2024-09-12T19:05:11Z"}]

# integration_a does not see the integration
integration = c.oauth.integrations.get(integration_a_guid)
association_list = integration.associations.find()
print(json.dumps(association_list))

# Output: 
# []

tdstein

This looks great! I have a few minor nits on exception handling, but I'm ok with merging the pull request as is or with changes.

tdstein · 2024-09-12T19:22:01Z

src/posit/connect/content.py

+        if "guid" not in self:
+            raise ValueError("ContentItemOAuth requires content guid")


I think you can remove this. If 'guid' is missing, we have bigger issues. And I believe the default KeyError thrown by self['guid'] if 'guid' is missing is sufficient.

cool, yep - makes sense. I was getting type checking errors when we were using property-based fields because the guids were str|None types, but the change to a self['guid'] lookup removes that issue entirely.

I'll make that change and then merge when CI passes.

tdstein · 2024-09-12T19:22:44Z

src/posit/connect/oauth/integrations.py

+        if "guid" not in self:
+            raise ValueError(
+                "IntegrationAssociations requires integration guid"
+            )


Same as above.

implements oauth association resource

37bf5f4

zackverham changed the title ~~implements oauth association resource~~ feat: Implements OAuth association API resource Sep 11, 2024

zackverham commented Sep 11, 2024

View reviewed changes

zackverham requested review from dbkegley and plascaray September 11, 2024 20:02

tdstein reviewed Sep 11, 2024

View reviewed changes

src/posit/connect/content.py Outdated Show resolved Hide resolved

tdstein reviewed Sep 11, 2024

View reviewed changes

zackverham added 6 commits September 12, 2024 13:22

responses / improvements from PR comments

ff92391

Merge branch 'main' into zack-oauth-integrations-api-associations

9f7b0f4

union with Nonetype breaks older python versions

20324d1

linting

2b875a0

filling out tests for new delete function

9f68e5b

fixing linting

aae85b9

zackverham marked this pull request as ready for review September 12, 2024 18:47

zackverham requested a review from tdstein September 12, 2024 18:47

tdstein approved these changes Sep 12, 2024

View reviewed changes

zackverham added 2 commits September 12, 2024 16:04

removing unneeded exception handling

03bf9ec

linting

808ca7c

zackverham merged commit 3832b43 into main Sep 12, 2024
32 checks passed

zackverham deleted the zack-oauth-integrations-api-associations branch September 12, 2024 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implements OAuth association API resource #286

feat: Implements OAuth association API resource #286

zackverham commented Sep 11, 2024 •

edited

Loading

github-actions bot commented Sep 11, 2024 •

edited

Loading

zackverham Sep 11, 2024

tdstein Sep 11, 2024

zackverham Sep 11, 2024

tdstein Sep 12, 2024

zackverham Sep 12, 2024

tdstein Sep 11, 2024

zackverham Sep 11, 2024

tdstein Sep 12, 2024

zackverham commented Sep 12, 2024

tdstein left a comment

tdstein Sep 12, 2024

zackverham Sep 12, 2024

tdstein Sep 12, 2024

		if "guid" not in self:
		raise ValueError("ContentItemOAuth requires content guid")

feat: Implements OAuth association API resource #286

feat: Implements OAuth association API resource #286

Conversation

zackverham commented Sep 11, 2024 • edited Loading

github-actions bot commented Sep 11, 2024 • edited Loading

☂️ Python Coverage

Overall Coverage

New Files

Modified Files

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

zackverham commented Sep 12, 2024

Test Notes

tdstein 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

zackverham commented Sep 11, 2024 •

edited

Loading

github-actions bot commented Sep 11, 2024 •

edited

Loading