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

[SFT-671] - Support ability to encrypt/decrypt submission data #882

Merged
merged 14 commits into from
Nov 9, 2023

Conversation

seandelaney
Copy link
Contributor

@seandelaney seandelaney commented Oct 12, 2023

  • Added new field property to encrypt field data.
  • Added checks to show new encrypt field data property for Lite and Pro editions.
  • Added new Encryption helper.
  • Updated Submission element to encrypt submission fields on beforeSave if field can be encrypted and setting enabled.
  • Updated SubmissionQuery to trigger decrypt submission fields on afterPopulateElement if field can be decrypted.
  • Added new EncryptionInterface and EncryptionTrait to control which fields can be encrypted/decrypted.
  • Fixed exporting submission data.
  • Added new Input Field Type to better manage form controls

@seandelaney seandelaney added the feature Makes this a feature label Oct 12, 2023
@kjmartens kjmartens self-requested a review October 12, 2023 20:47
Copy link
Contributor

@kjmartens kjmartens left a comment

Choose a reason for hiding this comment

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

When I try viewing the submission data inside the CP, front end and "Updating Search Indexes" in Craft Queue, I get an error:
base64_decode(): Argument #1 ($string) must be of type string, array given

When attempting to submit a multipage form (not sure if it's the fact that it's multipage or something else), the form fails at the final submit and has this error:

Exception 'TypeError' with message 'openssl_encrypt(): Argument #1 ($data) must be of type string, array given'

in /YOINK/freeform5.c4/vendor/yiisoft/yii2/base/Security.php:218

On a single page form it appeared to work ok for submit.

Copy link
Contributor

@gustavs-gutmanis gustavs-gutmanis left a comment

Choose a reason for hiding this comment

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

Adding some commentary.

packages/plugin/src/Elements/Db/SubmissionQuery.php Outdated Show resolved Hide resolved
packages/plugin/src/Elements/Submission.php Outdated Show resolved Hide resolved
packages/plugin/src/Fields/AbstractField.php Outdated Show resolved Hide resolved
packages/plugin/src/Fields/BaseGeneratedOptionsField.php Outdated Show resolved Hide resolved
@seandelaney
Copy link
Contributor Author

@kjmartens @gustavs-gutmanis

I think we need to spend some time refactoring how exports work. Multiple export routes with multiple options.

@seandelaney
Copy link
Contributor Author

@kjmartens

When I try viewing the submission data inside the CP, front end and "Updating Search Indexes" in Craft Queue, I get an error: base64_decode(): Argument #1 ($string) must be of type string, array given

When attempting to submit a multipage form (not sure if it's the fact that it's multipage or something else), the form fails at the final submit and has this error:

Exception 'TypeError' with message 'openssl_encrypt(): Argument #1 ($data) must be of type string, array given'

in /YOINK/freeform5.c4/vendor/yiisoft/yii2/base/Security.php:218

On a single page form it appeared to work ok for submit.

I cannot replicate these issues in latest changes. Can you please test again.

@kjmartens kjmartens dismissed their stale review October 25, 2023 19:21

Dismissing to note other changes

Copy link
Contributor

@kjmartens kjmartens left a comment

Choose a reason for hiding this comment

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

Array type fields seem to have issues with this feature. Affected fields are:

  • Checkboxes
  • Multiple Select
  • File Upload
  • File Upload Drag & Drop
  1. When the form contains ANY fields that contain an array of options, when submitting the form (and that field has data), the form submit fails with an error:
    Exception 'TypeError' with message 'openssl_encrypt(): Argument #1 ($data) must be of type string, array given' in /PATH/vendor/yiisoft/yii2/base/Security.php:218
  2. When the form contains ANY fields that contain an array of field data, it errors when loading the submission, even if it is NOT set to be encrypted.
    TypeError base64_decode(): Argument #1 ($string) must be of type string, array given

…d new field type for control and for better management
@seandelaney seandelaney requested a review from kjmartens October 26, 2023 18:39
kjmartens
kjmartens previously approved these changes Oct 26, 2023
@kjmartens kjmartens self-requested a review October 26, 2023 19:56
@kjmartens kjmartens dismissed their stale review October 26, 2023 19:57

New changes and issues

Copy link
Contributor

@kjmartens kjmartens left a comment

Choose a reason for hiding this comment

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

Much closer to working correctly now. However... 😅

  • Exporting via Quick Export and Export Profiles does not correctly decrypt the array fields (Checkboxes, Multiple Select, File Upload, File Upload Drag & Drop.

@seandelaney seandelaney requested a review from kjmartens October 27, 2023 12:15
@kjmartens
Copy link
Contributor

Seems good now. We should just wait on @gustavs-gutmanis's review when he gets back. 🙂

@kjmartens kjmartens self-requested a review October 27, 2023 17:36
Copy link
Contributor

@kjmartens kjmartens left a comment

Choose a reason for hiding this comment

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

A couple more issues I found with editions:

  1. Encryption is only supposed to be available for Pro, but appears on Lite as well (correctly hidden on Express).
  2. Decryption should happen regardless of edition. For example, if I was using Pro and encrypted some data and then later on downgraded to Lite (or Express) edition, I should still be able to access/view the decrypted submission data. Currently, it shows old data as encrypted still.

@seandelaney seandelaney requested a review from kjmartens October 30, 2023 15:37
@kjmartens kjmartens dismissed their stale review October 31, 2023 21:10

Retesting

Copy link
Contributor

@kjmartens kjmartens left a comment

Choose a reason for hiding this comment

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

Latest Changes:

  • Encryption still shows for Lite when it should only appear for Pro (correctly hidden on Express).

@seandelaney seandelaney requested a review from kjmartens November 1, 2023 16:01
Copy link
Contributor

@gustavs-gutmanis gustavs-gutmanis left a comment

Choose a reason for hiding this comment

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

Looking real nice now,

Just a couple of more questions before the approval.

@gustavs-gutmanis
Copy link
Contributor

gustavs-gutmanis commented Nov 6, 2023 via email

Copy link
Contributor

@gustavs-gutmanis gustavs-gutmanis left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

packages/plugin/src/Library/Helpers/EncryptionHelper.php Outdated Show resolved Hide resolved
@seandelaney seandelaney merged commit 74bbcd2 into v5 Nov 9, 2023
4 checks passed
@seandelaney seandelaney deleted the feat/SFT-671 branch November 9, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Makes this a feature
Development

Successfully merging this pull request may close these issues.

3 participants