Skip to content

Conversation

aclark4life
Copy link
Collaborator

@aclark4life aclark4life commented Jun 27, 2025

Previous attempts and additional context here:

@aclark4life

This comment was marked as resolved.

@timgraham

This comment was marked as resolved.

@aclark4life

This comment was marked as resolved.

@timgraham

This comment was marked as resolved.

@aclark4life

This comment was marked as resolved.

@aclark4life

This comment was marked as resolved.

@aclark4life

This comment was marked as resolved.

@aclark4life

This comment was marked as resolved.

@timgraham

This comment was marked as resolved.

@aclark4life

This comment was marked as resolved.

@aclark4life

This comment was marked as resolved.

@aclark4life

This comment was marked as resolved.

@timgraham

This comment was marked as resolved.

@aclark4life

This comment was marked as resolved.

@timgraham

This comment was marked as resolved.

@aclark4life aclark4life force-pushed the INTPYTHON-527 branch 3 times, most recently from 54bd164 to db4aadb Compare October 6, 2025 19:57
- Add mongocryptd to atlas CI
- Doc updates
- Remove custom db_type, not needed after mongodb#414
- SupportsQueryableEncryptionTests require MongoDB 8
- Update test model names prefix and suffix
- Isolate test failures
  - admin_scripts
  - check_framework
  - migrations
  - test_runner
  - user_commands
- Add support for EncryptedEmbeddedModelArrayField
- Refactor
  - Factor _get_data_key from _get_encrypted_fields
  - Refactor for DRY in _get_encrypted_fields method with _field_dict
    helper
Ruff killed the nested if statements
@aclark4life aclark4life force-pushed the INTPYTHON-527 branch 3 times, most recently from 7e155a9 to f7846a6 Compare October 8, 2025 23:51
- Remove extra check for auto_encryption_opts
  - Earlier check should suffice
- Remove Encrypted* class name prefix
  - We know these are the QE tests
- Move create data key back to _get_encrypted_fields
- Remove getattr for client_encryption
- Remove EmbeddedModelArrayField
Comment on lines +102 to +107
class EncryptedTimeField(EncryptedFieldMixin, models.TimeField):
pass


class EncryptedTextField(EncryptedFieldMixin, models.TextField):
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

out of alphabetical order

Comment on lines 514 to 519
if not key:
raise ValueError(
f"No key found in keyvault for keyAltName={key_alt_name}. "
"Run with '--create-data-keys' to create missing keys."
)
return key["_id"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We lose this helpful message:

  File "/home/tim/code/django-mongodb/django_mongodb_backend/schema.py", line 541, in _get_encrypted_fields
    data_key = key["_id"]
               ~~~^^^^^^^
TypeError: 'NoneType' object is not subscriptable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that logic was flawed. Updated to check for existing keys or create if not found.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then showencryptedfieldsmap isn't read-only and may have a side effect of creating keys?

What is still missing for me is the "why" of setting encrypted_fields_map in AutoEncryptionOpts. Based on past discussion in this PR, I recall it has something to do with security, but I don't think this is explained in this PR's documention or in the design doc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then showencryptedfieldsmap isn't read-only and may have a side effect of creating keys?

I've removed --create-data-keys so now we're just talking about showencryptedfieldsmap and yes, since it calls _get_encrypted_fields it will create the data keys if they aren't found in the key vault.

What is still missing for me is the "why" of setting encrypted_fields_map in AutoEncryptionOpts. Based on past discussion in this PR, I recall it has something to do with security, but I don't think this is explained in this PR's documention or in the design doc.

That's in the spec:

Supplying an encryptedFieldsMap provides more security than relying on an encryptedFields obtained from the server. It protects against a malicious server advertising a false encryptedFields.

I don't fully understand that explanation so I haven't gone too far in documenting it other than to say "recommended".

Comment on lines +59 to +63
def test_array(self):
self.assertEqual(len(self.movie.cast), 2)
self.assertEqual(self.movie.cast[0].name, "Actor One")
self.assertEqual(self.movie.cast[1].name, "Actor Two")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like the other tests, I think you meant to fetch the data from the database before checking it.

In addition to removing the arg from the showencryptedfields
command, reduces complexity in schema editor with removal of the
`create_data_keys` boolean. Previous logic may have been flawed in
looking up existing keys.
Also put the key vault inside the test database for easy tear down,
tested and working local.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants