arangodb · jsteemann · Mar 24, 2020 · Mar 24, 2020 · Mar 24, 2020 · Mar 24, 2020
diff --git a/arangod/Actions/RestActionHandler.cpp b/arangod/Actions/RestActionHandler.cpp
@@ -24,6 +24,7 @@
 #include "RestActionHandler.h"
 
 #include "Actions/actions.h"
+#include "Basics/StaticStrings.h"
 #include "Basics/StringUtils.h"
 #include "Statistics/RequestStatistics.h"
 #include "VocBase/vocbase.h"
@@ -40,7 +41,7 @@ RestActionHandler::RestActionHandler(application_features::ApplicationServer& se
 
 RestStatus RestActionHandler::execute() {
   // check the request path
-  if (_request->databaseName() == TRI_VOC_SYSTEM_DATABASE) {
+  if (_request->databaseName() == StaticStrings::SystemDatabase) {
     if (StringUtils::isPrefix(_request->requestPath(), "/_admin/aardvark")) {
       RequestStatistics::SET_IGNORE(_statistics);
     }

diff --git a/arangod/Auth/User.cpp b/arangod/Auth/User.cpp
@@ -559,7 +559,7 @@ auth::Level auth::User::collectionAuthLevel(std::string const& dbname,
   bool isSystem = cname[0] == '_';
   if (isSystem) {
     // disallow access to _system/_users for everyone
-    if (dbname == TRI_VOC_SYSTEM_DATABASE && cname == TRI_COL_NAME_USERS) {
+    if (dbname == StaticStrings::SystemDatabase && cname == TRI_COL_NAME_USERS) {
       return auth::Level::NONE;
     } else if (cname == "_queues") {
       return auth::Level::RO;

diff --git a/arangod/Cluster/ClusterInfo.cpp b/arangod/Cluster/ClusterInfo.cpp
@@ -32,6 +32,7 @@
 #include "Basics/MutexLocker.h"
 #include "Basics/NumberUtils.h"
 #include "Basics/RecursiveLocker.h"
+#include "Basics/StaticStrings.h"
 #include "Basics/StringUtils.h"
 #include "Basics/VelocyPackHelper.h"
 #include "Basics/WriteLocker.h"
@@ -1759,7 +1760,7 @@ Result ClusterInfo::dropDatabaseCoordinator(  // drop database
     double timeout                            // request timeout
 ) {
   TRI_ASSERT(ServerState::instance()->isCoordinator());
-  if (name == TRI_VOC_SYSTEM_DATABASE) {
+  if (name == StaticStrings::SystemDatabase) {
     return Result(TRI_ERROR_FORBIDDEN);
   }
 

diff --git a/arangod/MMFiles/MMFilesEngine.cpp b/arangod/MMFiles/MMFilesEngine.cpp
@@ -29,6 +29,7 @@
 #include "Basics/MutexLocker.h"
 #include "Basics/MutexUnlocker.h"
 #include "Basics/ReadLocker.h"
+#include "Basics/StaticStrings.h"
 #include "Basics/StringUtils.h"
 #include "Basics/VelocyPackHelper.h"
 #include "Basics/WriteLocker.h"
@@ -264,7 +265,7 @@ void MMFilesEngine::start() {
   if (names.empty()) {
     // no databases found, i.e. there is no system database!
     // create a database for the system database
-    int res = createDatabaseDirectory(TRI_NewTickServer(), TRI_VOC_SYSTEM_DATABASE);
+    int res = createDatabaseDirectory(TRI_NewTickServer(), StaticStrings::SystemDatabase);
 
     if (res != TRI_ERROR_NO_ERROR) {
       LOG_TOPIC("982c7", ERR, arangodb::Logger::ENGINES)

diff --git a/arangod/Replication/GlobalInitialSyncer.cpp b/arangod/Replication/GlobalInitialSyncer.cpp
@@ -25,6 +25,7 @@
 
 #include "ApplicationFeatures/ApplicationServer.h"
 #include "Basics/Result.h"
+#include "Basics/StaticStrings.h"
 #include "Basics/StringUtils.h"
 #include "Basics/VelocyPackHelper.h"
 #include "Replication/DatabaseInitialSyncer.h"
@@ -50,7 +51,7 @@ using namespace arangodb::rest;
 GlobalInitialSyncer::GlobalInitialSyncer(ReplicationApplierConfiguration const& configuration)
     : InitialSyncer(configuration) {
   // has to be set here, otherwise broken
-  _state.databaseName = TRI_VOC_SYSTEM_DATABASE;
+  _state.databaseName = StaticStrings::SystemDatabase;
 }
 
 GlobalInitialSyncer::~GlobalInitialSyncer() {

diff --git a/arangod/Replication/GlobalTailingSyncer.cpp b/arangod/Replication/GlobalTailingSyncer.cpp
@@ -22,6 +22,7 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 #include "GlobalTailingSyncer.h"
+#include "Basics/StaticStrings.h"
 #include "Basics/Thread.h"
 #include "Logger/LogMacros.h"
 #include "Logger/Logger.h"
@@ -43,7 +44,7 @@ GlobalTailingSyncer::GlobalTailingSyncer(ReplicationApplierConfiguration const&
                     configuration, initialTick, useTick, barrierId),
       _queriedTranslations(false) {
   _ignoreDatabaseMarkers = false;
-  _state.databaseName = TRI_VOC_SYSTEM_DATABASE;
+  _state.databaseName = StaticStrings::SystemDatabase;
 }
 
 std::string GlobalTailingSyncer::tailingBaseUrl(std::string const& command) {

diff --git a/arangod/Replication/TailingSyncer.cpp b/arangod/Replication/TailingSyncer.cpp
@@ -334,7 +334,7 @@ Result TailingSyncer::processDBMarker(TRI_r
10000
eplication_operation_e type,
 
     TRI_vocbase_t* vocbase = DatabaseFeature::DATABASE->lookupDatabase(name);
 
-    if (vocbase != nullptr && name != TRI_VOC_SYSTEM_DATABASE) {
+    if (vocbase != nullptr && name != StaticStrings::SystemDatabase) {
       LOG_TOPIC("0a3a4", WARN, Logger::REPLICATION)
           << "seeing database creation marker "
           << "for an already existing db. Dropping db...";
@@ -357,7 +357,7 @@ Result TailingSyncer::processDBMarker(TRI_replication_operation_e type,
   } else if (type == REPLICATION_DATABASE_DROP) {
     TRI_vocbase_t* vocbase = DatabaseFeature::DATABASE->lookupDatabase(name);
 
-    if (vocbase != nullptr && name != TRI_VOC_SYSTEM_DATABASE) {
+    if (vocbase != nullptr && name != StaticStrings::SystemDatabase) {
       // abort all ongoing transactions for the database to be dropped
       abortOngoingTransactions(name);
 

diff --git a/arangod/RestHandler/RestAdminClusterHandler.cpp b/arangod/RestHandler/RestAdminClusterHandler.cpp
@@ -272,10 +272,10 @@ std::string const RestAdminClusterHandler::RemoveServer = "removeServer";
 std::string const RestAdminClusterHandler::RebalanceShards = "rebalanceShards";
 
 RestStatus RestAdminClusterHandler::execute() {
-  if (!ExecContext::current().isAdminUser()) {
-    generateError(rest::ResponseCode::FORBIDDEN, TRI_ERROR_HTTP_FORBIDDEN);
-    return RestStatus::DONE;
-  }
+  // No more check for admin rights here, since we handle this in every individual
+  // method below. Some of them do no longer require admin access
+  // (e.g. /_admin/cluster/health). If you add a new API below here, please
+  // make sure to check for permissions!
 
   auto const& suffixes = request()->suffixes();
   size_t const len = suffixes.size();
@@ -965,6 +965,11 @@ RestStatus RestAdminClusterHandler::handleShardDistribution() {
     return RestStatus::DONE;
   }
 
+  if (!ExecContext::current().isAdminUser()) {
+    generateError(rest::ResponseCode::FORBIDDEN, TRI_ERROR_HTTP_FORBIDDEN);
+    return RestStatus::DONE;
+  }
+
   if (request()->requestType() != rest::RequestType::GET) {
     generateError(rest::ResponseCode::METHOD_NOT_ALLOWED, TRI_ERROR_HTTP_METHOD_NOT_ALLOWED);
     return RestStatus::DONE;
@@ -1014,6 +1019,11 @@ RestStatus RestAdminClusterHandler::handleCollectionShardDistribution() {
     return RestStatus::DONE;
   }
 
+  if (!ExecContext::current().isAdminUser()) {
+    generateError(rest::ResponseCode::FORBIDDEN, TRI_ERROR_HTTP_FORBIDDEN);
+    return RestStatus::DONE;
+  }
+
   switch (request()->requestType()) {
     case rest::RequestType::GET:
       return handleGetCollectionShardDistribution(
@@ -1392,7 +1402,8 @@ RestStatus RestAdminClusterHandler::handlePutNumberOfServers() {
 }
 
 RestStatus RestAdminClusterHandler::handleNumberOfServers() {
-  if (!ServerState::instance()->isCoordinator()) {
+  if (!ServerState::instance()->isCoordinator() ||
+      !ExecContext::current().isAdminUser()) {
     generateError(rest::ResponseCode::FORBIDDEN, TRI_ERROR_HTTP_FORBIDDEN,
                   "only allowed on coordinators");
     return RestStatus::DONE;
@@ -1410,11 +1421,9 @@ RestStatus RestAdminClusterHandler::handleNumberOfServers() {
 }
 
 RestStatus RestAdminClusterHandler::handleHealth() {
-  if (!ExecContext::current().isAdminUser()) {
-    generateError(rest::ResponseCode::FORBIDDEN, TRI_ERROR_HTTP_FORBIDDEN);
-    return RestStatus::DONE;
-  }
-
+  // We allow this API whenever one is authenticated in some way. There used
+  // to be a check for isAdminUser here. However, we want that the UI with
+  // the cluster health dashboard works for every authenticated user.
   if (request()->requestType() != rest::RequestType::GET) {
     generateError(rest::ResponseCode::METHOD_NOT_ALLOWED, TRI_ERROR_HTTP_METHOD_NOT_ALLOWED);
     return RestStatus::DONE;

diff --git a/arangod/RestHandler/RestAdminServerHandler.cpp b/arangod/RestHandler/RestAdminServerHandler.cpp
@@ -25,6 +25,7 @@
 
 #include "Actions/RestActionHandler.h"
 #include "ApplicationFeatures/ApplicationServer.h"
+#include "Basics/StaticStrings.h"
 #include "GeneralServer/AuthenticationFeature.h"
 #include "GeneralServer/GeneralServerFeature.h"
 #include "GeneralServer/SslServerFeature.h"
@@ -159,7 +160,7 @@ void RestAdminServerHandler::handleMode() {
     if (af->isActive() && !_request->user().empty()) {
       auth::Level lvl;
       if (af->userManager() != nullptr) {
-        lvl = af->userManager()->databaseAuthLevel(_request->user(), TRI_VOC_SYSTEM_DATABASE,
+        lvl = af->userManager()->databaseAuthLevel(_request->user(), StaticStrings::SystemDatabase,
                                                    /*configured*/ true);
       } else {
         lvl = auth::Level::RW;

diff --git a/arangod/RestHandler/RestReplicationHandler.cpp b/arangod/RestHandler/RestReplicationHandler.cpp
@@ -1049,7 +1049,9 @@ Result RestReplicationHandler::processRestoreCollection(VPackSlice const& collec
     return Result();
   }
 
-  ExecContextSuperuserScope escope(ExecContext::current().isAdminUser());
+  ExecContextSuperuserScope escope(
+      ExecContext::current().isSuperuser() ||
+      (ExecContext::current().isAdminUser() && !ServerState::readOnly()));
 
   auto* col = _vocbase.lookupCollection(name).get();
 
@@ -1427,7 +1429,9 @@ Result RestReplicationHandler::processRestoreData(std::string const& colName) {
   bool generateNewRevisionIds =
       !_request->parsedValue(StaticStrings::PreserveRevisionIds, false);
 
-  ExecContextSuperuserScope escope(ExecContext::current().isAdminUser());
+  ExecContextSuperuserScope escope(
+      ExecContext::current().isSuperuser() ||
+      (ExecContext::current().isAdminUser() && !ServerState::readOnly()));
 
   if (colName == TRI_COL_NAME_USERS) {
     // We need to handle the _users in a special way
@@ -1898,7 +1902,10 @@ Result RestReplicationHandler::processRestoreIndexes(VPackSlice const& collectio
 
   Result fres;
 
-  ExecContextSuperuserScope escope(ExecContext::current().isAdminUser());
+  ExecContextSuperuserScope escope(
+      ExecContext::current().isSuperuser() ||
+      (ExecContext::current().isAdminUser() && !ServerState::readOnly()));
+
   READ_LOCKER(readLocker, _vocbase._inventoryLock);
 
   // look up the collection

diff --git a/arangod/RestServer/DatabaseFeature.cpp b/arangod/RestServer/DatabaseFeature.cpp
@@ -33,6 +33,7 @@
 #include "Basics/FileUtils.h"
 #include "Basics/MutexLocker.h"
 #include "Basics/NumberUtils.h"
+#include "Basics/StaticStrings.h"
 #include "Basics/StringUtils.h"
 #include "Basics/VelocyPackHelper.h"
 #include "Basics/WriteLocker.h"
@@ -377,7 +378,7 @@ void DatabaseFeature::start() {
     FATAL_ERROR_EXIT();
   }
 
-  if (!lookupDatabase(TRI_VOC_SYSTEM_DATABASE)) {
+  if (!lookupDatabase(StaticStrings::SystemDatabase)) {
     LOG_TOPIC("97e7c", FATAL, arangodb::Logger::FIXME)
         << "No _system database found in database directory. Cannot start!";
     FATAL_ERROR_EXIT();
@@ -740,7 +741,7 @@ Result DatabaseFeature::createDatabase(CreateDatabaseInfo&& info, TRI_vocbase_t*
 /// @brief drop database
 int DatabaseFeature::dropDatabase(std::string const& name, bool waitForDeletion,
                                   bool removeAppsDirectory) {
-  if (name == TRI_VOC_SYSTEM_DATABASE) {
+  if (name == StaticStrings::SystemDatabase) {
     // prevent deletion of system database
     events::DropDatabase(name, TRI_ERROR_FORBIDDEN);
     return TRI_ERROR_FORBIDDEN;
@@ -892,7 +893,7 @@ std::vector<TRI_voc_tick_t> DatabaseFeature::getDatabaseIds(bool includeSystem)
       if (vocbase->isDropped()) {
         continue;
       }
-      if (includeSystem || vocbase->name() != TRI_VOC_SYSTEM_DATABASE) {
+      if (includeSystem || vocbase->name() != StaticStrings::SystemDatabase) {
         ids.emplace_back(vocbase->id());
       }
     }
@@ -1095,7 +1096,7 @@ void DatabaseFeature::updateContexts() {
     return;
   }
 
-  auto* vocbase = useDatabase(TRI_VOC_SYSTEM_DATABASE);
+  auto* vocbase = useDatabase(StaticStrings::SystemDatabase);
   TRI_ASSERT(vocbase);
 
   auto queryRegistry = QueryRegistryFeature::registry();

diff --git a/arangod/RestServer/SystemDatabaseFeature.cpp b/arangod/RestServer/SystemDatabaseFeature.cpp
@@ -24,6 +24,7 @@
 #include "SystemDatabaseFeature.h"
 
 #include "ApplicationFeatures/ApplicationServer.h"
+#include "Basics/StaticStrings.h"
 #include "Basics/application-exit.h"
 #include "Logger/LogMacros.h"
 #include "RestServer/DatabaseFeature.h"
@@ -55,7 +56,7 @@ SystemDatabaseFeature::SystemDatabaseFeature(application_features::ApplicationSe
 void SystemDatabaseFeature::start() {
   if (server().hasFeature<arangodb::DatabaseFeature>()) {
     auto& feature = server().getFeature<arangodb::DatabaseFeature>();
-    _vocbase.store(feature.lookupDatabase(TRI_VOC_SYSTEM_DATABASE));
+    _vocbase.store(feature.lookupDatabase(StaticStrings::SystemDatabase));
 
     return;
   }

diff --git a/arangod/RestServer/VocbaseContext.cpp b/arangod/RestServer/VocbaseContext.cpp
@@ -22,6 +22,7 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 #include "VocbaseContext.h"
+#include "Basics/StaticStrings.h"
 #include "Cluster/ServerState.h"
 #include "GeneralServer/AuthenticationFeature.h"
 #include "Logger/LogMacros.h"
@@ -35,8 +36,8 @@ namespace arangodb {
 
 VocbaseContext::VocbaseContext(GeneralRequest& req, TRI_vocbase_t& vocbase,
                                ExecContext::Type type, auth::Level systemLevel,
-                               auth::Level dbLevel)
-    : ExecContext(type, req.user(), req.databaseName(), systemLevel, dbLevel),
+                               auth::Level dbLevel, bool isAdminUser)
+    : ExecContext(type, req.user(), req.databaseName(), systemLevel, dbLevel, isAdminUser),
       _vocbase(vocbase) {
   // _vocbase has already been refcounted for us
   TRI_ASSERT(!_vocbase.isDangling());
@@ -58,7 +59,7 @@ VocbaseContext* VocbaseContext::create(GeneralRequest& req, TRI_vocbase_t& vocba
   if (isSuperUser) {
     return new VocbaseContext(req, vocbase, ExecContext::Type::Internal,
                               /*sysLevel*/ auth::Level::RW,
-                              /*dbLevel*/ auth::Level::RW);
+                              /*dbLevel*/ auth::Level::RW, true);
   }
 
   AuthenticationFeature* auth = AuthenticationFeature::instance();
@@ -68,19 +69,21 @@ VocbaseContext* VocbaseContext::create(GeneralRequest& req, TRI_vocbase_t& vocba
       // special read-only case
       return new VocbaseContext(req, vocbase, ExecContext::Type::Internal,
                                 /*sysLevel*/ auth::Level::RO,
-                                /*dbLevel*/ auth::Level::RO);
+                                /*dbLevel*/ auth::Level::RO, true);
     }
     return new VocbaseContext(req, vocbase, req.user().empty() ?
                               ExecContext::Type::Internal : ExecContext::Type::Default,
                               /*sysLevel*/ auth::Level::RW,
-                              /*dbLevel*/ auth::Level::RW);
+                              /*dbLevel*/ auth::Level::RW, true);
   }
 
   if (!req.authenticated()) {
     return new VocbaseContext(req, vocbase, ExecContext::Type::Default,
                               /*sysLevel*/ auth::Level::NONE,
-                              /*dbLevel*/ auth::Level::NONE);
-  } else if (req.user().empty()) {
+                              /*dbLevel*/ auth::Level::NONE, false);
+  } 
+
+  if (req.user().empty()) {
     std::string msg = "only jwt can be used to authenticate as superuser";
     LOG_TOPIC("2d0f6", WARN, Logger::AUTHENTICATION) << msg;
     THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_BAD_PARAMETER, msg);
@@ -93,26 +96,32 @@ VocbaseContext* VocbaseContext::create(GeneralRequest& req, TRI_vocbase_t& vocba
     return nullptr;
   }
 
-  auth::Level dbLvl = um->databaseAuthLevel(req.user(), req.databaseName());
+  auth::Level dbLvl = um->databaseAuthLevel(req.user(), req.databaseName(), false);
   auth::Level sysLvl = dbLvl;
-  if (req.databaseName() != TRI_VOC_SYSTEM_DATABASE) {
-    sysLvl = um->databaseAuthLevel(req.user(), TRI_VOC_SYSTEM_DATABASE);
+  if (req.databaseName() != StaticStrings::SystemDatabase) {
+    sysLvl = um->databaseAuthLevel(req.user(), StaticStrings::SystemDatabase, false);
+  }
+  bool isAdminUser = (sysLvl == auth::Level::RW);
+  if (!isAdminUser && ServerState::readOnly()) {
+    // in case we are in read-only mode, we need to re-check the original permissions
+    isAdminUser = um->databaseAuthLevel(req.user(), StaticStrings::SystemDatabase, true) ==
+                  auth::Level::RW;
   }
 
   return new VocbaseContext(req, vocbase, ExecContext::Type::Default,
                             /*sysLevel*/ sysLvl,
-                            /*dbLevel*/ dbLvl);
+                            /*dbLevel*/ dbLvl, isAdminUser);
 }
 
 void VocbaseContext::forceSuperuser() {
   TRI_ASSERT(_type != ExecContext::Type::Internal || _user.empty());
-  _type = ExecContext::Type::Internal;
   if (ServerState::readOnly()) {
-    _systemDbAuthLevel = auth::Level::RO;
-    _databaseAuthLevel = auth::Level::RO;
+    forceReadOnly();
   } else {
+    _type = ExecContext::Type::Internal;
     _systemDbAuthLevel = auth::Level::RW;
     _databaseAuthLevel = auth::Level::RW;
+    _isAdminUser = true;
   }
 }
 

diff --git a/arangod/RestServer/VocbaseContext.h b/arangod/RestServer/VocbaseContext.h
@@ -52,7 +52,7 @@ class VocbaseContext : public arangodb::ExecContext {
   TRI_vocbase_t& _vocbase;
 
   VocbaseContext(GeneralRequest& req, TRI_vocbase_t& vocbase, ExecContext::Type type,
-                 auth::Level systemLevel, auth::Level dbLevel);
+                 auth::Level systemLevel, auth::Level dbLevel, bool isAdminUser);
   VocbaseContext(VocbaseContext const&) = delete;
   VocbaseContext& operator=(VocbaseContext const&) = delete;
 };