8000 Bug fix/user manager version race (#14382) (#14392) · arangodb/arangodb@54b9a64 · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit 54b9a64

Browse files
mchackiKVS85
mchacki
and
KVS85
authored
Bug fix/user manager version race (#14382) (#14392)
* Bug fix/user manager version race (#14382) Fixes ES-863 * Moved Changelog entry to correct place * Ifdefed testonly method * Update CHANGELOG Co-authored-by: Vadim <vadim@arangodb.com>
1 parent b2adf44 commit 54b9a64

File tree

5 files changed

+286
-21
lines changed

5 files changed

+286
-21
lines changed

‎CHANGELOG

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,18 @@
11
v3.7.13 (XXXX-XX-XX)
22
--------------------
33

4+
* Fixed ES-863: reloading of users within the Cluster.
5+
If a Coordinator is asked to reload its users (e.g. by the UserManager in
6+
Foxx, it is also possible to do via API, but this is internal and on purpose
7+
not documented, so unlikely that it is used), in concurrency with user
8+
management updates there is a chance that the reload is not correctly
9+
performed on this coordinator. It may have missed the last update locally,
10+
causing one user to have an older state. It will be fixed on the next
11+
modification of any other users/permissions. Unfortunately this bug can
12+
cascade and when hit again, the coordinator can now be off by two updates.
13+
In DC2DC this situation is more likely to happen on the target datacenter,
14+
causing this datacenter to have other users/permissions than the source one.
15+
416
* Fix BTS-446: When finding a not yet fully initialized agency, do not
517
immediately fatal exit. Keep trying for (very generous) 5 minutes.
618

‎arangod/Auth/UserManager.cpp

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -87,10 +87,7 @@ auth::UserManager::UserManager(application_features::ApplicationServer& server)
8787
: _server(server), _globalVersion(1), _internalVersion(0) {}
8888
#else
8989
auth::UserManager::UserManager(application_features::ApplicationServer& server)
90-
: _server(server),
91-
_globalVersion(1),
92-
_internalVersion(0),
93-
_authHandler(nullptr) {}
90+
: _server(server), _globalVersion(1), _internalVersion(0), _authHandler(nullptr) {}
9491

9592
auth::UserManager::UserManager(application_features::ApplicationServer& server,
9693
std::unique_ptr<auth::Handler> handler)
@@ -129,7 +126,8 @@ static std::shared_ptr<VPackBuilder> QueryAllUsers(application_features::Applica
129126
if (vocbase == nullptr) {
130127
LOG_TOPIC("b8c47", DEBUG, arangodb::Logger::AUTHENTICATION)
131128
<< "system database is unknown";
132-
THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "system database is unknown");
129+
THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL,
130+
"system database is unknown");
133131
}
134132

135133
// we cannot set this execution context, otherwise the transaction
@@ -138,8 +136,8 @@ static std::shared_ptr<VPackBuilder> QueryAllUsers(application_features::Applica
138136
std::string const queryStr("FOR user IN _users RETURN user");
139137
auto emptyBuilder = std::make_shared<VPackBuilder>();
140138
arangodb::aql::Query query(transaction::StandaloneContext::Create(*vocbase),
141-
arangodb::aql::QueryString(queryStr),
142-
emptyBuilder, emptyBuilder);
139+
arangodb::aql::QueryString(queryStr), emptyBuilder,
140+
emptyBuilder);
143141

144142
query.queryOptions().cache = false;
145143
query.queryOptions().ttl = 30;
@@ -156,7 +154,8 @@ static std::shared_ptr<VPackBuilder> QueryAllUsers(application_features::Applica
156154
THROW_ARANGO_EXCEPTION(TRI_ERROR_REQUEST_CANCELED);
157155
}
158156
THROW_ARANGO_EXCEPTION_MESSAGE(queryResult.result.errorNumber(),
159-
"Error executing user query: " + queryResult.result.errorMessage());
157+
"Error executing user query: " +
158+
queryResult.result.errorMessage());
160159
}
161160

162161
VPackSlice usersSlice = queryResult.data->slice();
@@ -200,6 +199,13 @@ void auth::UserManager::loadFromDB() {
200199
return;
201200
}
202201

202+
TRI_IF_FAILURE("UserManager::performDBLookup") {
203+
// Used in GTest. It is used to identify
204+
// if the UserManager would have updated it's
205+
// cache in a specific situation.
206+
THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG);
207+
}
208+
203209
try {
204210
std::shared_ptr<VPackBuilder> builder = QueryAllUsers(_server);
205211
if (builder) {
@@ -325,7 +331,7 @@ Result auth::UserManager::storeUserInternal(auth::User const& entry, bool replac
325331
// user was outdated, we should trigger a reload
326332
triggerLocalReload();
327333
LOG_TOPIC("cf922", DEBUG, Logger::AUTHENTICATION)
328-
<< "Cannot update user : '" << res.errorMessage() << "'";
334+
<< "Cannot update user : '" << res.errorMessage() << "'";
329335
}
330336
}
331337
return res;
@@ -343,7 +349,8 @@ void auth::UserManager::createRootUser() {
343349
WRITE_LOCKER(writeGuard, _userCacheLock); // must be second
344350
UserMap::iterator const& it = _userCache.find("root");
345351
if (it != _userCache.end()) {
346-
LOG_TOPIC("bbc97", TRACE, Logger::AUTHENTICATION) << "\"root\" already exists";
352+
LOG_TOPIC("bbc97", TRACE, Logger::AUTHENTICATION)
353+
<< "\"root\" already exists";
347354
return;
348355
}
349356
TRI_ASSERT(_userCache.empty());
@@ -368,7 +375,8 @@ void auth::UserManager::createRootUser() {
368375
<< "unable to create user \"root\": " << ex.what();
369376
} catch (...) {
370377
// No action
371-
LOG_TOPIC("268eb", ERR, Logger::AUTHENTICATION) << "unable to create user \"root\"";
378+
LOG_TOPIC("268eb", ERR, Logger::AUTHENTICATION)
379+
<< "unable to create user \"root\"";
372380
}
373381

374382
triggerGlobalReload();
@@ -490,20 +498,20 @@ Result auth::UserManager::enumerateUsers(std::function<bool(auth::User&)>&& func
490498
}
491499
}
492500
}
493-
501+
494502
bool triggerUpdate = !toUpdate.empty();
495-
503+
496504
Result res;
497505
do {
498506
auto it = toUpdate.begin();
499-
while(it != toUpdate.end()) {
507+
while (it != toUpdate.end()) {
500508
WRITE_LOCKER(writeGuard, _userCacheLock);
501-
res = storeUserInternal(*it, /*replace*/true);
502-
509+
res = storeUserInternal(*it, /*replace*/ true);
510+
503511
if (res.is(TRI_ERROR_ARANGO_CONFLICT) && retryOnConflict) {
504512
res.reset();
505513
writeGuard.unlock();
506-
loadFromDB(); // should be noop iff nothing changed
514+
loadFromDB(); // should be noop iff nothing changed
507515
writeGuard.lock();
508516
UserMap::iterator it2 = _userCache.find(it->username());
509517
if (it2 != _userCache.end()) {
@@ -715,7 +723,8 @@ bool auth::UserManager::checkPassword(std::string const& username, std::string c
715723
AuthenticationFeature* af = AuthenticationFeature::instance();
716724
if (it != _userCache.end() && (it->second.source() == auth::Source::Local)) {
717725
if (af != nullptr && !af->localAuthentication()) {
718-
LOG_TOPIC("d3220", DEBUG, Logger::AUTHENTICATION) << "Local users are forbidden";
726+
LOG_TOPIC("d3220", DEBUG, Logger::AUTHENTICATION)
727+
<< "Local users are forbidden";
719728
return false;
720729
}
721730
auth::User const& user = it->second;
@@ -799,10 +808,12 @@ auth::Level auth::UserManager::collectionAuthLevel(std::string const& user,
799808
return level;
800809
}
801810

811+
#ifdef ARANGODB_USE_GOOGLE_TESTS
802812
/// Only used for testing
803813
void auth::UserManager::setAuthInfo(auth::UserMap const& newMap) {
804814
MUTEX_LOCKER(guard, _loadFromDBLock); // must be first
805815
WRITE_LOCKER(writeGuard, _userCacheLock); // must be second
806816
_userCache = newMap;
807817
_internalVersion.store(_globalVersion.load());
808818
}
819+
#endif

‎arangod/Auth/UserManager.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ class UserManager {
8080

8181
/// @brief reload user cache and token caches
8282
void triggerLocalReload() {
83-
_globalVersion.fetch_add(1, std::memory_order_release);
83+
_internalVersion.store(0, std::memory_order_release);
8484
}
8585

8686
/// @brief used for caching
@@ -104,8 +104,7 @@ class UserManager {
104104
bool active, velocypack::Slice extras);
105105

106106
/// Enumerate list of all users
107-
Result enumerateUsers(std::function<bool(auth::User&)>&&,
108-
bool retryOnConflict);
107+
Result enumerateUsers(std::function<bool(auth::User&)>&&, bool retryOnConflict);
109108
/// Update specific user
110109
Result updateUser(std::string const& user, UserCallback&&);
111110
/// Access user without modifying it
@@ -135,9 +134,11 @@ class UserManager {
135134
auth::Level collectionAuthLevel(std::string const& username, std::string const& dbname,
136135
std::string const& coll, bool configured = false);
137136

137+
#ifdef ARANGODB_USE_GOOGLE_TESTS
138138
/// Overwrite internally cached permissions, only use
139139
/// for testing purposes
140140
void setAuthInfo(auth::UserMap const& userEntryMap);
141+
#endif
141142

142143
#ifdef USE_ENTERPRISE
143144

0 commit comments

Comments
 (0)
0