Check submission encryption status before Instance creation #1938
Conversation
DavisRayM
force-pushed
the
1934-encrypted-form-restrictions
branch
3 times, most recently
from
537fd14
to
7d4507f
Compare
Test that encrypted submissions can not be made to XForms that are unencrypted and that unencrypted submissions can not be made to encrypted XForms.
DavisRayM
force-pushed
the
1934-encrypted-form-restrictions
branch
from
7d4507f
to
49b0221
Compare
onadata/libs/utils/logger_tools.py
Outdated
| f"{submisison_status} submissions are not" | ||
| f" allowed for {form_status} forms.") | ||
|
|
There was a problem hiding this comment.
Can we use translation strings here to allow this error message to be translated. And please remember to run the command for updating translation files,
onadata/libs/utils/logger_tools.py
Outdated
| @@ -468,6 +490,8 @@ def safe_create_instance(username, xml_file, media_files, uuid, request): | |||
| except InstanceEmptyError: | |||
| error = OpenRosaResponseBadRequest( | |||
| _(u"Received empty submission. No instance was created")) | |||
| except InstanceEncryptionError as e: | |||
| error = OpenRosaResponseBadRequest(e) | |||
There was a problem hiding this comment.
Would you also not need to apply text(e) as is the pattern in this section, see the trend below where the error variable e is concerned.
onadata/libs/utils/logger_tools.py
Outdated
| from the submissions | ||
| """ | ||
| submission_element = ET.fromstring(xml) | ||
| encryption_status = submission_element.attrib.get('encrypted') == "yes" |
There was a problem hiding this comment.
I wonder, is this the only attribute that we need to check to determine a submission is encrypted? Should we also confirm presence of or lack of base64EncryptedKey and encryptedXmlFile.
There was a problem hiding this comment.
All three attributes are required in a properly built Encrypted submission; Initially thought checking for just one would be okay. But, now I believe we can check if all three are present and also add functionality to determine whether the encrypted submission is valid
I'll add these checks and re-request a review
onadata/libs/utils/logger_tools.py
Outdated
| if encrypted_attrib == "yes" or len(required_encryption_elems) > 1: | ||
| if (not len(required_encryption_elems) == 2 or |
There was a problem hiding this comment.
The expression len(required_encryption_elems) is used more than once here, can we assign it to variable and use the variable instead?
onadata/libs/utils/logger_tools.py
Outdated
|
|
||
| if encryption_status != xform.encrypted: | ||
| form_status = "encrypted" if xform.encrypted else "unencrypted" | ||
| submisison_status = "Encrypted" if encryption_status else "Unencrypted" |
There was a problem hiding this comment.
Since this is part of output to a user, you probably want to use the translated values.
Again, the only situation that matters is the "encrypted" form, if the form is not encrypted there is no need to reject the submission on this basis.
There was a problem hiding this comment.
Again, the only situation that matters is the "encrypted" form, if the form is not encrypted there is no need to reject the submission on this basis.
How about in a situation where an encrypted submission is made to an unencrypted form? Doubt it would happen in a normal submission flow but is it something we should reject?
There was a problem hiding this comment.
No, not really. It is not going to affect exports adversely. That element does bring to question what other validations we need to do.
- Add "encryption_elems_num" variable to contain the number of required encryption elements present in a Submissions XML - Use translated strings to hold the "encrypted" & "unencrypted" strings - Update translations
Changes / Features implemented
Steps taken to verify this change does what is intended
Side effects of implementing this change
Closes #1934