Merge pull request #775 from kobotoolbox/772-briefcase-1.16+-compatib…

…ility-issues

Fix broken attachment downloads with ODK Briefcase v1.16+

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

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)

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,16 +225,27 @@ 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 settings.SUPPORT_BRIEFCASE_SUBMISSION_DATE:
+            # 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),
             '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):

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 @@
+<?xml version='1.0' ?><transportation xmlns="http://opendatakit.org/submission" id="transportation_2011_07_25"><transport><available_transportation_types_to_referral_facility>ambulance bicycle</available_transportation_types_to_referral_facility><loop_over_transport_types_frequency><ambulance><frequency_to_referral_facility>daily</frequency_to_referral_facility></ambulance><bicycle><frequency_to_referral_facility>weekly</frequency_to_referral_facility></bicycle><boat_canoe /><bus /><donkey_mule_cart /><keke_pepe /><lorry /><motorbike /><taxi /><other /></loop_over_transport_types_frequency></transport><meta><instanceID>uuid:f3d8dc65-91a6-4d0f-9e97-802128083390</instanceID></meta></transportation>

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 @@
+<?xml version='1.0' ?><transportation xmlns="http://opendatakit.org/submission" id="transportation_2011_07_25"><transport><available_transportation_types_to_referral_facility>none</available_transportation_types_to_referral_facility><loop_over_transport_types_frequency><ambulance /><bicycle /><boat_canoe /><bus /><donkey_mule_cart /><keke_pepe /><lorry /><motorbike /><taxi /><other /></loop_over_transport_types_frequency></transport><meta><instanceID>uuid:5b2cc313-fc09-437e-8149-fcd32f695d41</instanceID></meta></transportation>

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 @@
+<?xml version='1.0' ?><transportation xmlns="http://opendatakit.org/submission" id="transportation_2011_07_25"><transport><available_transportation_types_to_referral_facility>ambulance</available_transportation_types_to_referral_facility><loop_over_transport_types_frequency><ambulance><frequency_to_referral_facility>weekly</frequency_to_referral_facility></ambulance><bicycle /><boat_canoe /><bus /><donkey_mule_cart /><keke_pepe /><lorry /><motorbike /><taxi /><other /></loop_over_transport_types_frequency></transport><meta><instanceID>uuid:9c6f3468-cfda-46e8-84c1-75458e72805d</instanceID></meta></transportation>

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 @@
+<?xml version='1.0' ?><transportation xmlns="http://opendatakit.org/submission" id="transportation_2011_07_25"><transport><available_transportation_types_to_referral_facility>taxi other</available_transportation_types_to_referral_facility><available_transportation_types_to_referral_facility_other>camel</available_transportation_types_to_referral_facility_other><loop_over_transport_types_frequency><ambulance /><bicycle /><boat_canoe /><bus /><donkey_mule_cart /><keke_pepe /><lorry /><motorbike /><taxi><frequency_to_referral_facility>daily</frequency_to_referral_facility></taxi><other><frequency_to_referral_facility>other</frequency_to_referral_facility></other></loop_over_transport_types_frequency></transport><meta><instanceID>uuid:9f0a1508-c3b7-4c99-be00-9b237c26bcbf</instanceID></meta></transportation>

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"})

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)

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):

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              #
 ################################