fix: do not call teams.all() on anonymous user

In the same move, refactor the can_edit/_view/_delete functions to
only take the request, which is what really happen in the code, and
adapt the test in that way.

umap/decorators.py

@@ -33,12 +33,11 @@ def can_edit_map(view_func):
     @wraps(view_func)
     def wrapper(request, *args, **kwargs):
         map_inst = get_object_or_404(Map, pk=kwargs["map_id"])
-        user = request.user
         kwargs["map_inst"] = map_inst  # Avoid rerequesting the map in the view
         if map_inst.edit_status >= map_inst.COLLABORATORS:
-            can_edit = map_inst.can_edit(user=user, request=request)
+            can_edit = map_inst.can_edit(request=request)
             if not can_edit:
-                if map_inst.owner and not user.is_authenticated:
+                if map_inst.owner and not request.user.is_authenticated:
                     return simple_json_response(login_required=str(LOGIN_URL))
                 return HttpResponseForbidden()
         return view_func(request, *args, **kwargs)

umap/models.py

@@ -281,8 +281,10 @@ class Map(NamedModel):
     def get_author(self):
         return self.team or self.owner
 
-    def is_owner(self, user=None, request=None):
-        if user and self.owner == user:
+    def is_owner(self, request=None):
+        if not request:
+            return False
+        if request.user and self.owner == request.user:
             return True
         return self.is_anonymous_owner(request)
 
@@ -297,14 +299,16 @@ class Map(NamedModel):
             has_anonymous_cookie = False
         return has_anonymous_cookie
 
-    def can_delete(self, user=None, request=None):
-        if self.owner and user != self.owner:
+    def can_delete(self, request=None):
+        if not request:
+            return False
+        if self.owner and request.user != self.owner:
             return False
         if not self.owner and not self.is_anonymous_owner(request):
             return False
         return True
 
-    def can_edit(self, user=None, request=None):
+    def can_edit(self, request=None):
         """
         Define if a user can edit or not the instance, according to his account
         or the request.
@@ -318,12 +322,18 @@ class Map(NamedModel):
             - anyone otherwise (ANONYMOUS)
         """
         can = False
-        if request and not self.owner:
-            if settings.UMAP_ALLOW_ANONYMOUS and self.is_anonymous_owner(request):
+        if not request:
+            return False
+        user = request.user
+        if (
+            not self.owner
+            and settings.UMAP_ALLOW_ANONYMOUS
+            and self.is_anonymous_owner(request)
+        ):
             can = True
-        if self.edit_status == self.ANONYMOUS:
+        elif self.edit_status == self.ANONYMOUS:
             can = True
-        elif user is None:
+        elif not user.is_authenticated:
             can = False
         elif user == self.owner:
             can = True
@@ -477,7 +487,7 @@ class DataLayer(NamedModel):
         path.append(str(self.map.pk))
         return os.path.join(*path)
 
-    def metadata(self, user=None, request=None):
+    def metadata(self, request=None):
         # Retrocompat: minimal settings for maps not saved after settings property
         # has been introduced
         obj = self.settings or {
@@ -488,7 +498,7 @@ class DataLayer(NamedModel):
             obj["old_id"] = self.old_id
         obj["id"] = self.pk
         obj["permissions"] = {"edit_status": self.edit_status}
-        obj["editMode"] = "advanced" if self.can_edit(user, request) else "disabled"
+        obj["editMode"] = "advanced" if self.can_edit(request) else "disabled"
         return obj
 
     def clone(self, map_inst=None):
@@ -557,22 +567,25 @@ class DataLayer(NamedModel):
             if name.startswith(prefixes) and name.endswith(".gz"):
                 self.geojson.storage.delete(os.path.join(root, name))
 
-    def can_edit(self, user=None, request=None):
+    def can_edit(self, request=None):
         """
         Define if a user can edit or not the instance, according to his account
         or the request.
         """
         if self.edit_status == self.INHERIT:
-            return self.map.can_edit(user, request)
+            return self.map.can_edit(request)
         can = False
+        if not request:
+            return False
+        user = request.user
         if not self.map.owner:
             if settings.UMAP_ALLOW_ANONYMOUS and self.map.is_anonymous_owner(request):
                 can = True
         if self.edit_status == self.ANONYMOUS:
             can = True
-        elif user is not None and user == self.map.owner:
+        elif user.is_authenticated and user == self.map.owner:
             can = True
-        elif user is not None and self.edit_status == self.COLLABORATORS:
+        elif user.is_authenticated and self.edit_status == self.COLLABORATORS:
             if user in self.map.editors.all() or self.map.team in user.teams.all():
                 can = True
         return can

umap/templatetags/umap_tags.py

@@ -45,7 +45,7 @@ def tilelayer_preview(tilelayer):
 
 @register.filter
 def can_delete_map(map, request):
-    return map.can_delete(request.user, request)
+    return map.can_delete(request)
 
 
 @register.filter

umap/tests/conftest.py

@@ -90,3 +90,8 @@ def datalayer(map):
 @pytest.fixture
 def tilelayer():
     return TileLayerFactory()
+
+
+@pytest.fixture
+def fake_request(rf):
+    return rf.get("/")

umap/tests/test_datalayer.py

@@ -1,7 +1,7 @@
-import os
 from pathlib import Path
 
 import pytest
+from django.contrib.auth.models import AnonymousUser
 from django.core.files.base import ContentFile
 
 from umap.models import DataLayer, Map
@@ -100,28 +100,33 @@ def test_should_remove_old_versions_on_save(map, settings):
     assert names == [Path(datalayer.geojson.name).name, newer, medium]
 
 
-def test_anonymous_cannot_edit_in_editors_mode(datalayer):
+def test_anonymous_cannot_edit_in_editors_mode(datalayer, fake_request):
     datalayer.edit_status = DataLayer.COLLABORATORS
     datalayer.save()
-    assert not datalayer.can_edit()
+    fake_request.user = AnonymousUser()
+    assert not datalayer.can_edit(fake_request)
 
 
-def test_owner_can_edit_in_editors_mode(datalayer, user):
+def test_owner_can_edit_in_editors_mode(datalayer, user, fake_request):
     datalayer.edit_status = DataLayer.COLLABORATORS
     datalayer.save()
-    assert datalayer.can_edit(datalayer.map.owner)
+    fake_request.user = datalayer.map.owner
+    assert datalayer.can_edit(fake_request)
 
 
-def test_editor_can_edit_in_collaborators_mode(datalayer, user):
+def test_editor_can_edit_in_collaborators_mode(datalayer, user, fake_request):
     map = datalayer.map
     map.editors.add(user)
     map.save()
     datalayer.edit_status = DataLayer.COLLABORATORS
     datalayer.save()
-    assert datalayer.can_edit(user)
+    fake_request.user = user
+    assert datalayer.can_edit(fake_request)
 
 
-def test_team_members_can_edit_in_collaborators_mode(datalayer, user, team):
+def test_team_members_can_edit_in_collaborators_mode(
+    datalayer, user, team, fake_request
+):
     user.teams.add(team)
     user.save()
     map = datalayer.map
@@ -129,60 +134,69 @@ def test_team_members_can_edit_in_collaborators_mode(datalayer, user, team):
     map.save()
     datalayer.edit_status = DataLayer.COLLABORATORS
     datalayer.save()
-    assert datalayer.can_edit(user)
+    fake_request.user = user
+    assert datalayer.can_edit(fake_request)
 
 
-def test_anonymous_can_edit_in_public_mode(datalayer):
+def test_anonymous_can_edit_in_public_mode(datalayer, fake_request):
     datalayer.edit_status = DataLayer.ANONYMOUS
     datalayer.save()
-    assert datalayer.can_edit()
+    fake_request.user = AnonymousUser()
+    assert datalayer.can_edit(fake_request)
 
 
-def test_owner_can_edit_in_public_mode(datalayer, user):
+def test_owner_can_edit_in_public_mode(datalayer, user, fake_request):
     datalayer.edit_status = DataLayer.ANONYMOUS
     datalayer.save()
-    assert datalayer.can_edit(datalayer.map.owner)
+    fake_request.user = datalayer.map.owner
+    assert datalayer.can_edit(fake_request)
 
 
-def test_editor_can_edit_in_public_mode(datalayer, user):
+def test_editor_can_edit_in_public_mode(datalayer, user, fake_request):
     map = datalayer.map
     map.editors.add(user)
     map.save()
     datalayer.edit_status = DataLayer.ANONYMOUS
     datalayer.save()
-    assert datalayer.can_edit(user)
+    fake_request.user = user
+    assert datalayer.can_edit(fake_request)
 
 
-def test_anonymous_cannot_edit_in_anonymous_owner_mode(datalayer):
+def test_anonymous_cannot_edit_in_anonymous_owner_mode(datalayer, fake_request):
     datalayer.edit_status = DataLayer.OWNER
     datalayer.save()
     map = datalayer.map
     map.owner = None
     map.save()
-    assert not datalayer.can_edit()
+    fake_request.user = AnonymousUser()
+    assert not datalayer.can_edit(fake_request)
 
 
-def test_owner_can_edit_in_inherit_mode_and_map_in_owner_mode(datalayer):
+def test_owner_can_edit_in_inherit_mode_and_map_in_owner_mode(datalayer, fake_request):
     datalayer.edit_status = DataLayer.INHERIT
     datalayer.save()
     map = datalayer.map
     map.edit_status = Map.OWNER
     map.save()
-    assert datalayer.can_edit(map.owner)
+    fake_request.user = map.owner
+    assert datalayer.can_edit(fake_request)
 
 
-def test_editors_cannot_edit_in_inherit_mode_and_map_in_owner_mode(datalayer, user):
+def test_editors_cannot_edit_in_inherit_mode_and_map_in_owner_mode(
+    datalayer, user, fake_request
+):
     datalayer.edit_status = DataLayer.INHERIT
     datalayer.save()
     map = datalayer.map
     map.editors.add(user)
     map.edit_status = Map.OWNER
     map.save()
-    assert not datalayer.can_edit(user)
+    fake_request.user = user
+    assert not datalayer.can_edit(fake_request)
 
 
 def test_team_members_cannot_edit_in_inherit_mode_and_map_in_owner_mode(
-    datalayer, user, team
+    datalayer, user, team, fake_request
 ):
     datalayer.edit_status = DataLayer.INHERIT
     datalayer.save()
@@ -192,50 +206,66 @@ def test_team_members_cannot_edit_in_inherit_mode_and_map_in_owner_mode(
     map.team = team
     map.edit_status = Map.OWNER
     map.save()
-    assert not datalayer.can_edit(user)
+    fake_request.user = user
+    assert not datalayer.can_edit(fake_request)
 
 
-def test_anonymous_cannot_edit_in_inherit_mode_and_map_in_owner_mode(datalayer):
+def test_anonymous_cannot_edit_in_inherit_mode_and_map_in_owner_mode(
+    datalayer, fake_request
+):
     datalayer.edit_status = DataLayer.INHERIT
     datalayer.save()
     map = datalayer.map
     map.edit_status = Map.OWNER
     map.save()
-    assert not datalayer.can_edit()
+    fake_request.user = AnonymousUser()
+    assert not datalayer.can_edit(fake_request)
 
 
-def test_owner_can_edit_in_inherit_mode_and_map_in_editors_mode(datalayer):
+def test_owner_can_edit_in_inherit_mode_and_map_in_editors_mode(
+    datalayer, fake_request
+):
     datalayer.edit_status = DataLayer.INHERIT
     datalayer.save()
     map = datalayer.map
     map.edit_status = Map.COLLABORATORS
     map.save()
-    assert datalayer.can_edit(map.owner)
+    fake_request.user = map.owner
+    assert datalayer.can_edit(fake_request)
 
 
-def test_editors_can_edit_in_inherit_mode_and_map_in_editors_mode(datalayer, user):
+def test_editors_can_edit_in_inherit_mode_and_map_in_editors_mode(
+    datalayer, user, fake_request
+):
     datalayer.edit_status = DataLayer.INHERIT
     datalayer.save()
     map = datalayer.map
     map.editors.add(user)
     map.edit_status = Map.COLLABORATORS
     map.save()
-    assert datalayer.can_edit(user)
+    fake_request.user = user
+    assert datalayer.can_edit(fake_request)
 
 
-def test_anonymous_cannot_edit_in_inherit_mode_and_map_in_editors_mode(datalayer):
+def test_anonymous_cannot_edit_in_inherit_mode_and_map_in_editors_mode(
+    datalayer, fake_request
+):
     datalayer.edit_status = DataLayer.INHERIT
     datalayer.save()
     map = datalayer.map
     map.edit_status = Map.COLLABORATORS
     map.save()
-    assert not datalayer.can_edit()
+    fake_request.user = AnonymousUser()
+    assert not datalayer.can_edit(fake_request)
 
 
-def test_anonymous_can_edit_in_inherit_mode_and_map_in_public_mode(datalayer):
+def test_anonymous_can_edit_in_inherit_mode_and_map_in_public_mode(
+    datalayer, fake_request
+):
     datalayer.edit_status = DataLayer.INHERIT
     datalayer.save()
     map = datalayer.map
     map.edit_status = Map.ANONYMOUS
     map.save()
-    assert datalayer.can_edit()
+    fake_request.user = AnonymousUser()
+    assert datalayer.can_edit(fake_request)

umap/tests/test_map.py

@@ -9,63 +9,69 @@ from .base import MapFactory
 pytestmark = pytest.mark.django_db
 
 
-def test_anonymous_can_edit_if_status_anonymous(map):
-    anonymous = AnonymousUser()
+def test_anonymous_can_edit_if_status_anonymous(map, fake_request):
     map.edit_status = map.ANONYMOUS
     map.save()
-    assert map.can_edit(anonymous)
+    fake_request.user = AnonymousUser()
+    assert map.can_edit(fake_request)
 
 
-def test_anonymous_cannot_edit_if_not_status_anonymous(map):
-    anonymous = AnonymousUser()
+def test_anonymous_cannot_edit_if_not_status_anonymous(map, fake_request):
     map.edit_status = map.OWNER
     map.save()
-    assert not map.can_edit(anonymous)
+    fake_request.user = AnonymousUser()
+    assert not map.can_edit(fake_request)
 
 
-def test_non_editors_can_edit_if_status_anonymous(map, user):
+def test_non_editors_can_edit_if_status_anonymous(map, user, fake_request):
     assert map.owner != user
     map.edit_status = map.ANONYMOUS
     map.save()
-    assert map.can_edit(user)
+    fake_request.user = user
+    assert map.can_edit(fake_request)
 
 
-def test_non_editors_cannot_edit_if_not_status_anonymous(map, user):
+def test_non_editors_cannot_edit_if_not_status_anonymous(map, user, fake_request):
     map.edit_status = map.OWNER
     map.save()
-    assert not map.can_edit(user)
+    fake_request.user = user
+    assert not map.can_edit(fake_request)
 
 
-def test_editors_cannot_edit_if_status_owner(map, user):
+def test_editors_cannot_edit_if_status_owner(map, user, fake_request):
     map.edit_status = map.OWNER
     map.editors.add(user)
     map.save()
-    assert not map.can_edit(user)
+    fake_request.user = user
+    assert not map.can_edit(fake_request)
 
 
-def test_editors_can_edit_if_status_collaborators(map, user):
+def test_editors_can_edit_if_status_collaborators(map, user, fake_request):
     map.edit_status = map.COLLABORATORS
     map.editors.add(user)
     map.save()
-    assert map.can_edit(user)
+    fake_request.user = user
+    assert map.can_edit(fake_request)
 
 
-def test_team_members_cannot_edit_if_status_owner(map, user, team):
+def test_team_members_cannot_edit_if_status_owner(map, user, team, fake_request):
     user.teams.add(team)
     user.save()
     map.edit_status = map.OWNER
     map.team = team
     map.save()
-    assert not map.can_edit(user)
+    fake_request.user = user
+    assert not map.can_edit(fake_request)
 
 
-def test_team_members_can_edit_if_status_collaborators(map, user, team):
+def test_team_members_can_edit_if_status_collaborators(map, user, team, fake_request):
     user.teams.add(team)
     user.save()
     map.edit_status = map.COLLABORATORS
     map.team = team
     map.save()
-    assert map.can_edit(user)
+    fake_request.user = user
+    assert map.can_edit(fake_request)
 
 
 def test_logged_in_user_should_be_allowed_for_anonymous_map_with_anonymous_edit_status(
@@ -77,14 +83,17 @@ def test_logged_in_user_should_be_allowed_for_anonymous_map_with_anonymous_edit_
     url = reverse("map_update", kwargs={"map_id": map.pk})
     request = rf.get(url)
     request.user = user
-    assert map.can_edit(user, request)
+    assert map.can_edit(request)
 
 
-def test_anonymous_user_should_not_be_allowed_for_anonymous_map(map, user, rf):  # noqa
+def test_anonymous_user_should_not_be_allowed_for_anonymous_map(
+    map, user, fake_request
+):
     map.owner = None
     map.edit_status = map.OWNER
     map.save()
-    assert not map.can_edit()
+    fake_request.user = AnonymousUser()
+    assert not map.can_edit(fake_request)
 
 
 def test_clone_should_return_new_instance(map, user):

umap/views.py

@@ -546,7 +546,7 @@ class SessionMixin:
         data = {}
         user = self.request.user
         if hasattr(self, "object"):
-            data["is_owner"] = self.object.is_owner(user, self.request)
+            data["is_owner"] = self.object.is_owner(self.request)
         if user.is_anonymous:
             return data
         return {
@@ -725,20 +725,14 @@ class MapView(MapDetailMixin, PermissionsMixin, DetailView):
         return self.object.get_absolute_url()
 
     def get_datalayers(self):
-        return [
-            dl.metadata(self.request.user, self.request)
-            for dl in self.object.datalayer_set.all()
-        ]
+        return [dl.metadata(self.request) for dl in self.object.datalayer_set.all()]
 
     @property
     def edit_mode(self):
         edit_mode = "disabled"
-        if self.object.can_edit(self.request.user, self.request):
+        if self.object.can_edit(self.request):
             edit_mode = "advanced"
-        elif any(
-            d.can_edit(self.request.user, self.request)
-            for d in self.object.datalayer_set.all()
-        ):
+        elif any(d.can_edit(self.request) for d in self.object.datalayer_set.all()):
             edit_mode = "simple"
         return edit_mode
 
@@ -901,7 +895,7 @@ def get_websocket_auth_token(request, map_id, map_inst):
     map_object: Map = Map.objects.get(pk=map_id)
 
     permissions = ["edit"]
-    if map_object.is_owner(request.user, request):
+    if map_object.is_owner(request):
         permissions.append("owner")
 
     if request.user.is_authenticated:
@@ -959,7 +953,7 @@ class AttachAnonymousMap(View):
         if (
             self.object.owner
             or not self.object.is_anonymous_owner(self.request)
-            or not self.object.can_edit(self.request.user, self.request)
+            or not self.object.can_edit(self.request)
             or not self.request.user.is_authenticated
         ):
             return HttpResponseForbidden()
@@ -976,7 +970,7 @@ class SendEditLink(FormLessEditMixin, FormView):
         if (
             self.object.owner
             or not self.object.is_anonymous_owner(self.request)
-            or not self.object.can_edit(self.request.user, self.request)
+            or not self.object.can_edit(self.request)
         ):
             return HttpResponseForbidden()
         form = self.get_form()
@@ -1009,7 +1003,7 @@ class MapDelete(DeleteView):
 
     def form_valid(self, form):
         self.object = self.get_object()
-        if not self.object.can_delete(self.request.user, self.request):
+        if not self.object.can_delete(self.request):
             return HttpResponseForbidden(_("Only its owner can delete the map."))
         self.object.delete()
         home_url = reverse("home")
@@ -1186,9 +1180,7 @@ class DataLayerCreate(FormLessEditMixin, GZipMixin, CreateView):
         form.instance.map = self.kwargs["map_inst"]
         self.object = form.save()
         # Simple response with only metadata (including new id)
-        response = simple_json_response(
-            **self.object.metadata(self.request.user, self.request)
-        )
+        response = simple_json_response(**self.object.metadata(self.request))
         response["X-Datalayer-Version"] = self.version
         return response
 
@@ -1242,7 +1234,7 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView):
         if self.object.map.pk != int(self.kwargs["map_id"]):
             return HttpResponseForbidden()
 
-        if not self.object.can_edit(user=self.request.user, request=self.request):
+        if not self.object.can_edit(request=self.request):
             return HttpResponseForbidden()
 
         reference_version = self.request.headers.get("X-Datalayer-Reference")
@@ -1262,7 +1254,7 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView):
 
     def form_valid(self, form):
         self.object = form.save()
-        data = {**self.object.metadata(self.request.user, self.request)}
+        data = {**self.object.metadata(self.request)}
         if self.request.session.get("needs_reload"):
             data["geojson"] = json.loads(self.object.geojson.read().decode())
             self.request.session["needs_reload"] = False