Skip to content

Fix record encryptor trying to decrypt or decode non-String values#7536

Merged
mrcasals merged 5 commits intodecidim:developfrom
mainio:fix/7487-deep-hashes
Mar 4, 2021
Merged

Fix record encryptor trying to decrypt or decode non-String values#7536
mrcasals merged 5 commits intodecidim:developfrom
mainio:fix/7487-deep-hashes

Conversation

@ahukkanen
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen commented Mar 4, 2021

🎩 What? Why?

Currently the record decryption fails if the unencrypted values are something else than a String or something that can be converted to a String as explained at:
#7487 (comment)

This checks the type of the value before passing it to decryption or to JSON decoding for hash values.

📌 Related Issues

Testing

See the testing instructions at: #7488

But for the authorization record, add deeper hash values to match this case:

Decidim::Authorization.create!(
  user: Decidim::User.first,
  name: "postal_code",
  metadata: {},
  verification_metadata: {
    foo: { bar: "baz" }
  }
)

📋 Checklist

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

- Fix decrypting a value that is not a String (likely it's not
  encrypted)
- Fix decrypting unecrypted deep hash values for legacy data
  migrations
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Mar 4, 2021

Yay, @ahukkanen! thank you so much for the quick fixes on this! ❤️

@mrcasals mrcasals added module: core type: fix PRs that implement a fix for a bug labels Mar 4, 2021
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Mar 4, 2021

This needs to be backported to 0.24.

@ahukkanen
Copy link
Copy Markdown
Contributor Author

@mrcasals Note that I changed the fix a bit. I moved the value type check to Decidim::AttributeEncryptor.

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Mar 4, 2021

Awesome, thanks @ahukkanen. I'll wait until this is merged to create the backport 😄

mrcasals
mrcasals previously approved these changes Mar 4, 2021
@mrcasals mrcasals merged commit 91db968 into decidim:develop Mar 4, 2021
@ahukkanen ahukkanen deleted the fix/7487-deep-hashes branch March 4, 2021 11:16
entantoencuanto added a commit that referenced this pull request Mar 4, 2021
* develop:
  Update the workflow cleanup action to the latest version (#7535)
  Don't schedule CI jobs for locales PRs (#7534)
  Fix record encryptor trying to decrypt or decode non-String values (#7536)
  Add Votings landing page to the design app (#7527)
  New Crowdin updates (#7530)
  Fix non-unique IDs element in filter hash cash (#7531)
  New Crowdin updates (#7485)
  Add statistics cell to votings landing page and reuse it in other places (#7413)
  Add Votings landing page layout (#7440)
  Add share modal to budgets (#7519)
  Do not change the global test app configs during specs (#7525)
  Change the order of attachments in budgets (#7524)
  Remove console warnings from the conversations views (#7523)
  Don't allow filtering meetings by user group if setting is disabled (#7514)
  Remove duplicated migration (#7517)
  New Admin users cannot accept Terms and conditions (#7516)
  Let installations delay TranslatorJob initialization (#7507)
  Exit on CI workflow dispatch failures (#7502)
  Invalidate all user sessions when destroying the account (#7506)
  Audit vote (#7442)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: core type: fix PRs that implement a fix for a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't migrate the DB after #6947

2 participants