From 2872412f9c3feb50ae1f630c49f95e38d76ccc2c Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Mon, 26 Feb 2018 12:10:20 -0800 Subject: [PATCH] fixes for comments from code reviews --- .../resources/web/content/js/content.js | 16 +++---- .../src/ContentSettingsBackupHandler.cpp | 29 +++++++----- domain-server/src/DomainGatekeeper.cpp | 2 +- domain-server/src/DomainServer.cpp | 3 +- .../src/DomainServerSettingsManager.cpp | 45 ++++++++++--------- .../src/DomainServerSettingsManager.h | 17 +++++-- 6 files changed, 65 insertions(+), 47 deletions(-) diff --git a/domain-server/resources/web/content/js/content.js b/domain-server/resources/web/content/js/content.js index 6e80fde7f6..3fec5a9000 100644 --- a/domain-server/resources/web/content/js/content.js +++ b/domain-server/resources/web/content/js/content.js @@ -36,9 +36,7 @@ $(document).ready(function(){ // when the selected file is changed, enable the button if there's a selected file $('body').on('change', '#' + RESTORE_SETTINGS_FILE_ID, function() { - if ($(this).val()) { - $('#' + RESTORE_SETTINGS_UPLOAD_ID).attr('disabled', false); - } + $('#' + RESTORE_SETTINGS_UPLOAD_ID).attr('disabled', $(this).val().length == 0); }); // when the upload button is clicked, send the file to the DS @@ -48,7 +46,7 @@ $(document).ready(function(){ e.preventDefault(); swalAreYouSure( - "Your domain content will be replaced by the uploaded Content Archive or entity file", + "Your domain content will be replaced by the uploaded content archive or entity file", "Restore content", function() { var files = $('#' + RESTORE_SETTINGS_FILE_ID).prop('files'); @@ -100,7 +98,7 @@ $(document).ready(function(){ // construct the HTML needed for the content archives panel var html = "
"; html += ""; - html += "Your domain server makes regular archives of the content in your domain. In the list below, you can see and download all of your domain content and settings backups. " + html += "Your domain server makes regular archives of the content in your domain. In the list below, you can see and download all of your backups of domain content and content settings." html += "Click here to manage automatic content archive intervals."; html += "
"; html += ""; @@ -149,6 +147,7 @@ $(document).ready(function(){ if (isRestoring && !data.status.isRecovering) { // we were recovering and we finished - the DS is going to restart so show the restart modal showRestartModal(); + return; } isRestoring = data.status.isRecovering; @@ -171,7 +170,7 @@ $(document).ready(function(){ function updateProgressBars($progressBar, value) { $progressBar.attr('aria-valuenow', value).attr('style', 'width: ' + value + '%'); - $progressBar.find('.sr-only').html(data.status.recoveryProgress + "% Complete"); + $progressBar.find('.sr-only').html(value + "% Complete"); } // before we add any new rows and update existing ones @@ -218,12 +217,9 @@ $(document).ready(function(){ $backupRow.addClass(ACTIVE_BACKUP_ROW_CLASS); } - var automaticRows = ""; - if (automaticBackups.length > 0) { for (var backupIndex in automaticBackups) { updateOrAddTableRow(automaticBackups[backupIndex], AUTOMATIC_ARCHIVES_TBODY_ID); - } } @@ -382,7 +378,7 @@ $(document).ready(function(){ // show a sweet alert to ask the user to provide a name for their content archive swal({ - title: "Generate a Content Archive", + title: "Generate a content archive", type: "input", text: "This will capture the state of all the content in your domain right now, which you can save as a backup and restore from later.", confirmButtonText: "Generate Archive", diff --git a/domain-server/src/ContentSettingsBackupHandler.cpp b/domain-server/src/ContentSettingsBackupHandler.cpp index 7848cb1701..8ffd25b7a4 100644 --- a/domain-server/src/ContentSettingsBackupHandler.cpp +++ b/domain-server/src/ContentSettingsBackupHandler.cpp @@ -23,20 +23,31 @@ static const QString CONTENT_SETTINGS_BACKUP_FILENAME = "content-settings.json"; void ContentSettingsBackupHandler::createBackup(const QString& backupName, QuaZip& zip) { - // grab the content settings as JSON,excluding default values and values hidden from backup - QJsonObject contentSettingsJSON = _settingsManager.settingsResponseObjectForType("", true, false, true, false, true); + // grab the content settings as JSON, excluding default values and values hidden from backup + QJsonObject contentSettingsJSON = _settingsManager.settingsResponseObjectForType( + "", // include all settings types + DomainServerSettingsManager::Authenticated, DomainServerSettingsManager::NoDomainSettings, + DomainServerSettingsManager::IncludeContentSettings, DomainServerSettingsManager::NoDefaultSettings, + DomainServerSettingsManager::ForBackup + ); - // make a QJSonDocument using the object + // make a QJsonDocument using the object QJsonDocument contentSettingsDocument { contentSettingsJSON }; QuaZipFile zipFile { &zip }; - zipFile.open(QIODevice::WriteOnly, QuaZipNewInfo(CONTENT_SETTINGS_BACKUP_FILENAME)); - zipFile.write(contentSettingsDocument.toJson()); - zipFile.close(); + if (zipFile.open(QIODevice::WriteOnly, QuaZipNewInfo(CONTENT_SETTINGS_BACKUP_FILENAME))) { + if (zipFile.write(contentSettingsDocument.toJson()) == -1) { + qCritical().nospace() << "Failed to write to " << CONTENT_SETTINGS_BACKUP_FILENAME << ": " << zipFile.getZipError(); + } - if (zipFile.getZipError() != UNZ_OK) { - qCritical().nospace() << "Failed to zip " << CONTENT_SETTINGS_BACKUP_FILENAME << ": " << zipFile.getZipError(); + zipFile.close(); + + if (zipFile.getZipError() != UNZ_OK) { + qCritical().nospace() << "Failed to zip " << CONTENT_SETTINGS_BACKUP_FILENAME << ": " << zipFile.getZipError(); + } + } else { + qCritical().nospace() << "Failed to open " << CONTENT_SETTINGS_BACKUP_FILENAME << ": " << zipFile.getZipError(); } } @@ -59,7 +70,5 @@ void ContentSettingsBackupHandler::recoverBackup(const QString& backupName, QuaZ if (!_settingsManager.restoreSettingsFromObject(jsonDocument.object(), ContentSettings)) { qCritical() << "Failed to restore settings from" << CONTENT_SETTINGS_BACKUP_FILENAME << "in content archive"; - return; } - } diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index ac94d953d5..7d0b538f6e 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -613,7 +613,7 @@ bool DomainGatekeeper::isWithinMaxCapacity() { // find out what our maximum capacity is QVariant maximumUserCapacityVariant = _server->_settingsManager.valueForKeyPath(MAXIMUM_USER_CAPACITY); - unsigned int maximumUserCapacity = !maximumUserCapacityVariant.isValid() ? maximumUserCapacityVariant.toUInt() : 0; + unsigned int maximumUserCapacity = maximumUserCapacityVariant.isValid() ? maximumUserCapacityVariant.toUInt() : 0; if (maximumUserCapacity > 0) { unsigned int connectedUsers = _server->countConnectedUsers(); diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 4311a3ee69..0aace84aef 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -751,7 +751,7 @@ bool DomainServer::resetAccountManagerAccessToken() { if (accessToken.isEmpty()) { QVariant accessTokenVariant = _settingsManager.valueForKeyPath(ACCESS_TOKEN_KEY_PATH); - if (accessTokenVariant.isValid() && accessTokenVariant.canConvert(QMetaType::QString)) { + if (accessTokenVariant.canConvert(QMetaType::QString)) { accessToken = accessTokenVariant.toString(); } else { qWarning() << "No access token is present. Some operations that use the metaverse API will fail."; @@ -1637,7 +1637,6 @@ void DomainServer::sendHeartbeatToIceServer() { qWarning() << "Waiting for keypair generation to complete before sending ICE heartbeat."; if (!limitedNodeList->getSessionUUID().isNull()) { - qDebug() << "generating keypair"; accountManager->generateNewDomainKeypair(limitedNodeList->getSessionUUID()); } else { qWarning() << "Attempting to send ICE server heartbeat with no domain ID. This is not supported"; diff --git a/domain-server/src/DomainServerSettingsManager.cpp b/domain-server/src/DomainServerSettingsManager.cpp index f910799f9e..c46b2eda49 100644 --- a/domain-server/src/DomainServerSettingsManager.cpp +++ b/domain-server/src/DomainServerSettingsManager.cpp @@ -33,7 +33,6 @@ #include #include //for KillAvatarReason #include -#include #include "DomainServerNodeData.h" @@ -686,12 +685,12 @@ void DomainServerSettingsManager::unpackPermissions() { #ifdef WANT_DEBUG qDebug() << "--------------- permissions ---------------------"; - std::list permissionsSets { + std::array permissionsSets {{ &_standardAgentPermissions, &_agentPermissions, &_groupPermissions, &_groupForbiddens, &_ipPermissions, &_macPermissions, &_machineFingerprintPermissions - }; + }}; foreach (auto permissionSet, permissionsSets) { auto& permissionKeyMap = permissionSet->get(); @@ -1169,17 +1168,20 @@ bool DomainServerSettingsManager::handleAuthenticatedHTTPRequest(HTTPConnection QJsonObject rootObject; - bool forDomainSettings = (url.path() == SETTINGS_PATH_JSON); - bool forContentSettings = (url.path() == CONTENT_SETTINGS_PATH_JSON);; + DomainSettingsInclusion domainSettingsInclusion = (url.path() == SETTINGS_PATH_JSON) + ? IncludeDomainSettings : NoDomainSettings; + ContentSettingsInclusion contentSettingsInclusion = (url.path() == CONTENT_SETTINGS_PATH_JSON) + ? IncludeContentSettings : NoContentSettings; - rootObject[SETTINGS_RESPONSE_DESCRIPTION_KEY] = forDomainSettings + rootObject[SETTINGS_RESPONSE_DESCRIPTION_KEY] = (url.path() == SETTINGS_PATH_JSON) ? _domainSettingsDescription : _contentSettingsDescription; // grab a domain settings object for all types, filtered for the right class of settings // and exclude default values - rootObject[SETTINGS_RESPONSE_VALUE_KEY] = settingsResponseObjectForType("", true, - forDomainSettings, forContentSettings, - true); + rootObject[SETTINGS_RESPONSE_VALUE_KEY] = settingsResponseObjectForType("", Authenticated, + domainSettingsInclusion, + contentSettingsInclusion, + IncludeDefaultSettings); connection->respond(HTTPConnection::StatusCode200, QJsonDocument(rootObject).toJson(), "application/json"); @@ -1191,7 +1193,8 @@ bool DomainServerSettingsManager::handleAuthenticatedHTTPRequest(HTTPConnection } else if (url.path() == SETTINGS_BACKUP_PATH) { // grab the settings backup as an authenticated user // for the domain settings type only, excluding hidden and default values - auto currentDomainSettingsJSON = settingsResponseObjectForType("", true, true, false, false, true); + auto currentDomainSettingsJSON = settingsResponseObjectForType("", Authenticated, IncludeDomainSettings, + NoContentSettings, NoDefaultSettings, ForBackup); // setup headers that tell the client to download the file wth a special name Headers downloadHeaders; @@ -1343,13 +1346,15 @@ bool DomainServerSettingsManager::restoreSettingsFromObject(QJsonObject settings } } -QJsonObject DomainServerSettingsManager::settingsResponseObjectForType(const QString& typeValue, bool isAuthenticated, - bool includeDomainSettings, - bool includeContentSettings, - bool includeDefaults, bool isForBackup) { +QJsonObject DomainServerSettingsManager::settingsResponseObjectForType(const QString& typeValue, + SettingsRequestAuthentication authentication, + DomainSettingsInclusion domainSettingsInclusion, + ContentSettingsInclusion contentSettingsInclusion, + DefaultSettingsInclusion defaultSettingsInclusion, + SettingsBackupFlag settingsBackupFlag) { QJsonObject responseObject; - if (!typeValue.isEmpty() || isAuthenticated) { + if (!typeValue.isEmpty() || authentication == Authenticated) { // convert the string type value to a QJsonValue QJsonValue queryType = typeValue.isEmpty() ? QJsonValue() : QJsonValue(typeValue.toInt()); @@ -1358,9 +1363,9 @@ QJsonObject DomainServerSettingsManager::settingsResponseObjectForType(const QSt // only enumerate the requested settings type (domain setting or content setting) QJsonArray* filteredDescriptionArray = &_descriptionArray; - if (includeDomainSettings && !includeContentSettings) { + if (domainSettingsInclusion == IncludeDomainSettings && contentSettingsInclusion != IncludeContentSettings) { filteredDescriptionArray = &_domainSettingsDescription; - } else if (includeContentSettings && !includeDomainSettings) { + } else if (contentSettingsInclusion == IncludeContentSettings && domainSettingsInclusion != IncludeDomainSettings) { filteredDescriptionArray = &_contentSettingsDescription; } @@ -1383,14 +1388,14 @@ QJsonObject DomainServerSettingsManager::settingsResponseObjectForType(const QSt bool includedInBackups = !settingObject.contains(DESCRIPTION_BACKUP_FLAG_KEY) || settingObject[DESCRIPTION_BACKUP_FLAG_KEY].toBool(); - if (!settingObject[VALUE_HIDDEN_FLAG_KEY].toBool() && (!isForBackup || includedInBackups)) { + if (!settingObject[VALUE_HIDDEN_FLAG_KEY].toBool() && (settingsBackupFlag != ForBackup || includedInBackups)) { QJsonArray affectedTypesArray = settingObject[AFFECTED_TYPES_JSON_KEY].toArray(); if (affectedTypesArray.isEmpty()) { affectedTypesArray = groupObject[AFFECTED_TYPES_JSON_KEY].toArray(); } if (affectedTypesArray.contains(queryType) || - (queryType.isNull() && isAuthenticated)) { + (queryType.isNull() && authentication == Authenticated)) { QString settingName = settingObject[DESCRIPTION_NAME_KEY].toString(); // we need to check if the settings map has a value for this setting @@ -1408,7 +1413,7 @@ QJsonObject DomainServerSettingsManager::settingsResponseObjectForType(const QSt // final check for inclusion // either we include default values or we don't but this isn't a default value - if (includeDefaults || variantValue.isValid()) { + if ((defaultSettingsInclusion == IncludeDefaultSettings) || variantValue.isValid()) { QJsonValue result; if (!variantValue.isValid()) { diff --git a/domain-server/src/DomainServerSettingsManager.h b/domain-server/src/DomainServerSettingsManager.h index d81547410b..252ff79ae2 100644 --- a/domain-server/src/DomainServerSettingsManager.h +++ b/domain-server/src/DomainServerSettingsManager.h @@ -110,10 +110,19 @@ public: void debugDumpGroupsState(); + enum SettingsRequestAuthentication { NotAuthenticated, Authenticated }; + enum DomainSettingsInclusion { NoDomainSettings, IncludeDomainSettings }; + enum ContentSettingsInclusion { NoContentSettings, IncludeContentSettings }; + enum DefaultSettingsInclusion { NoDefaultSettings, IncludeDefaultSettings }; + enum SettingsBackupFlag { NotForBackup, ForBackup }; + /// thread safe method to retrieve a JSON representation of settings - Q_INVOKABLE QJsonObject settingsResponseObjectForType(const QString& typeValue, bool isAuthenticated = false, - bool includeDomainSettings = true, bool includeContentSettings = true, - bool includeDefaults = true, bool isForBackup = false); + QJsonObject settingsResponseObjectForType(const QString& typeValue, + SettingsRequestAuthentication authentication = NotAuthenticated, + DomainSettingsInclusion domainSettingsInclusion = IncludeDomainSettings, + ContentSettingsInclusion contentSettingsInclusion = IncludeContentSettings, + DefaultSettingsInclusion defaultSettingsInclusion = IncludeDefaultSettings, + SettingsBackupFlag settingsBackupFlag = NotForBackup); /// thread safe method to restore settings from a JSON object Q_INVOKABLE bool restoreSettingsFromObject(QJsonObject settingsToRestore, SettingsType settingsType); @@ -156,7 +165,7 @@ private: QJsonArray _contentSettingsDescription; QJsonObject _settingsMenuGroups; - // any method that calls _valueForKeyPath on this _configMap must get a write lock it keeps until it + // any method that calls valueForKeyPath on this _configMap must get a write lock it keeps until it // is done with the returned QVariant* HifiConfigVariantMap _configMap;