SERVER-9518 Fix problem related to iterator invalidation.

Also, correctly handle deleted users and intermittent availability failures of the
user data collection.

Includes some unit testing.

5 files changed, 162 insertions, 38 deletions

diff --git a/src/mongo/db/auth/authorization_session.cpp b/src/mongo/db/auth/authorization_session.cpp
index 02d659a1b6a..656d6b148e7 100644
--- a/src/mongo/db/auth/authorization_session.cpp
+++ b/src/mongo/db/auth/authorization_session.cpp
@@ -241,6 +241,7 @@ namespace {
     }
 
     Status AuthorizationSession::_checkAuthForPrivilegeHelper(const Privilege& privilege) {
+        AuthorizationManager& authMan = getAuthorizationManager();
         Privilege modifiedPrivilege = _modifyPrivilegeForSpecialCases(privilege);
 
         // Need to check not just the resource of the privilege, but also just the database
@@ -252,24 +253,41 @@ namespace {
 
 
         ActionSet unmetRequirements = modifiedPrivilege.getActions();
-        for (UserSet::iterator it = _authenticatedUsers.begin();
-                it != _authenticatedUsers.end(); ++it) {
+        UserSet::iterator it = _authenticatedUsers.begin();
+        while (it != _authenticatedUsers.end()) {
             User* user = *it;
 
             if (!user->isValid()) {
-                // Need to release and re-acquire user if it's been invalidated.
+                // 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();
-
-                _authenticatedUsers.removeByDBName(name.getDB());
-                getAuthorizationManager().releaseUser(user);
-
-                Status status = getAuthorizationManager().acquireUser(name, &user);
-                if (!status.isOK()) {
-                    return Status(ErrorCodes::Unauthorized,
-                                  mongoutils::str::stream() << "Re-acquiring invalidated user "
-                                          "failed due to: " << status.reason());
+                User* updatedUser;
+
+                Status status = authMan.acquireUser(name, &updatedUser);
+                switch (status.code()) {
+                case ErrorCodes::OK: {
+                    // Success! Replace the old User object with the updated one.
+                    fassert(17067, _authenticatedUsers.replaceAt(it, updatedUser) == user);
+                    authMan.releaseUser(user);
+                    user = updatedUser;
+                    LOG(1) << "Updated session cache of user information for " << name;
+                    break;
+                }
+                case ErrorCodes::UserNotFound: {
+                    // User does not exist anymore; remove it from _authenticatedUsers.
+                    fassert(17068, _authenticatedUsers.removeAt(it) == user);
+                    authMan.releaseUser(user);
+                    LOG(1) << "Removed deleted user " << name <<
+                        " from session cache of user information.";
+                    continue;  // No need to advance "it" in this case.
+                }
+                default:
+                    // Unrecognized error; assume that it's transient, and continue working with the
+                    // out-of-date privilege data.
+                    warning() << "Could not fetch updated user privilege information for " <<
+                        name << "; continuing to use old information.  Reason is " << status;
+                    break;
                 }
-                _authenticatedUsers.add(user);
             }
 
             for (int i = 0; i < static_cast<int>(boost::size(resourceSearchList)); ++i) {
@@ -279,6 +297,7 @@ namespace {
                 if (unmetRequirements.empty())
                     return Status::OK();
             }
+            ++it;
         }
 
         return Status(ErrorCodes::Unauthorized, "unauthorized");
diff --git a/src/mongo/db/auth/authorization_session_test.cpp b/src/mongo/db/auth/authorization_session_test.cpp
index 6000e616368..d4f506315a9 100644
--- a/src/mongo/db/auth/authorization_session_test.cpp
+++ b/src/mongo/db/auth/authorization_session_test.cpp
@@ -33,16 +33,36 @@
 namespace mongo {
 namespace {
 
+    class FailureCapableAuthzManagerExternalStateMock :
+        public AuthzManagerExternalStateMock {
+    public:
+        FailureCapableAuthzManagerExternalStateMock() : _findsShouldFail(false) {}
+        virtual ~FailureCapableAuthzManagerExternalStateMock() {}
+
+        void setFindsShouldFail(bool enable) { _findsShouldFail = enable; }
+
+        virtual Status _findUser(const std::string& usersNamespace,
+                                 const BSONObj& query,
+                                 BSONObj* result) const {
+            if (_findsShouldFail) {
+                return Status(ErrorCodes::UnknownError, "_findUser set to fail in mock.");
+            }
+            return AuthzManagerExternalStateMock::_findUser(usersNamespace, query, result);
+        }
+
+    private:
+        bool _findsShouldFail;
+    };
 
     class AuthorizationSessionTest : public ::mongo::unittest::Test {
     public:
-        AuthzManagerExternalStateMock* managerState;
+        FailureCapableAuthzManagerExternalStateMock* managerState;
         AuthzSessionExternalStateMock* sessionState;
         scoped_ptr<AuthorizationManager> authzManager;
         scoped_ptr<AuthorizationSession> authzSession;
 
         void setUp() {
-            managerState = new AuthzManagerExternalStateMock();
+            managerState = new FailureCapableAuthzManagerExternalStateMock();
             authzManager.reset(new AuthorizationManager(managerState));
             sessionState = new AuthzSessionExternalStateMock(authzManager.get());
             authzSession.reset(new AuthorizationSession(sessionState));
@@ -126,6 +146,47 @@ namespace {
         authzManager->invalidateUser(user);
         ASSERT_TRUE(authzSession->checkAuthorization("test", ActionType::find));
         ASSERT_FALSE(authzSession->checkAuthorization("test", ActionType::insert));
+
+        user = authzSession->lookupUser(UserName("spencer", "test"));
+        ASSERT(user->isValid());
+
+        // Delete the user.
+        managerState->clearPrivilegeDocuments();
+        // Make sure that invalidating the user causes the session to reload its privileges.
+        authzManager->invalidateUser(user);
+        ASSERT_FALSE(authzSession->checkAuthorization("test", ActionType::find));
+        ASSERT_FALSE(authzSession->checkAuthorization("test", ActionType::insert));
+        ASSERT_FALSE(authzSession->lookupUser(UserName("spencer", "test")));
+    }
+
+    TEST_F(AuthorizationSessionTest, UseOldUserInfoInFaceOfConnectivityProblems) {
+        // Add a readWrite user
+        ASSERT_OK(managerState->insertPrivilegeDocument("test",
+                BSON("user" << "spencer" <<
+                     "pwd" << "a" <<
+                     "roles" << BSON_ARRAY("readWrite"))));
+        ASSERT_OK(authzSession->addAndAuthorizeUser(UserName("spencer", "test")));
+
+        ASSERT_TRUE(authzSession->checkAuthorization("test", ActionType::find));
+        ASSERT_TRUE(authzSession->checkAuthorization("test", ActionType::insert));
+
+        User* user = authzSession->lookupUser(UserName("spencer", "test"));
+        ASSERT(user->isValid());
+
+        // Change the user to be read-only
+        managerState->setFindsShouldFail(true);
+        managerState->clearPrivilegeDocuments();
+        ASSERT_OK(managerState->insertPrivilegeDocument("test",
+                BSON("user" << "spencer" <<
+                     "pwd" << "a" <<
+                     "roles" << BSON_ARRAY("read"))));
+
+        // Even though the user's privileges have been reduced, since we've configured user
+        // document lookup to fail, the authz session should continue to use its known out-of-date
+        // privilege data.
+        authzManager->invalidateUser(user);
+        ASSERT_TRUE(authzSession->checkAuthorization("test", ActionType::find));
+        ASSERT_TRUE(authzSession->checkAuthorization("test", ActionType::insert));
     }
 
 
diff --git a/src/mongo/db/auth/user_set.cpp b/src/mongo/db/auth/user_set.cpp
index f89c67df44c..c36a1c093f7 100644
--- a/src/mongo/db/auth/user_set.cpp
+++ b/src/mongo/db/auth/user_set.cpp
@@ -23,12 +23,11 @@
 
 namespace mongo {
 
-    UserSet::UserSet() {}
+    UserSet::UserSet() : _users(), _usersEnd(_users.end()) {}
     UserSet::~UserSet() {}
 
     User* UserSet::add(User* user) {
-        for (std::vector<User*>::iterator it = _users.begin();
-                it != _users.end(); ++it) {
+        for (mutable_iterator it = mbegin(); it != mend(); ++it) {
             User* current = *it;
             if (current->getName().getDB() == user->getName().getDB()) {
                 // There can be only one user per database.
@@ -36,22 +35,43 @@ namespace mongo {
                 return current;
             }
         }
-        _users.push_back(user);
+        if (_usersEnd == _users.end()) {
+            _users.push_back(user);
+            _usersEnd = _users.end();
+        }
+        else {
+            *_usersEnd = user;
+            ++_usersEnd;
+        }
         return NULL;
     }
 
     User* UserSet::removeByDBName(const StringData& dbname) {
-        for (std::vector<User*>::iterator it = _users.begin();
-                it != _users.end(); ++it) {
+        for (iterator it = begin(); it != end(); ++it) {
             User* current = *it;
             if (current->getName().getDB() == dbname) {
-                _users.erase(it);
-                return current;
+                return removeAt(it);
             }
         }
         return NULL;
     }
 
+    User* UserSet::replaceAt(iterator it, User* replacement) {
+        size_t offset = it - begin();
+        User* old = _users[offset];
+        _users[offset] = replacement;
+        return old;
+    }
+
+    User* UserSet::removeAt(iterator it) {
+        size_t offset = it - begin();
+        User* old = _users[offset];
+        --_usersEnd;
+        _users[offset] = *_usersEnd;
+        *_usersEnd = NULL;
+        return old;
+    }
+
     User* UserSet::lookup(const UserName& name) const {
         User* user = lookupByDBName(name.getDB());
         if (user && user->getName() == name) {
@@ -61,8 +81,7 @@ namespace mongo {
     }
 
     User* UserSet::lookupByDBName(const StringData& dbname) const {
-        for (std::vector<User*>::const_iterator it = _users.begin();
-                it != _users.end(); ++it) {
+        for (iterator it = begin(); it != end(); ++it) {
             User* current = *it;
             if (current->getName().getDB() == dbname) {
                 return current;
diff --git a/src/mongo/db/auth/user_set.h b/src/mongo/db/auth/user_set.h
index d772d22cbda..ad193654e52 100644
--- a/src/mongo/db/auth/user_set.h
+++ b/src/mongo/db/auth/user_set.h
@@ -47,10 +47,7 @@ namespace mongo {
          */
         class NameIterator {
         public:
-            explicit NameIterator(const std::vector<User*>& users) :
-                _curr(users.begin()),
-                _end(users.end()) {
-            }
+            explicit NameIterator(iterator begin, iterator end) : _curr(begin), _end(end) {}
 
             NameIterator() {}
 
@@ -75,17 +72,34 @@ namespace mongo {
         ~UserSet();
 
         /**
-         * Adds a User to the UserSet
-         * The UserSet does not take ownership of the User.  All User objects are owned by the
-         * user cache in the AuthorizationManager.
-         * As there can only be one user per database in the UserSet, if a User already exists
-         * for the new User's database, the old user will be removed from the set and returned.
-         * It is the caller's responsibility to then release that user back to the
-         * authorizationManger.  If no user already exists for the new user's database, returns NULL
+         * Adds a User to the UserSet.
+         *
+         * The UserSet does not take ownership of the User.
+         *
+         * As there can only be one user per database in the UserSet, if a User already exists for
+         * the new User's database, the old user will be removed from the set and returned.  It is
+         * the caller's responsibility to then release that user.  If no user already exists for the
+         * new user's database, returns NULL.
+         *
+         * Invalidates any outstanding iterators or NameIterators.
          */
         User* add(User* user);
 
         /**
+         * Replaces the user at "it" with "replacement."  Does not take ownership of the User.
+         * Returns a pointer to the old user referenced by "it".  Does _not_ invalidate "iterator"
+         * instances.
+         */
+        User* replaceAt(iterator it, User* replacement);
+
+        /**
+         * Removes the user at "it", and returns a pointer to it.  After this call, "it" remains
+         * valid.  It will either equal "end()", or refer to some user between the values of "it"
+         * and "end()" before this call was made.
+         */
+        User* removeAt(iterator it);
+
+        /**
          * Removes the User whose authentication credentials came from dbname, and returns that
          * user.  It is the caller's responsibility to then release that user back to the
          * authorizationManger.  If no user exists for the given database, returns NULL;
@@ -107,15 +121,21 @@ namespace mongo {
 
         // Gets an iterator over the names of the users stored in the set.  The iterator is
         // valid until the next non-const method is called on the UserSet.
-        NameIterator getNames() const { return NameIterator(_users); }
+        NameIterator getNames() const { return NameIterator(begin(), end()); }
 
         iterator begin() const { return _users.begin(); }
-        iterator end() const { return _users.end(); }
+        iterator end() const { return _usersEnd; }
 
     private:
+        typedef std::vector<User*>::iterator mutable_iterator;
+
+        mutable_iterator mbegin() { return _users.begin(); }
+        mutable_iterator mend() { return _usersEnd; }
+
         // The UserSet maintains ownership of the Users in it, and is responsible for
         // returning them to the AuthorizationManager when done with them.
         std::vector<User*> _users;
+        std::vector<User*>::iterator _usersEnd;
     };
 
 } // namespace mongo
diff --git a/src/mongo/db/auth/user_set_test.cpp b/src/mongo/db/auth/user_set_test.cpp
index 273f6ab85c4..3a2141855c6 100644
--- a/src/mongo/db/auth/user_set_test.cpp
+++ b/src/mongo/db/auth/user_set_test.cpp
@@ -79,6 +79,11 @@ namespace {
         ASSERT_NULL(set.lookupByDBName("test"));
         ASSERT_EQUALS(p3, set.lookup(UserName("Bob", "test2")));
         ASSERT_EQUALS(p3, set.lookupByDBName("test2"));
+
+        UserSet::NameIterator iter = set.getNames();
+        ASSERT_TRUE(iter.more());
+        ASSERT_EQUALS(iter.next(), UserName("Bob", "test2"));
+        ASSERT_FALSE(iter.more());
     }
 
     TEST(UserSetTest, IterateNames) {