SERVER-39058 Synchronize user set modification in AuthorizationSession with Client

17 files changed, 68 insertions, 35 deletions

diff --git a/src/mongo/db/auth/authorization_session.h b/src/mongo/db/auth/authorization_session.h
index 1c1377df18b..0c4bc0d4f4a 100644
--- a/src/mongo/db/auth/authorization_session.h
+++ b/src/mongo/db/auth/authorization_session.h
@@ -170,12 +170,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(StringData 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.
@@ -338,8 +341,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 65e7558a7f5..f73e8a46b6d 100644
--- a/src/mongo/db/auth/authorization_session_impl.cpp
+++ b/src/mongo/db/auth/authorization_session_impl.cpp
@@ -49,6 +49,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"
@@ -138,6 +139,7 @@ Status AuthorizationSessionImpl::addAndAuthorizeUser(OperationContext* opCtx,
         return AuthorizationManager::authenticationFailedStatus;
     }
 
+    stdx::lock_guard<Client> lk(*opCtx->getClient());
     _authenticatedUsers.add(std::move(user));
 
     // If there are any users and roles in the impersonation data, clear it out.
@@ -167,7 +169,8 @@ User* AuthorizationSessionImpl::getSingleUser() {
     return lookupUser(userName);
 }
 
-void AuthorizationSessionImpl::logoutDatabase(StringData dbname) {
+void AuthorizationSessionImpl::logoutDatabase(OperationContext* opCtx, StringData dbname) {
+    stdx::lock_guard<Client> lk(*opCtx->getClient());
     _authenticatedUsers.removeByDBName(dbname);
     clearImpersonatedUserData();
     _buildAuthenticatedRolesVector();
@@ -192,11 +195,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;
 
@@ -720,10 +732,6 @@ void AuthorizationSessionImpl::_refreshUserInfoAsNeeded(OperationContext* opCtx)
     while (it != _authenticatedUsers.end()) {
         auto& user = *it;
         if (!user->isValid()) {
-            // The user is invalid, so make sure that we erase it from _authenticateUsers at the
-            // end of this block.
-            auto removeGuard = makeGuard([&] { _authenticatedUsers.removeAt(it++); });
-
             // 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();
@@ -731,6 +739,15 @@ void AuthorizationSessionImpl::_refreshUserInfoAsNeeded(OperationContext* opCtx)
 
             auto swUser = authMan.acquireUserForSessionRefresh(opCtx, name, user->getID());
             auto& status = swUser.getStatus();
+
+            // 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());
+
+            // The user is invalid, so make sure that we erase it from _authenticateUsers at the
+            // end of this block.
+            auto removeGuard = makeGuard([&] { _authenticatedUsers.removeAt(it++); });
+
             switch (status.code()) {
                 case ErrorCodes::OK: {
                     updatedUser = std::move(swUser.getValue());
@@ -911,7 +928,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();
@@ -939,6 +956,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 0b6026a1e4a..2cdc380ec77 100644
--- a/src/mongo/db/auth/authorization_session_impl.h
+++ b/src/mongo/db/auth/authorization_session_impl.h
@@ -94,9 +94,11 @@ public:
 
     std::string getAuthenticatedUserNamesToken() override;
 
-    void logoutDatabase(StringData 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;
 
@@ -186,7 +188,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 4e7d6751ba5..b0ec73151e4 100644
--- a/src/mongo/db/auth/authorization_session_test.cpp
+++ b/src/mongo/db/auth/authorization_session_test.cpp
@@ -228,7 +228,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(
@@ -236,7 +236,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 90fe580b0b6..7666955b122 100644
--- a/src/mongo/db/commands/authentication_commands.cpp
+++ b/src/mongo/db/commands/authentication_commands.cpp
@@ -121,7 +121,7 @@ Status _authenticateX509(OperationContext* opCtx, const UserName& user, const BS
                 }
             }
 
-            authorizationSession->grantInternalAuthorization();
+            authorizationSession->grantInternalAuthorization(client);
         }
         // Handle normal client authentication, only applies to client-server connections
         else {
@@ -336,14 +336,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 1306013467d..f6394998de4 100644
--- a/src/mongo/db/commands/kill_op_cmd_base.cpp
+++ b/src/mongo/db/commands/kill_op_cmd_base.cpp
@@ -95,7 +95,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 4d9c82548b0..891c18a9153 100644
--- a/src/mongo/db/index_builder.cpp
+++ b/src/mongo/db/index_builder.cpp
@@ -111,7 +111,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/pipeline/mongo_process_common.cpp b/src/mongo/db/pipeline/mongo_process_common.cpp
index 3ace59e5e23..5db5dd993ee 100644
--- a/src/mongo/db/pipeline/mongo_process_common.cpp
+++ b/src/mongo/db/pipeline/mongo_process_common.cpp
@@ -66,7 +66,7 @@ std::vector<BSONObj> MongoProcessCommon::getCurrentOps(
         // 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 3181bc132ee..140371c2895 100644
--- a/src/mongo/db/repl/bgsync.cpp
+++ b/src/mongo/db/repl/bgsync.cpp
@@ -167,7 +167,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/oplog_applier.cpp b/src/mongo/db/repl/oplog_applier.cpp
index eb06a50d681..a3c4e1953f8 100644
--- a/src/mongo/db/repl/oplog_applier.cpp
+++ b/src/mongo/db/repl/oplog_applier.cpp
@@ -57,7 +57,7 @@ std::unique_ptr<ThreadPool> OplogApplier::makeWriterPool(int threadCount) {
     options.maxThreads = options.minThreads = static_cast<size_t>(threadCount);
     options.onCreateThread = [](const std::string&) {
         Client::initThread(getThreadName());
-        AuthorizationSession::get(cc())->grantInternalAuthorization();
+        AuthorizationSession::get(cc())->grantInternalAuthorization(&cc());
     };
     auto pool = stdx::make_unique<ThreadPool>(options);
     pool->startup();
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 dba73cf46cd..8c6636f86c4 100644
--- a/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp
+++ b/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp
@@ -150,7 +150,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/task_runner.cpp b/src/mongo/db/repl/task_runner.cpp
index f14a7a34937..2226ad2bcba 100644
--- a/src/mongo/db/repl/task_runner.cpp
+++ b/src/mongo/db/repl/task_runner.cpp
@@ -135,7 +135,7 @@ void TaskRunner::_runTasks() {
     // client used to create the operation context.
     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 def44ce2e83..5113d76d537 100644
--- a/src/mongo/db/s/migration_destination_manager.cpp
+++ b/src/mongo/db/s/migration_destination_manager.cpp
@@ -698,7 +698,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/s/transaction_coordinator_futures_util.cpp b/src/mongo/db/s/transaction_coordinator_futures_util.cpp
index 81d101c91e8..c3b68b56887 100644
--- a/src/mongo/db/s/transaction_coordinator_futures_util.cpp
+++ b/src/mongo/db/s/transaction_coordinator_futures_util.cpp
@@ -96,7 +96,8 @@ Future<executor::TaskExecutor::ResponseStatus> AsyncWorkScheduler::scheduleRemot
                                                                                   opCtx) {
             // Note: This internal authorization is tied to the lifetime of 'opCtx', which is
             // destroyed by 'scheduleWork' immediately after this lambda ends.
-            AuthorizationSession::get(Client::getCurrent())->grantInternalAuthorization();
+            AuthorizationSession::get(Client::getCurrent())
+                ->grantInternalAuthorization(Client::getCurrent());
 
             if (MONGO_FAIL_POINT(hangWhileTargetingLocalHost)) {
                 LOG(0) << "Hit hangWhileTargetingLocalHost failpoint";
diff --git a/src/mongo/db/s/txn_two_phase_commit_cmds.cpp b/src/mongo/db/s/txn_two_phase_commit_cmds.cpp
index 276c569fd66..d883cbffc76 100644
--- a/src/mongo/db/s/txn_two_phase_commit_cmds.cpp
+++ b/src/mongo/db/s/txn_two_phase_commit_cmds.cpp
@@ -287,7 +287,7 @@ public:
                 auto uniqueOpCtx = Client::getCurrent()->makeOperationContext();
                 auto opCtx = uniqueOpCtx.get();
 
-                AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization();
+                AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(opCtx);
 
                 auto requestOpMsg =
                     OpMsgRequest::fromDBAndBody(NamespaceString::kAdminDb, abortRequestObj)
diff --git a/src/mongo/db/ttl.cpp b/src/mongo/db/ttl.cpp
index 93db8042248..ddbbba429f5 100644
--- a/src/mongo/db/ttl.cpp
+++ b/src/mongo/db/ttl.cpp
@@ -82,7 +82,7 @@ public:
 
     virtual void run() {
         ThreadClient tc(name(), getGlobalServiceContext());
-        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 13a06665d8c..92b8741aa9a 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 StringData) override {
+    void grantInternalAuthorization(OperationContext* opCtx) override {
+        // Always okay to do something, on embedded.
+    }
+
+    void logoutDatabase(OperationContext* opCtx, const StringData) override {
         UASSERT_NOT_IMPLEMENTED;
     }
 
@@ -237,7 +241,7 @@ public:
         UASSERT_NOT_IMPLEMENTED;
     }
 
-    bool isCoauthorizedWithClient(Client*) override {
+    bool isCoauthorizedWithClient(Client*, WithLock opClientLock) override {
         return true;
     }