ENG-2608 Make encryption optional in consent v3 columns#7413
ENG-2608 Make encryption optional in consent v3 columns#7413
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Greptile SummaryThis PR introduces optional encryption for the v3
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 41d7c2e |
|
@greptile re-review |
| is_encrypted = Column( | ||
| Boolean, | ||
| nullable=False, | ||
| default=lambda: CONFIG.consent.consent_v3_encryption_enabled, | ||
| index=True, | ||
| ) |
There was a problem hiding this comment.
Missing server_default on boolean column
The is_encrypted column uses a Python-side default but has no server_default. This means any insert that bypasses the ORM (e.g., raw SQL, bulk inserts, or future migration scripts) will fail with a NOT NULL constraint violation since the database itself has no default for this column. Adding a server_default makes the schema more robust and is consistent with how other boolean columns in this model are defined (e.g., is_latest on line 67 uses server_default=text("false")).
| is_encrypted = Column( | |
| Boolean, | |
| nullable=False, | |
| default=lambda: CONFIG.consent.consent_v3_encryption_enabled, | |
| index=True, | |
| ) | |
| is_encrypted = Column( | |
| Boolean, | |
| nullable=False, | |
| default=lambda: CONFIG.consent.consent_v3_encryption_enabled, | |
| server_default=text("true"), | |
| index=True, | |
| ) |
Context Used: Rule from dashboard - Use server_default="f" for boolean columns in Alembic migrations instead of default=False or oth... (source)
| db.commit() | ||
| result.total_processed += len(rows) |
There was a problem hiding this comment.
total_processed counts fetched rows, not transformed ones
result.total_processed += len(rows) adds the count of all fetched rows, including those that _process_batch skipped because they were already in the target state. This makes the progress callback and the CLI's final message (e.g., "Encrypted 5000 rows") misleading when some rows were already encrypted/decrypted.
Consider tracking actually-transformed rows separately, or renaming this to something like total_examined to set correct expectations.
johnewart
left a comment
There was a problem hiding this comment.
LGTM as a whole but can we put the logic in the service? 🤞
| ) | ||
| updated_at = Column(DateTime(timezone=True), nullable=True) | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
I would put this in one of the services as it's more of a preferences-as-a-whole question
There was a problem hiding this comment.
hmm we could but then it has to go in fidesplus, which is "further away" from where the is_encrypted field is defined... I was hoping we could leave all the plus code "unaware" of the is_encrypted flag. Do you think it makes sense to move all this into fidesplus?
src/fides/api/app_setup.py
Outdated
| logger.debug("Connection to cache succeeded") | ||
|
|
||
|
|
||
| def check_consent_encryption_consistency() -> None: |
There was a problem hiding this comment.
Again, I think this can just be in the service - if privacy_preferences_service.has_encryption_mismatch() or something 😄
Ticket ENG-2608
Description Of Changes
This PR adds a new setting that allows disabling the encryption at the column-level for the consent v3
privacy_preferencestable. The default (and recommended) value for the setting is keeping encryption enabled.Code Changes
consent_v3_encryption_enabledconsent setting ,with a default value ofTrueoptionally_encrypted_typefor optionally-encrypted columnsis_encryptedcolumn to the privacy preferences tablerecord_datacolumn to use this new type , using the value from the new settingfides db migrate-consent-encryptionto easily allow encrypting/decrypting existing records ; this is not meant for production usage, just for easy of dev / testing.Steps to Confirm
falseand restart the serverfides db migrate-consent-encryption --direction=decrypt. Check rows are now decryptedPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works