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 914b46e
Show file tree
Hide file tree
Showing 14 changed files with 98 additions and 75 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
9 changes: 3 additions & 6 deletions onadata/libs/utils/logger_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,10 @@ 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.
The user has permissions iff:
* the form does not require auth
* the user is authed, the form requires auth and user can submit data
Since we have a username, the Instance creation logic will
handle checking for the forms existence by its id_string.
: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 914b46e

Please sign in to comment.