From 43b6e77235afe395ac3684419a86064a3710504c Mon Sep 17 00:00:00 2001 From: David Rowe Date: Mon, 3 Aug 2020 09:17:12 +1200 Subject: [PATCH] Tidy networking code --- .../resources/describe-settings.json | 6 +-- domain-server/src/DomainGatekeeper.cpp | 37 ++++++------- domain-server/src/DomainGatekeeper.h | 2 +- .../networking/src/DomainAccountManager.cpp | 54 ++++++++++--------- .../networking/src/DomainAccountManager.h | 4 +- 5 files changed, 54 insertions(+), 49 deletions(-) diff --git a/domain-server/resources/describe-settings.json b/domain-server/resources/describe-settings.json index 2593740969..5560fdba56 100644 --- a/domain-server/resources/describe-settings.json +++ b/domain-server/resources/describe-settings.json @@ -85,9 +85,9 @@ "backup": false }, { - "name": "oauth2_url_base", - "label": "Authentication URL Base", - "help": "The URL base that the Interface will use to login via OAuth2.", + "name": "oauth2_url_path", + "label": "Authentication URL", + "help": "The URL that the Interface will use to login via OAuth2.", "advanced": true }, { diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index 6227ddb634..16c7383bf9 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -450,7 +450,7 @@ SharedNodePointer DomainGatekeeper::processAssignmentConnectRequest(const NodeCo } const QString AUTHENTICATION_ENAABLED = "authentication.enable_oauth2"; -const QString AUTHENTICATION_OAUTH2_URL_BASE = "authentication.oauth2_url_base"; +const QString AUTHENTICATION_OAUTH2_URL_PATH = "authentication.oauth2_url_path"; const QString AUTHENTICATION_WORDPRESS_URL_BASE = "authentication.wordpress_url_base"; const QString MAXIMUM_USER_CAPACITY = "security.maximum_user_capacity"; const QString MAXIMUM_USER_CAPACITY_REDIRECT_LOCATION = "security.maximum_user_capacity_redirect_location"; @@ -548,10 +548,9 @@ SharedNodePointer DomainGatekeeper::processAgentConnectRequest(const NodeConnect if (!userPerms.can(NodePermissions::Permission::canConnectToDomain)) { if (domainHasLogin()) { QString domainAuthURL; - auto domainAuthURLVariant = _server->_settingsManager.valueForKeyPath(AUTHENTICATION_OAUTH2_URL_BASE); + auto domainAuthURLVariant = _server->_settingsManager.valueForKeyPath(AUTHENTICATION_OAUTH2_URL_PATH); if (domainAuthURLVariant.canConvert()) { domainAuthURL = domainAuthURLVariant.toString(); - qDebug() << "Domain authorization URL:" << domainAuthURL; } sendConnectionDeniedPacket("You lack the required permissions to connect to this domain.", nodeConnection.senderSockAddr, DomainHandler::ConnectionRefusedReason::NotAuthorizedDomain, domainAuthURL); @@ -1220,7 +1219,7 @@ bool DomainGatekeeper::domainHasLogin() { // The domain may have its own users and groups in a WordPress site. // ####### TODO: Add checks of any further domain server settings used. return _server->_settingsManager.valueForKeyPath(AUTHENTICATION_ENAABLED).toBool() - && !_server->_settingsManager.valueForKeyPath(AUTHENTICATION_OAUTH2_URL_BASE).toString().isEmpty() + && !_server->_settingsManager.valueForKeyPath(AUTHENTICATION_OAUTH2_URL_PATH).toString().isEmpty() && !_server->_settingsManager.valueForKeyPath(AUTHENTICATION_WORDPRESS_URL_BASE).toString().isEmpty(); } @@ -1233,7 +1232,7 @@ void DomainGatekeeper::requestDomainUser(const QString& username, const QString& } if (_inFlightDomainUserIdentityRequests.contains(username)) { - // Domain identify request for this username is already flight. + // Domain identify request for this username is already in progress. return; } _inFlightDomainUserIdentityRequests.insert(username, QPair(accessToken, refreshToken)); @@ -1248,9 +1247,9 @@ void DomainGatekeeper::requestDomainUser(const QString& username, const QString& } // Get data pertaining to "me", the user who generated the access token. - // ####### TODO: Confirm API_ROUTE w.r.t. OAuth2 plugin's capabilities. - QString API_ROUTE = "wp/v2/users/me?context=edit&_fields=id,username,roles"; - QUrl domainUserURL = apiBase + API_ROUTE; + // ####### TODO: Confirm API route and data w.r.t. OAuth2 plugin's capabilities. [plugin] + const QString WORDPRESS_USER_ROUTE = "wp/v2/users/me?context=edit&_fields=id,username,roles"; + QUrl domainUserURL = apiBase + WORDPRESS_USER_ROUTE; // ####### TODO: Append a random key to check in response? @@ -1258,7 +1257,6 @@ void DomainGatekeeper::requestDomainUser(const QString& username, const QString& request.setHeader(QNetworkRequest::UserAgentHeader, NetworkingConstants::VIRCADIA_USER_AGENT); request.setHeader(QNetworkRequest::ContentTypeHeader, "application/x-www-form-urlencoded"); - // ####### TODO: WordPress plugin's authorization requirements. request.setRawHeader(QByteArray("Authorization"), QString("Bearer " + accessToken).toUtf8()); QByteArray formData; // No data to send. @@ -1278,12 +1276,13 @@ void DomainGatekeeper::requestDomainUserFinished() { QNetworkReply* requestReply = reinterpret_cast(sender()); + QJsonDocument jsonResponse = QJsonDocument::fromJson(requestReply->readAll()); + const QJsonObject& rootObject = jsonResponse.object(); + auto httpStatus = requestReply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); if (200 <= httpStatus && httpStatus < 300) { // Success. - QJsonDocument jsonResponse = QJsonDocument::fromJson(requestReply->readAll()); - const QJsonObject& rootObject = jsonResponse.object(); - // ####### Expected response: + // ####### TODO: Verify Expected response. [plugin] /* { id: 2, @@ -1292,22 +1291,24 @@ void DomainGatekeeper::requestDomainUserFinished() { } */ - // ####### TODO: Handle invalid / unexpected response. - QString username = rootObject["username"].toString().toLower(); - // ####### TODO: Handle invalid username or one that isn't in the _inFlight list. - if (_inFlightDomainUserIdentityRequests.contains(username)) { // Success! Verified user. _verifiedDomainUserIdentities.insert(username, _inFlightDomainUserIdentityRequests.value(username)); _inFlightDomainUserIdentityRequests.remove(username); } else { - // Unexpected response. - // ####### TODO + // Failure. + qDebug() << "Unexpected username in response for user details -" << username; } + // ####### TODO: Handle roles if available. [plugin] + } else { // Failure. + + // ####### TODO: Error fields to report. [plugin] + qDebug() << "Error in response for user details -" << rootObject["error"].toString(); + // ####### TODO: Is this the best way to handle _inFlightDomainUserIdentityRequests? // If there's a brief network glitch will it recover? // Perhaps clear on a timer? Cancel timer upon subsequent successful responses? diff --git a/domain-server/src/DomainGatekeeper.h b/domain-server/src/DomainGatekeeper.h index 263d5b853d..0e1da5d6ee 100644 --- a/domain-server/src/DomainGatekeeper.h +++ b/domain-server/src/DomainGatekeeper.h @@ -155,7 +155,7 @@ private: void requestDomainUser(const QString& username, const QString& accessToken, const QString& refreshToken); typedef QHash> DomainUserIdentities; // > - DomainUserIdentities _inFlightDomainUserIdentityRequests; // Keep track of domain user identity requests in progress. + DomainUserIdentities _inFlightDomainUserIdentityRequests; // Domain user identity requests currently in progress. DomainUserIdentities _verifiedDomainUserIdentities; // Verified domain users. void getDomainGroupMemberships(const QString& domainUserName); diff --git a/libraries/networking/src/DomainAccountManager.cpp b/libraries/networking/src/DomainAccountManager.cpp index 7fc02893eb..5cf707e732 100644 --- a/libraries/networking/src/DomainAccountManager.cpp +++ b/libraries/networking/src/DomainAccountManager.cpp @@ -60,7 +60,12 @@ void DomainAccountManager::setAuthURL(const QUrl& authURL) { qCDebug(networking) << "AccountManager URL for authenticated requests has been changed to" << qPrintable(_authURL.toString()); - // ####### TODO: See AccountManager::setAuthURL(). + _access_token = ""; + _refresh_token = ""; + + // ####### TODO: Restore and refresh OAuth2 tokens if have them for this domain. + + // ####### TODO: Handle "keep me logged in". } } @@ -84,9 +89,7 @@ void DomainAccountManager::requestAccessToken(const QString& username, const QSt formData.append("scope=" + DOMAIN_ACCOUNT_MANAGER_REQUESTED_SCOPE); // ####### TODO: Include state? - QUrl domainAuthURL = _authURL; - domainAuthURL.setPath("/token"); // ####### TODO: miniOrange-mandated URL. ####### TODO: Should this be included in the server settings value? - request.setUrl(domainAuthURL); + request.setUrl(_authURL); request.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); @@ -96,40 +99,40 @@ void DomainAccountManager::requestAccessToken(const QString& username, const QSt } void DomainAccountManager::requestAccessTokenFinished() { + QNetworkReply* requestReply = reinterpret_cast(sender()); QJsonDocument jsonResponse = QJsonDocument::fromJson(requestReply->readAll()); const QJsonObject& rootObject = jsonResponse.object(); - // ####### TODO: Test HTTP response codes rather than object contains "error". - // #### reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200 - if (!rootObject.contains("error")) { - // ####### TODO: Process response scope? - // ####### TODO: Process response state? + auto httpStatus = requestReply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + if (200 <= httpStatus && httpStatus < 300) { + // ####### TODO: Check that token type == "Bearer"? + // ####### TODO: Process response state? + // ####### TODO: Process response scope? - if (!rootObject.contains("access_token") - // ####### TODO: Does WordPRess plugin provide "expires_in"? - // If so, handle here, or is it just the domain server that needs to use it? - //|| !rootObject.contains("expires_in") - || !rootObject.contains("token_type")) { + if (rootObject.contains("access_token")) { + // Success. - qCDebug(networking) << "Received a response for password grant that is missing one or more expected values."; - emit loginFailed(); - - } else { - - // clear the path from the response URL so we have the right root URL for this access token QUrl rootURL = requestReply->url(); rootURL.setPath(""); - qCDebug(networking) << "Storing a domain account with access-token for" << qPrintable(rootURL.toString()); - setAccessTokenFromJSON(rootObject); + setTokensFromJSON(rootObject, rootURL); - emit loginComplete(rootURL); + emit loginComplete(); + + } else { + // Failure. + qCDebug(networking) << "Received a response for password grant that is missing one or more expected values."; + emit loginFailed(); } + } else { - qCDebug(networking) << "Error in response for password grant -" << rootObject["error_description"].toString(); + // Failure. + + // ####### TODO: Error object fields to report. [plugin] + qCDebug(networking) << "Error in response for password grant -" << rootObject["error"].toString(); emit loginFailed(); } } @@ -171,13 +174,14 @@ bool DomainAccountManager::hasValidAccessToken() { } -void DomainAccountManager::setAccessTokenFromJSON(const QJsonObject& jsonObject) { +void DomainAccountManager::setTokensFromJSON(const QJsonObject& jsonObject, const QUrl& url) { _access_token = jsonObject["access_token"].toString(); _refresh_token = jsonObject["refresh_token"].toString(); // ####### TODO: Enable and use these. // ####### TODO: Protect these per AccountManager? /* + qCDebug(networking) << "Storing a domain account with access-token for" << qPrintable(url.toString()); domainAccessToken.set(jsonObject["access_token"].toString()); domainAccessRefreshToken.set(jsonObject["refresh_token"].toString()); domainAccessTokenExpiresIn.set(QDateTime::currentMSecsSinceEpoch() + (jsonObject["expires_in"].toDouble() * 1000)); diff --git a/libraries/networking/src/DomainAccountManager.h b/libraries/networking/src/DomainAccountManager.h index 08b625a246..6578963bf8 100644 --- a/libraries/networking/src/DomainAccountManager.h +++ b/libraries/networking/src/DomainAccountManager.h @@ -37,7 +37,7 @@ public slots: void requestAccessTokenFinished(); signals: void authRequired(); - void loginComplete(const QUrl& authURL); + void loginComplete(); void loginFailed(); void logoutComplete(); void newTokens(); @@ -47,7 +47,7 @@ private slots: private: bool hasValidAccessToken(); bool accessTokenIsExpired(); - void setAccessTokenFromJSON(const QJsonObject&); + void setTokensFromJSON(const QJsonObject&, const QUrl& url); void sendInterfaceAccessTokenToServer(); QUrl _authURL;