studio/docs
Archived
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

♻️(backend) stop requiring owner for non-root documents

Browse Source 
If root documents are guaranteed to have a owner, non-root documents
will automatically have them as owner by inheritance. We should not
require non-root documents to have their own direct owner because
this will make it difficult to manage access rights when we move
documents around or when we want to remove access rights for someone
on a document subtree... There should be as few overrides as possible.
This commit is contained in:
Samuel Paccoud - DINUM
2025-05-04 22:16:34 +02:00
committed by Anthony LC
parent 1ab237af3b
commit 184b5c015b
7 changed files with 298 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@@ -25,6 +25,7 @@ and this project adheres to
### Changed
- ♻️(backend) stop requiring owner for non-root documents #846
- ♻️(backend) simplify roles by ranking them and return only the max role #846
- 📌(yjs) stop pinning node to minor version on yjs docker image #1005
- 🧑‍💻(docker) add .next to .dockerignore #1055

73
src/backend/core/api/viewsets.py
View File

@@ -234,43 +234,6 @@ class ResourceAccessViewsetMixin:
context["resource_id"] = self.kwargs["resource_id"]
return context
def destroy(self, request, *args, **kwargs):
"""Forbid deleting the last owner access"""
instance = self.get_object()
resource = getattr(instance, self.resource_field_name)
# Check if the access being deleted is the last owner access for the resource
if (
instance.role == "owner"
and resource.accesses.filter(role="owner").count() == 1
):
return drf.response.Response(
{"detail": "Cannot delete the last owner access for the resource."},
status=drf.status.HTTP_403_FORBIDDEN,
)
return super().destroy(request, *args, **kwargs)
def perform_update(self, serializer):
"""Check that we don't change the role if it leads to losing the last owner."""
instance = serializer.instance
# Check if the role is being updated and the new role is not "owner"
if (
"role" in self.request.data
and self.request.data["role"] != models.RoleChoices.OWNER
):
resource = getattr(instance, self.resource_field_name)
# Check if the access being updated is the last owner access for the resource
if (
instance.role == models.RoleChoices.OWNER
and resource.accesses.filter(role=models.RoleChoices.OWNER).count() == 1
):
message = "Cannot change the role to a non-owner role for the last owner access."
raise drf.exceptions.PermissionDenied({"detail": message})
serializer.save()
class DocumentMetadata(drf.metadata.SimpleMetadata):
"""Custom metadata class to add information"""
@@ -726,7 +689,7 @@ class DocumentViewSet(
position = validated_data["position"]
message = None
owner_accesses = []
if position in [
enums.MoveNodePositionChoices.FIRST_CHILD,
enums.MoveNodePositionChoices.LAST_CHILD,
@@ -736,12 +699,15 @@ class DocumentViewSet(
"You do not have permission to move documents "
"as a child to this target document."
)
elif not target_document.is_root():
if not target_document.get_parent().get_abilities(user).get("move"):
message = (
"You do not have permission to move documents "
"as a sibling of this target document."
)
elif target_document.is_root():
owner_accesses = document.get_root().accesses.filter(
role=models.RoleChoices.OWNER
)
elif not target_document.get_parent().get_abilities(user).get("move"):
message = (
"You do not have permission to move documents "
"as a sibling of this target document."
)
if message:
return drf.response.Response(
@@ -751,6 +717,19 @@ class DocumentViewSet(
document.move(target_document, pos=position)
# Make sure we have at least one owner
if (
owner_accesses
and not document.accesses.filter(role=models.RoleChoices.OWNER).exists()
):
for owner_access in owner_accesses:
models.DocumentAccess.objects.update_or_create(
document=document,
user=owner_access.user,
team=owner_access.team,
defaults={"role": models.RoleChoices.OWNER},
)
return drf.response.Response(
{"message": "Document moved successfully."}, status=status.HTTP_200_OK
)
@@ -797,11 +776,7 @@ class DocumentViewSet(
creator=request.user,
**serializer.validated_data,
)
models.DocumentAccess.objects.create(
document=child_document,
user=request.user,
role=models.RoleChoices.OWNER,
)
# Set the created instance to the serializer
serializer.instance = child_document

9
src/backend/core/models.py
View File

@@ -1121,9 +1121,12 @@ class DocumentAccess(BaseAccess):
is_owner_or_admin = role in PRIVILEGED_ROLES
if self.role == RoleChoices.OWNER:
can_delete = (
role == RoleChoices.OWNER
and DocumentAccess.objects.filter(
can_delete = role == RoleChoices.OWNER and (
# check if document is not root trying to avoid an extra query
# "document_path" is annotated by the viewset's list method
len(getattr(self, "document_path", "")) > Document.steplen
or not self.document.is_root()
or DocumentAccess.objects.filter(
document_id=self.document_id, role=RoleChoices.OWNER
).count()
> 1

144
src/backend/core/tests/documents/test_api_document_accesses.py
View File

@@ -224,12 +224,7 @@ def test_api_document_accesses_list_authenticated_related_privileged(
other_access = factories.UserDocumentAccessFactory(user=user)
factories.UserDocumentAccessFactory(document=other_access.document)
nb_queries = 3
if role == "owner":
# Queries that secure the owner status
nb_queries += sum(acc.role == "owner" for acc in document_accesses)
with django_assert_num_queries(nb_queries):
with django_assert_num_queries(3):
response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/")
assert response.status_code == 200
@@ -335,7 +330,12 @@ def test_api_document_accesses_retrieve_set_role_to_child():
],
[
["owner", "reader", "reader", "reader"],
[[], [], [], ["reader", "editor", "administrator", "owner"]],
[
["reader", "editor", "administrator", "owner"],
[],
[],
["reader", "editor", "administrator", "owner"],
],
],
[
["owner", "reader", "reader", "owner"],
@@ -412,7 +412,12 @@ def test_api_document_accesses_list_authenticated_related_same_user(roles, resul
],
[
["owner", "reader", "reader", "reader"],
[[], [], [], ["reader", "editor", "administrator", "owner"]],
[
["reader", "editor", "administrator", "owner"],
[],
[],
["reader", "editor", "administrator", "owner"],
],
],
[
["owner", "reader", "reader", "owner"],
@@ -425,7 +430,12 @@ def test_api_document_accesses_list_authenticated_related_same_user(roles, resul
],
[
["reader", "reader", "reader", "owner"],
[["reader", "editor", "administrator", "owner"], [], [], []],
[
["reader", "editor", "administrator", "owner"],
[],
[],
["reader", "editor", "administrator", "owner"],
],
],
[
["reader", "administrator", "reader", "editor"],
@@ -929,7 +939,7 @@ def test_api_document_accesses_update_owner(
@pytest.mark.parametrize("via", VIA)
def test_api_document_accesses_update_owner_self(
def test_api_document_accesses_update_owner_self_root(
via,
mock_user_teams,
mock_reset_connections, # pylint: disable=redefined-outer-name
@@ -990,6 +1000,51 @@ def test_api_document_accesses_update_owner_self(
assert access.role == new_role
@pytest.mark.parametrize("via", VIA)
def test_api_document_accesses_update_owner_self_child(
via,
mock_user_teams,
mock_reset_connections, # pylint: disable=redefined-outer-name
):
"""
A user who is owner of a document should be allowed to update
their own user access even if they are the only owner in the document,
provided the document is not a root.
"""
user = factories.UserFactory(with_owned_document=True)
client = APIClient()
client.force_login(user)
parent = factories.DocumentFactory()
document = factories.DocumentFactory(parent=parent)
access = None
if via == USER:
access = factories.UserDocumentAccessFactory(
document=document, user=user, role="owner"
)
elif via == TEAM:
mock_user_teams.return_value = ["lasuite", "unknown"]
access = factories.TeamDocumentAccessFactory(
document=document, team="lasuite", role="owner"
)
old_values = serializers.DocumentAccessSerializer(instance=access).data
new_role = random.choice(["administrator", "editor", "reader"])
user_id = str(access.user_id) if via == USER else None
with mock_reset_connections(document.id, user_id):
response = client.put(
f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/",
data={**old_values, "role": new_role},
format="json",
)
assert response.status_code == 200
access.refresh_from_db()
assert access.role == new_role
# Delete
@@ -1170,17 +1225,16 @@ def test_api_document_accesses_delete_owners(
f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/",
)
assert response.status_code == 204
assert models.DocumentAccess.objects.count() == 1
assert response.status_code == 204
assert models.DocumentAccess.objects.count() == 1
@pytest.mark.parametrize("via", VIA)
def test_api_document_accesses_delete_owners_last_owner(via, mock_user_teams):
def test_api_document_accesses_delete_owners_last_owner_root(via, mock_user_teams):
"""
It should not be possible to delete the last owner access from a document
It should not be possible to delete the last owner access from a root document
"""
user = factories.UserFactory(with_owned_document=True)
client = APIClient()
client.force_login(user)
@@ -1203,3 +1257,63 @@ def test_api_document_accesses_delete_owners_last_owner(via, mock_user_teams):
assert response.status_code == 403
assert models.DocumentAccess.objects.count() == 2
def test_api_document_accesses_delete_owners_last_owner_child_user(
mock_reset_connections, # pylint: disable=redefined-outer-name
):
"""
It should be possible to delete the last owner access from a document that is not a root.
"""
user = factories.UserFactory(with_owned_document=True)
client = APIClient()
client.force_login(user)
parent = factories.DocumentFactory()
document = factories.DocumentFactory(parent=parent)
access = None
access = factories.UserDocumentAccessFactory(
document=document, user=user, role="owner"
)
assert models.DocumentAccess.objects.count() == 2
with mock_reset_connections(document.id, str(access.user_id)):
response = client.delete(
f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/",
)
assert response.status_code == 204
assert models.DocumentAccess.objects.count() == 1
@pytest.mark.skip(
reason="Pending fix on https://github.com/suitenumerique/docs/issues/969"
)
def test_api_document_accesses_delete_owners_last_owner_child_team(
mock_user_teams,
mock_reset_connections, # pylint: disable=redefined-outer-name
):
"""
It should be possible to delete the last owner access from a document that
is not a root.
"""
user = factories.UserFactory(with_owned_document=True)
client = APIClient()
client.force_login(user)
parent = factories.DocumentFactory()
document = factories.DocumentFactory(parent=parent)
access = None
mock_user_teams.return_value = ["lasuite", "unknown"]
access = factories.TeamDocumentAccessFactory(
document=document, team="lasuite", role="owner"
)
assert models.DocumentAccess.objects.count() == 2
with mock_reset_connections(document.id, str(access.user_id)):
response = client.delete(
f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/",
)
assert response.status_code == 204
assert models.DocumentAccess.objects.count() == 1

6
src/backend/core/tests/documents/test_api_documents_children_create.py
View File

@@ -114,7 +114,8 @@ def test_api_documents_children_create_authenticated_success(reach, role, depth)
child = Document.objects.get(id=response.json()["id"])
assert child.title == "my child"
assert child.link_reach == "restricted"
assert child.accesses.filter(role="owner", user=user).exists()
# Access objects on the child are not necessary
assert child.accesses.exists() is False
@pytest.mark.parametrize("depth", [1, 2, 3])
@@ -182,7 +183,8 @@ def test_api_documents_children_create_related_success(role, depth):
child = Document.objects.get(id=response.json()["id"])
assert child.title == "my child"
assert child.link_reach == "restricted"
assert child.accesses.filter(role="owner", user=user).exists()
# Access objects on the child are not necessary
assert child.accesses.exists() is False
def test_api_documents_children_create_authenticated_title_null():

105
src/backend/core/tests/documents/test_api_documents_move.py
View File

@@ -124,8 +124,8 @@ def test_api_documents_move_authenticated_target_roles_mocked(
target_role, target_parent_role, position
):
"""
Authenticated users with insufficient permissions on the target document (or its
parent depending on the position chosen), should not be allowed to move documents.
Only authenticated users with sufficient permissions on the target document (or its
parent depending on the position chosen), should be allowed to move documents.
"""
user = factories.UserFactory()
@@ -208,6 +208,107 @@ def test_api_documents_move_authenticated_target_roles_mocked(
assert document.is_root() is True
def test_api_documents_move_authenticated_no_owner_user_and_team():
"""
Moving a document with no owner to the root of the tree should automatically declare
the owner of the previous root of the document as owner of the document itself.
"""
user = factories.UserFactory()
client = APIClient()
client.force_login(user)
parent_owner = factories.UserFactory()
parent = factories.DocumentFactory(
users=[(parent_owner, "owner")], teams=[("lasuite", "owner")]
)
# A document with no owner
document = factories.DocumentFactory(parent=parent, users=[(user, "administrator")])
child = factories.DocumentFactory(parent=document)
target = factories.DocumentFactory()
response = client.post(
f"/api/v1.0/documents/{document.id!s}/move/",
data={"target_document_id": str(target.id), "position": "first-sibling"},
)
assert response.status_code == 200
assert response.json() == {"message": "Document moved successfully."}
assert list(target.get_siblings()) == [document, parent, target]
document.refresh_from_db()
assert list(document.get_children()) == [child]
assert document.accesses.count() == 3
assert document.accesses.get(user__isnull=False, role="owner").user == parent_owner
assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite"
assert document.accesses.get(role="administrator").user == user
def test_api_documents_move_authenticated_no_owner_same_user():
"""
Moving a document should not fail if the user moving a document with no owner was
at the same time owner of the previous root and has a role on the document being moved.
"""
user = factories.UserFactory()
client = APIClient()
client.force_login(user)
parent = factories.DocumentFactory(
users=[(user, "owner")], teams=[("lasuite", "owner")]
)
# A document with no owner
document = factories.DocumentFactory(parent=parent, users=[(user, "reader")])
child = factories.DocumentFactory(parent=document)
target = factories.DocumentFactory()
response = client.post(
f"/api/v1.0/documents/{document.id!s}/move/",
data={"target_document_id": str(target.id), "position": "first-sibling"},
)
assert response.status_code == 200
assert response.json() == {"message": "Document moved successfully."}
assert list(target.get_siblings()) == [document, parent, target]
document.refresh_from_db()
assert list(document.get_children()) == [child]
assert document.accesses.count() == 2
assert document.accesses.get(user__isnull=False, role="owner").user == user
assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite"
def test_api_documents_move_authenticated_no_owner_same_team():
"""
Moving a document should not fail if the team that is owner of the document root was
already declared on the document with a different role.
"""
user = factories.UserFactory()
client = APIClient()
client.force_login(user)
parent = factories.DocumentFactory(teams=[("lasuite", "owner")])
# A document with no owner but same team
document = factories.DocumentFactory(
parent=parent, users=[(user, "administrator")], teams=[("lasuite", "reader")]
)
child = factories.DocumentFactory(parent=document)
target = factories.DocumentFactory()
response = client.post(
f"/api/v1.0/documents/{document.id!s}/move/",
data={"target_document_id": str(target.id), "position": "first-sibling"},
)
assert response.status_code == 200
assert response.json() == {"message": "Document moved successfully."}
assert list(target.get_siblings()) == [document, parent, target]
document.refresh_from_db()
assert list(document.get_children()) == [child]
assert document.accesses.count() == 2
assert document.accesses.get(user__isnull=False, role="administrator").user == user
assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite"
def test_api_documents_move_authenticated_deleted_document():
"""
It should not be possible to move a deleted document or its descendants, even

34
src/backend/core/tests/test_models_document_accesses.py
View File

@@ -127,12 +127,18 @@ def test_models_document_access_get_abilities_for_owner_of_self_allowed():
}
def test_models_document_access_get_abilities_for_owner_of_self_last():
def test_models_document_access_get_abilities_for_owner_of_self_last_on_root(
django_assert_num_queries,
):
"""
Check abilities of self access for the owner of a document when there is only one owner left.
Check abilities of self access for the owner of a root document when there
is only one owner left.
"""
access = factories.UserDocumentAccessFactory(role="owner")
abilities = access.get_abilities(access.user)
with django_assert_num_queries(2):
abilities = access.get_abilities(access.user)
assert abilities == {
"destroy": False,
"retrieve": True,
@@ -142,6 +148,28 @@ def test_models_document_access_get_abilities_for_owner_of_self_last():
}
def test_models_document_access_get_abilities_for_owner_of_self_last_on_child(
django_assert_num_queries,
):
"""
Check abilities of self access for the owner of a child document when there
is only one owner left.
"""
parent = factories.DocumentFactory()
access = factories.UserDocumentAccessFactory(document__parent=parent, role="owner")
with django_assert_num_queries(1):
abilities = access.get_abilities(access.user)
assert abilities == {
"destroy": True,
"retrieve": True,
"update": True,
"partial_update": True,
"set_role_to": ["reader", "editor", "administrator", "owner"],
}
def test_models_document_access_get_abilities_for_owner_of_owner():
"""Check abilities of owner access for the owner of a document."""
access = factories.UserDocumentAccessFactory(role="owner")