Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Permissions.delete(*permissions) method #339

Merged
merged 17 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
from posit import connect
from posit.connect.content import ContentItem


class TestContentPermissions:
content: ContentItem

@classmethod
def setup_class(cls):
cls.client = connect.Client()
cls.content = cls.client.content.create(name="example")

cls.user_aron = cls.client.users.create(
username="permission_aron",
email="[email protected]",
password="permission_s3cur3p@ssword",
)
cls.user_bill = cls.client.users.create(
username="permission_bill",
email="[email protected]",
password="permission_s3cur3p@ssword",
)

cls.group_friends = cls.client.groups.create(name="Friends")

@classmethod
def teardown_class(cls):
cls.content.delete()
assert cls.client.content.count() == 0

cls.group_friends.delete()
assert cls.client.groups.count() == 0

def test_permissions_add_destroy(self):
assert self.client.groups.count() == 1
assert self.client.users.count() == 3
assert self.content.permissions.find() == []

# Add permissions
self.content.permissions.create(
principal_guid=self.user_aron["guid"],
principal_type="user",
role="viewer",
)
self.content.permissions.create(
principal_guid=self.group_friends["guid"],
principal_type="group",
role="owner",
)
schloerke marked this conversation as resolved.
Show resolved Hide resolved

def assert_permissions_match_guids(permissions, objs_with_guid):
for permission, obj_with_guid in zip(permissions, objs_with_guid):
assert permission["principal_guid"] == obj_with_guid["guid"]

# Prove they have been added
assert_permissions_match_guids(
self.content.permissions.find(),
[self.user_aron, self.group_friends],
)

# Remove permissions (and from some that isn't an owner)
destroyed_permissions = self.content.permissions.destroy(self.user_aron, self.user_bill)
assert_permissions_match_guids(
destroyed_permissions,
[self.user_aron],
)

# Prove they have been removed
assert_permissions_match_guids(
self.content.permissions.find(),
[self.group_friends],
)

# Remove the last permission
destroyed_permissions = self.content.permissions.destroy(self.group_friends)
assert_permissions_match_guids(
destroyed_permissions,
[self.group_friends],
)

# Prove they have been removed
assert self.content.permissions.find() == []
8 changes: 6 additions & 2 deletions integration/tests/posit/connect/test_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ class TestUser:
@classmethod
def setup_class(cls):
cls.client = client = connect.Client()

# Play nicely with other tests
cls.existing_user_count = client.users.count()

cls.aron = client.users.create(
username="aron",
email="[email protected]",
Expand All @@ -29,8 +33,8 @@ def test_lock(self):
assert len(self.client.users.find(account_status="locked")) == 0

def test_count(self):
# aron, bill, cole, and me
assert self.client.users.count() == 4
# aron, bill, cole, and me (and existing user)
assert self.client.users.count() == 3 + self.existing_user_count

def test_find(self):
assert self.client.users.find(prefix="aron") == [self.aron]
Expand Down
94 changes: 91 additions & 3 deletions src/posit/connect/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,20 @@

from __future__ import annotations

from typing import List, overload
from typing import TYPE_CHECKING, List, overload

from requests.sessions import Session as Session

from .resources import Resource, ResourceParameters, Resources

if TYPE_CHECKING:
from .groups import Group
from .users import User


class Permission(Resource):
def delete(self) -> None:
"""Delete the permission."""
def destroy(self) -> None:
"""Destroy the permission."""
path = f"v1/content/{self['content_guid']}/permissions/{self['id']}"
url = self.params.url + path
self.params.session.delete(url)
Expand Down Expand Up @@ -137,3 +141,87 @@ def get(self, uid: str) -> Permission:
url = self.params.url + path
response = self.params.session.get(url)
return Permission(self.params, **response.json())

def destroy(self, *permissions: str | Group | User | Permission) -> list[Permission]:
"""Remove supplied content item permissions.

Removes all provided permissions from the content item's permissions. If a permission isn't
found, it is silently ignored.

Parameters
----------
*permissions : str | Group | User | Permission
The content item permissions to remove. If a `str` is received, it is compared against
the `Permissions`' `principal_guid`. If a `Group` or `User` is received, the associated
schloerke marked this conversation as resolved.
Show resolved Hide resolved
`Permission` will be removed.

Returns
-------
list[Permission]
The removed permissions. If a permission is not found, it is not included in the
returned list.
schloerke marked this conversation as resolved.
Show resolved Hide resolved

Examples
--------
```python
from posit import connect

#### User-defined inputs ####
# 1. specify the guid for the content item
content_guid = "CONTENT_GUID_HERE"
# 2. specify either the principal_guid or group name prefix
principal_guid = "USER_OR_GROUP_GUID_HERE"
group_name_prefix = "GROUP_NAME_PREFIX_HERE"
############################

client = connect.Client()

# Remove a single permission by principal_guid
client.content.get(content_guid).permissions.destroy(principal_guid)

# Remove by user (if principal_guid is a user)
user = client.users.get(principal_guid)
client.content.get(content_guid).permissions.destroy(user)

# Remove by group (if principal_guid is a group)
group = client.groups.get(principal_guid)
client.content.get(content_guid).permissions.destroy(group)

# Remove all groups with a matching prefix name
groups = client.groups.find(prefix=group_name_prefix)
client.content.get(content_guid).permissions.destroy(*groups)

# Confirm new permissions
client.content.get(content_guid).permissions.find()
```
"""
from .groups import Group
from .users import User

if len(permissions) == 0:
raise ValueError("Expected at least one `permission` to remove")

principal_guids: set[str] = set()

for arg in permissions:
if isinstance(arg, str):
principal_guid = arg
Comment on lines +207 to +208
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have other facilities for checking if a string is the right shape to be a GUID? That might be nice to check here since someone might try passing client.content.get(content_guid).permissions.destroy("my group name") or the like?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. We do not have the functionality.

I believe the thought process is to have the server return an error and we display the error.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nods that makes sense. What does the server return as the error in that case? Is it something we expect SDK authors to know how to recover from?

Doing something more than ^^^ is probably scope creep, but I am curious what the ergonomics would be

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we instead change the approach of this method to always submit the delete API calls to the server? (rather than only the permissions we know exist.)

Then the server will always display an error. And we wouldn't need to do any validation on our end.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AAAH right on line 223 it will silently be ignore if it's anything other than a matching GUID. HMMM I don't have a strong intuition which direction would be better here, pre-checking or always sending and failing. There certainly are complications with either.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we instead change the approach of this method to always submit the delete API calls to the server? (rather than only the permissions we know exist.)

For posterity, did you end up implementing this or does it still (silently) ignore non-GUIDs (or really anything that doesn't match a GUID already present if it's a string)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still silently ignores non existent permissions

elif isinstance(arg, Group):
principal_guid: str = arg["guid"]
elif isinstance(arg, User):
principal_guid: str = arg["guid"]
schloerke marked this conversation as resolved.
Show resolved Hide resolved
elif isinstance(arg, Permission):
principal_guid: str = arg["principal_guid"]
else:
raise TypeError(
f"destroy() expected argument type 'str', 'User', 'Group', or 'Permission' but got '{type(arg).__name__}'",
)
principal_guids.add(principal_guid)

destroyed_permissions: list[Permission] = []
for permission in self.find():
if permission["principal_guid"] in principal_guids:
permission.destroy()
destroyed_permissions.append(permission)
schloerke marked this conversation as resolved.
Show resolved Hide resolved

return destroyed_permissions
8 changes: 3 additions & 5 deletions tests/posit/connect/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@

schloerke marked this conversation as resolved.
Show resolved Hide resolved
import pyjson5 as json

from posit.connect._json import Jsonifiable, JsonifiableDict, JsonifiableList


def load_mock(path: str) -> Jsonifiable:
def load_mock(path: str):
"""
Load mock data from a file.

Expand Down Expand Up @@ -33,13 +31,13 @@ def load_mock(path: str) -> Jsonifiable:
return json.loads((Path(__file__).parent / "__api__" / path).read_text())


def load_mock_dict(path: str) -> JsonifiableDict:
def load_mock_dict(path: str) -> dict:
result = load_mock(path)
assert isinstance(result, dict)
return result


def load_mock_list(path: str) -> JsonifiableList:
def load_mock_list(path: str) -> list:
result = load_mock(path)
assert isinstance(result, list)
return result
Expand Down
85 changes: 83 additions & 2 deletions tests/posit/connect/test_permissions.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
import random
import uuid

import pytest
import requests
import responses
from responses import matchers

from posit.connect.groups import Group
from posit.connect.permissions import Permission, Permissions
from posit.connect.resources import ResourceParameters
from posit.connect.urls import Url
from posit.connect.users import User

from .api import load_mock, load_mock_dict, load_mock_list


class TestPermissionDelete:
class TestPermissionDestroy:
@responses.activate
def test(self):
# data
Expand All @@ -30,7 +33,7 @@ def test(self):
permission = Permission(params, **fake_permission)

# invoke
permission.delete()
permission.destroy()

# assert
assert mock_delete.call_count == 1
Expand Down Expand Up @@ -262,3 +265,81 @@ def test(self):

# assert
assert permission == fake_permission


class TestPermissionsDestroy:
@responses.activate
def test_destroy(self):
# data
permission_uid = "94"
content_guid = "f2f37341-e21d-3d80-c698-a935ad614066"
fake_permissions = load_mock_list(f"v1/content/{content_guid}/permissions.json")
fake_followup_permissions = fake_permissions.copy()
fake_followup_permissions.pop(0)
fake_permission = load_mock_dict(
f"v1/content/{content_guid}/permissions/{permission_uid}.json"
)
fake_user = load_mock_dict("v1/user.json")
fake_group = load_mock_dict("v1/groups/6f300623-1e0c-48e6-a473-ddf630c0c0c3.json")

# behavior

# Used in internal for-loop
mock_permissions_get = [
responses.get(
f"https://connect.example/__api__/v1/content/{content_guid}/permissions",
json=fake_permissions,
),
responses.get(
f"https://connect.example/__api__/v1/content/{content_guid}/permissions",
json=fake_followup_permissions,
),
]
# permission delete
mock_permission_delete = responses.delete(
f"https://connect.example/__api__/v1/content/{content_guid}/permissions/{permission_uid}",
)

# setup
params = ResourceParameters(requests.Session(), Url("https://connect.example/__api__"))
permissions = Permissions(params, content_guid=content_guid)

# (Doesn't match any permissions, but that's okay)
user_to_remove = User(params, **fake_user)
group_to_remove = Group(params, **fake_group)
permission_to_remove = Permission(params, **fake_permission)

# invoke
destroyed_permission = permissions.destroy(
fake_permission["principal_guid"],
# Make sure duplicates are dropped
fake_permission["principal_guid"],
# Extract info from User, Group, Permission
user_to_remove,
group_to_remove,
permission_to_remove,
)

# Assert bad input value

with pytest.raises(TypeError):
schloerke marked this conversation as resolved.
Show resolved Hide resolved
permissions.destroy(
42 # pyright: ignore[reportArgumentType]
)
with pytest.raises(ValueError):
permissions.destroy()

# Assert values
assert mock_permissions_get[0].call_count == 1
assert mock_permissions_get[1].call_count == 0
assert mock_permission_delete.call_count == 1
assert len(destroyed_permission) == 1
assert destroyed_permission[0] == fake_permission

# Invoking again is a no-op
destroyed_permission = permissions.destroy(fake_permission["principal_guid"])

assert mock_permissions_get[0].call_count == 1
assert mock_permissions_get[1].call_count == 1
assert mock_permission_delete.call_count == 1
assert len(destroyed_permission) == 0
Loading