PHPLIB-1708 Cast empty KMS provider into an object #1757
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GromNaN
force-pushed
the
PHPLIB-1708
branch
2 times, most recently
from
1c52490 to
d96aa71
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // The server requires an empty document for automatic credentials. | ||
| if (isset($options['kmsProviders']) && is_array($options['kmsProviders'])) { | ||
| foreach ($options['kmsProviders'] as $name => $provider) { |
Check notice
Code scanning / Psalm
MixedAssignment Note
|
Fixed in Doctrine MongoDB ODM Bundle by setting a DI |
src/Client.php
Outdated
| @@ -457,6 +447,28 @@ public function watch(array $pipeline = [], array $options = []) | |||
| return $operation->execute($server); | |||
| } | |||
|
|
|||
| private function formatEncryptionOptions(array $options): array | |||
There was a problem hiding this comment.
| private function formatEncryptionOptions(array $options): array | |
| private function prepareAutoEncryptionOptions(array $options): array |
I'm not sure if there's prior art in PHPLIB, but I use "prepare" extensively in PHPC for this sort of thing. Up to you, though.
I do like including "AutoEncryption" in the name here since it refers to that driver option.
There was a problem hiding this comment.
Ok for "prepare", but no for "AutoEncryption": the method createClientEncryption isn't specific to auto encryption.
src/Client.php
Outdated
| if (isset($options['kmsProviders']) && is_array($options['kmsProviders'])) { | ||
| foreach ($options['kmsProviders'] as $name => $provider) { | ||
| if ($provider === []) { | ||
| $options['kmsProviders'][$name] = Document::fromPHP([]); |
There was a problem hiding this comment.
Any reason to use Document::fromPHP([]) instead of (object) [] or new stdClass? PHPC is going to end up encoding this as BSON anyway.
There was a problem hiding this comment.
I have no idea what's the more efficient. Changed to new stdClass
| @@ -37,6 +37,18 @@ public function testConstructorAutoEncryptionOpts(): void | |||
| new Client(static::getUri(), [], ['autoEncryption' => $autoEncryptionOpts]); | |||
| } | |||
|
|
|||
| #[DoesNotPerformAssertions] | |||
| public function testConstructorEmptyKmsProvider(): void | |||
There was a problem hiding this comment.
Would this test have failed without the above patch? If not, is anything being tested at all?
There was a problem hiding this comment.
Yes:
MongoDB\Driver\Exception\EncryptionException: expected BSON document for field: gcp
Fix PHPLIB-1708
For some KMS providers, all options can be omitted. The authentication is done using the env var or the system.
https://github.com/mongodb/specifications/blob/master/source/client-side-encryption/client-side-encryption.md#credentialproviders
The approach in doctrine/mongodb-odm#2801 is not sufficient because Symfony is not able to dump the empty object instance into the container.