From fc15786481d4e5e9ae1192425944e19d446a418e Mon Sep 17 00:00:00 2001 From: Serhii Lysenko <96662597+serhiitea@users.noreply.github.com> Date: Thu, 27 Mar 2025 18:28:39 +0000 Subject: [PATCH] SERVER-102870: Make $mergeCursors internal (#34112) GitOrigin-RevId: ee527360b84c6798535ee0895de3c7186b3522f9 --- .../api_version_stage_allowance_checks.js | 12 +++---- jstests/aggregation/bugs/server95350.js | 34 ------------------ jstests/auth/lib/commands_lib.js | 36 +++++++++++++++++++ .../db/pipeline/lite_parsed_document_source.h | 26 ++++++++++++++ .../s/query/document_source_merge_cursors.cpp | 8 ++--- 5 files changed, 72 insertions(+), 44 deletions(-) delete mode 100644 jstests/aggregation/bugs/server95350.js diff --git a/jstests/aggregation/api_version_stage_allowance_checks.js b/jstests/aggregation/api_version_stage_allowance_checks.js index 0c3e404842e..abde8d84186 100644 --- a/jstests/aggregation/api_version_stage_allowance_checks.js +++ b/jstests/aggregation/api_version_stage_allowance_checks.js @@ -32,7 +32,7 @@ const testInternalClient = (function createInternalClient() { const curDB = testInternalClient.getDB(dbName); -// Tests that the internal stage '$mergeCursors' does not throw 'ApiStrictError' with an internal +// Tests that the internal stage '$mergeCursors' is allowed in requests with an internal // client and 'apiStrict' set to true. let result = curDB.runCommand({ aggregate: collName, @@ -52,8 +52,8 @@ let result = curDB.runCommand({ }); assert.commandWorked(result); -// Tests that the internal stage '$mergeCursors' throws 'ApiStrictError' with default external -// client when 'apiStrict' is set to true. +// Tests that the internal stage '$mergeCursors' is not allowed in user requests with default +// external client when 'apiStrict' is set to true. result = testDB.runCommand({ aggregate: collName, pipeline: [{ @@ -70,9 +70,9 @@ result = testDB.runCommand({ apiVersion: "1", apiStrict: true }); -assert.commandFailedWithCode(result, ErrorCodes.APIStrictError); +assert.commandFailedWithCode(result, 5491300); -// Tests that the internal stage '$mergeCursors' should not fail with 'ApiStrictError' with default +// Tests that the internal stage '$mergeCursors' is not allowed with default // external client without specifying 'apiStrict' flag. result = testDB.runCommand({ aggregate: collName, @@ -89,7 +89,7 @@ result = testDB.runCommand({ writeConcern: {w: "majority"}, apiVersion: "1" }); -assert.commandWorked(result); +assert.commandFailedWithCode(result, 5491300); // Tests that the 'exchange' option cannot be specified by external client with 'apiStrict' set to // true. diff --git a/jstests/aggregation/bugs/server95350.js b/jstests/aggregation/bugs/server95350.js deleted file mode 100644 index b1f939cb483..00000000000 --- a/jstests/aggregation/bugs/server95350.js +++ /dev/null @@ -1,34 +0,0 @@ -/** - * SERVER-95350: Fix jstests/aggregation/api_version_stage_allowance_checks.js. - */ -(function() { -"use strict"; - -load("jstests/libs/collection_drop_recreate.js"); // For assertDropAndRecreateCollection. - -const collName = 'test'; -assertDropAndRecreateCollection(db, collName); - -let docs = []; -for (let i = 0; i < 10; ++i) { - docs.push({x: i, y: i, z: i}); -} -db[collName].insertMany(docs); - -assert.commandWorked(db.runCommand({ - explain: { - aggregate: collName, - pipeline: [{ - $mergeCursors: { - sort: {y: 1, z: 1}, - compareWholeSortKey: false, - remotes: [], - nss: "test.mergeCursors", - allowPartialResults: false, - } - }], - cursor: {}, - readConcern: {}, - }, -})); -})(); diff --git a/jstests/auth/lib/commands_lib.js b/jstests/auth/lib/commands_lib.js index a6a98491420..a09bace674c 100644 --- a/jstests/auth/lib/commands_lib.js +++ b/jstests/auth/lib/commands_lib.js @@ -6637,6 +6637,42 @@ var authCommandsLib = { }, ] }, + { + testname: "aggregate_$mergeCursors", + command: { + aggregate: "foo", + pipeline: [{ + $mergeCursors: { + sort: {y: 1, z: 1}, + compareWholeSortKey: false, + remotes: [], + nss: "test.mergeCursors", + allowPartialResults: false, + } + }], + cursor: {}, + }, + testcases: [ + { + runOnDb: firstDbName, + roles: {__system: 1}, + // $mergeCursors requires __system role OR a user with internal and find action types as privileges. + expectFail: true, + privileges: [ + {resource: {cluster: true}, actions: ["internal"]}, + {resource: {db: firstDbName, collection: "foo"}, actions: ["find"]}, + ], + }, + { + runOnDb: firstDbName, + // Find action type as a privilege alone is not sufficient for $mergeCursors. + expectAuthzFailure: true, + privileges: [ + {resource: {db: firstDbName, collection: "foo"}, actions: ["find"]}, + ], + }, + ], + }, { testname: "validate_db_metadata_command_specific_db", command: { diff --git a/src/mongo/db/pipeline/lite_parsed_document_source.h b/src/mongo/db/pipeline/lite_parsed_document_source.h index aed1d4fefcf..19458633dc7 100644 --- a/src/mongo/db/pipeline/lite_parsed_document_source.h +++ b/src/mongo/db/pipeline/lite_parsed_document_source.h @@ -295,6 +295,32 @@ public: } }; +class LiteParsedDocumentSourceInternal final : public LiteParsedDocumentSource { +public: + /** + * Creates the default LiteParsedDocumentSource for internal document sources. This requires + * the privilege on 'internal' action. This should still be used with caution. Make sure your + * stage doesn't need to communicate any special behavior before registering a DocumentSource + * using this parser. + */ + static std::unique_ptr parse(const NamespaceString& nss, + const BSONElement& spec) { + return std::make_unique(spec.fieldName()); + } + + LiteParsedDocumentSourceInternal(std::string parseTimeName) + : LiteParsedDocumentSource(std::move(parseTimeName)) {} + + stdx::unordered_set getInvolvedNamespaces() const final { + return stdx::unordered_set(); + } + + PrivilegeVector requiredPrivileges(bool isMongos, bool bypassDocumentValidation) const final { + return {Privilege(ResourcePattern::forClusterResource(), ActionSet{ActionType::internal})}; + } +}; + + /** * Helper class for DocumentSources which reference a foreign collection. */ diff --git a/src/mongo/s/query/document_source_merge_cursors.cpp b/src/mongo/s/query/document_source_merge_cursors.cpp index 02e4a1d24d5..1bb09503e99 100644 --- a/src/mongo/s/query/document_source_merge_cursors.cpp +++ b/src/mongo/s/query/document_source_merge_cursors.cpp @@ -38,10 +38,10 @@ namespace mongo { -REGISTER_DOCUMENT_SOURCE(mergeCursors, - LiteParsedDocumentSourceDefault::parse, - DocumentSourceMergeCursors::createFromBson, - AllowedWithApiStrict::kInternal); +REGISTER_INTERNAL_DOCUMENT_SOURCE(mergeCursors, + LiteParsedDocumentSourceInternal::parse, + DocumentSourceMergeCursors::createFromBson, + true); constexpr StringData DocumentSourceMergeCursors::kStageName;