feat: expose a ClientEncryption object on encrypted Models #15356
Conversation
| const { paths, singleNestedPaths } = schema; | ||
| yield* Object.keys(paths); | ||
| yield* Object.keys(singleNestedPaths); | ||
| function allNestedPaths(schema) { |
There was a problem hiding this comment.
Just a follow up from #15320 (comment), which merged before I could make the change.
test/encryption/encryption.test.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it.skip('uses the same credentialProviders', async function() { |
There was a problem hiding this comment.
This is a feature of 6.15+ drivers - is this something you plan to upgrade to soon? If not, I can remove this test. If we do adopt 6.15, then I'll leave this test in and we can unskip it when the time comes.
There was a problem hiding this comment.
I will ship a Mongoose release that uses Node driver 6.16 this week.
There was a problem hiding this comment.
Pull Request Overview
A pull request to expose a ClientEncryption object on encrypted models so that users can manage data keys using the Key Management API, while also adding integration tests for encryption functionality.
- Added a static Model.clientEncryption() method to return a ClientEncryption instance.
- Refactored the discriminator helper function allNestedPaths to return an array instead of using a generator.
- Updated documentation to explain how to obtain a ClientEncryption object.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/model.js | Added a clientEncryption helper to create a ClientEncryption instance. |
| lib/helpers/model/discriminator.js | Refactored allNestedPaths to an array-returning function. |
| docs/field-level-encryption.md | Extended documentation to cover managing data keys via ClientEncryption. |
Comments suppressed due to low confidence (1)
lib/helpers/model/discriminator.js:47
- [nitpick] Changing from a generator to returning an array removes lazy evaluation; ensure that this change aligns with all consumer expectations of allNestedPaths.
function allNestedPaths(schema) {
lib/model.js
Outdated
| @@ -69,6 +69,7 @@ const minimize = require('./helpers/minimize'); | |||
| const MongooseBulkSaveIncompleteError = require('./error/bulkSaveIncompleteError'); | |||
| const ObjectExpectedError = require('./error/objectExpected'); | |||
| const decorateBulkWriteResult = require('./helpers/model/decorateBulkWriteResult'); | |||
| const { ClientEncryption } = require('mongodb'); | |||
There was a problem hiding this comment.
Can we move this over to the driver layer? Maybe make lib/drivers/node-mongodb-native/index.js export ClientEncryption?
We currently avoid importing the Node driver directly anywhere other than lib/drivers/node-mongodb-native' (exception being lib/index`) because of Mongoose's browser build.
There was a problem hiding this comment.
I think I've moved the CE export to the Node driver layer. Let me know if that isn't what you were looking for!
lib/model.js
Outdated
| @@ -4880,6 +4881,29 @@ Model.compile = function compile(name, schema, collectionName, connection, base) | |||
| return model; | |||
| }; | |||
|
|
|||
| Model.clientEncryption = function clientEncryption() { | |||
| /** @type(import('mongodb').MongoClient) */ | |||
There was a problem hiding this comment.
Does this comment do anything? I don't think Mongoose supports this type definition syntax
There was a problem hiding this comment.
I meant to revert this. This is TS's type import syntax, which works anywhere there is a TS compiler or language server running (VSCode). I've been using comments like this for auto-completion.
test/encryption/encryption.test.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it.skip('uses the same credentialProviders', async function() { |
There was a problem hiding this comment.
I will ship a Mongoose release that uses Node driver 6.16 this week.
| proxyOptions, | ||
| tlsOptions | ||
| } = autoEncryptionOptions; | ||
| return new ClientEncryption(keyVaultClient ?? client, |
There was a problem hiding this comment.
GitHub Copilot makes a good point - is it worthwhile to cache this or is it inexpensive to create multiple ClientEncryption instances?
There was a problem hiding this comment.
They're generally inexpensive to create. But I also don't expect users to create lots of CE objects - when using auto encryption, they're intended to be used to manage data keys, which isn't something you'd typically do in production code (more of a DBA-type operation).
baileympearson
force-pushed
the
NODE-6894
branch
from
907bfd8 to
9a0a987
Compare
baileympearson
force-pushed
the
NODE-6894
branch
from
1fb7965 to
db72dbf
Compare
lib/model.js
Outdated
| @@ -4880,6 +4880,33 @@ Model.compile = function compile(name, schema, collectionName, connection, base) | |||
| return model; | |||
| }; | |||
|
|
|||
| Model.clientEncryption = function clientEncryption() { | |||
| const ClientEncryption = driver.get().ClientEncryption; | |||
There was a problem hiding this comment.
You should be able to use this.base.driver.get() instead of require('./driver') directly, see Model.bulkWrite() for an example.
Technically drivers are scoped to Mongoose instances, so you can have multiple Mongoose instances using different drivers in one process.
There was a problem hiding this comment.
Didn't realize that - the getter/setters in driver.js seemed to be setting a module-scoped variable.
Fixed!
There was a problem hiding this comment.
Yeah the module scoped variable is the default driver, but individual Mongoose instances can have their own driver which overrides the default.
Summary
With the changes in this PR, users will be able to obtain a ClientEncryption object from encrypted models. This allows users to use the Key Management API to manage data keys for their encrypted collections.
Additionally, I've added miscellaneous other integration tests that confirm that the encryption implementation works with other mongoose features, namely auto index creation and auto collection creation.