SERVER-39058 Synchronize user set modification in AuthorizationSession with Clientr3.4.20-rc0r3.4.20

(cherry picked from commit a9277e874039f32ce0d848fcdfb10de705c96fd9)

16 files changed, 56 insertions, 30 deletions

diff --git a/src/mongo/db/auth/authorization_session.cpp b/src/mongo/db/auth/authorization_session.cpp
index c4f219c15de..bc41c9ab310 100644
--- a/src/mongo/db/auth/authorization_session.cpp
+++ b/src/mongo/db/auth/authorization_session.cpp
@@ -48,6 +48,7 @@
 #include "mongo/db/client.h"
 #include "mongo/db/jsobj.h"
 #include "mongo/db/namespace_string.h"
+#include "mongo/db/operation_context.h"
 #include "mongo/db/pipeline/pipeline.h"
 #include "mongo/util/assert_util.h"
 #include "mongo/util/log.h"
@@ -125,7 +126,8 @@ User* AuthorizationSession::lookupUser(const UserName& name) {
     return _authenticatedUsers.lookup(name);
 }
 
-void AuthorizationSession::logoutDatabase(const std::string& dbname) {
+void AuthorizationSession::logoutDatabase(OperationContext* opCtx, const std::string& dbname) {
+    stdx::lock_guard<Client> lk(*opCtx->getClient());
     User* removedUser = _authenticatedUsers.removeByDBName(dbname);
     if (removedUser) {
         getAuthorizationManager().releaseUser(removedUser);
@@ -153,11 +155,20 @@ std::string AuthorizationSession::getAuthenticatedUserNamesToken() {
     return ret;
 }
 
-void AuthorizationSession::grantInternalAuthorization() {
+void AuthorizationSession::grantInternalAuthorization(Client* client) {
+    stdx::lock_guard<Client> lk(*client);
     _authenticatedUsers.add(internalSecurity.user);
     _buildAuthenticatedRolesVector();
 }
 
+/**
+ * Overloaded function - takes in the opCtx of the current AuthSession
+ * and calls the function above.
+ */
+void AuthorizationSession::grantInternalAuthorization(OperationContext* opCtx) {
+    grantInternalAuthorization(opCtx->getClient());
+}
+
 PrivilegeVector AuthorizationSession::getDefaultPrivileges() {
     PrivilegeVector defaultPrivileges;
 
@@ -748,6 +759,8 @@ void AuthorizationSession::_refreshUserInfoAsNeeded(OperationContext* txn) {
             User* updatedUser;
 
             Status status = authMan.acquireUser(txn, name, &updatedUser);
+            stdx::lock_guard<Client> lk(*txn->getClient());
+
             switch (status.code()) {
                 case ErrorCodes::OK: {
                     // Success! Replace the old User object with the updated one.
@@ -835,7 +848,7 @@ void AuthorizationSession::setImpersonatedUserData(std::vector<UserName> usernam
     _impersonationFlag = true;
 }
 
-bool AuthorizationSession::isCoauthorizedWithClient(Client* opClient) {
+bool AuthorizationSession::isCoauthorizedWithClient(Client* opClient, WithLock opClientLock) {
     auto getUserNames = [](AuthorizationSession* authSession) {
         if (authSession->isImpersonating()) {
             return authSession->getImpersonatedUserNames();
diff --git a/src/mongo/db/auth/authorization_session.h b/src/mongo/db/auth/authorization_session.h
index 2cd46a8d59f..96352b2a560 100644
--- a/src/mongo/db/auth/authorization_session.h
+++ b/src/mongo/db/auth/authorization_session.h
@@ -42,6 +42,7 @@
 #include "mongo/db/auth/user_name.h"
 #include "mongo/db/auth/user_set.h"
 #include "mongo/db/namespace_string.h"
+#include "mongo/util/concurrency/with_lock.h"
 
 namespace mongo {
 
@@ -130,12 +131,15 @@ public:
     std::string getAuthenticatedUserNamesToken();
 
     // Removes any authenticated principals whose authorization credentials came from the given
-    // database, and revokes any privileges that were granted via that principal.
-    void logoutDatabase(const std::string& dbname);
+    // database, and revokes any privileges that were granted via that principal. This function
+    // modifies state. Synchronizes with the Client lock.
+    void logoutDatabase(OperationContext* opCtx, const std::string& dbname);
 
     // Adds the internalSecurity user to the set of authenticated users.
-    // Used to grant internal threads full access.
-    void grantInternalAuthorization();
+    // Used to grant internal threads full access. Takes in the Client
+    // as a parameter so it can take out a lock on the client.
+    void grantInternalAuthorization(Client* client);
+    void grantInternalAuthorization(OperationContext* opCtx);
 
     // Generates a vector of default privileges that are granted to any user,
     // regardless of which roles that user does or does not possess.
@@ -268,8 +272,12 @@ public:
     // authenticated user. If either object has impersonated users,
     // those users will be considered as 'authenticated' for the purpose of this check.
     //
-    // The existence of 'opClient' must be guaranteed through locks taken by the caller.
-    bool isCoauthorizedWithClient(Client* opClient);
+    // The existence of 'opClient' must be guaranteed through locks taken by the caller,
+    // as demonstrated by opClientLock which must be a lock taken on opClient.
+    //
+    // Returns true if the current auth session and the opClient's auth session have users
+    // in common.
+    bool isCoauthorizedWithClient(Client* opClient, WithLock opClientLock);
 
     // Tells whether impersonation is active or not.  This state is set when
     // setImpersonatedUserData is called and cleared when clearImpersonatedUserData is
diff --git a/src/mongo/db/auth/authorization_session_test.cpp b/src/mongo/db/auth/authorization_session_test.cpp
index 0c242b6c13d..29f9966c08d 100644
--- a/src/mongo/db/auth/authorization_session_test.cpp
+++ b/src/mongo/db/auth/authorization_session_test.cpp
@@ -40,6 +40,7 @@
 #include "mongo/db/json.h"
 #include "mongo/db/namespace_string.h"
 #include "mongo/db/operation_context_noop.h"
+#include "mongo/db/service_context_noop.h"
 #include "mongo/stdx/memory.h"
 #include "mongo/unittest/unittest.h"
 #include "mongo/util/map_util.h"
@@ -76,8 +77,10 @@ private:
 
 class AuthorizationSessionTest : public ::mongo::unittest::Test {
 public:
+    std::unique_ptr<ServiceContextNoop> _service = stdx::make_unique<ServiceContextNoop>();
+    ServiceContext::UniqueClient _client = _service.get()->makeClient("test");
     FailureCapableAuthzManagerExternalStateMock* managerState;
-    OperationContextNoop _txn;
+    OperationContextNoop _txn{_client.get(), 0};
     AuthzSessionExternalStateMock* sessionState;
     std::unique_ptr<AuthorizationManager> authzManager;
     std::unique_ptr<AuthorizationSessionForTest> authzSession;
@@ -203,7 +206,7 @@ TEST_F(AuthorizationSessionTest, AddUserAndCheckAuthorization) {
     ASSERT_TRUE(
         authzSession->isAuthorizedForActionsOnResource(testFooCollResource, ActionType::insert));
 
-    authzSession->logoutDatabase("test");
+    authzSession->logoutDatabase(&_txn, "test");
     ASSERT_TRUE(
         authzSession->isAuthorizedForActionsOnResource(otherFooCollResource, ActionType::insert));
     ASSERT_TRUE(
@@ -211,7 +214,7 @@ TEST_F(AuthorizationSessionTest, AddUserAndCheckAuthorization) {
     ASSERT_FALSE(
         authzSession->isAuthorizedForActionsOnResource(testFooCollResource, ActionType::collMod));
 
-    authzSession->logoutDatabase("admin");
+    authzSession->logoutDatabase(&_txn, "admin");
     ASSERT_FALSE(
         authzSession->isAuthorizedForActionsOnResource(otherFooCollResource, ActionType::insert));
     ASSERT_FALSE(
diff --git a/src/mongo/db/commands/authentication_commands.cpp b/src/mongo/db/commands/authentication_commands.cpp
index 68a483b24f9..852c8ab56ed 100644
--- a/src/mongo/db/commands/authentication_commands.cpp
+++ b/src/mongo/db/commands/authentication_commands.cpp
@@ -346,7 +346,7 @@ Status CmdAuthenticate::_authenticateX509(OperationContext* txn,
                               "authentication. The current configuration does not allow "
                               "x.509 cluster authentication, check the --clusterAuthMode flag");
             }
-            authorizationSession->grantInternalAuthorization();
+            authorizationSession->grantInternalAuthorization(client);
         }
         // Handle normal client authentication, only applies to client-server connections
         else {
@@ -386,14 +386,14 @@ public:
              string& errmsg,
              BSONObjBuilder& result) {
         AuthorizationSession* authSession = AuthorizationSession::get(Client::getCurrent());
-        authSession->logoutDatabase(dbname);
+        authSession->logoutDatabase(txn, dbname);
         if (Command::testCommandsEnabled && dbname == "admin") {
             // Allows logging out as the internal user against the admin database, however
             // this actually logs out of the local database as well. This is to
             // support the auth passthrough test framework on mongos (since you can't use the
             // local database on a mongos, so you can't logout as the internal user
             // without this).
-            authSession->logoutDatabase("local");
+            authSession->logoutDatabase(txn, "local");
         }
         return true;
     }
diff --git a/src/mongo/db/commands/current_op.cpp b/src/mongo/db/commands/current_op.cpp
index 0c5572ea385..9f67c4baee7 100644
--- a/src/mongo/db/commands/current_op.cpp
+++ b/src/mongo/db/commands/current_op.cpp
@@ -124,7 +124,8 @@ public:
             stdx::lock_guard<Client> lk(*client);
 
             if (ownOpsOnly &&
-                !AuthorizationSession::get(txn->getClient())->isCoauthorizedWithClient(client)) {
+                !AuthorizationSession::get(txn->getClient())
+                     ->isCoauthorizedWithClient(client, lk)) {
                 continue;
             }
 
diff --git a/src/mongo/db/commands/kill_op.cpp b/src/mongo/db/commands/kill_op.cpp
index f96f3692ecd..1e11fb67466 100644
--- a/src/mongo/db/commands/kill_op.cpp
+++ b/src/mongo/db/commands/kill_op.cpp
@@ -93,7 +93,7 @@ public:
             if (opCtx && opCtx->getOpID() == opId) {
                 if (authzSession->isAuthorizedForActionsOnResource(
                         ResourcePattern::forClusterResource(), ActionType::killop) ||
-                    authzSession->isCoauthorizedWithClient(opClient)) {
+                    authzSession->isCoauthorizedWithClient(opClient, lk)) {
                     return {std::make_tuple(std::move(lk), opCtx)};
                 }
                 break;
diff --git a/src/mongo/db/dbwebserver.cpp b/src/mongo/db/dbwebserver.cpp
index 50fa68a8176..26483994632 100644
--- a/src/mongo/db/dbwebserver.cpp
+++ b/src/mongo/db/dbwebserver.cpp
@@ -495,7 +495,7 @@ bool DbWebServer::_allowed(OperationContext* txn,
     }
 
     if (from.isLocalHost() && !_webUsers->haveAdminUsers(txn)) {
-        authSess->grantInternalAuthorization();
+        authSess->grantInternalAuthorization(txn);
         return true;
     }
 
diff --git a/src/mongo/db/index_builder.cpp b/src/mongo/db/index_builder.cpp
index 59e83d268f1..1627cc20c9d 100644
--- a/src/mongo/db/index_builder.cpp
+++ b/src/mongo/db/index_builder.cpp
@@ -86,7 +86,7 @@ void IndexBuilder::run() {
     OperationContext& txn = *txnPtr;
     txn.lockState()->setShouldConflictWithSecondaryBatchApplication(false);
 
-    AuthorizationSession::get(txn.getClient())->grantInternalAuthorization();
+    AuthorizationSession::get(txn.getClient())->grantInternalAuthorization(txnPtr.get());
 
     {
         stdx::lock_guard<Client> lk(*txn.getClient());
diff --git a/src/mongo/db/index_rebuilder.cpp b/src/mongo/db/index_rebuilder.cpp
index de00ef920d1..b2857b65c0e 100644
--- a/src/mongo/db/index_rebuilder.cpp
+++ b/src/mongo/db/index_rebuilder.cpp
@@ -150,7 +150,7 @@ void forceRestartInProgressIndexesOnCollection(OperationContext* opCtx, const Na
 }
 
 void restartInProgressIndexesFromLastShutdown(OperationContext* txn) {
-    AuthorizationSession::get(txn->getClient())->grantInternalAuthorization();
+    AuthorizationSession::get(txn->getClient())->grantInternalAuthorization(txn);
 
     std::vector<std::string> dbNames;
 
diff --git a/src/mongo/db/repl/bgsync.cpp b/src/mongo/db/repl/bgsync.cpp
index ac49485ede0..551cf74ac59 100644
--- a/src/mongo/db/repl/bgsync.cpp
+++ b/src/mongo/db/repl/bgsync.cpp
@@ -183,7 +183,7 @@ bool BackgroundSync::_inShutdown_inlock() const {
 
 void BackgroundSync::_run() {
     Client::initThread("rsBackgroundSync");
-    AuthorizationSession::get(cc())->grantInternalAuthorization();
+    AuthorizationSession::get(cc())->grantInternalAuthorization(&cc());
 
     while (!inShutdown()) {
         try {
diff --git a/src/mongo/db/repl/master_slave.cpp b/src/mongo/db/repl/master_slave.cpp
index 6929d5be297..82233f36753 100644
--- a/src/mongo/db/repl/master_slave.cpp
+++ b/src/mongo/db/repl/master_slave.cpp
@@ -1315,7 +1315,8 @@ static void replMasterThread() {
         // when things are idle.
         const ServiceContext::UniqueOperationContext txnPtr = cc().makeOperationContext();
         OperationContext& txn = *txnPtr;
-        AuthorizationSession::get(txn.getClient())->grantInternalAuthorization();
+        AuthorizationSession::get(txn.getClient())->grantInternalAuthorization(txnPtr.get());
+
 
         Lock::GlobalWrite globalWrite(txn.lockState(), 1);
         if (globalWrite.isLocked()) {
@@ -1341,7 +1342,7 @@ static void replSlaveThread() {
 
     const ServiceContext::UniqueOperationContext txnPtr = cc().makeOperationContext();
     OperationContext& txn = *txnPtr;
-    AuthorizationSession::get(txn.getClient())->grantInternalAuthorization();
+    AuthorizationSession::get(txn.getClient())->grantInternalAuthorization(txnPtr.get());
     DisableDocumentValidation validationDisabler(&txn);
 
     while (1) {
@@ -1368,7 +1369,7 @@ void startMasterSlave(OperationContext* txn) {
     if (!replSettings.isSlave() && !replSettings.isMaster())
         return;
 
-    AuthorizationSession::get(txn->getClient())->grantInternalAuthorization();
+    AuthorizationSession::get(txn->getClient())->grantInternalAuthorization(txn);
 
     {
         ReplSource temp(txn);  // Ensures local.me is populated
diff --git a/src/mongo/db/repl/rs_sync.cpp b/src/mongo/db/repl/rs_sync.cpp
index b04972c29e4..dd5dd99752e 100644
--- a/src/mongo/db/repl/rs_sync.cpp
+++ b/src/mongo/db/repl/rs_sync.cpp
@@ -65,7 +65,7 @@ void RSDataSync::join() {
 
 void RSDataSync::_run() {
     Client::initThread("rsSync");
-    AuthorizationSession::get(cc())->grantInternalAuthorization();
+    AuthorizationSession::get(cc())->grantInternalAuthorization(&cc());
 
     // Overwrite prefetch index mode in BackgroundSync if ReplSettings has a mode set.
     auto&& replSettings = _replCoord->getSettings();
diff --git a/src/mongo/db/repl/sync_tail.cpp b/src/mongo/db/repl/sync_tail.cpp
index dfd1d564f90..ac75166052d 100644
--- a/src/mongo/db/repl/sync_tail.cpp
+++ b/src/mongo/db/repl/sync_tail.cpp
@@ -148,7 +148,7 @@ ServerStatusMetricField<TimerStats> displayOpBatchesApplied("repl.apply.batches"
 void initializePrefetchThread() {
     if (!Client::getCurrent()) {
         Client::initThreadIfNotAlready();
-        AuthorizationSession::get(cc())->grantInternalAuthorization();
+        AuthorizationSession::get(cc())->grantInternalAuthorization(&cc());
     }
 }
 
@@ -461,7 +461,7 @@ void initializeWriterThread() {
     // Only do this once per thread
     if (!Client::getCurrent()) {
         Client::initThreadIfNotAlready();
-        AuthorizationSession::get(cc())->grantInternalAuthorization();
+        AuthorizationSession::get(cc())->grantInternalAuthorization(&cc());
     }
 }
 
diff --git a/src/mongo/db/repl/task_runner.cpp b/src/mongo/db/repl/task_runner.cpp
index 134160bbdf1..668f6cd7fc3 100644
--- a/src/mongo/db/repl/task_runner.cpp
+++ b/src/mongo/db/repl/task_runner.cpp
@@ -136,7 +136,7 @@ void TaskRunner::_runTasks() {
     Client::initThreadIfNotAlready();
     Client* client = &cc();
     if (AuthorizationManager::get(client->getServiceContext())->isAuthEnabled()) {
-        AuthorizationSession::get(client)->grantInternalAuthorization();
+        AuthorizationSession::get(client)->grantInternalAuthorization(client);
     }
     ServiceContext::UniqueOperationContext txn;
 
diff --git a/src/mongo/db/s/migration_destination_manager.cpp b/src/mongo/db/s/migration_destination_manager.cpp
index 3c62381a1f8..ee38f8f39fe 100644
--- a/src/mongo/db/s/migration_destination_manager.cpp
+++ b/src/mongo/db/s/migration_destination_manager.cpp
@@ -479,7 +479,7 @@ void MigrationDestinationManager::_migrateThread(BSONObj min,
 
     if (getGlobalAuthorizationManager()->isAuthEnabled()) {
         ShardedConnectionInfo::addHook();
-        AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization();
+        AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(&cc());
     }
 
     try {
diff --git a/src/mongo/db/ttl.cpp b/src/mongo/db/ttl.cpp
index 9688f661578..09785151326 100644
--- a/src/mongo/db/ttl.cpp
+++ b/src/mongo/db/ttl.cpp
@@ -85,7 +85,7 @@ public:
 
     virtual void run() {
         Client::initThread(name().c_str());
-        AuthorizationSession::get(cc())->grantInternalAuthorization();
+        AuthorizationSession::get(cc())->grantInternalAuthorization(&cc());
 
         while (!inShutdown()) {
             {