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

Require "edit submissions", not "add submissions", for working with transcripts and translations #4749

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion kobo/apps/subsequences/api_view.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import json
from copy import deepcopy

from jsonschema import validate
from jsonschema.exceptions import ValidationError as SchemaValidationError
Expand Down Expand Up @@ -48,8 +49,17 @@ def _check_asr_mt_access_if_applicable(user, posted_data):
raise PermissionDenied('ASR/MT features are not available')


class AdvancedSubmissionPermission(SubmissionPermission):
"""
Regular `SubmissionPermission` maps POST to `add_submissions`, but
`change_submissions` should be required here
"""
perms_map = deepcopy(SubmissionPermission.perms_map)
perms_map['POST'] = ['%(app_label)s.change_%(model_name)s']

noliveleger marked this conversation as resolved.
Show resolved Hide resolved

class AdvancedSubmissionView(APIView):
permission_classes = [SubmissionPermission]
permission_classes = [AdvancedSubmissionPermission]
queryset = Asset.objects.all()
asset = None

Expand Down
116 changes: 116 additions & 0 deletions kobo/apps/subsequences/tests/test_submission_extras_api_post.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@
from kobo.apps.languages.models.transcription import (
TranscriptionService, TranscriptionServiceLanguageM2M)
from kpi.utils.fuzzy_int import FuzzyInt
from kpi.constants import (
PERM_ADD_SUBMISSIONS,
PERM_CHANGE_ASSET,
PERM_CHANGE_SUBMISSIONS,
PERM_VIEW_ASSET,
PERM_VIEW_SUBMISSIONS,
)
from ..constants import GOOGLETS, make_async_cache_key
from ..models import SubmissionExtras
from .test_submission_extras_content import sample_asset
Expand Down Expand Up @@ -88,6 +95,115 @@ def test_translation_revisions_stored_properly(self):
}
}

def test_transx_requires_change_asset_permission(self):
"""
Submit a transcript and translation as the owning user; then, switch to
another user and attempt editing with various permissions assigned
"""

# Enable transcripts and translations for the example question
self.set_asset_advanced_features(
{
'transcript': {'values': ['q1']},
'translation': {
'values': ['q1'],
'languages': ['tx1', 'tx2'],
},
}
)
resp = self.client.get(self.asset_url)
assert resp.status_code == 200
schema = resp.json()['advanced_submission_schema']

# Submit transcript and translation as the owner
original_transcript = 'they said hello'
original_translation = 'T H E Y S A I D H E L L O'
package = {
'submission': 'abc123-def456',
'q1': {
'transcript': {
'value': original_transcript,
'languageCode': 'en',
},
'translation': {
'tx1': {
'value': original_translation,
'languageCode': 'xx',
}
},
},
}
resp = self.client.post(schema['url'], package, format='json')
assert resp.status_code == 200
q1_transx = resp.json()['q1']
assert q1_transx['transcript']['value'] == original_transcript
assert q1_transx['translation']['tx1']['value'] == original_translation

# Become a user with no access to the project
other_user = User.objects.create(username='ethan')
self.client.force_login(other_user)
modified = deepcopy(package)
modified_transcript = 'they said goodbye'
modified_translation = 'T H E Y S A I D G O O D B Y E'
modified['q1']['transcript']['value'] = modified_transcript
modified['q1']['translation']['tx1']['value'] = modified_translation

# Attempt to change transcript should be rejected with no permissions
# assigned
resp = self.client.post(schema['url'], modified, format='json')
assert resp.status_code == 404

# …and should be rejected with any of these insufficient permissions
# assigned
self.asset.assign_perm(other_user, PERM_ADD_SUBMISSIONS)
resp = self.client.post(schema['url'], modified, format='json')
assert resp.status_code == 404

self.asset.assign_perm(other_user, PERM_VIEW_ASSET)
resp = self.client.post(schema['url'], modified, format='json')
assert resp.status_code == 404

self.asset.assign_perm(other_user, PERM_CHANGE_ASSET)
resp = self.client.post(schema['url'], modified, format='json')
assert resp.status_code == 404

self.asset.assign_perm(other_user, PERM_VIEW_SUBMISSIONS)
resp = self.client.post(schema['url'], modified, format='json')
assert resp.status_code == 403
Comment on lines +158 to +172
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR but I wondering why PERM_VIEW_ASSET, PERM_CHANGE_ASSET return a 404 while PERM_VIEW_SUBMISSIONS returns a 403. I was assuming that as soon as user had PERM_VIEW_ASSET, users would receive a 403 when they don't have required permissions on nested objects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question; I'd assume it's just the usual DRF behavior of:

  • Did you send a GET for a specific object that you are not allowed to see? We're not telling you whether or not it exists, so 404
  • Did you try to take a disallowed action (POST, PATCH) on an object that you are allowed to see? → 403

I didn't expect nested objects to behave differently, but we can look into it more if you think something doesn't make sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! that's what I'm expecting too.

I missed an important line!

I was thinking the model was Asset. Everything is good.


# Original content should be intact after rejections
extras = list(self.asset.submission_extras.all())
assert len(extras) == 1
extras = extras[0]
assert extras.submission_uuid == 'abc123-def456'
assert (
extras.content['q1']['transcript']['value'] == original_transcript
)
assert (
extras.content['q1']['translation']['tx1']['value']
== original_translation
)

# Transcript modification should succeed after granting
# 'change_submissions' permission
self.asset.assign_perm(other_user, PERM_CHANGE_SUBMISSIONS)
resp = self.client.post(schema['url'], modified, format='json')
assert resp.status_code == 200
q1_transx = resp.json()['q1']
assert q1_transx['transcript']['value'] == modified_transcript
assert q1_transx['translation']['tx1']['value'] == modified_translation
extras = list(self.asset.submission_extras.all())
assert len(extras) == 1
extras = extras[0]
assert extras.submission_uuid == 'abc123-def456'
assert (
extras.content['q1']['transcript']['value'] == modified_transcript
)
assert (
extras.content['q1']['translation']['tx1']['value']
== modified_translation
)


class TranscriptFieldRevisionsOnlyTests(ValidateSubmissionTest):
def setUp(self):
Expand Down