From 918eb6e85436a342bccd8df7dedb76202953cf53 Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Wed, 20 Oct 2021 10:45:16 -0400 Subject: [PATCH 1/2] Send Digest Challenge response to anonymous user when fetching attachments --- onadata/apps/api/viewsets/briefcase_api.py | 19 +++++++++++--- onadata/apps/viewer/views.py | 16 +++++++++++- onadata/libs/utils/user_auth.py | 29 ++++++++++++++-------- 3 files changed, 50 insertions(+), 14 deletions(-) diff --git a/onadata/apps/api/viewsets/briefcase_api.py b/onadata/apps/api/viewsets/briefcase_api.py index e4998646d..b636ce100 100644 --- a/onadata/apps/api/viewsets/briefcase_api.py +++ b/onadata/apps/api/viewsets/briefcase_api.py @@ -1,11 +1,13 @@ # coding: utf-8 +from xml.dom import NotFoundErr + +from django.conf import settings from django.core.files import File from django.core.validators import ValidationError from django.contrib.auth.models import User from django.http import Http404 from django.utils.translation import ugettext as _ from django.utils import six - from rest_framework import exceptions from rest_framework import mixins from rest_framework import status @@ -96,8 +98,9 @@ def __init__(self, *args, **kwargs): DigestAuthentication, ] self.authentication_classes = authentication_classes + [ - auth_class for auth_class in self.authentication_classes - if not auth_class in authentication_classes + auth_class + for auth_class in self.authentication_classes + if auth_class not in authentication_classes ] def get_object(self): @@ -222,6 +225,16 @@ def retrieve(self, request, *args, **kwargs): submission_xml_root_node.setAttribute( 'submissionDate', self.object.date_created.isoformat() ) + + # Added this because of https://github.com/onaio/onadata/pull/2139 + # Should bring support to ODK v1.17+ + if getattr(settings, 'SUPPORT_BRIEFCASE_SUBMISSION_DATE', True): + # Remove namespace attribute if any + try: + submission_xml_root_node.removeAttribute('xmlns') + except NotFoundErr: + pass + data = { 'submission_data': submission_xml_root_node.toxml(), 'media_files': Attachment.objects.filter(instance=self.object), diff --git a/onadata/apps/viewer/views.py b/onadata/apps/viewer/views.py index a4d7650a7..1fca4a292 100644 --- a/onadata/apps/viewer/views.py +++ b/onadata/apps/viewer/views.py @@ -21,6 +21,7 @@ from django.utils.http import urlquote from django.utils.translation import ugettext as _ from django.views.decorators.http import require_POST +from django_digest import HttpDigestAuthenticator from rest_framework.settings import api_settings from onadata.apps.logger.models import XForm, Attachment @@ -416,6 +417,17 @@ def attachment_url(request, size='medium'): break if not has_permission(xform, xform.user, request): + # New versions of ODK Briefcase (1.16+) do not sent Digest + # authentication headers anymore directly. So, if user does not + # pass `has_permission` and user is anonymous, we need to notify them + # that access is unauthorized (i.e.: send a HTTP 401) and give them + # a chance to authenticate. + if request.user.is_anonymous: + authenticator = HttpDigestAuthenticator() + if not authenticator.authenticate(request): + return authenticator.build_challenge_response() + + # Otherwise, return a HTTP 403 (access forbidden) return HttpResponseForbidden(_('Not shared.')) media_url = None @@ -426,7 +438,9 @@ def attachment_url(request, size='medium'): try: media_url = image_url(attachment, size) except: - media_file_logger.error('could not get thumbnail for image', exc_info=True) + media_file_logger.error( + 'could not get thumbnail for image', exc_info=True + ) if media_url: # We want nginx to serve the media (instead of redirecting the media itself) diff --git a/onadata/libs/utils/user_auth.py b/onadata/libs/utils/user_auth.py index ca59734dd..bfdfc7df1 100644 --- a/onadata/libs/utils/user_auth.py +++ b/onadata/libs/utils/user_auth.py @@ -65,24 +65,33 @@ def set_profile_data(data, content_user): def has_permission(xform, owner, request, shared=False): user = request.user - return shared or xform.shared_data or \ - (hasattr(request, 'session') and - request.session.get('public_link') == xform.uuid) or \ - owner == user or \ - user.has_perm('logger.' + CAN_VIEW_XFORM, xform) or \ - user.has_perm('logger.' + CAN_CHANGE_XFORM, xform) + return ( + shared + or xform.shared_data + or ( + hasattr(request, 'session') + and request.session.get('public_link') == xform.uuid + ) + or owner == user + or user.has_perm('logger.' + CAN_VIEW_XFORM, xform) + or user.has_perm('logger.' + CAN_CHANGE_XFORM, xform) + ) def has_delete_data_permission(xform, owner, request): user = request.user - return owner == user or \ - user.has_perm('logger.' + CAN_DELETE_DATA_XFORM, xform) + return ( + owner == user + or user.has_perm('logger.' + CAN_DELETE_DATA_XFORM, xform) + ) def has_edit_permission(xform, owner, request): user = request.user - return owner == user or \ - user.has_perm('logger.' + CAN_CHANGE_XFORM, xform) + return ( + owner == user + or user.has_perm('logger.' + CAN_CHANGE_XFORM, xform) + ) def check_and_set_user_and_form(username, id_string, request): From e6a31a1fdf2085f196e457583bea111a8d13b17f Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Wed, 20 Oct 2021 14:50:47 -0400 Subject: [PATCH 2/2] Add unit tests --- .../tests/viewsets/test_abstract_viewset.py | 38 ++++++++++---- .../api/tests/viewsets/test_briefcase_api.py | 50 +++++++++++++++++++ onadata/apps/api/viewsets/briefcase_api.py | 11 ++-- ...ansport_2011-07-25_19-05-36_with_xmlns.xml | 1 + ...ansport_2011-07-25_19-05-49_with_xmlns.xml | 1 + ...ansport_2011-07-25_19-06-01_with_xmlns.xml | 1 + ...ansport_2011-07-25_19-06-14_with_xmlns.xml | 1 + .../apps/viewer/tests/test_attachment_url.py | 15 ++++++ onadata/settings/base.py | 6 +++ 9 files changed, 110 insertions(+), 14 deletions(-) create mode 100755 onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-05-36/transport_2011-07-25_19-05-36_with_xmlns.xml create mode 100755 onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-05-49/transport_2011-07-25_19-05-49_with_xmlns.xml create mode 100755 onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-06-01/transport_2011-07-25_19-06-01_with_xmlns.xml create mode 100755 onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-06-14/transport_2011-07-25_19-06-14_with_xmlns.xml diff --git a/onadata/apps/api/tests/viewsets/test_abstract_viewset.py b/onadata/apps/api/tests/viewsets/test_abstract_viewset.py index ff57d407b..25313eb55 100644 --- a/onadata/apps/api/tests/viewsets/test_abstract_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_abstract_viewset.py @@ -240,18 +240,38 @@ def _make_submissions(self, username=None, add_uuid=False, self.assertEqual(xform.num_of_submissions, post_count) self.assertEqual(xform.user.profile.num_of_submissions, post_count) - def _submit_transport_instance_w_attachment(self, - survey_at=0, - media_file=None): - s = self.surveys[survey_at] + def _submit_transport_instance_w_attachment( + self, survey_at=0, media_file=None, with_namespace=False + ): + survey_datetime = self.surveys[survey_at] if not media_file: media_file = "1335783522563.jpg" - path = os.path.join(self.main_directory, 'fixtures', - 'transportation', 'instances', s, media_file) + path = os.path.join( + self.main_directory, + 'fixtures', + 'transportation', + 'instances', + survey_datetime, + media_file, + ) + with open(path, 'rb') as f: - self._make_submission(os.path.join( - self.main_directory, 'fixtures', - 'transportation', 'instances', s, s + '.xml'), media_file=f) + xml_filename = ( + f'{survey_datetime}_with_xmlns.xml' + if with_namespace + else f'{survey_datetime}.xml' + ) + self._make_submission( + os.path.join( + self.main_directory, + 'fixtures', + 'transportation', + 'instances', + survey_datetime, + xml_filename, + ), + media_file=f, + ) attachment = Attachment.objects.all().reverse()[0] self.attachment = attachment diff --git a/onadata/apps/api/tests/viewsets/test_briefcase_api.py b/onadata/apps/api/tests/viewsets/test_briefcase_api.py index f44704b94..8b9b973a9 100644 --- a/onadata/apps/api/tests/viewsets/test_briefcase_api.py +++ b/onadata/apps/api/tests/viewsets/test_briefcase_api.py @@ -3,6 +3,7 @@ from django.urls import reverse from django.core.files.storage import get_storage_class +from django.test import override_settings from django_digest.test import DigestAuth from rest_framework.test import APIRequestFactory @@ -225,6 +226,53 @@ def test_view_download_submission(self): self.assertContains(response, instance_id, status_code=200) self.assertMultiLineEqual(response.content.decode('utf-8'), text) + def test_view_download_submission_no_xmlns(self): + view = BriefcaseApi.as_view({'get': 'retrieve'}) + self._publish_xml_form() + self.maxDiff = None + self._submit_transport_instance_w_attachment(with_namespace=True) + instance_id = '5b2cc313-fc09-437e-8149-fcd32f695d41' + instance = Instance.objects.get(uuid=instance_id) + form_id = '%(formId)s[@version=null and @uiVersion=null]/' \ + '%(formId)s[@key=uuid:%(instanceId)s]' % { + 'formId': self.xform.id_string, + 'instanceId': instance_id} + params = {'formId': form_id} + auth = DigestAuth(self.login_username, self.login_password) + request = self.factory.get( + self._download_submission_url, data=params) + response = view(request, username=self.user.username) + self.assertEqual(response.status_code, 401) + request.META.update(auth(request.META, response)) + response = view(request, username=self.user.username) + download_submission_path = os.path.join( + self.main_directory, 'fixtures', 'transportation', + 'view', 'downloadSubmission.xml') + response.render() + self.assertContains(response, instance_id, status_code=200) + self.assertNotIn( + 'transportation id="transportation_2011_07_25" ' + 'xlmns="http://opendatakit.org/submission" ' + 'instanceID="uuid:5b2cc313-fc09-437e-8149-fcd32f695d41"' + f' submissionDate="{instance.date_created.isoformat()}" ', + response.content.decode() + ) + + with override_settings(SUPPORT_BRIEFCASE_SUBMISSION_DATE=False): + request = self.factory.get( + self._download_submission_url, data=params) + response = view(request, username=self.user.username) + request.META.update(auth(request.META, response)) + response = view(request, username=self.user.username) + response.render() + self.assertNotIn( + 'transportation id="transportation_2011_07_25" ' + 'xmlns="http://opendatakit.org/submission" ' + 'instanceID="uuid:5b2cc313-fc09-437e-8149-fcd32f695d41" ' + f'submissionDate="{instance.date_created.isoformat()}" ', + response.content.decode() + ) + def test_view_download_submission_other_user(self): view = BriefcaseApi.as_view({'get': 'retrieve'}) self._publish_xml_form() @@ -397,6 +445,8 @@ def test_submission_with_instance_id_on_root_node(self): self.assertContains(response, instanceId, status_code=201) self.assertEqual(Instance.objects.count(), count + 1) + + def tearDown(self): if self.user: delete_user_storage(self.user.username) diff --git a/onadata/apps/api/viewsets/briefcase_api.py b/onadata/apps/api/viewsets/briefcase_api.py index b636ce100..d5ff676c2 100644 --- a/onadata/apps/api/viewsets/briefcase_api.py +++ b/onadata/apps/api/viewsets/briefcase_api.py @@ -228,7 +228,7 @@ def retrieve(self, request, *args, **kwargs): # Added this because of https://github.com/onaio/onadata/pull/2139 # Should bring support to ODK v1.17+ - if getattr(settings, 'SUPPORT_BRIEFCASE_SUBMISSION_DATE', True): + if settings.SUPPORT_BRIEFCASE_SUBMISSION_DATE: # Remove namespace attribute if any try: submission_xml_root_node.removeAttribute('xmlns') @@ -241,10 +241,11 @@ def retrieve(self, request, *args, **kwargs): 'host': request.build_absolute_uri().replace( request.get_full_path(), '') } - return Response(data, - headers=self.get_openrosa_headers(request, - location=False), - template_name='downloadSubmission.xml') + return Response( + data, + headers=self.get_openrosa_headers(request, location=False), + template_name='downloadSubmission.xml', + ) @action(detail=True, methods=['GET']) def manifest(self, request, *args, **kwargs): diff --git a/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-05-36/transport_2011-07-25_19-05-36_with_xmlns.xml b/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-05-36/transport_2011-07-25_19-05-36_with_xmlns.xml new file mode 100755 index 000000000..64144cda0 --- /dev/null +++ b/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-05-36/transport_2011-07-25_19-05-36_with_xmlns.xml @@ -0,0 +1 @@ +ambulance bicycledailyweeklyuuid:f3d8dc65-91a6-4d0f-9e97-802128083390 diff --git a/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-05-49/transport_2011-07-25_19-05-49_with_xmlns.xml b/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-05-49/transport_2011-07-25_19-05-49_with_xmlns.xml new file mode 100755 index 000000000..7c764efc4 --- /dev/null +++ b/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-05-49/transport_2011-07-25_19-05-49_with_xmlns.xml @@ -0,0 +1 @@ +noneuuid:5b2cc313-fc09-437e-8149-fcd32f695d41 diff --git a/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-06-01/transport_2011-07-25_19-06-01_with_xmlns.xml b/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-06-01/transport_2011-07-25_19-06-01_with_xmlns.xml new file mode 100755 index 000000000..132609cbf --- /dev/null +++ b/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-06-01/transport_2011-07-25_19-06-01_with_xmlns.xml @@ -0,0 +1 @@ +ambulanceweeklyuuid:9c6f3468-cfda-46e8-84c1-75458e72805d diff --git a/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-06-14/transport_2011-07-25_19-06-14_with_xmlns.xml b/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-06-14/transport_2011-07-25_19-06-14_with_xmlns.xml new file mode 100755 index 000000000..381b9b026 --- /dev/null +++ b/onadata/apps/main/tests/fixtures/transportation/instances/transport_2011-07-25_19-06-14/transport_2011-07-25_19-06-14_with_xmlns.xml @@ -0,0 +1 @@ +taxi othercameldailyotheruuid:9f0a1508-c3b7-4c99-be00-9b237c26bcbf diff --git a/onadata/apps/viewer/tests/test_attachment_url.py b/onadata/apps/viewer/tests/test_attachment_url.py index 0bdea5a1f..51d107012 100644 --- a/onadata/apps/viewer/tests/test_attachment_url.py +++ b/onadata/apps/viewer/tests/test_attachment_url.py @@ -1,5 +1,8 @@ # coding: utf-8 +import requests from django.urls import reverse +from django_digest.test import DigestAuth +from django_digest.test import Client as DigestClient from onadata.apps.main.tests.test_base import TestBase from onadata.apps.logger.models import Attachment @@ -25,6 +28,18 @@ def test_attachment_url(self): self.url, {"media_file": self.attachment_media_file}) self.assertEqual(response.status_code, 200) # nginx is used as proxy + def test_attachment_url_with_digest_auth(self): + self.client.logout() + response = self.client.get( + self.url, {'media_file': self.attachment_media_file} + ) + self.assertEqual(response.status_code, 401) # nginx is used as proxy + self.assertTrue('WWW-Authenticate' in response) + digest_client = DigestClient() + digest_client.set_authorization(self.login_username, self.login_password) + response = digest_client.get(self.url, {'media_file': self.attachment_media_file}) + self.assertEqual(response.status_code, 200) + def test_attachment_not_found(self): response = self.client.get( self.url, {"media_file": "non_existent_attachment.jpg"}) diff --git a/onadata/settings/base.py b/onadata/settings/base.py index 3acd6b9bf..c0170d269 100644 --- a/onadata/settings/base.py +++ b/onadata/settings/base.py @@ -571,6 +571,12 @@ def skip_suspicious_operations(record): # the database is SQLite (e.g.: running unit tests locally). USE_POSTGRESQL = True +# Added this because of https://github.com/onaio/onadata/pull/2139 +# Should bring support to ODK v1.17+ +SUPPORT_BRIEFCASE_SUBMISSION_DATE = ( + os.environ.get('SUPPORT_BRIEFCASE_SUBMISSION_DATE') != 'True' +) + ################################ # Celery settings # ################################