diff options
author | Xiangyu Yao <xiangyu.yao@mongodb.com> | 2019-08-28 00:47:34 +0000 |
---|---|---|
committer | evergreen <evergreen@mongodb.com> | 2019-08-28 00:47:34 +0000 |
commit | 18f95f8ad46c685d8529dba2d5655b3e4ef968c1 (patch) | |
tree | 1c7d69da6eedf16d1c317ca1e9c4b36bc460edd6 | |
parent | 33621eab05f101b161ae8834cb8efc3980e8f17f (diff) | |
download | mongo-18f95f8ad46c685d8529dba2d5655b3e4ef968c1.tar.gz |
SERVER-41947 Disallow using the system.views collection name as the source or target names in the rename command
7 files changed, 30 insertions, 152 deletions
diff --git a/buildscripts/resmokeconfig/suites/causally_consistent_jscore_passthrough_auth.yml b/buildscripts/resmokeconfig/suites/causally_consistent_jscore_passthrough_auth.yml index 620ad2fe5be..1728969d16f 100644 --- a/buildscripts/resmokeconfig/suites/causally_consistent_jscore_passthrough_auth.yml +++ b/buildscripts/resmokeconfig/suites/causally_consistent_jscore_passthrough_auth.yml @@ -125,7 +125,6 @@ selector: - jstests/core/views/view_with_invalid_dbname.js - jstests/core/views/views_creation.js - jstests/core/views/views_drop.js - - jstests/core/views/views_rename.js # The tests below use applyOps, SERVER-1439. - jstests/core/list_collections1.js diff --git a/jstests/change_streams/whole_cluster_metadata_notifications.js b/jstests/change_streams/whole_cluster_metadata_notifications.js index 8c72df1ba44..e960affc2ef 100644 --- a/jstests/change_streams/whole_cluster_metadata_notifications.js +++ b/jstests/change_streams/whole_cluster_metadata_notifications.js @@ -191,46 +191,6 @@ for (let collToInvalidate of [db1Coll, db2Coll]) { assert.commandWorked(collToInvalidate.insert({_id: 2})); assert.eq(cst.getOneChange(aggCursor).operationType, "insert"); - // Test that renaming a "system" collection to a user collection *does* return a rename - // notification. - assert.commandWorked( - testDB.runCommand({create: "view1", viewOn: collToInvalidate.getName(), pipeline: []})); - assert.commandWorked(testDB.system.views.renameCollection("non_system_collection")); - cst.assertNextChangesEqual({ - cursor: aggCursor, - expectedChanges: [{ - operationType: "rename", - ns: {db: testDB.getName(), coll: "system.views"}, - to: {db: testDB.getName(), coll: "non_system_collection"} - }], - }); - - // Test that renaming a "system" collection to a different "system" collection does not - // result in a notification in the change stream. - aggCursor = cst.startWatchingAllChangesForCluster(); - assert.commandWorked( - testDB.runCommand({create: "view1", viewOn: collToInvalidate.getName(), pipeline: []})); - // Note that the target of the rename must be a valid "system" collection. - assert.commandWorked(testDB.system.views.renameCollection("system.users")); - // Verify that the change stream filters out the rename above, instead returning the - // next insert to the test collection. - assert.commandWorked(collToInvalidate.insert({_id: 1})); - change = cst.getOneChange(aggCursor); - assert.eq(change.operationType, "insert", tojson(change)); - assert.eq(change.ns, {db: testDB.getName(), coll: collToInvalidate.getName()}); - - // Test that renaming a user collection to a "system" collection *does* return a rename - // notification. - assert.commandWorked(collToInvalidate.renameCollection("system.views")); - cst.assertNextChangesEqual({ - cursor: aggCursor, - expectedChanges: [{ - operationType: "rename", - ns: {db: testDB.getName(), coll: collToInvalidate.getName()}, - to: {db: testDB.getName(), coll: "system.views"} - }], - }); - // Drop the "system.views" collection to avoid view catalog errors in subsequent tests. assertDropCollection(testDB, "system.views"); diff --git a/jstests/change_streams/whole_db_metadata_notifications.js b/jstests/change_streams/whole_db_metadata_notifications.js index 1500402bc1c..45f751c9aa0 100644 --- a/jstests/change_streams/whole_db_metadata_notifications.js +++ b/jstests/change_streams/whole_db_metadata_notifications.js @@ -194,54 +194,6 @@ change = cst.getOneChange(aggCursor); assert.eq(change.operationType, "insert", tojson(change)); assert.eq(change.ns, {db: testDB.getName(), coll: coll.getName()}); -// Test that renaming a "system" collection *does* return a notification if the target of -// the rename is a non-system collection. -assert.commandWorked(testDB.runCommand({create: "view1", viewOn: coll.getName(), pipeline: []})); -assert.commandWorked(testDB.system.views.renameCollection("non_system_collection")); -cst.assertNextChangesEqual({ - cursor: aggCursor, - expectedChanges: [{ - operationType: "rename", - ns: {db: testDB.getName(), coll: "system.views"}, - to: {db: testDB.getName(), coll: "non_system_collection"} - }], -}); - -// Test that renaming a "system" collection to a different "system" collection does not -// result in a notification in the change stream. -aggCursor = cst.startWatchingChanges({pipeline: [{$changeStream: {}}], collection: 1}); -assert.commandWorked(testDB.runCommand({create: "view1", viewOn: coll.getName(), pipeline: []})); -// Note that the target of the rename must be a valid "system" collection. -assert.commandWorked(testDB.system.views.renameCollection("system.users")); -// Verify that the change stream filters out the rename above, instead returning the next insert -// to the test collection. -assert.commandWorked(coll.insert({_id: 1})); -change = cst.getOneChange(aggCursor); -assert.eq(change.operationType, "insert", tojson(change)); -assert.eq(change.ns, {db: testDB.getName(), coll: coll.getName()}); - -// Test that renaming a user collection to a "system" collection *is* returned in the change -// stream. -assert.commandWorked(coll.renameCollection("system.views")); -cst.assertNextChangesEqual({ - cursor: aggCursor, - expectedChanges: [{ - operationType: "rename", - ns: {db: testDB.getName(), coll: coll.getName()}, - to: {db: testDB.getName(), coll: "system.views"} - }], -}); - -// Drop the "system.views" collection to avoid view catalog errors in subsequent tests. -assertDropCollection(testDB, "system.views"); -assertDropCollection(testDB, "non_system_collection"); -cst.assertNextChangesEqual({ - cursor: aggCursor, - expectedChanges: [ - {operationType: "drop", ns: {db: testDB.getName(), coll: "non_system_collection"}}, - ] -}); - // Dropping the database should generate a 'dropDatabase' notification followed by an // 'invalidate'. assert.commandWorked(testDB.dropDatabase()); diff --git a/jstests/core/views/views_rename.js b/jstests/core/views/views_rename.js deleted file mode 100644 index ee0d2bfbd11..00000000000 --- a/jstests/core/views/views_rename.js +++ /dev/null @@ -1,27 +0,0 @@ -// @tags: [ -// assumes_superuser_permissions, -// requires_fastcount, -// requires_non_retryable_commands, -// ] - -(function() { -// SERVER-30406 Test that renaming system.views correctly invalidates the view catalog -'use strict'; - -const collName = "views_rename_test"; -let coll = db.getCollection(collName); - -db.view.drop(); -coll.drop(); -assert.commandWorked(db.createView("view", collName, [])); -assert.commandWorked(coll.insert({_id: 1})); -assert.eq(db.view.find().count(), 1, "couldn't find document in view"); -assert.commandWorked(db.system.views.renameCollection("views", /*dropTarget*/ true)); -assert.eq(db.view.find().count(), - 0, - "find on view should have returned no results after renaming away system.views"); -assert.commandWorked(db.views.renameCollection("system.views")); -assert.eq(db.view.find().count(), - 1, - "find on view should have worked again after renaming system.views back in place"); -})(); diff --git a/jstests/noPassthrough/view_catalog_deadlock_with_rename.js b/jstests/noPassthrough/view_catalog_deadlock_with_rename.js deleted file mode 100644 index ec3aa9d21fd..00000000000 --- a/jstests/noPassthrough/view_catalog_deadlock_with_rename.js +++ /dev/null @@ -1,36 +0,0 @@ -/** - * This test demonstrates the deadlock we found in SERVER-40994: Rename locks - * the source and target based on their resourceId order, and a delete op on an - * non-existent collection (which also triggers a view catalog reload) locks the - * same two namespaces in different order. - * - * The fix is to always lock 'system.views' collection in the end. - */ -(function() { -'use strict'; - -const conn = MongoRunner.runMongod(); -const db = conn.getDB('test'); - -assert.commandWorked(db.runCommand({insert: 'a', documents: [{x: 1}]})); -assert.commandWorked(db.runCommand({insert: 'b', documents: [{x: 1}]})); - -assert.commandWorked(db.createView('viewA', 'a', [])); - -// Will cause a view catalog reload. -assert.commandWorked(db.runCommand( - {insert: 'system.views', documents: [{_id: 'test.viewB', viewOn: 'b', pipeline: []}]})); - -const renameSystemViews = startParallelShell(function() { - // This used to first lock 'test.system.views' and then 'test.aaabb' in X mode. - assert.commandWorked( - db.adminCommand({renameCollection: 'test.system.views', to: 'test.aaabb'})); -}, conn.port); - -// This triggers view catalog reload. Therefore it first locked 'test.aaabb' in IX mode and then -// 'test.system.views' in IS mode. -assert.commandWorked(db.runCommand({delete: 'aaabb', deletes: [{q: {x: 2}, limit: 1}]})); - -renameSystemViews(); -MongoRunner.stopMongod(conn); -})(); diff --git a/jstests/noPassthroughWithMongod/disallow_system_views_rename.js b/jstests/noPassthroughWithMongod/disallow_system_views_rename.js new file mode 100644 index 00000000000..c9bc901041e --- /dev/null +++ b/jstests/noPassthroughWithMongod/disallow_system_views_rename.js @@ -0,0 +1,24 @@ +(function() { +'use strict'; +assert.commandWorked(db.runCommand({insert: 'a', documents: [{x: 1}]})); + +// Disallow renaming to system.views +assert.commandFailedWithCode(db.adminCommand({renameCollection: 'test.a', to: 'test.system.views'}), + ErrorCodes.IllegalOperation); + +assert.commandWorked(db.createView('viewA', 'a', [])); + +// Disallow renaming from system.views +assert.commandFailedWithCode( + db.adminCommand({renameCollection: 'test.system.views', to: 'test.aaabb'}), + ErrorCodes.IllegalOperation); + +// User can still rename system.views (or to system.views) via applyOps command. +assert.commandWorked(db.adminCommand({ + applyOps: [{op: 'c', ns: 'test.$cmd', o: {renameCollection: 'test.system.views', to: 'test.b'}}] +})); + +assert.commandWorked(db.adminCommand({ + applyOps: [{op: 'c', ns: 'test.$cmd', o: {renameCollection: 'test.b', to: 'test.system.views'}}] +})); +})(); diff --git a/src/mongo/db/catalog/rename_collection.cpp b/src/mongo/db/catalog/rename_collection.cpp index 13914e84a22..472a7f46868 100644 --- a/src/mongo/db/catalog/rename_collection.cpp +++ b/src/mongo/db/catalog/rename_collection.cpp @@ -747,6 +747,12 @@ Status renameCollection(OperationContext* opCtx, << source); } + if (source.isSystemDotViews() || target.isSystemDotViews()) { + return Status( + ErrorCodes::IllegalOperation, + "renaming system.views collection or renaming to system.views is not allowed"); + } + const std::string dropTargetMsg = options.dropTarget ? " and drop " + target.toString() + "." : "."; log() << "renameCollectionForCommand: rename " << source << " to " << target << dropTargetMsg; |