Skip to content

Feature/profile cipher for securing data#1225

Merged
phycodurus merged 60 commits intodevfrom
feature/profile-cipher-for-securing-data
Jul 8, 2025
Merged

Feature/profile cipher for securing data#1225
phycodurus merged 60 commits intodevfrom
feature/profile-cipher-for-securing-data

Conversation

@phycodurus
Copy link
Copy Markdown
Member

@phycodurus phycodurus commented May 27, 2025

This PR covers the signals, utility functions, and other changes required to store sensitive data, encrypted in the TOMToolkit database. See tom_eso for an example of how to use it. Briefly,

  • a new UserSession model is create for each User upon log in. It just has two foreign keys to the User and the User's Session. It allows access to a User's Session outside the context of a request, where the Session is added by the middlewhere.
  • When a user changes their password and the Session is updated to keep them logged in, the UserSession is updated in tom_base.view.UserUpdateView.
  • A set of signal receivers in signals.py looks for saves to the User model and calls out to session_utils.py to keep the EncryptedBinaryFields up-to-date.
  • Some tests have been added in tom_common.test.py

This is for properly encrypting sensitive User data (creds for
external services).
We'll store the cipher in the Session and not as a runtime
property of the Profile, because the cipher must persist
across requests. Because there are several signal receivers
associate with the encryption of sensitive data, we'll just
put the cipher create code in signals.py as well.
the Fernet instance can't be serialized to be saved in the
session. So, we just save the key and use it to instantiate
a new Fernet instance when we need it.
or rather don't update the session hash to perserve the login session.
The user will have to log back in. This is to trigger the UserSession
object creation to maintain encrypted database fields across password
changes.
@phycodurus phycodurus requested a review from Copilot May 27, 2025 15:09
@phycodurus phycodurus linked an issue May 27, 2025 that may be closed by this pull request
16 tasks

This comment was marked as outdated.

phycodurus and others added 11 commits May 27, 2025 10:21
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This reverts commit 92b757d1b22598b2ad27407a42f8fa026987e28b.
They have moved to session_utils.py and these fuctions generally
deal with encryptioning fields and that mechanism uses sessions
to keep track of things across HTTP requests.
When the user changes their password good practice is to call
update_session_auth_hash to keep them logged in. This create a new
Session object. That means that the old UserSession object gets
deleted on CASCADE. This create a new UserSession, linking the
User with their new Session.

Previously, the call to update_session_auth_hash was in dispatch(),
but now it's here in form_valid().
This makes it match the urlpattern; also make sure it's an int.
This makes it match the urlpattern; also make sure it's an int.
@phycodurus phycodurus requested a review from Copilot June 14, 2025 04:26

This comment was marked as outdated.

@phycodurus
Copy link
Copy Markdown
Member Author

phycodurus commented Jun 24, 2025

This breaks if the user password is changed by admin via the tom_common.views.UserPasswordChangeView route. I assume this has something to do with either not catching the profile update signal and/or not being in the correct session with the correct user when the password is changed?

This has been addressed in the latest commits. We now clear a User's encrypted fields if and Admin changes their password. Upon that password change, the data in those fields became irretrievable and stale. So, it makes sense to clear it out.

Also, added a confirmation and warning to the admin that encrypted fields for the User would be cleared out.

This simplifies the encrypted property-containing class
defintition to this:

class MyModel(EncryptableModelMixin, models.Model):
    _encyrpted_api_key = models.BinaryField(null=True)
    EncryptedProperty('_encrypted_api_key')

You still have to instantiate a cipher and attach it to the
the Model before you set/get, but there's no getting aournd that.
@phycodurus phycodurus requested review from Fingel and jchate6 June 25, 2025 00:07
@phycodurus
Copy link
Copy Markdown
Member Author

I'm confused by the reencrypt_app_fields integration point.

Is there a benefit to using auto_reencrypt_model_instances_for_user rather than just passing the field name and keeping the rest of the code in tom_base?

This has been refactored away. Encrypting a Model field no longer requires changes to the AppConfig

Copy link
Copy Markdown
Contributor

@jchate6 jchate6 left a comment

Choose a reason for hiding this comment

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

Sorry, ran out of time. Will finish the review on the rest of the code next week, but didn't want to leave you hanging for tomorrow!

Adds two helper fuctions (get and set) that encapulate the logic for
  - geting the User's encryption key
  - creating the Fernet cipher
  - attatching the cipher to the Model instance
  - removing the cipher when finished.
Copy link
Copy Markdown
Contributor

@jchate6 jchate6 left a comment

Choose a reason for hiding this comment

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

A few more things

This clarifies the contract between the mixin and it's Model
and also simplifies the logic in session_utils.py. It does introduce
an documentation issue that EncrtypableModelMixin subclasses
MUST NOT then have their own user OneToOneField.
@phycodurus phycodurus requested a review from jchate6 July 3, 2025 19:49
else:
return reverse_lazy('user-update', kwargs={'pk': self.request.user.id})

def get_context_data(self, **kwargs):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a work around for a bug with the user-update page where the "user" value in the context is set to the user being edited rather than the current user, so that even the name in the top right of the navbar temporarily gets set to that user than the currently logged in user. We should probably fix this, but who has the time. (and I doubt many people will notice.)

Copy link
Copy Markdown
Contributor

@jchate6 jchate6 left a comment

Choose a reason for hiding this comment

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

I have made a few commits for little things. do a pull before any other changes.
I'm also unclear on the local imports but it's not enough to hold this up any more.

this was necessary before the refactor of these functions
to session_utils.py, but not after
@phycodurus phycodurus merged commit 5cf35a2 into dev Jul 8, 2025
24 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Merged (to dev) in TOM Toolkit Jul 8, 2025
@phycodurus phycodurus deleted the feature/profile-cipher-for-securing-data branch July 8, 2025 04:03
@jchate6 jchate6 moved this from Merged (to dev) to Released in TOM Toolkit Jul 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

move EncryptedBinaryField model to tom_common from tom_eso Securely store credentials in a TOM

5 participants