Skip to content

Commit

Permalink
Apply requested changes for PR#904
Browse files Browse the repository at this point in the history
  • Loading branch information
noliveleger committed Jan 10, 2024
1 parent 87054fb commit e5e8857
Show file tree
Hide file tree
Showing 14 changed files with 98 additions and 76 deletions.
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 @@ -355,7 +355,7 @@ def test_cannot_get_enketo_edit_url_without_require_auth(self):
self.assertTrue(
response.data[0].startswith(
'Cannot edit submissions while "Require authentication '
'to see forms and submit data" is disabled for your '
'to see form and submit data" is disabled for your '
'project'
)
)
Expand Down
7 changes: 4 additions & 3 deletions onadata/apps/api/tests/viewsets/test_xform_list_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -484,10 +484,11 @@ def test_retrieve_xform_manifest_as_anonymous(self):
'get': 'manifest'
})
request = self.factory.get('/')
# Not auth required but XForm cannot be found except if `required_auth`
# is True.
# See `TestXFormListApiWithoutAuthRequired.test_retrieve_xform_manifest()`
response = self.view(request, pk=self.xform.pk, username=self.user.username)
# The project (self.xform) requires auth by default. Anonymous user cannot
# access it. It is also true for the project manifest, thus anonymous
# should receive a 404.
# See `TestXFormListApiWithoutAuthRequired.test_retrieve_xform_manifest()`
self.assertEqual(response.status_code, 404)

def test_head_xform_manifest(self):
Expand Down
40 changes: 19 additions & 21 deletions onadata/apps/api/viewsets/briefcase_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def get_object(self):
uuid = _extract_uuid(form_id)

obj = get_instance_or_404(
xform__id_string__exact=id_string,
xform__id_string=id_string,
uuid=uuid,
)
self.check_object_permissions(self.request, obj.xform)
Expand All @@ -118,7 +118,8 @@ def get_object(self):
def filter_queryset(self, queryset):
username = self.kwargs.get('username')
if username is None:
# If no username is specified, the request must be authenticated
# Briefcase does not allow anonymous access, user should always be
# authenticated
if self.request.user.is_anonymous:
# raises a permission denied exception, forces authentication
self.permission_denied(self.request)
Expand All @@ -127,20 +128,20 @@ def filter_queryset(self, queryset):
# including those shared by other users
queryset = super().filter_queryset(queryset)
else:
raise exceptions.PermissionDenied(
detail=t(
'Cannot access non secure URL. Remove username from '
'aggregate server URL in configuration settings.'
),
code=status.HTTP_403_FORBIDDEN,
)
# With the advent of project-level anonymous access in #904, Briefcase
# requests must no longer use endpoints whose URLs contain usernames.
# Ideally, Briefcase would display error messages returned by this method,
# but sadly that is not the case.
# Raise an empty PermissionDenied since it's impossible to have
# Briefcase display any guidance.
raise exceptions.PermissionDenied()

form_id = self.request.GET.get('formId', '')

if form_id.find('[') != -1:
form_id = _extract_id_string(form_id)

xform = get_object_or_404(queryset, id_string__exact=form_id)
xform = get_object_or_404(queryset, id_string=form_id)
self.check_object_permissions(self.request, xform)
instances = Instance.objects.filter(xform=xform).order_by('pk')
num_entries = self.request.GET.get('numEntries')
Expand Down Expand Up @@ -172,17 +173,14 @@ def create(self, request, *args, **kwargs):

xform_def = request.FILES.get('form_def_file', None)
response_status = status.HTTP_201_CREATED
if username := kwargs.get('username'):
raise exceptions.PermissionDenied(
detail=t(
'Cannot User %(user)s has no permission to add xforms to '
'account %(account)s'
% {
'user': request.user.username,
'account': username,
}
)
)
# With the advent of project-level anonymous access in #904, Briefcase
# requests must no longer use endpoints whose URLs contain usernames.
# Ideally, Briefcase would display error messages returned by this method,
# but sadly that is not the case.
# Raise an empty PermissionDenied since it's impossible to have
# Briefcase display any guidance.
if kwargs.get('username'):
raise exceptions.PermissionDenied()

data = {}

Expand Down
19 changes: 9 additions & 10 deletions onadata/apps/api/viewsets/data_viewset.py
Original file line number Diff line number Diff line change
Expand Up @@ -648,15 +648,6 @@ def enketo(self, request, *args, **kwargs):
permission_classes=[EnketoSubmissionEditPermissions],
)
def enketo_edit(self, request, *args, **kwargs):
"""
Trying to edit in Enketo while `profile.require_auth == False` leads to
an infinite authentication loop because Enketo never sends credentials
unless it receives a 401 response to an unauthenticated HEAD request.
There's no way to send such a response for editing only while
simultaneously allowing anonymous submissions to the same endpoint.
Avoid the infinite loop by blocking doomed requests here and returning
a helpful error message.
"""
return self._enketo_request(request, action_='edit', *args, **kwargs)

@action(
Expand All @@ -678,9 +669,17 @@ def _enketo_request(self, request, action_, *args, **kwargs):
raise ParseError(t('`return_url` not provided.'))

if not object_.xform.require_auth and action_ == 'edit':
# Trying to edit in Enketo while `xform.require_auth == False`
# leads to an infinite authentication loop because Enketo never
# sends credentials unless it receives a 401 response to an
# unauthenticated HEAD request.
# There's no way to send such a response for editing only while
# simultaneously allowing anonymous submissions to the same endpoint.
# Avoid the infinite loop by blocking doomed requests here and
# returning a helpful error message.
raise ValidationError(t(
'Cannot edit submissions while "Require authentication to '
'see forms and submit data" is disabled for your project'
'see form and submit data" is disabled for your project'
))

try:
Expand Down
5 changes: 2 additions & 3 deletions onadata/apps/api/viewsets/xform_list_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ def filter_queryset(self, queryset):
# including those shared by other users
queryset = super().filter_queryset(queryset)
else:
# Only returns non-secured projects with URL starts with username
# Only return projects that allow anonymous submissions when path
# starts with a username
queryset = queryset.filter(
user__username=username.lower(), require_auth=False
)
Expand All @@ -111,8 +112,6 @@ def list(self, request, *args, **kwargs):
if request.method == 'HEAD':
return self.get_response_for_head_request()



serializer = self.get_serializer(
object_list, many=True, require_auth=not bool(kwargs.get('username'))
)
Expand Down
25 changes: 25 additions & 0 deletions onadata/apps/logger/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# coding: utf-8
from django.apps import AppConfig
from django.conf import settings
from django.core.checks import register, Error


class LoggerAppConfig(AppConfig):
Expand All @@ -14,3 +16,26 @@ def ready(self):
from kobo_service_account.utils import reversion_monkey_patch
reversion_monkey_patch()
super().ready()


@register()
def check_enketo_redis_main_url(app_configs, **kwargs):
"""
`ENKETO_REDIS_MAIN_URL` is required to make the app run properly.
"""
errors = []
if not settings.CACHES.get('enketo_redis_main'):
# We need to set `BACKEND` property. Otherwise, this error is shadowed
# by Django CACHES system checks.
settings.CACHES['enketo_redis_main']['BACKEND'] = (
'django.core.cache.backends.dummy.DummyCache'
)
errors.append(
Error(
f'Please set environment variable `ENKETO_REDIS_MAIN_URL`',
hint='Enketo Express Redis main URL is missing.',
obj=settings,
id='kobo.logger.enketo_redis_main.E001',
)
)
return errors
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,11 @@

from django.conf import settings
from django.db import migrations
from redis import Redis
from django_redis import get_redis_connection


CHUNK_SIZE = 2000

redis_client = Redis.from_url(settings.ENKETO_REDIS_MAIN_URL)


def restore_open_rosa_server_in_redis(apps, schema_editor):

Expand All @@ -28,7 +26,7 @@ def restore_open_rosa_server_in_redis(apps, schema_editor):
XForm = apps.get_model('logger', 'XForm') # noqa

parsed_url = urlparse(settings.KOBOCAT_URL)
server_url = f"{settings.KOBOCAT_URL.rstrip('/')}"
server_url = settings.KOBOCAT_URL.rstrip('/')

xforms_iter = (
XForm.objects.filter(require_auth=False)
Expand Down Expand Up @@ -59,25 +57,34 @@ def restore_open_rosa_server_in_redis(apps, schema_editor):
end
end
"""
redis_client = get_redis_connection('enketo_redis_main')
pipeline = redis_client.pipeline()
pipeline.eval(lua_script, 0)
pipeline.execute()


def restore_require_auth_at_profile_level(apps, schema_editor):

if settings.SKIP_HEAVY_MIGRATIONS:
return

XForm = apps.get_model('logger', 'XForm') # noqa
UserProfile = apps.get_model('main', 'UserProfile') # noqa

UserProfile.objects.filter(
user_id__in=XForm.objects.filter(require_auth=False).values_list(
'user_id'
)
).update(require_auth=False)
XForm.objects.filter(require_auth=True).update(require_auth=False)
print(
"""
Restoring authentication at the profile level cannot be done
automatically.
This is an example of what can be done:
⚠️ WARNING ⚠️ The example below makes all projects publicly
accessible when profile level is restored even if, at least, one project
was publicly accessible at project level.
```python
UserProfile.objects.filter(
user_id__in=XForm.objects.filter(require_auth=False).values_list(
'user_id'
)
).update(require_auth=False)
XForm.objects.filter(require_auth=True).update(require_auth=False)
```
"""
)


def set_require_auth_at_project_level(apps, schema_editor):
Expand Down Expand Up @@ -141,6 +148,7 @@ def update_open_rosa_server_in_redis(apps, schema_editor):
end
end
"""
redis_client = get_redis_connection('enketo_redis_main')
pipeline = redis_client.pipeline()
pipeline.eval(lua_script, 0)
pipeline.execute()
Expand Down
2 changes: 1 addition & 1 deletion onadata/apps/logger/models/xform.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class XForm(BaseModel):
user = models.ForeignKey(User, related_name='xforms', null=True, on_delete=models.CASCADE)
require_auth = models.BooleanField(
default=True,
verbose_name=t('Require authentication to see forms and submit data'),
verbose_name=t('Require authentication to see form and submit data'),
)
shared = models.BooleanField(default=False)
shared_data = models.BooleanField(default=False)
Expand Down
2 changes: 1 addition & 1 deletion onadata/apps/logger/tests/test_briefcase_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ def formList(*args, **kwargs): # noqa
def xformsDownload(*args, **kwargs): # noqa
view = XFormListApi.as_view({'get': 'retrieve'})
response = view(*args, **kwargs)
response.render()
return response

def xformsManifest(*args, **kwargs): # noqa
Expand Down Expand Up @@ -61,7 +62,6 @@ def form_list_xml(url, request, **kwargs):
res = formList(req)
elif url.path.endswith('form.xml'):
res = xformsDownload(req, pk=xform_id)
res.render()
elif url.path.find('xformsManifest') > -1:
res = xformsManifest(req, pk=xform_id)
elif url.path.find('xformsMedia') > -1:
Expand Down
7 changes: 2 additions & 5 deletions onadata/apps/logger/tests/test_form_submission.py
Original file line number Diff line number Diff line change
Expand Up @@ -438,8 +438,7 @@ def test_anonymous_cannot_edit_submissions(self):
"..", "fixtures", "tutorial", "instances",
"tutorial_2012-06-27_11-27-53_w_uuid_edited.xml"
)
# …without "Require authentication to see forms and submit data"
# self.assertFalse(self.user.profile.require_auth)
# …without "Require authentication to see form and submit data"
self.xform.require_auth = False
self.xform.save(update_fields=['require_auth'])

Expand Down Expand Up @@ -476,8 +475,6 @@ def test_authorized_user_can_edit_submissions_without_require_auth(self):
submissions at the same time.
"""

# self.assertFalse(self.user.profile.require_auth)

xml_submission_file_path = os.path.join(
os.path.dirname(os.path.abspath(__file__)),
"..", "fixtures", "tutorial", "instances",
Expand Down Expand Up @@ -576,7 +573,7 @@ def test_unauthorized_cannot_edit_submissions(self):
"..", "fixtures", "tutorial", "instances",
"tutorial_2012-06-27_11-27-53_w_uuid_edited.xml"
)
# …without "Require authentication to see forms and submit data"
# …without "Require authentication to see form and submit data"
self.xform.require_auth = False
self.xform.save(update_fields=['require_auth'])
self._make_submission(
Expand Down
2 changes: 1 addition & 1 deletion onadata/apps/main/tests/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def _create_user_and_login(self, username="bob", password="bob"):
self.login_password = password
self.user = self._create_user(username, password)

# create user profile and set require_auth to false for tests
# create user profile if it does not exist
UserProfile.objects.get_or_create(user=self.user)

self.client = self._login(username, password)
Expand Down
8 changes: 4 additions & 4 deletions onadata/libs/utils/briefcase_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -346,10 +346,10 @@ def push(self):
if form_xml in form_files:
form_xml_path = os.path.join(dir_path, form_xml)
x = self._upload_xform(form_xml_path, form_xml)
# if isinstance(x, dict):
# self.logger.error("Failed to publish %s" % form_dir)
# else:
# self.logger.debug("Successfully published %s" % form_dir)
if isinstance(x, dict):
self.logger.error("Failed to publish %s" % form_dir)
else:
self.logger.debug("Successfully published %s" % form_dir)
if 'instances' in form_dirs:
self.logger.debug("Uploading instances")
c = self._upload_instances(os.path.join(dir_path, 'instances'))
Expand Down
10 changes: 3 additions & 7 deletions onadata/libs/utils/logger_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,9 @@ def check_submission_permissions(
"""
Check that permission is required and the request user has permission.
The user does not have permissions if:
* the user is authed,
* the form require auth,
* the xform user is not submitting.
Since we have a username, the Instance creation logic will
handle checking for the forms existence by its id_string.
If the form does not require auth, anyone can submit, regardless of whether
they are authenticated. Otherwise, if the form does require auth, the
user must be the owner or have CAN_ADD_SUBMISSIONS.
:returns: None.
:raises: PermissionDenied based on the above criteria.
Expand Down
5 changes: 2 additions & 3 deletions onadata/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -203,11 +203,11 @@ def skip_suspicious_operations(record):
'django_digest',
'corsheaders',
'oauth2_provider',
'onadata.apps.logger.LoggerAppConfig',
'rest_framework',
'rest_framework.authtoken',
'taggit',
'readonly',
'onadata.apps.logger.LoggerAppConfig',
'onadata.apps.viewer',
'onadata.apps.main',
'onadata.apps.restservice',
Expand Down Expand Up @@ -395,6 +395,7 @@ def skip_suspicious_operations(record):
CACHES = {
# Set CACHE_URL to override. Only redis is supported.
'default': env.cache(default='redis://redis_cache:6380/3'),
'enketo_redis_main': env.cache('ENKETO_REDIS_MAIN_URL', None),
}

DIGEST_NONCE_BACKEND = 'onadata.apps.django_digest_backends.cache.RedisCacheNonceStorage'
Expand Down Expand Up @@ -756,8 +757,6 @@ def skip_suspicious_operations(record):
# All internal communications between containers must be HTTP only.
ENKETO_PROTOCOL = os.environ.get('ENKETO_PROTOCOL', 'https')

ENKETO_REDIS_MAIN_URL = os.environ.get('ENKETO_REDIS_MAIN_URL', 'redis://localhost:6379/')


################################
# MongoDB settings #
Expand Down

0 comments on commit e5e8857

Please sign in to comment.