From b9cf1dabbe193d3717d8b53a24f666ecf91b40d8 Mon Sep 17 00:00:00 2001 From: Spencer T Brody Date: Mon, 22 Jul 2013 15:55:45 -0400 Subject: SERVER-9518 Catch exceptions in ExternalState functions --- src/mongo/db/auth/authorization_manager.cpp | 6 +- src/mongo/db/auth/authorization_session_test.cpp | 12 +- src/mongo/db/auth/authz_manager_external_state.cpp | 19 ++- src/mongo/db/auth/authz_manager_external_state.h | 15 +- .../db/auth/authz_manager_external_state_d.cpp | 130 +++++++++-------- src/mongo/db/auth/authz_manager_external_state_d.h | 9 +- .../db/auth/authz_manager_external_state_mock.cpp | 18 ++- .../db/auth/authz_manager_external_state_mock.h | 9 +- .../db/auth/authz_manager_external_state_s.cpp | 157 ++++++++++++--------- src/mongo/db/auth/authz_manager_external_state_s.h | 9 +- .../db/auth/authz_session_external_state_mock.h | 8 +- 11 files changed, 227 insertions(+), 165 deletions(-) (limited to 'src/mongo') diff --git a/src/mongo/db/auth/authorization_manager.cpp b/src/mongo/db/auth/authorization_manager.cpp index 88318b64804..cbd71eaa0ff 100644 --- a/src/mongo/db/auth/authorization_manager.cpp +++ b/src/mongo/db/auth/authorization_manager.cpp @@ -840,7 +840,11 @@ namespace { for (std::vector::iterator dbIt = dbNames.begin(); dbIt != dbNames.end(); ++dbIt) { std::string dbname = *dbIt; - std::vector privDocs = _externalState->getAllV1PrivilegeDocsForDB(dbname); + std::vector privDocs; + Status status = _externalState->getAllV1PrivilegeDocsForDB(dbname, &privDocs); + if (!status.isOK()) { + return status; + } for (std::vector::iterator docIt = privDocs.begin(); docIt != privDocs.end(); ++docIt) { diff --git a/src/mongo/db/auth/authorization_session_test.cpp b/src/mongo/db/auth/authorization_session_test.cpp index da3617fce2a..d0123ad4c55 100644 --- a/src/mongo/db/auth/authorization_session_test.cpp +++ b/src/mongo/db/auth/authorization_session_test.cpp @@ -71,9 +71,9 @@ namespace { public: AuthManagerExternalStateImplictPriv() : AuthzManagerExternalStateMock() {} - virtual bool _findUser(const string& usersNamespace, - const BSONObj& query, - BSONObj* result) const { + virtual Status _findUser(const string& usersNamespace, + const BSONObj& query, + BSONObj* result) const { NamespaceString nsstring(usersNamespace); std::string user = query[AuthorizationManager::USER_NAME_FIELD_NAME].String(); @@ -87,8 +87,10 @@ namespace { *result = mapFindWithDefault(_privilegeDocs, std::make_pair(nsstring.db().toString(), UserName(user, userSource)), - BSON("invalid" << 1)); - return !(*result)["invalid"].trueValue(); + BSON("invalid" << 1)); + return (*result)["invalid"].trueValue() ? + Status(ErrorCodes::UserNotFound, "User not found") : + Status::OK(); } void addPrivilegeDocument(const string& dbname, diff --git a/src/mongo/db/auth/authz_manager_external_state.cpp b/src/mongo/db/auth/authz_manager_external_state.cpp index 63a759bf2d1..723c94d0b55 100644 --- a/src/mongo/db/auth/authz_manager_external_state.cpp +++ b/src/mongo/db/auth/authz_manager_external_state.cpp @@ -70,12 +70,17 @@ namespace mongo { userName.getDB()); } - bool found = _findUser(usersNamespace, queryBuilder.obj(), &userBSONObj); - if (!found) { - return Status(ErrorCodes::UserNotFound, - mongoutils::str::stream() << "auth: couldn't find user " << - userName.toString() << ", " << usersNamespace, - 0); + Status found = _findUser(usersNamespace, queryBuilder.obj(), &userBSONObj); + if (!found.isOK()) { + if (found.code() == ErrorCodes::UserNotFound) { + // Return more detailed status that includes user name. + return Status(ErrorCodes::UserNotFound, + mongoutils::str::stream() << "auth: couldn't find user " << + userName.toString() << ", " << usersNamespace, + 0); + } else { + return found; + } } *result = userBSONObj.getOwned(); @@ -87,7 +92,7 @@ namespace mongo { BSONObj userBSONObj; BSONObj query; - return _findUser(usersNamespace, query, &userBSONObj); + return _findUser(usersNamespace, query, &userBSONObj).isOK(); } } // namespace mongo diff --git a/src/mongo/db/auth/authz_manager_external_state.h b/src/mongo/db/auth/authz_manager_external_state.h index b6463fec5d7..74584b867c6 100644 --- a/src/mongo/db/auth/authz_manager_external_state.h +++ b/src/mongo/db/auth/authz_manager_external_state.h @@ -64,19 +64,22 @@ namespace mongo { virtual Status getAllDatabaseNames(std::vector* dbnames) const = 0; /** - * Returns a vector of every privilege document from the given database's + * Puts into the *privDocs vector every privilege document from the given database's * system.users collection. */ - virtual std::vector getAllV1PrivilegeDocsForDB(const std::string& dbname) const = 0; + virtual Status getAllV1PrivilegeDocsForDB(const std::string& dbname, + std::vector* privDocs) const = 0; protected: AuthzManagerExternalState(); // This class should never be instantiated directly. // Queries the userNamespace with the given query and returns the privilegeDocument found - // in *result. Returns true if it finds a document matching the query, or false if not. - virtual bool _findUser(const std::string& usersNamespace, - const BSONObj& query, - BSONObj* result) const = 0; + // in *result. Returns Status::OK if it finds a document matching the query. If it doesn't + // find a document matching the query, returns a Status with code UserNotFound. Other + // errors may return other Status codes. + virtual Status _findUser(const std::string& usersNamespace, + const BSONObj& query, + BSONObj* result) const = 0; }; diff --git a/src/mongo/db/auth/authz_manager_external_state_d.cpp b/src/mongo/db/auth/authz_manager_external_state_d.cpp index b824ad5ba86..6a72f3b8fac 100644 --- a/src/mongo/db/auth/authz_manager_external_state_d.cpp +++ b/src/mongo/db/auth/authz_manager_external_state_d.cpp @@ -28,76 +28,93 @@ namespace mongo { +namespace { + static Status userNotFoundStatus(ErrorCodes::UserNotFound, "User not found"); +} + AuthzManagerExternalStateMongod::AuthzManagerExternalStateMongod() {} AuthzManagerExternalStateMongod::~AuthzManagerExternalStateMongod() {} Status AuthzManagerExternalStateMongod::insertPrivilegeDocument(const string& dbname, const BSONObj& userObj) const { - string userNS = dbname + ".system.users"; - DBDirectClient client; - { - Client::GodScope gs; - // TODO(spencer): Once we're no longer fully rebuilding the user cache on every change - // to user data we should remove the global lock and uncomment the WriteContext below - Lock::GlobalWrite w; - // Client::WriteContext ctx(userNS); - client.insert(userNS, userObj); - } - - // 30 second timeout for w:majority - BSONObj res = client.getLastErrorDetailed(false, false, -1, 30*1000); - string errstr = client.getLastErrorString(res); - if (errstr.empty()) { - return Status::OK(); + try { + string userNS = dbname + ".system.users"; + DBDirectClient client; + { + Client::GodScope gs; + // TODO(spencer): Once we're no longer fully rebuilding the user cache on every + // change to user data we should remove the global lock and uncomment the + // WriteContext below + Lock::GlobalWrite w; + // Client::WriteContext ctx(userNS); + client.insert(userNS, userObj); + } + + // 30 second timeout for w:majority + BSONObj res = client.getLastErrorDetailed(false, false, -1, 30*1000); + string errstr = client.getLastErrorString(res); + if (errstr.empty()) { + return Status::OK(); + } + if (res.hasField("code") && res["code"].Int() == ASSERT_ID_DUPKEY) { + return Status(ErrorCodes::DuplicateKey, + mongoutils::str::stream() << "User \"" << userObj["user"].String() << + "\" already exists on database \"" << dbname << "\""); + } + return Status(ErrorCodes::UserModificationFailed, errstr); + } catch (const DBException& e) { + return e.toStatus(); } - if (res.hasField("code") && res["code"].Int() == ASSERT_ID_DUPKEY) { - return Status(ErrorCodes::DuplicateKey, - mongoutils::str::stream() << "User \"" << userObj["user"].String() << - "\" already exists on database \"" << dbname << "\""); - } - return Status(ErrorCodes::UserModificationFailed, errstr); } Status AuthzManagerExternalStateMongod::updatePrivilegeDocument( const UserName& user, const BSONObj& updateObj) const { - string userNS = mongoutils::str::stream() << user.getDB() << ".system.users"; - DBDirectClient client; - { - Client::GodScope gs; - // TODO(spencer): Once we're no longer fully rebuilding the user cache on every change - // to user data we should remove the global lock and uncomment the WriteContext below - Lock::GlobalWrite w; - // Client::WriteContext ctx(userNS); - client.update(userNS, - QUERY("user" << user.getUser() << "userSource" << BSONNULL), - updateObj); - } - - // 30 second timeout for w:majority - BSONObj res = client.getLastErrorDetailed(false, false, -1, 30*1000); - string err = client.getLastErrorString(res); - if (!err.empty()) { - return Status(ErrorCodes::UserModificationFailed, err); - } + try { + string userNS = mongoutils::str::stream() << user.getDB() << ".system.users"; + DBDirectClient client; + { + Client::GodScope gs; + // TODO(spencer): Once we're no longer fully rebuilding the user cache on every + // change to user data we should remove the global lock and uncomment the + // WriteContext below + Lock::GlobalWrite w; + // Client::WriteContext ctx(userNS); + client.update(userNS, + QUERY("user" << user.getUser() << "userSource" << BSONNULL), + updateObj); + } + + // 30 second timeout for w:majority + BSONObj res = client.getLastErrorDetailed(false, false, -1, 30*1000); + string err = client.getLastErrorString(res); + if (!err.empty()) { + return Status(ErrorCodes::UserModificationFailed, err); + } + + int numUpdated = res["n"].numberInt(); + dassert(numUpdated <= 1 && numUpdated >= 0); + if (numUpdated == 0) { + return Status(ErrorCodes::UserNotFound, + mongoutils::str::stream() << "User " << user.getFullName() << + " not found"); + } - int numUpdated = res["n"].numberInt(); - dassert(numUpdated <= 1 && numUpdated >= 0); - if (numUpdated == 0) { - return Status(ErrorCodes::UserNotFound, - mongoutils::str::stream() << "User " << user.getFullName() << - " not found"); + return Status::OK(); + } catch (const DBException& e) { + return e.toStatus(); } - - return Status::OK(); } - bool AuthzManagerExternalStateMongod::_findUser(const string& usersNamespace, - const BSONObj& query, - BSONObj* result) const { + Status AuthzManagerExternalStateMongod::_findUser(const string& usersNamespace, + const BSONObj& query, + BSONObj* result) const { Client::GodScope gs; Client::ReadContext ctx(usersNamespace); - return Helpers::findOne(usersNamespace, query, *result); + if (!Helpers::findOne(usersNamespace, query, *result)) { + return userNotFoundStatus; + } + return Status::OK(); } Status AuthzManagerExternalStateMongod::getAllDatabaseNames( @@ -106,12 +123,13 @@ namespace mongo { return Status::OK(); } - std::vector AuthzManagerExternalStateMongod::getAllV1PrivilegeDocsForDB( - const std::string& dbname) const { + Status AuthzManagerExternalStateMongod::getAllV1PrivilegeDocsForDB( + const std::string& dbname, std::vector* privDocs) const { Client::GodScope gs; Client::ReadContext ctx(dbname); - return Helpers::findAll(dbname, BSONObj()); + *privDocs = Helpers::findAll(dbname, BSONObj()); + return Status::OK(); } } // namespace mongo diff --git a/src/mongo/db/auth/authz_manager_external_state_d.h b/src/mongo/db/auth/authz_manager_external_state_d.h index e1a41d33aa6..3998d6e7885 100644 --- a/src/mongo/db/auth/authz_manager_external_state_d.h +++ b/src/mongo/db/auth/authz_manager_external_state_d.h @@ -43,12 +43,13 @@ namespace mongo { virtual Status getAllDatabaseNames(std::vector* dbnames) const; - virtual std::vector getAllV1PrivilegeDocsForDB(const std::string& dbname) const; + virtual Status getAllV1PrivilegeDocsForDB(const std::string& dbname, + std::vector* privDocs) const; protected: - virtual bool _findUser(const string& usersNamespace, - const BSONObj& query, - BSONObj* result) const; + virtual Status _findUser(const string& usersNamespace, + const BSONObj& query, + BSONObj* result) const; }; } // namespace mongo diff --git a/src/mongo/db/auth/authz_manager_external_state_mock.cpp b/src/mongo/db/auth/authz_manager_external_state_mock.cpp index cafbd92cb21..6dbbfffb48a 100644 --- a/src/mongo/db/auth/authz_manager_external_state_mock.cpp +++ b/src/mongo/db/auth/authz_manager_external_state_mock.cpp @@ -55,24 +55,22 @@ namespace mongo { return Status::OK(); } - std::vector AuthzManagerExternalStateMock::getAllV1PrivilegeDocsForDB( - const std::string& dbname) const { - std::vector out; - + Status AuthzManagerExternalStateMock::getAllV1PrivilegeDocsForDB( + const std::string& dbname, std::vector* privDocs) const { const std::vector& dbDocs = _userDocuments.find(dbname)->second; for (std::vector::const_iterator it = dbDocs.begin(); it != dbDocs.end(); ++it) { - out.push_back(*it); + privDocs->push_back(*it); } - return out; + return Status::OK(); } - bool AuthzManagerExternalStateMock::_findUser(const std::string& usersNamespace, + Status AuthzManagerExternalStateMock::_findUser(const std::string& usersNamespace, const BSONObj& query, BSONObj* result) const { StatusWithMatchExpression parseResult = MatchExpressionParser::parse(query); if (!parseResult.isOK()) { - return false; + return parseResult.getStatus(); } MatchExpression* matcher = parseResult.getValue(); @@ -83,11 +81,11 @@ namespace mongo { if (nsToDatabase(usersNamespace) == mapIt->first && matcher->matchesBSON(*vecIt)) { *result = *vecIt; - return true; + return Status::OK(); } } } - return false; + return Status(ErrorCodes::UserNotFound, "User not found"); } } // namespace mongo diff --git a/src/mongo/db/auth/authz_manager_external_state_mock.h b/src/mongo/db/auth/authz_manager_external_state_mock.h index 51457848a86..894088047b1 100644 --- a/src/mongo/db/auth/authz_manager_external_state_mock.h +++ b/src/mongo/db/auth/authz_manager_external_state_mock.h @@ -52,11 +52,12 @@ namespace mongo { virtual Status getAllDatabaseNames(std::vector* dbnames) const; - virtual std::vector getAllV1PrivilegeDocsForDB(const std::string& dbname) const; + virtual Status getAllV1PrivilegeDocsForDB(const std::string& dbname, + std::vector* privDocs) const; - virtual bool _findUser(const std::string& usersNamespace, - const BSONObj& query, - BSONObj* result) const; + virtual Status _findUser(const std::string& usersNamespace, + const BSONObj& query, + BSONObj* result) const; private: diff --git a/src/mongo/db/auth/authz_manager_external_state_s.cpp b/src/mongo/db/auth/authz_manager_external_state_s.cpp index d308d056607..5dcb0530731 100644 --- a/src/mongo/db/auth/authz_manager_external_state_s.cpp +++ b/src/mongo/db/auth/authz_manager_external_state_s.cpp @@ -28,6 +28,10 @@ namespace mongo { +namespace { + static Status userNotFoundStatus(ErrorCodes::UserNotFound, "User not found"); +} + AuthzManagerExternalStateMongos::AuthzManagerExternalStateMongos() {} AuthzManagerExternalStateMongos::~AuthzManagerExternalStateMongos() {} @@ -46,95 +50,120 @@ namespace mongo { } } - bool AuthzManagerExternalStateMongos::_findUser(const string& usersNamespace, - const BSONObj& query, - BSONObj* result) const { - scoped_ptr conn(getConnectionForUsersCollection(usersNamespace)); - *result = conn->get()->findOne(usersNamespace, query).getOwned(); - conn->done(); - return !result->isEmpty(); + Status AuthzManagerExternalStateMongos::_findUser(const string& usersNamespace, + const BSONObj& query, + BSONObj* result) const { + try { + scoped_ptr conn(getConnectionForUsersCollection(usersNamespace)); + *result = conn->get()->findOne(usersNamespace, query).getOwned(); + conn->done(); + if (result->isEmpty()) { + return userNotFoundStatus; + } + return Status::OK(); + } catch (const DBException& e) { + return e.toStatus(); + } } Status AuthzManagerExternalStateMongos::insertPrivilegeDocument(const string& dbname, const BSONObj& userObj) const { - string userNS = dbname + ".system.users"; - scoped_ptr conn(getConnectionForUsersCollection(userNS)); + try { + string userNS = dbname + ".system.users"; + scoped_ptr conn(getConnectionForUsersCollection(userNS)); - conn->get()->insert(userNS, userObj); + conn->get()->insert(userNS, userObj); - // 30 second timeout for w:majority - BSONObj res = conn->get()->getLastErrorDetailed(false, false, -1, 30*1000); - string errstr = conn->get()->getLastErrorString(res); - conn->done(); + // 30 second timeout for w:majority + BSONObj res = conn->get()->getLastErrorDetailed(false, false, -1, 30*1000); + string errstr = conn->get()->getLastErrorString(res); + conn->done(); - if (errstr.empty()) { - return Status::OK(); - } - if (res.hasField("code") && res["code"].Int() == ASSERT_ID_DUPKEY) { - return Status(ErrorCodes::DuplicateKey, - mongoutils::str::stream() << "User \"" << userObj["user"].String() << - "\" already exists on database \"" << dbname << "\""); + if (errstr.empty()) { + return Status::OK(); + } + if (res.hasField("code") && res["code"].Int() == ASSERT_ID_DUPKEY) { + return Status(ErrorCodes::DuplicateKey, + mongoutils::str::stream() << "User \"" << userObj["user"].String() << + "\" already exists on database \"" << dbname << "\""); + } + return Status(ErrorCodes::UserModificationFailed, errstr); + } catch (const DBException& e) { + return e.toStatus(); } - return Status(ErrorCodes::UserModificationFailed, errstr); } Status AuthzManagerExternalStateMongos::updatePrivilegeDocument( const UserName& user, const BSONObj& updateObj) const { - string userNS = mongoutils::str::stream() << user.getDB() << ".system.users"; - scoped_ptr conn(getConnectionForUsersCollection(userNS)); + try { + string userNS = mongoutils::str::stream() << user.getDB() << ".system.users"; + scoped_ptr conn(getConnectionForUsersCollection(userNS)); - conn->get()->update(userNS, - QUERY("user" << user.getUser() << "userSource" << BSONNULL), - updateObj); + conn->get()->update(userNS, + QUERY("user" << user.getUser() << "userSource" << BSONNULL), + updateObj); - // 30 second timeout for w:majority - BSONObj res = conn->get()->getLastErrorDetailed(false, false, -1, 30*1000); - string err = conn->get()->getLastErrorString(res); - conn->done(); + // 30 second timeout for w:majority + BSONObj res = conn->get()->getLastErrorDetailed(false, false, -1, 30*1000); + string err = conn->get()->getLastErrorString(res); + conn->done(); - if (!err.empty()) { - return Status(ErrorCodes::UserModificationFailed, err); - } + if (!err.empty()) { + return Status(ErrorCodes::UserModificationFailed, err); + } - int numUpdated = res["n"].numberInt(); - dassert(numUpdated <= 1 && numUpdated >= 0); - if (numUpdated == 0) { - return Status(ErrorCodes::UserNotFound, - mongoutils::str::stream() << "User " << user.getFullName() << - " not found"); - } + int numUpdated = res["n"].numberInt(); + dassert(numUpdated <= 1 && numUpdated >= 0); + if (numUpdated == 0) { + return Status(ErrorCodes::UserNotFound, + mongoutils::str::stream() << "User " << user.getFullName() << + " not found"); + } - return Status::OK(); + return Status::OK(); + } catch (const DBException& e) { + return e.toStatus(); + } } Status AuthzManagerExternalStateMongos::getAllDatabaseNames( std::vector* dbnames) const { - std::vector dbDocs; - scoped_ptr conn(getConnectionForUsersCollection("config.databases")); - conn->get()->findN(dbDocs, DatabaseType::ConfigNS, Query(), 0); - conn->done(); - - for (std::vector::const_iterator it = dbDocs.begin(); - it != dbDocs.end(); ++it) { - DatabaseType dbInfo; - std::string errmsg; - if (!dbInfo.parseBSON( *it, &errmsg) || !dbInfo.isValid( &errmsg )) { - return Status(ErrorCodes::FailedToParse, errmsg); + try { + std::vector dbDocs; + scoped_ptr conn( + getConnectionForUsersCollection("config.databases")); + conn->get()->findN(dbDocs, DatabaseType::ConfigNS, Query(), 0); + conn->done(); + + for (std::vector::const_iterator it = dbDocs.begin(); + it != dbDocs.end(); ++it) { + DatabaseType dbInfo; + std::string errmsg; + if (!dbInfo.parseBSON( *it, &errmsg) || !dbInfo.isValid( &errmsg )) { + return Status(ErrorCodes::FailedToParse, errmsg); + } + dbnames->push_back(dbInfo.getName()); } - dbnames->push_back(dbInfo.getName()); + dbnames->push_back("config"); // config db isn't listed in config.databases + return Status::OK(); + } catch (const DBException& e) { + return e.toStatus(); } - dbnames->push_back("config"); // config db isn't listed in config.databases - return Status::OK(); } - std::vector AuthzManagerExternalStateMongos::getAllV1PrivilegeDocsForDB( - const std::string& dbname) const { - std::vector userDocs; - std::string usersNamespace = dbname + ".system.users"; - scoped_ptr conn(getConnectionForUsersCollection(usersNamespace)); - conn->get()->findN(userDocs, usersNamespace, Query(), 0); - conn->done(); - return userDocs; + Status AuthzManagerExternalStateMongos::getAllV1PrivilegeDocsForDB( + const std::string& dbname, std::vector* privDocs) const { + try { + std::vector userDocs; + std::string usersNamespace = dbname + ".system.users"; + scoped_ptr conn(getConnectionForUsersCollection(usersNamespace)); + conn->get()->findN(userDocs, usersNamespace, Query(), 0); + conn->done(); + *privDocs = userDocs; + return Status::OK(); + } catch (const DBException& e) { + return e.toStatus(); + } } } // namespace mongo diff --git a/src/mongo/db/auth/authz_manager_external_state_s.h b/src/mongo/db/auth/authz_manager_external_state_s.h index 86b2c85bb31..7a80b80d124 100644 --- a/src/mongo/db/auth/authz_manager_external_state_s.h +++ b/src/mongo/db/auth/authz_manager_external_state_s.h @@ -43,12 +43,13 @@ namespace mongo { virtual Status getAllDatabaseNames(std::vector* dbnames) const; - virtual std::vector getAllV1PrivilegeDocsForDB(const std::string& dbname) const; + virtual Status getAllV1PrivilegeDocsForDB(const std::string& dbname, + std::vector* privDocs) const; protected: - virtual bool _findUser(const string& usersNamespace, - const BSONObj& query, - BSONObj* result) const; + virtual Status _findUser(const string& usersNamespace, + const BSONObj& query, + BSONObj* result) const; }; } // namespace mongo diff --git a/src/mongo/db/auth/authz_session_external_state_mock.h b/src/mongo/db/auth/authz_session_external_state_mock.h index 2fe79a4f258..967c50d78b2 100644 --- a/src/mongo/db/auth/authz_session_external_state_mock.h +++ b/src/mongo/db/auth/authz_session_external_state_mock.h @@ -41,10 +41,10 @@ namespace mongo { _returnValue = returnValue; } - virtual bool _findUser(const std::string& usersNamespace, - const BSONObj& query, - BSONObj* result) const { - return false; + virtual Status _findUser(const std::string& usersNamespace, + const BSONObj& query, + BSONObj* result) const { + return Status(ErrorCodes::UserNotFound, "User not found"); } virtual void startRequest() {} -- cgit v1.2.1