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

Fix merged dataset permissions not applied on share #2598

Merged
merged 7 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion onadata/apps/api/tests/viewsets/test_data_viewset.py
Original file line number Diff line number Diff line change
Expand Up @@ -3721,7 +3721,7 @@ def setUp(self):
self.logger = logging.getLogger("console_logger")

# pylint: disable=invalid-name,too-many-locals
@flaky(max_runs=5)
@flaky(max_runs=8)
def test_data_retrieve_instance_osm_format(self):
"""Test /data endpoint OSM format."""
filenames = [
Expand Down
2 changes: 2 additions & 0 deletions onadata/apps/api/tests/viewsets/test_xform_viewset.py
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,7 @@ def setUp(self):
)

@patch("onadata.apps.api.viewsets.xform_viewset.send_message")
@flaky
def test_replace_form_with_external_choices(self, mock_send_message):
with HTTMock(enketo_mock):
xls_file_path = os.path.join(
Expand Down Expand Up @@ -2283,6 +2284,7 @@ def test_form_clone_shared_forms(self):
self.assertEqual(response.status_code, 201)
self.assertEqual(count + 1, XForm.objects.count())

@flaky
def test_return_error_on_clone_duplicate(self):
with HTTMock(enketo_mock):
self._publish_xls_form_to_project()
Expand Down
2 changes: 2 additions & 0 deletions onadata/apps/logger/tests/test_briefcase_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import requests
import requests_mock
from django_digest.test import Client as DigestClient
from flaky import flaky
from six.moves.urllib.parse import urljoin

from onadata.apps.logger.models import Instance, XForm
Expand Down Expand Up @@ -168,6 +169,7 @@ def _download_submissions(self):
mocker.head(requests_mock.ANY, content=submission_list)
self.briefcase_client.download_instances(self.xform.id_string)

@flaky(max_runs=8)
def test_download_xform_xml(self):
"""
Download xform via briefcase api
Expand Down
7 changes: 7 additions & 0 deletions onadata/libs/models/share_project.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ def remove_xform_permissions(project, user, role):
for xform in project.xform_set.all():
# pylint: disable=protected-access
role._remove_obj_permissions(user, xform)
# Removed MergedXForm permissions if XForm is also a MergedXForm
if hasattr(xform, "mergedxform"):
role._remove_obj_permissions(user, xform.mergedxform)


def remove_dataview_permissions(project, user, role):
Expand Down Expand Up @@ -85,6 +88,10 @@ def save(self, **kwargs):
role = ROLES.get(meta_perm[1])
role.add(self.user, xform)

# Set MergedXForm permissions if XForm is also a MergedXForm
if hasattr(xform, "mergedxform"):
role.add(self.user, xform.mergedxform)

for dataview in self.project.dataview_set.all():
if dataview.matches_parent:
role.add(self.user, dataview.xform)
Expand Down
46 changes: 41 additions & 5 deletions onadata/libs/tests/models/test_share_project.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
"""Tests for module onadata.libs.models.share_project"""

from unittest.mock import patch, call
from pyxform.builder import create_survey_element_from_dict

from onadata.apps.logger.models.data_view import DataView
from onadata.apps.logger.models.project import Project
from onadata.apps.logger.models.merged_xform import MergedXForm
from onadata.apps.logger.models.xform import XForm
from onadata.apps.main.tests.test_base import TestBase
from onadata.libs.models.share_project import ShareProject
Expand Down Expand Up @@ -35,7 +37,7 @@ def setUp(self):
project = Project.objects.create(
name="Demo", organization=self.user, created_by=self.user
)
self._publish_markdown(md_xform, self.user, project)
self._publish_markdown(md_xform, self.user, project, id_string="a")
self.dataview_form = XForm.objects.all().order_by("-pk")[0]
DataView.objects.create(
name="Demo",
Expand All @@ -44,20 +46,43 @@ def setUp(self):
matches_parent=True,
columns=[],
)
# MergedXForm
self._publish_markdown(md_xform, self.user, project, id_string="b")
xf1 = XForm.objects.get(id_string="a")
xf2 = XForm.objects.get(id_string="b")
survey = create_survey_element_from_dict(xf1.json_dict())
survey["id_string"] = "c"
survey["sms_keyword"] = survey["id_string"]
survey["title"] = "Merged XForm"
self.merged_xf = MergedXForm.objects.create(
id_string=survey["id_string"],
sms_id_string=survey["id_string"],
title=survey["title"],
user=self.user,
created_by=self.user,
is_merged_dataset=True,
project=self.project,
xml=survey.to_xml(),
json=survey.to_json(),
)
self.merged_xf.xforms.add(xf1)
self.merged_xf.xforms.add(xf2)
self.alice = self._create_user("alice", "Yuao8(-)")

@patch("onadata.libs.models.share_project.safe_delete")
def test_share(self, mock_safe_delete, mock_propagate):
"""A project is shared with a user

Permissions assigned to project, xform and dataview
Permissions assigned to project, xform, mergedxform and dataview
"""
instance = ShareProject(self.project, self.alice, "manager")
instance.save()
self.alice.refresh_from_db()
self.assertTrue(ManagerRole.user_has_role(self.alice, self.project))
self.assertTrue(ManagerRole.user_has_role(self.alice, self.xform))
self.assertTrue(ManagerRole.user_has_role(self.alice, self.dataview_form))
self.assertTrue(ManagerRole.user_has_role(self.alice, self.merged_xf))
self.assertTrue(ManagerRole.user_has_role(self.alice, self.merged_xf.xform_ptr))
mock_propagate.assert_called_once_with(args=[self.project.pk])
# Cache is invalidated
mock_safe_delete.assert_has_calls(
Expand All @@ -69,21 +94,32 @@ def test_share(self, mock_safe_delete, mock_propagate):

@patch("onadata.libs.models.share_project.safe_delete")
def test_remove(self, mock_safe_delete, mock_propagate):
"""A user is removed from a project"""
# Add user
"""A user is removed from a project

Permissions removed from project, xform, mergedxform and dataview
"""
# Simulate share project
ManagerRole.add(self.alice, self.project)
ManagerRole.add(self.alice, self.xform)
ManagerRole.add(self.alice, self.dataview_form)

ManagerRole.add(self.alice, self.merged_xf)
ManagerRole.add(self.alice, self.merged_xf.xform_ptr)
# Confirm project shared
self.assertTrue(ManagerRole.user_has_role(self.alice, self.project))
self.assertTrue(ManagerRole.user_has_role(self.alice, self.xform))
self.assertTrue(ManagerRole.user_has_role(self.alice, self.dataview_form))
self.assertTrue(ManagerRole.user_has_role(self.alice, self.merged_xf))
self.assertTrue(ManagerRole.user_has_role(self.alice, self.merged_xf.xform_ptr))
# Remove user
instance = ShareProject(self.project, self.alice, "manager", True)
instance.save()
self.assertFalse(ManagerRole.user_has_role(self.alice, self.project))
self.assertFalse(ManagerRole.user_has_role(self.alice, self.xform))
self.assertFalse(ManagerRole.user_has_role(self.alice, self.dataview_form))
self.assertFalse(ManagerRole.user_has_role(self.alice, self.merged_xf))
self.assertFalse(
ManagerRole.user_has_role(self.alice, self.merged_xf.xform_ptr)
)
mock_propagate.assert_called_once_with(args=[self.project.pk])
# Cache is invalidated
mock_safe_delete.assert_has_calls(
Expand Down
Loading