Skip to content

Commit

Permalink
Fix Cadasta#642: anonymous users can view full project list (Cadasta#681
Browse files Browse the repository at this point in the history
)

There are two parts to this:

1. The test is a bit funny, because it didn't create any projects.  When
   there are no objects to test permissions against, tutelary pretends
   that there's a `None` object, and that gives a 403.  When there are
   project instances around, the permissions checking works and you get
   a 200 and a project list as the result.  I've split the test into "no
   projects" and "has projects" cases.

2. The `users` field appearing in the project list results came from a
   problem with the `DetailSerializer` class.  This serializer mixin is
   intended to remove specific fields from model instance serialization
   results in "non-detail" views.  For a view that returns a list of
   objects of a model class that has a `DetailSerializer` mixin,
   detail-only fields (specified by a `Meta` option) are removed from
   each instance in the serializer result.  However, if the model class
   has a *member* that also has a `DetailSerializer` mixin, the
   detail-only fields of the object contained in each member of the
   result list *aren't* removed.  The effect of this is that detail-only
   fields appear in nested objects in the serializer results.  In this
   case, the user list is a detail-only field.  To fix this, I've added
   an extra `hide_detail` keyword argument for `DetailSerializer` that
   can be used to force the mixin to hide detail-only fields in contexts
   where it normally wouldn't do so.  Adding that to the place in
   `ProjectSerializer` where the serializer for the organization is
   defined prevents the user list from appearing in the serializer
   results.
  • Loading branch information
ian-ross authored and oliverroick committed Sep 13, 2016
1 parent 1b5e565 commit e1b1155
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 3 deletions.
3 changes: 2 additions & 1 deletion cadasta/core/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@
class DetailSerializer:
def __init__(self, *args, **kwargs):
detail = kwargs.pop('detail', False)
hide_detail = kwargs.pop('hide_detail', False)
super(DetailSerializer, self).__init__(*args, **kwargs)

is_list = type(self.instance) in [list, QuerySet]
if is_list and not detail:
if hide_detail or (is_list and not detail):
for field_name in self.Meta.detail_only_fields:
self.fields.pop(field_name)

Expand Down
2 changes: 1 addition & 1 deletion cadasta/organization/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def create(self, *args, **kwargs):

class ProjectSerializer(DetailSerializer, serializers.ModelSerializer):
users = UserSerializer(many=True, read_only=True)
organization = OrganizationSerializer(read_only=True)
organization = OrganizationSerializer(hide_detail=True, read_only=True)
country = CountryField(required=False)

def validate_name(self, value):
Expand Down
12 changes: 12 additions & 0 deletions cadasta/organization/tests/test_views_api_projects.py
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,18 @@ def test_full_list_with_superuser(self):
assert len(response.content) == 4

def test_full_list_with_unauthorized_user(self):
"""
It should return projects without member information.
"""
ProjectFactory.create_batch(2, organization=self.organization)
ProjectFactory.create_batch(2)
response = self.request()
assert response.status_code == 200
assert len(response.content) == 4
assert all(['users' not in proj['organization']
for proj in response.content])

def test_empty_list_with_unauthorized_user(self):
"""
It should 403 "You do not have permission to perform this action."
"""
Expand Down
2 changes: 1 addition & 1 deletion cadasta/organization/views/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ class ProjectList(APIPermissionRequiredMixin, mixins.ProjectQuerySetMixin,
filter_fields = ('archived',)
search_fields = ('name', 'organization__name', 'country', 'description',)
ordering_fields = ('name', 'organization', 'country', 'description',)
permission_required = {'project.list'}
permission_required = 'project.list'


class ProjectDetail(APIPermissionRequiredMixin,
Expand Down

0 comments on commit e1b1155

Please sign in to comment.