Encrypted values overwritten if submission edited by user without 'view encrypted values' permissions

Thank you very much for the report, we will need to fix this before the first alpha release.

Here's a test verifying the bug.

I've tried implementing hook_webform_element_alter() to disable the encrypted fields if the user doesn't have the 'view encrypted values' permission, but the values are still being saved for those fields, so that's not going to work.

/**
 * Implements hook_webform_element_alter().
 */
function webform_encrypt_webform_element_alter(array &$element, \Drupal\Core\Form\FormStateInterface $form_state, array $context) {
  /** @var \Drupal\webform\WebformInterface $webform */
  $webform = $form_state->getFormObject()->getEntity()->getWebform();
  // Disable fields that are set to be encrypted when the user has does not
  // have the 'view encrypted values' permission.
  $config = $webform->getThirdPartySetting('webform_encrypt', 'element');
  if (!empty($config[$element['#webform_key']]['encrypt']) && !\Drupal::currentUser()->hasPermission('view encrypted values')) {
    $element['#disabled'] = TRUE;
  }
}

I've also tried tackling this on webform_encrypt_entity_presave(), we can check there if the value to be saved is [Value Encrypted] but we can't save only the other values as far as I know.

We could consider removing access to editing webform submissions where any field is encrypted if the user doesn't have the 'view encrypted values' permission.

Any other ideas?

Bumping down the priority, since there is a work around for this (giving the permission to view encrypted values).

I am also going to mark this as by design for now, since if you are going to allow your users to edit submissions, it can be assumed they are going to need to view encrypted values.

If someone comes up with a viable way to prevent this situation, feel free to reopen.

Re-opening to pose the question that if a user who can edit a submission should inherently be able to view encrpyted values should we just add that permission check to the storage controller? This would prevent any possibility of missing the permission and allwoingdata to be corrupted. e.g.

if ($check_permissions && !\Drupal::currentUser()->hasPermission('view encrypted values') && !\Drupal::currentUser()->hasPermission('edit any webform submission')) {

Would probably have to go further and check to see if the user owns the submission and then check for "edit own webform submission" as well.

Untested but conceptual....

  protected function decrypt($string, $encryption_profile, $webform_submission, $check_permissions = TRUE) {

    if ($check_permissions &&
        !\Drupal::currentUser()->hasPermission('view encrypted values') &&
        !\Drupal::currentUser()->hasPermission('edit any webform submission') &&
        !($webform_submission->get('uid') == \Drupal::currentUser()->id() && \Drupal::currentUser()->hasPermission('edit own webform submission'))
    ) {
      return '[Value Encrypted]';
    }

Would involve passing $webform_submission around the place.... Also noticed that $check_permissions flagis never actually used, do we still need this?

If you give a user the permission to edit webform submissions, they should be able to see the values in them, other wise there isn't really a point in being able to edit webform submissions.

So +1 from me, #8 makes a lot of sense.
If we do this we should also make it very clear in the documentation & project page that this is how the module works so as to avoid misunderstandings.

Here is a quick attempt at a patch, probably wont apply as its based on the nested fields patch in #2924957: Unable to encrypt nested/multiple nested fields leaving here to retest if/when the other issue gets committed.

Proposed text: "This module provides the 'view encrypted values' permission which will prevent encrpyted values from being viewed on a per role basis. Please note however that if a user has the 'edit own webform submission' or 'edit any webform submission' permissions they will also be able to view the encrypted values."

Thanks @Rob Holmes!

Let's wait until #2924957: Unable to encrypt nested/multiple nested fields gets in before continuing work here. Hopefully it will be committed soon.

#2924957: Unable to encrypt nested/multiple nested fields just got in :)

Queued for retest.

The changes to WebformEncryptSubmissionStorage::decrypt() on #10 would affect not only when a user editing a submission, but also when a user views a submission. So this means we would be checking unrelated permissions in this case like 'edit any webform submission'.

In other words, this approach is not going to work I'm afraid. This also highlights the need for more test coverage regarding users with different permissions (we should have tests that would've picked this up).

The only option I see here is denying the user access to update webform submissions that are encrypted unless they have the 'view encrypted values' permission, so as to prevent data loss at least. We could accomplish this via swapping in a new access handler, so here is a patch taking that approach, though completely untested as I've run out of time.

Oops

Thanks @Manuel, that makes sense to me, ill try to test later today.

Manually tested and only users able to both edit submissions and view encrypted values can access the edit submission page.

The only feedback is:

+++ b/src/WebformEncryptSubmissionAccessControlHandler.php
@@ -0,0 +1,34 @@
+        if ($config[$element_name]['encrypt'] && $account->hasPermission('view encrypted values') == FALSE) {

isset($config[$element_name]) should be prepended to the condition, otherwise a notice is raised if element is not encrypted.

Also tests on #3 are still valid, the only change is user without 'view encrypted values' permission shouldn't be able to access the page:

public function testEditSubmissions() {
    $assert_session = $this->assertSession();
    $page = $this->getSession()->getPage();

    $this->drupalLogin($this->notViewEncryptedUser);
    // Make a submission.
    $edit = [
      'test_text_field' => 'Test text field encrypted value',
      'test_text_area' => 'Test text area encrypted value',
      'test_not_encrypted' => 'Test not encrypted value',
    ];
    $this->drupalPostForm('/webform/test_encryption', $edit, 'Submit');
    $assert_session->responseContains('New submission added to Test encryption.');

    // Ensure form is not accessible by user without the view encrypted values
    // permission.
    $edit_submission_path = 'admin/structure/webform/manage/test_encryption/submission/1/edit';
    $this->drupalGet($edit_submission_path);
    $assert_session->responseContains('You are not authorized to access this page.');

    // Verify with the view encrypted values permission that form submission is
    // editable.
    $this->drupalLogin($this->viewEncryptedUser);
    $this->drupalGet($edit_submission_path);
    $assert_session->fieldValueEquals('test_text_field', $edit['test_text_field']);
    $assert_session->fieldValueEquals('test_text_area', $edit['test_text_area']);
    $assert_session->fieldValueEquals('test_not_encrypted', $edit['test_not_encrypted']);

    // Save the form without changing any values.
    $this->drupalPostForm($edit_submission_path, [], 'Save');

    // Check submission is still editeable and values are unchanged .
    $this->drupalGet($edit_submission_path);
    $assert_session->fieldValueEquals('test_text_field', $edit['test_text_field']);
    $assert_session->fieldValueEquals('test_text_area', $edit['test_text_area']);
    $assert_session->fieldValueEquals('test_not_encrypted', $edit['test_not_encrypted']);
  }

Final thought: do we need to test users with only 'view encrypted values' (and not 'edit any/own submission') are forbidden? Just to avoid in principle any future regression.

Thanks @gambry for the review, all good points.

Addressing #19 here.

Testing Drupal\Tests\webform_encrypt\Functional\WebformEncryptFunctionalTest
E

Time: 7.98 seconds, Memory: 4.00MB

There was 1 error:

1) Drupal\Tests\webform_encrypt\Functional\WebformEncryptFunctionalTest::testFieldEncryption
Behat\Mink\Exception\ExpectationException: The string "09/15/2019" was not found anywhere in the HTML response of the current page.

I'm not sure what's going on with the dates here.

At least head is failing too. I didn't have a chance to check it myself. Will give it a go later today, in the meanwhile I remember @thecraighammond on #2925621: Encryption of pre-existing data #19 added the property 'format' => short to the date. That should fix it.

#2936159: Hardcoded value for the date field in WebformEncryptFunctionalTest got in, let's give #20 another run.

+++ b/src/WebformEncryptSubmissionAccessControlHandler.php
@@ -0,0 +1,34 @@
+    if ($operation == 'update') {
...
+        if (isset($config[$element_name]['encrypt']) && $config[$element_name]['encrypt'] && $account->hasPermission('view encrypted values') == FALSE) {

nitpick, these should be ===.

Also there is a coding standard error, but I can't find out what that is. I suspect is the unused $page variable on WebformEncryptEditSumissionsTest $page = $this->getSession()->getPage();.

Setting RTBC as both are small issues and can be fixed on commit. Nice one!

Fixing nitpicks =)

+++ b/src/WebformEncryptSubmissionAccessControlHandler.php
@@ -0,0 +1,34 @@
+    if ($operation == 'update') {

Then this needs indentical operator too.

Fair enough :)

Adding attribution to @gambry for reviews.

All looks good!

Thanks a lot!