diff --git a/CHANGELOG.md b/CHANGELOG.md index e85e8de6e..9ffe306e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to - ✨(buildpack) add PaaS deployment support, tested with Scalingo #2293 - 🔧(backend) allow configuring settings OIDC_OP_USER_ENDPOINT_FORMAT - ⚡️(helm) create a dedicated svc and deployment for yprovider converter #2368 +- ✨(backend) allow to leave a document #2365 ### Changed diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index f180cf8db..71efacad3 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -19,7 +19,7 @@ from django.core.cache import cache from django.core.exceptions import ValidationError from django.core.files.storage import default_storage from django.core.validators import URLValidator -from django.db import connection, transaction +from django.db import DatabaseError, connection, transaction from django.db import models as db from django.db.models.expressions import RawSQL from django.db.models.functions import Greatest, Left, Length @@ -598,6 +598,8 @@ class DocumentViewSet( user = self.request.user queryset = queryset.annotate_is_favorite(user) queryset = queryset.annotate_user_roles(user) + queryset = queryset.annotate_user_has_link_trace(user) + return queryset def get_response_for_queryset(self, queryset, context=None): @@ -638,6 +640,7 @@ class DocumentViewSet( for parent in ( models.Document.objects.annotate_user_roles(self.request.user) .annotate_is_favorite(self.request.user) + .annotate_user_has_link_trace(self.request.user) .filter(path__in=missing_parent_paths) .iterator() ): @@ -675,7 +678,7 @@ class DocumentViewSet( for field in ["is_creator_me", "title", "q"]: queryset = filterset.filters[field].filter(queryset, filter_data[field]) - queryset = queryset.annotate_user_roles(user) + queryset = queryset.annotate_user_roles(user).annotate_user_has_link_trace(user) # Among the results, we may have documents that are ancestors/descendants # of each other. In this case we want to keep only the highest ancestors. @@ -706,7 +709,6 @@ class DocumentViewSet( """ user = self.request.user instance = self.get_object() - serializer = self.get_serializer(instance) # The `create` query generates 5 db queries which are much less efficient than an # `exists` query. The user will visit the document many times after the first visit @@ -717,6 +719,11 @@ class DocumentViewSet( ): models.LinkTrace.objects.create(document=instance, user=request.user) + # To avoid N+1 query, we force the `user_has_link_trace` normally set by the + # queryset.annotate_user_has_link_trace method. If the user is connected, it must be True. + instance.user_has_link_trace = user.is_authenticated + + serializer = self.get_serializer(instance) return drf.response.Response(serializer.data) def _apply_uploaded_file_conversion(self, serializer): @@ -880,7 +887,7 @@ class DocumentViewSet( queryset = queryset.filter(id__in=favorite_documents_ids) queryset = queryset.filter(ancestors_deleted_at__isnull=True) queryset = queryset.order_by("-updated_at") - queryset = queryset.annotate_user_roles(user) + queryset = queryset.annotate_user_roles(user).annotate_user_has_link_trace(user) queryset = queryset.annotate( is_favorite=db.Value(True, output_field=db.BooleanField()) ) @@ -922,7 +929,9 @@ class DocumentViewSet( deleted_at__isnull=False, deleted_at__gte=models.get_trashbin_cutoff(), ) - queryset = queryset.annotate_user_roles(self.request.user) + queryset = queryset.annotate_user_roles( + self.request.user + ).annotate_user_has_link_trace(self.request.user) return self.get_response_for_queryset(queryset) @@ -1174,7 +1183,7 @@ class DocumentViewSet( for field in ["is_creator_me", "title", "q"]: queryset = filterset.filters[field].filter(queryset, filter_data[field]) - queryset = queryset.annotate_user_roles(user) + queryset = queryset.annotate_user_roles(user).annotate_user_has_link_trace(user) # Annotate favorite status and filter if applicable as late as possible queryset = queryset.annotate_is_favorite(user) @@ -1273,6 +1282,7 @@ class DocumentViewSet( queryset = queryset.order_by("path") queryset = queryset.annotate_user_roles(user) queryset = queryset.annotate_is_favorite(user) + queryset = queryset.annotate_user_has_link_trace(user) # Pass ancestors' links paths mapping to the serializer as a context variable # in order to allow saving time while computing abilities on the instance @@ -1590,6 +1600,7 @@ class DocumentViewSet( .filter(ancestors_deleted_at__isnull=True) .annotate_user_roles(user) .annotate_is_favorite(user) + .annotate_user_has_link_trace(user) ) queryset = filterset.filter_queryset(queryset) @@ -2499,6 +2510,36 @@ class DocumentViewSet( } ) + @drf.decorators.action( + detail=True, + methods=["post"], + ) + def leave(self, request, *args, **kwargs): + """ + Remove document_accesses if exists and the link_trace related to the current document + for the connected user. + """ + # Check for permissions. + document = self.get_object() + + try: + with transaction.atomic(): + models.DocumentAccess.objects.filter( + document__path__startswith=document.path, user=request.user + ).delete() + models.LinkTrace.objects.filter( + document__path__startswith=document.path, user=request.user + ).delete() + except DatabaseError: + logger.error( + "Impossible to leave document %s for user %s", + str(document.id), + str(request.user.id), + ) + raise + + return drf.response.Response(status=drf.status.HTTP_204_NO_CONTENT) + class DocumentAccessViewSet( ResourceAccessViewsetMixin, diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 9b4b8a46e..8ae5b1312 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -852,6 +852,22 @@ class DocumentQuerySet(MP_NodeQuerySet): user_roles=models.Value([], output_field=output_field), ) + def annotate_user_has_link_trace(self, user): + """ + Annotate document queryset with a boolean to know if the current user + has a link_trace on the current document. + """ + + if user.is_authenticated: + link_trace_exists_subquery = LinkTrace.objects.filter( + document_id=models.OuterRef("pk"), user=user + ) + return self.annotate( + user_has_link_trace=models.Exists(link_trace_exists_subquery) + ) + + return self.annotate(user_has_link_trace=models.Value(False)) + class DocumentManager(MP_NodeManager.from_queryset(DocumentQuerySet)): """ @@ -1150,6 +1166,17 @@ class Document(MP_Node, BaseModel): return RoleChoices.max(*roles) + def has_link_trace(self, user): + """Return if the user has a link trace on this document.""" + + if not user.is_authenticated: + return False + + try: + return self.user_has_link_trace + except AttributeError: + return LinkTrace.objects.filter(document=self, user=user).exists() + def compute_ancestors_links_paths_mapping(self): """ Compute the ancestors links for the current document up to the highest readable ancestor. @@ -1227,7 +1254,7 @@ class Document(MP_Node, BaseModel): """Actual link role on the document.""" return self.computed_link_definition["link_role"] - def get_abilities(self, user): + def get_abilities(self, user): # pylint: disable=too-many-locals """ Compute and return abilities for a given user on the document. """ @@ -1248,6 +1275,14 @@ class Document(MP_Node, BaseModel): is_owner_or_admin or role == RoleChoices.EDITOR ) and not is_deleted + # compute can_leave + # A user can leave a document if it has non privileged role on the document or it has + # access to it with a link_trace + can_leave = user.is_authenticated and ( + (has_access_role and not is_owner_or_admin) + or (not has_access_role and self.has_link_trace(user)) + ) + link_select_options = LinkReachChoices.get_select_options( **self.ancestors_link_definition ) @@ -1312,6 +1347,7 @@ class Document(MP_Node, BaseModel): "favorite": can_get and user.is_authenticated, "link_configuration": is_owner_or_admin, "invite_owner": is_owner and not is_deleted, + "leave": can_leave, "move": is_owner_or_admin and not is_deleted, "partial_update": can_update, "restore": is_owner, diff --git a/src/backend/core/tests/documents/test_api_documents_leave.py b/src/backend/core/tests/documents/test_api_documents_leave.py new file mode 100644 index 000000000..82c0eded4 --- /dev/null +++ b/src/backend/core/tests/documents/test_api_documents_leave.py @@ -0,0 +1,298 @@ +"""Test for the leave document API""" + +import pytest +from rest_framework import status +from rest_framework.test import APIClient + +from core import factories, models + +pytestmark = pytest.mark.django_db + + +@pytest.mark.parametrize( + "link_reach", + models.LinkReachChoices.values, +) +def test_api_documents_leave_document_anonymous_user(link_reach): + """ + Anonymous user are not allowed to access the leave feature no matter the document link reach. + """ + + document = factories.DocumentFactory(link_reach=link_reach) + + client = APIClient() + response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/") + + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + +@pytest.mark.parametrize( + "link_reach", + models.LinkReachChoices.values, +) +def test_api_documents_leave_connected_user_without_access_nor_link_trace(link_reach): + """ + A connected user with no access or link_trace on the document can not access the leave feature + """ + + user = factories.UserFactory() + other_users = factories.UserFactory.create_batch(3) + + document = factories.DocumentFactory(link_reach=link_reach, link_traces=other_users) + + factories.UserDocumentAccessFactory.create_batch(4, document=document) + + assert not models.LinkTrace.objects.filter(document=document, user=user).exists() + assert not models.DocumentAccess.objects.filter( + document=document, user=user + ).exists() + + assert models.LinkTrace.objects.count() == 3 + assert models.DocumentAccess.objects.count() == 4 + + client = APIClient() + client.force_login(user) + response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/") + + assert response.status_code == status.HTTP_403_FORBIDDEN + + assert models.LinkTrace.objects.count() == 3 + assert models.DocumentAccess.objects.count() == 4 + + +@pytest.mark.parametrize( + "link_reach", + [models.LinkReachChoices.PUBLIC, models.LinkReachChoices.AUTHENTICATED], +) +def test_api_documents_leave_connected_user_with_link_trace(link_reach): + """ + A connected user with link_trace on a document can leave it. + """ + + user = factories.UserFactory() + other_users = factories.UserFactory.create_batch(3) + + document = factories.DocumentFactory( + link_reach=link_reach, link_traces=[user, *other_users] + ) + factories.UserDocumentAccessFactory.create_batch(4, document=document) + + assert models.LinkTrace.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.count() == 4 + assert models.DocumentAccess.objects.count() == 4 + + client = APIClient() + client.force_login(user) + response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/") + + assert response.status_code == status.HTTP_204_NO_CONTENT + + assert not models.LinkTrace.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.count() == 3 + assert models.DocumentAccess.objects.count() == 4 + + +@pytest.mark.parametrize( + "link_reach", + models.LinkReachChoices.values, +) +@pytest.mark.parametrize( + "role", [role for role in models.RoleChoices if role not in models.PRIVILEGED_ROLES] +) +def test_api_documents_leave_connected_user_with_access(role, link_reach): + """Connected user with a DocumentAccess can leave it.""" + + user = factories.UserFactory() + other_users = factories.UserFactory.create_batch(3) + + document = factories.DocumentFactory( + link_reach=link_reach, link_traces=other_users, users=[(user, role)] + ) + factories.UserDocumentAccessFactory.create_batch(4, document=document) + + assert models.DocumentAccess.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.count() == 3 + assert models.DocumentAccess.objects.count() == 5 + + client = APIClient() + client.force_login(user) + response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/") + + assert response.status_code == status.HTTP_204_NO_CONTENT + + assert not models.DocumentAccess.objects.filter( + document=document, user=user + ).exists() + assert models.LinkTrace.objects.count() == 3 + assert models.DocumentAccess.objects.count() == 4 + + +@pytest.mark.parametrize( + "link_reach", + models.LinkReachChoices.values, +) +@pytest.mark.parametrize( + "role", [role for role in models.RoleChoices if role in models.PRIVILEGED_ROLES] +) +def test_api_documents_leave_connected_access_with_privileged_role_not_allowed( + role, link_reach +): + """Connected user with privileged access role can not leave a document.""" + + user = factories.UserFactory() + other_users = factories.UserFactory.create_batch(3) + + document = factories.DocumentFactory( + link_reach=link_reach, link_traces=[user, *other_users], users=[(user, role)] + ) + factories.UserDocumentAccessFactory.create_batch(4, document=document) + + assert models.DocumentAccess.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.count() == 4 + assert models.DocumentAccess.objects.count() == 5 + + client = APIClient() + client.force_login(user) + response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/") + + assert response.status_code == status.HTTP_403_FORBIDDEN + + assert models.DocumentAccess.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.count() == 4 + assert models.DocumentAccess.objects.count() == 5 + + +@pytest.mark.parametrize( + "link_reach", + models.LinkReachChoices.values, +) +@pytest.mark.parametrize( + "role", [role for role in models.RoleChoices if role not in models.PRIVILEGED_ROLES] +) +def test_api_documents_leave_connected_user_with_access_and_link_trace( + role, link_reach +): + """Connected user with a DocumentAccess can leave it.""" + + user = factories.UserFactory() + other_users = factories.UserFactory.create_batch(3) + + document = factories.DocumentFactory( + link_reach=link_reach, link_traces=[user, *other_users], users=[(user, role)] + ) + factories.UserDocumentAccessFactory.create_batch(4, document=document) + + assert models.DocumentAccess.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.count() == 4 + assert models.DocumentAccess.objects.count() == 5 + + client = APIClient() + client.force_login(user) + response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/") + + assert response.status_code == status.HTTP_204_NO_CONTENT + + assert not models.DocumentAccess.objects.filter( + document=document, user=user + ).exists() + assert not models.LinkTrace.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.count() == 3 + assert models.DocumentAccess.objects.count() == 4 + + +@pytest.mark.parametrize( + "link_reach", + models.LinkReachChoices.values, +) +@pytest.mark.parametrize( + "role", [role for role in models.RoleChoices if role not in models.PRIVILEGED_ROLES] +) +def test_api_documents_leave_connected_accessing_multiple_documents_leave_only_one( + role, link_reach +): + """Connected user accessing multiple document leaving one should keep access to the others.""" + + user = factories.UserFactory() + other_users = factories.UserFactory.create_batch(3) + + document = factories.DocumentFactory( + link_reach=link_reach, link_traces=[user, *other_users], users=[(user, role)] + ) + factories.UserDocumentAccessFactory.create_batch(4, document=document) + + # Create access to other documents for the same user + factories.UserDocumentAccessFactory.create_batch(4, user=user) + + assert models.DocumentAccess.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.count() == 4 + assert models.DocumentAccess.objects.count() == 9 + + client = APIClient() + client.force_login(user) + response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/") + + assert response.status_code == status.HTTP_204_NO_CONTENT + + assert not models.DocumentAccess.objects.filter( + document=document, user=user + ).exists() + assert not models.LinkTrace.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.count() == 3 + assert models.DocumentAccess.objects.count() == 8 + + +@pytest.mark.parametrize( + "link_reach", + models.LinkReachChoices.values, +) +@pytest.mark.parametrize( + "role", [role for role in models.RoleChoices if role not in models.PRIVILEGED_ROLES] +) +def test_api_documents_leave_connected_leave_also_sub_documents(role, link_reach): + """User connected with access and link_trace to a tree should leave all the tree.""" + + user = factories.UserFactory() + other_users = factories.UserFactory.create_batch(3) + + document = factories.DocumentFactory( + link_reach=link_reach, link_traces=[user, *other_users], users=[(user, role)] + ) + child = factories.DocumentFactory(parent=document, link_traces=[user, *other_users]) + grand_child = factories.DocumentFactory( + parent=child, link_traces=[user, *other_users], users=[(user, role)] + ) + + factories.UserDocumentAccessFactory.create_batch(4, document=document) + + # Create access to other documents for the same user + factories.UserDocumentAccessFactory.create_batch(4, user=user) + + assert models.DocumentAccess.objects.filter(document=document, user=user).exists() + assert models.DocumentAccess.objects.filter( + document=grand_child, user=user + ).exists() + assert models.LinkTrace.objects.filter(document=document, user=user).exists() + assert models.LinkTrace.objects.filter(document=child, user=user).exists() + assert models.LinkTrace.objects.filter(document=grand_child, user=user).exists() + assert models.LinkTrace.objects.count() == 12 + assert models.DocumentAccess.objects.count() == 10 + + client = APIClient() + client.force_login(user) + response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/") + + assert response.status_code == status.HTTP_204_NO_CONTENT + + assert not models.DocumentAccess.objects.filter( + document=document, user=user + ).exists() + assert not models.DocumentAccess.objects.filter( + document=grand_child, user=user + ).exists() + assert not models.LinkTrace.objects.filter(document=document, user=user).exists() + assert not models.LinkTrace.objects.filter(document=child, user=user).exists() + assert not models.LinkTrace.objects.filter(document=grand_child, user=user).exists() + assert models.LinkTrace.objects.count() == 9 + assert models.DocumentAccess.objects.count() == 8 diff --git a/src/backend/core/tests/documents/test_api_documents_retrieve.py b/src/backend/core/tests/documents/test_api_documents_retrieve.py index b209fb279..db7907ffb 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -54,6 +54,7 @@ def test_api_documents_retrieve_anonymous_public_standalone(): }, "content_patch": document.link_role == "editor", "content_retrieve": True, + "leave": False, "media_auth": True, "media_check": True, "move": False, @@ -132,6 +133,7 @@ def test_api_documents_retrieve_anonymous_public_parent(): ), "content_patch": grand_parent.link_role == "editor", "content_retrieve": True, + "leave": False, "media_auth": True, "media_check": True, "move": False, @@ -243,6 +245,7 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( }, "content_patch": document.link_role == "editor", "content_retrieve": True, + "leave": True, "media_auth": True, "media_check": True, "move": False, @@ -329,6 +332,7 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea "move": False, "content_patch": grand_parent.link_role == "editor", "content_retrieve": True, + "leave": True, "media_auth": True, "media_check": True, "partial_update": grand_parent.link_role == "editor", @@ -527,6 +531,7 @@ def test_api_documents_retrieve_authenticated_related_parent(): ), "content_patch": access.role not in ["reader", "commenter"], "content_retrieve": True, + "leave": access.role not in ["administrator", "owner"], "media_auth": True, "media_check": True, "move": access.role in ["administrator", "owner"], diff --git a/src/backend/core/tests/documents/test_api_documents_trashbin.py b/src/backend/core/tests/documents/test_api_documents_trashbin.py index 7b90eeeb9..d7b61266f 100644 --- a/src/backend/core/tests/documents/test_api_documents_trashbin.py +++ b/src/backend/core/tests/documents/test_api_documents_trashbin.py @@ -96,6 +96,7 @@ def test_api_documents_trashbin_format(): }, "content_patch": False, "content_retrieve": True, + "leave": False, "media_auth": False, "media_check": False, "move": False, # Can't move a deleted document diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index b1b12aece..e9405ce38 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -173,6 +173,7 @@ def test_models_documents_get_abilities_forbidden( "invite_owner": False, "content_patch": False, "content_retrieve": False, + "leave": False, "media_auth": False, "media_check": False, "move": False, @@ -192,7 +193,7 @@ def test_models_documents_get_abilities_forbidden( "versions_retrieve": False, "search": False, } - nb_queries = 1 if is_authenticated else 0 + nb_queries = 2 if is_authenticated else 0 with django_assert_num_queries(nb_queries): assert document.get_abilities(user) == expected_abilities document.soft_delete() @@ -247,6 +248,7 @@ def test_models_documents_get_abilities_reader( }, "content_patch": False, "content_retrieve": True, + "leave": False, "media_auth": True, "media_check": True, "move": False, @@ -260,7 +262,7 @@ def test_models_documents_get_abilities_reader( "versions_retrieve": False, "search": True, } - nb_queries = 1 if is_authenticated else 0 + nb_queries = 2 if is_authenticated else 0 with django_assert_num_queries(nb_queries): assert document.get_abilities(user) == expected_abilities @@ -320,6 +322,7 @@ def test_models_documents_get_abilities_commenter( }, "content_patch": False, "content_retrieve": True, + "leave": False, "media_auth": True, "media_check": True, "move": False, @@ -333,7 +336,7 @@ def test_models_documents_get_abilities_commenter( "versions_retrieve": False, "search": True, } - nb_queries = 1 if is_authenticated else 0 + nb_queries = 2 if is_authenticated else 0 with django_assert_num_queries(nb_queries): assert document.get_abilities(user) == expected_abilities @@ -390,6 +393,7 @@ def test_models_documents_get_abilities_editor( }, "content_patch": True, "content_retrieve": True, + "leave": False, "media_auth": True, "media_check": True, "move": False, @@ -403,7 +407,7 @@ def test_models_documents_get_abilities_editor( "versions_retrieve": False, "search": True, } - nb_queries = 1 if is_authenticated else 0 + nb_queries = 2 if is_authenticated else 0 with django_assert_num_queries(nb_queries): assert document.get_abilities(user) == expected_abilities document.soft_delete() @@ -449,6 +453,7 @@ def test_models_documents_get_abilities_owner(django_assert_num_queries): }, "content_patch": True, "content_retrieve": True, + "leave": False, "media_auth": True, "media_check": True, "move": True, @@ -494,6 +499,7 @@ def test_models_documents_get_abilities_owner(django_assert_num_queries): }, "content_patch": False, "content_retrieve": True, + "leave": False, "media_auth": False, "media_check": False, "move": False, @@ -543,6 +549,7 @@ def test_models_documents_get_abilities_administrator(django_assert_num_queries) }, "content_patch": True, "content_retrieve": True, + "leave": False, "media_auth": True, "media_check": True, "move": True, @@ -602,6 +609,7 @@ def test_models_documents_get_abilities_editor_user(django_assert_num_queries): }, "content_patch": True, "content_retrieve": True, + "leave": True, "media_auth": True, "media_check": True, "move": False, @@ -669,6 +677,7 @@ def test_models_documents_get_abilities_reader_user( }, "content_patch": access_from_link, "content_retrieve": True, + "leave": True, "media_auth": True, "media_check": True, "move": False, @@ -737,6 +746,7 @@ def test_models_documents_get_abilities_commenter_user( }, "content_patch": access_from_link, "content_retrieve": True, + "leave": True, "media_auth": True, "media_check": True, "move": False, @@ -801,6 +811,7 @@ def test_models_documents_get_abilities_preset_role(django_assert_num_queries): }, "content_patch": False, "content_retrieve": True, + "leave": True, "media_auth": True, "media_check": True, "move": False,