Skip to content

Encrypt authorization metadata#6947

Merged
tramuntanal merged 20 commits intodecidim:developfrom
mainio:feature/authorization-metadata-encryption
Dec 17, 2020
Merged

Encrypt authorization metadata#6947
tramuntanal merged 20 commits intodecidim:developfrom
mainio:feature/authorization-metadata-encryption

Conversation

@ahukkanen
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen commented Nov 27, 2020

🎩 What? Why?

The authorization metadata can contain sensitive personal information and it should be therefore encrypted.

This PR implements a new concern that provides automated encryption for active record attributes so that they will be stored encrypted in the database but when accessed through the code, they are available as plain text.

This is applied to the metadata attributes in the Decidim::Authorization record.

The change is transparent and should not affect any Decidim APIs or code relying on the metadata. The only implication to existing code can be if the JSON object has been queried directly through the database. Now that the values are encrypted in the database, these kinds of queries no longer work.

Testing

  • Create a new authorization from the rails console, e.g. Decidim::Authorization.create!(name: "testing", metadata: { foo: "bar" }, decidim_user_id: 1, unique_id: "uniqid", granted_at: DateTime.now)
  • Fetch that record and display it in the console to see that the metadata values are encrypted Decidim::Authorization.last
  • Access the record's metadata in order to verify it is decrypted through the accessor method Decidim::Authorization.last.metadata

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • 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.

@mrcasals
Copy link
Copy Markdown
Contributor

@ahukkanen I think the JSON field is queried in some authorization examples when matching authorizations by postal code. I think the idea was to encrypt only what was needed, manually, but leave the JSON field accessible to have useful clear values there.

@ahukkanen
Copy link
Copy Markdown
Contributor Author

@mrcasals Postal code would count as a personal information.

@ahukkanen
Copy link
Copy Markdown
Contributor Author

And I know the information is queried in some queries, they just have to be changed.

Can you talk to @andreslucena about this?

@tramuntanal
Copy link
Copy Markdown
Contributor

Hi @ahukkanen this is a possibly breaking change for multiple verification systems. Is there a related issue or meta-decidim proposal?

@ahukkanen
Copy link
Copy Markdown
Contributor Author

@tramuntanal No, there's no Metadecidim proposal. Can you please talk to @andreslucena about this?

@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented Dec 10, 2020

I've already talked with @andreslucena , ok @ahukkanen we'll review

@tramuntanal tramuntanal self-assigned this Dec 10, 2020
Copy link
Copy Markdown
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

Hi @ahukkanen ,
I've left a couple of questions.

Also I'm noting that this PR will break code in other repos that rely on querying custom attributes in the metadata field like:

Maybe we can create an extras field of type jsonb to persist complementary data used for querying. Although we're opening the door to persist private data unencrypted.
What are your thoughts @decidim/product ?

@ahukkanen
Copy link
Copy Markdown
Contributor Author

@tramuntanal I would personally like to avoid adding a possibility to store sensitive data unencrypted.

I would not suggest storing strongly identifiable data in plain text to the database but in case you really have to, couldn't you use the extended_data hash column for non-identifiable statistics information?

add_column :decidim_users, :extended_data, :jsonb, default: {}

@tramuntanal
Copy link
Copy Markdown
Contributor

Indeed @ahukkanen , I agree that it is best to have personal data encrypted, no doubt!

My worries are about breaking code and other modules.
We will need to add some Upgrade notes in the changelog to warn about the possible incompatibilities, can you please?

@ahukkanen
Copy link
Copy Markdown
Contributor Author

@tramuntanal I have added a CHANGELOG note.

Copy link
Copy Markdown
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

Good @ahukkanen !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants