SERVER-39058 Synchronize user set modification in AuthorizationSession with Client

(cherry picked from commit a9277e874039f32ce0d848fcdfb10de705c96fd9)

16 files changed, 63 insertions, 32 deletions

diff --git a/src/mongo/db/auth/authorization_session.h b/src/mongo/db/auth/authorization_session.h
index fd6210ab800..0d81faef43f 100644
--- a/src/mongo/db/auth/authorization_session.h
+++ b/src/mongo/db/auth/authorization_session.h
@@ -43,6 +43,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 {
 
@@ -170,12 +171,15 @@ public:
     virtual std::string getAuthenticatedUserNamesToken() = 0;
 
     // Removes any authenticated principals whose authorization credentials came from the given
-    // database, and revokes any privileges that were granted via that principal.
-    virtual void logoutDatabase(const std::string& dbname) = 0;
+    // database, and revokes any privileges that were granted via that principal. This function
+    // modifies state. Synchronizes with the Client lock.
+    virtual void logoutDatabase(OperationContext* opCtx, StringData dbname) = 0;
 
     // Adds the internalSecurity user to the set of authenticated users.
-    // Used to grant internal threads full access.
-    virtual void grantInternalAuthorization() = 0;
+    // Used to grant internal threads full access. Takes in the Client
+    // as a parameter so it can take out a lock on the client.
+    virtual void grantInternalAuthorization(Client* client) = 0;
+    virtual void grantInternalAuthorization(OperationContext* opCtx) = 0;
 
     // Generates a vector of default privileges that are granted to any user,
     // regardless of which roles that user does or does not possess.
@@ -339,8 +343,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.
-    virtual bool isCoauthorizedWithClient(Client* opClient) = 0;
+    // 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.
+    virtual bool isCoauthorizedWithClient(Client* opClient, WithLock opClientLock) = 0;
 
     // Returns true if the session and 'userNameIter' share an authenticated user, or if both have
     // no authenticated users. Impersonated users are not considered as 'authenticated' for the
diff --git a/src/mongo/db/auth/authorization_session_impl.cpp b/src/mongo/db/auth/authorization_session_impl.cpp
index 446f84cd392..14b508a9bc4 100644
--- a/src/mongo/db/auth/authorization_session_impl.cpp
+++ b/src/mongo/db/auth/authorization_session_impl.cpp
@@ -51,6 +51,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/aggregation_request.h"
 #include "mongo/db/pipeline/lite_parsed_pipeline.h"
 #include "mongo/util/assert_util.h"
@@ -172,6 +173,7 @@ Status AuthorizationSessionImpl::addAndAuthorizeUser(OperationContext* opCtx,
 
     // Calling add() on the UserSet may return a user that was replaced because it was from the
     // same database.
+    stdx::lock_guard<Client> lk(*opCtx->getClient());
     userHolder.reset(_authenticatedUsers.add(userHolder.release()));
 
     // If there are any users and roles in the impersonation data, clear it out.
@@ -201,7 +203,8 @@ User* AuthorizationSessionImpl::getSingleUser() {
     return lookupUser(userName);
 }
 
-void AuthorizationSessionImpl::logoutDatabase(const std::string& dbname) {
+void AuthorizationSessionImpl::logoutDatabase(OperationContext* opCtx, StringData dbname) {
+    stdx::lock_guard<Client> lk(*opCtx->getClient());
     User* removedUser = _authenticatedUsers.removeByDBName(dbname);
     if (removedUser) {
         getAuthorizationManager().releaseUser(removedUser);
@@ -229,11 +232,20 @@ std::string AuthorizationSessionImpl::getAuthenticatedUserNamesToken() {
     return ret;
 }
 
-void AuthorizationSessionImpl::grantInternalAuthorization() {
+void AuthorizationSessionImpl::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 AuthorizationSessionImpl::grantInternalAuthorization(OperationContext* opCtx) {
+    grantInternalAuthorization(opCtx->getClient());
+}
+
 PrivilegeVector AuthorizationSessionImpl::getDefaultPrivileges() {
     PrivilegeVector defaultPrivileges;
 
@@ -819,14 +831,17 @@ void AuthorizationSessionImpl::_refreshUserInfoAsNeeded(OperationContext* opCtx)
     UserSet::iterator it = _authenticatedUsers.begin();
     while (it != _authenticatedUsers.end()) {
         User* user = *it;
-
         if (!user->isValid()) {
             // Make a good faith effort to acquire an up-to-date user object, since the one
             // we've cached is marked "out-of-date."
             UserName name = user->getName();
             User* updatedUser;
-
             Status status = authMan.acquireUser(opCtx, name, &updatedUser);
+
+            // Take out a lock on the client here to ensure that no one reads while
+            // _authenticatedUsers is being modified.
+            stdx::lock_guard<Client> lk(*opCtx->getClient());
+
             switch (status.code()) {
                 case ErrorCodes::OK: {
 
@@ -1015,7 +1030,7 @@ void AuthorizationSessionImpl::setImpersonatedUserData(std::vector<UserName> use
     _impersonationFlag = true;
 }
 
-bool AuthorizationSessionImpl::isCoauthorizedWithClient(Client* opClient) {
+bool AuthorizationSessionImpl::isCoauthorizedWithClient(Client* opClient, WithLock opClientLock) {
     auto getUserNames = [](AuthorizationSession* authSession) {
         if (authSession->isImpersonating()) {
             return authSession->getImpersonatedUserNames();
@@ -1043,6 +1058,7 @@ bool AuthorizationSessionImpl::isCoauthorizedWith(UserNameIterator userNameIter)
     if (!getAuthorizationManager().isAuthEnabled()) {
         return true;
     }
+
     if (!userNameIter.more() && !isAuthenticated()) {
         return true;
     }
diff --git a/src/mongo/db/auth/authorization_session_impl.h b/src/mongo/db/auth/authorization_session_impl.h
index 0bf65595cf7..1006d32a8f2 100644
--- a/src/mongo/db/auth/authorization_session_impl.h
+++ b/src/mongo/db/auth/authorization_session_impl.h
@@ -97,9 +97,11 @@ public:
 
     std::string getAuthenticatedUserNamesToken() override;
 
-    void logoutDatabase(const std::string& dbname) override;
+    void logoutDatabase(OperationContext* opCtx, StringData dbname) override;
 
-    void grantInternalAuthorization() override;
+    void grantInternalAuthorization(Client* client) override;
+
+    void grantInternalAuthorization(OperationContext* opCtx) override;
 
     PrivilegeVector getDefaultPrivileges() override;
 
@@ -190,7 +192,7 @@ public:
 
     void clearImpersonatedUserData() override;
 
-    bool isCoauthorizedWithClient(Client* opClient) override;
+    bool isCoauthorizedWithClient(Client* opClient, WithLock opClientLock) override;
 
     bool isCoauthorizedWith(UserNameIterator userNameIter) override;
 
diff --git a/src/mongo/db/auth/authorization_session_test.cpp b/src/mongo/db/auth/authorization_session_test.cpp
index 5651639ee02..e103f507cd6 100644
--- a/src/mongo/db/auth/authorization_session_test.cpp
+++ b/src/mongo/db/auth/authorization_session_test.cpp
@@ -238,7 +238,7 @@ TEST_F(AuthorizationSessionTest, AddUserAndCheckAuthorization) {
     ASSERT_TRUE(
         authzSession->isAuthorizedForActionsOnResource(testFooCollResource, ActionType::insert));
 
-    authzSession->logoutDatabase("test");
+    authzSession->logoutDatabase(_opCtx.get(), "test");
     ASSERT_TRUE(
         authzSession->isAuthorizedForActionsOnResource(otherFooCollResource, ActionType::insert));
     ASSERT_TRUE(
@@ -246,7 +246,7 @@ TEST_F(AuthorizationSessionTest, AddUserAndCheckAuthorization) {
     ASSERT_FALSE(
         authzSession->isAuthorizedForActionsOnResource(testFooCollResource, ActionType::collMod));
 
-    authzSession->logoutDatabase("admin");
+    authzSession->logoutDatabase(_opCtx.get(), "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 226944af259..3578f361801 100644
--- a/src/mongo/db/commands/authentication_commands.cpp
+++ b/src/mongo/db/commands/authentication_commands.cpp
@@ -105,7 +105,7 @@ Status _authenticateX509(OperationContext* opCtx, const UserName& user, const BS
                               "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 {
@@ -318,14 +318,14 @@ public:
              const BSONObj& cmdObj,
              BSONObjBuilder& result) {
         AuthorizationSession* authSession = AuthorizationSession::get(Client::getCurrent());
-        authSession->logoutDatabase(dbname);
+        authSession->logoutDatabase(opCtx, dbname);
         if (getTestCommandsEnabled() && 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(opCtx, "local");
         }
         return true;
     }
diff --git a/src/mongo/db/commands/kill_op_cmd_base.cpp b/src/mongo/db/commands/kill_op_cmd_base.cpp
index 91824925137..b40a306f7cb 100644
--- a/src/mongo/db/commands/kill_op_cmd_base.cpp
+++ b/src/mongo/db/commands/kill_op_cmd_base.cpp
@@ -96,7 +96,8 @@ KillOpCmdBase::findOpForKilling(Client* client, unsigned int opId) {
         OperationContext* opToKill = std::get<1>(*lockAndOpCtx);
         if (authzSession->isAuthorizedForActionsOnResource(ResourcePattern::forClusterResource(),
                                                            ActionType::killop) ||
-            authzSession->isCoauthorizedWithClient(opToKill->getClient())) {
+            authzSession->isCoauthorizedWithClient(opToKill->getClient(),
+                                                   std::get<0>(*lockAndOpCtx))) {
             return lockAndOpCtx;
         }
     }
diff --git a/src/mongo/db/index_builder.cpp b/src/mongo/db/index_builder.cpp
index 12dd1683fd7..efb8da93795 100644
--- a/src/mongo/db/index_builder.cpp
+++ b/src/mongo/db/index_builder.cpp
@@ -138,7 +138,7 @@ void IndexBuilder::run() {
         unreplicatedWrites.emplace(opCtx.get());
     }
 
-    AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization();
+    AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(opCtx.get());
 
     {
         stdx::lock_guard<Client> lk(*(opCtx->getClient()));
diff --git a/src/mongo/db/index_rebuilder.cpp b/src/mongo/db/index_rebuilder.cpp
index 325abe84654..fe6c51f3823 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* opCtx) {
-    AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization();
+    AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(opCtx);
 
     std::vector<std::string> dbNames;
 
diff --git a/src/mongo/db/pipeline/mongo_process_common.cpp b/src/mongo/db/pipeline/mongo_process_common.cpp
index 905d326e8be..ed409c71ee0 100644
--- a/src/mongo/db/pipeline/mongo_process_common.cpp
+++ b/src/mongo/db/pipeline/mongo_process_common.cpp
@@ -58,7 +58,7 @@ std::vector<BSONObj> MongoProcessCommon::getCurrentOps(OperationContext* opCtx,
         // If auth is disabled, ignore the allUsers parameter.
         if (ctxAuth->getAuthorizationManager().isAuthEnabled() &&
             userMode == CurrentOpUserMode::kExcludeOthers &&
-            !ctxAuth->isCoauthorizedWithClient(client)) {
+            !ctxAuth->isCoauthorizedWithClient(client, lk)) {
             continue;
         }
 
diff --git a/src/mongo/db/repl/bgsync.cpp b/src/mongo/db/repl/bgsync.cpp
index b7f7e5acb4c..08502a9b66b 100644
--- a/src/mongo/db/repl/bgsync.cpp
+++ b/src/mongo/db/repl/bgsync.cpp
@@ -214,7 +214,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/replication_coordinator_external_state_impl.cpp b/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp
index c3e2a123aff..9a455930d43 100644
--- a/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp
+++ b/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp
@@ -156,7 +156,7 @@ auto makeThreadPool(const std::string& poolName) {
     threadPoolOptions.poolName = poolName;
     threadPoolOptions.onCreateThread = [](const std::string& threadName) {
         Client::initThread(threadName.c_str());
-        AuthorizationSession::get(cc())->grantInternalAuthorization();
+        AuthorizationSession::get(cc())->grantInternalAuthorization(&cc());
     };
     return stdx::make_unique<ThreadPool>(threadPoolOptions);
 }
diff --git a/src/mongo/db/repl/sync_tail.cpp b/src/mongo/db/repl/sync_tail.cpp
index cd508472499..e507a9338ab 100644
--- a/src/mongo/db/repl/sync_tail.cpp
+++ b/src/mongo/db/repl/sync_tail.cpp
@@ -340,7 +340,7 @@ std::unique_ptr<ThreadPool> SyncTail::makeWriterPool(int threadCount) {
         // Only do this once per thread
         if (!Client::getCurrent()) {
             Client::initThreadIfNotAlready();
-            AuthorizationSession::get(cc())->grantInternalAuthorization();
+            AuthorizationSession::get(cc())->grantInternalAuthorization(&cc());
         }
     };
     auto pool = stdx::make_unique<ThreadPool>(options);
diff --git a/src/mongo/db/repl/task_runner.cpp b/src/mongo/db/repl/task_runner.cpp
index 94bd65d1d8d..2c85545e729 100644
--- a/src/mongo/db/repl/task_runner.cpp
+++ b/src/mongo/db/repl/task_runner.cpp
@@ -137,7 +137,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 opCtx;
 
diff --git a/src/mongo/db/s/migration_destination_manager.cpp b/src/mongo/db/s/migration_destination_manager.cpp
index 85e48f00c63..2233daf726f 100644
--- a/src/mongo/db/s/migration_destination_manager.cpp
+++ b/src/mongo/db/s/migration_destination_manager.cpp
@@ -674,7 +674,7 @@ void MigrationDestinationManager::_migrateThread() {
 
 
     if (AuthorizationManager::get(opCtx->getServiceContext())->isAuthEnabled()) {
-        AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization();
+        AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(opCtx.get());
     }
 
     try {
diff --git a/src/mongo/db/ttl.cpp b/src/mongo/db/ttl.cpp
index aa22d6d62a4..45252ed951b 100644
--- a/src/mongo/db/ttl.cpp
+++ b/src/mongo/db/ttl.cpp
@@ -88,7 +88,7 @@ public:
     virtual void run() {
         Client::initThread(name().c_str());
         ON_BLOCK_EXIT([] { Client::destroy(); });
-        AuthorizationSession::get(cc())->grantInternalAuthorization();
+        AuthorizationSession::get(cc())->grantInternalAuthorization(&cc());
 
         while (!globalInShutdownDeprecated()) {
             {
diff --git a/src/mongo/embedded/embedded_auth_session.cpp b/src/mongo/embedded/embedded_auth_session.cpp
index d225e507958..ac5d068a650 100644
--- a/src/mongo/embedded/embedded_auth_session.cpp
+++ b/src/mongo/embedded/embedded_auth_session.cpp
@@ -97,11 +97,15 @@ public:
         UASSERT_NOT_IMPLEMENTED;
     }
 
-    void grantInternalAuthorization() override {
+    void grantInternalAuthorization(Client* client) override {
         // Always okay to do something, on embedded.
     }
 
-    void logoutDatabase(const std::string&) override {
+    void grantInternalAuthorization(OperationContext* opCtx) override {
+        // Always okay to do something, on embedded.
+    }
+
+    void logoutDatabase(OperationContext* opCtx, const StringData) override {
         UASSERT_NOT_IMPLEMENTED;
     }
 
@@ -234,7 +238,7 @@ public:
         UASSERT_NOT_IMPLEMENTED;
     }
 
-    bool isCoauthorizedWithClient(Client*) override {
+    bool isCoauthorizedWithClient(Client*, WithLock opClientLock) override {
         return true;
     }