Skip to content

PHPLIB-1702: Always consult server encryptedFieldsMap when dropping collections #1742

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Collection.php
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ public function drop(array $options = []): void

if (! isset($options['encryptedFields'])) {
$options['encryptedFields'] = get_encrypted_fields_from_driver($this->databaseName, $this->collectionName, $this->manager)
?? get_encrypted_fields_from_server($this->databaseName, $this->collectionName, $this->manager, $server);
?? get_encrypted_fields_from_server($this->databaseName, $this->collectionName, $server);
}

$operation = isset($options['encryptedFields'])
Expand Down
2 changes: 1 addition & 1 deletion src/Database.php
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ public function dropCollection(string $collectionName, array $options = []): voi

if (! isset($options['encryptedFields'])) {
$options['encryptedFields'] = get_encrypted_fields_from_driver($this->databaseName, $collectionName, $this->manager)
?? get_encrypted_fields_from_server($this->databaseName, $collectionName, $this->manager, $server);
?? get_encrypted_fields_from_server($this->databaseName, $collectionName, $server);
}

$operation = isset($options['encryptedFields'])
Expand Down
7 changes: 1 addition & 6 deletions src/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -198,13 +198,8 @@ function get_encrypted_fields_from_driver(string $databaseName, string $collecti
* @see Collection::drop()
* @see Database::dropCollection()
*/
function get_encrypted_fields_from_server(string $databaseName, string $collectionName, Manager $manager, Server $server): array|object|null
function get_encrypted_fields_from_server(string $databaseName, string $collectionName, Server $server): array|object|null
{
// No-op if the encryptedFieldsMap autoEncryption driver option was omitted
if ($manager->getEncryptedFieldsMap() === null) {
return null;
}

$collectionInfoIterator = (new ListCollections($databaseName, ['filter' => ['name' => $collectionName]]))->execute($server);

foreach ($collectionInfoIterator as $collectionInfo) {
Expand Down
64 changes: 64 additions & 0 deletions tests/Collection/DropEncryptedCollectionFunctionalTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php

namespace MongoDB\Tests\Collection;

use MongoDB\BSON\Binary;
use MongoDB\Database;
use MongoDB\Driver\ClientEncryption;
use MongoDB\Driver\WriteConcern;

use function iterator_count;
use function str_repeat;

class DropEncryptedCollectionFunctionalTest extends FunctionalTestCase
{
protected ClientEncryption $clientEncryption;
protected Database $database;

public function setUp(): void
{
parent::setUp();

$this->skipIfClientSideEncryptionIsNotSupported();

if ($this->isStandalone()) {
$this->markTestSkipped('Queryable encryption requires replica sets');
}

$this->skipIfServerVersion('<', '7.0.0', 'Queryable encryption requires MongoDB 7.0 or later');

$client = static::createTestClient();

// Ensure the key vault collection is dropped before each test
$collection = $client->selectCollection('keyvault', 'datakeys', ['writeConcern' => new WriteConcern(WriteConcern::MAJORITY)]);
Copy link
Member

Choose a reason for hiding this comment

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

You'll get used to the new name of this method.

Suggested change
$collection = $client->selectCollection('keyvault', 'datakeys', ['writeConcern' => new WriteConcern(WriteConcern::MAJORITY)]);
$collection = $client->getCollection('keyvault', 'datakeys', ['writeConcern' => new WriteConcern(WriteConcern::MAJORITY)]);

Why do you need to set the writeConcern, as that doesn't guarantee that all replica are up to date.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm borrowing that from the CSFLE spec tests, which use w:majority for all ops targeting the key vault collection. I reckon it makes little difference for tests, but in a production environment this would ensure that any modifications will persist through a replica set election.

$collection->drop();

$this->clientEncryption = $client->createClientEncryption([
'keyVaultNamespace' => 'keyvault.datakeys',
'kmsProviders' => ['local' => ['key' => new Binary(str_repeat("\0", 96)) ]],
]);

$this->database = $client->getDatabase($this->getDatabaseName());
}

/** @see https://jira.mongodb.org/browse/PHPLIB-1702 */
public function testDropConsultsEncryptedFieldsFromServer(): void
{
$originalNumCollections = iterator_count($this->database->listCollectionNames());

$this->database->createEncryptedCollection(
$this->getCollectionName(),
$this->clientEncryption,
'local',
null,
['encryptedFields' => ['fields' => []]],
);

// createEncryptedCollection should create three collections
$this->assertCount($originalNumCollections + 3, $this->database->listCollectionNames());

$this->collection->drop();

$this->assertCount($originalNumCollections, $this->database->listCollectionNames());
}
}
60 changes: 60 additions & 0 deletions tests/Database/DropEncryptedCollectionFunctionalTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
<?php

namespace MongoDB\Tests\Database;

use MongoDB\BSON\Binary;
use MongoDB\Driver\ClientEncryption;
use MongoDB\Driver\WriteConcern;

use function iterator_count;
use function str_repeat;

class DropEncryptedCollectionFunctionalTest extends FunctionalTestCase
{
protected ClientEncryption $clientEncryption;

public function setUp(): void
Copy link
Member

Choose a reason for hiding this comment

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

Both test classes has the same setUp method. They should probably be a single test class with 2 test methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Converted this to a single test file in a new PR.

{
parent::setUp();

$this->skipIfClientSideEncryptionIsNotSupported();

if ($this->isStandalone()) {
$this->markTestSkipped('Queryable encryption requires replica sets');
}

$this->skipIfServerVersion('<', '7.0.0', 'Queryable encryption requires MongoDB 7.0 or later');

$client = static::createTestClient();

// Ensure the key vault collection is dropped before each test
$collection = $client->selectCollection('keyvault', 'datakeys', ['writeConcern' => new WriteConcern(WriteConcern::MAJORITY)]);
$collection->drop();

$this->clientEncryption = $client->createClientEncryption([
'keyVaultNamespace' => 'keyvault.datakeys',
'kmsProviders' => ['local' => ['key' => new Binary(str_repeat("\0", 96)) ]],
]);
}

/** @see https://jira.mongodb.org/browse/PHPLIB-1702 */
public function testDropCollectionConsultsEncryptedFieldsFromServer(): void
{
$originalNumCollections = iterator_count($this->database->listCollectionNames());

$this->database->createEncryptedCollection(
$this->getCollectionName(),
$this->clientEncryption,
'local',
null,
['encryptedFields' => ['fields' => []]],
);

// createEncryptedCollection should create three collections
$this->assertCount($originalNumCollections + 3, $this->database->listCollectionNames());

$this->database->dropCollection($this->getCollectionName());

$this->assertCount($originalNumCollections, $this->database->listCollectionNames());
}
}
Loading