-
-
Notifications
You must be signed in to change notification settings - Fork 79
Feature/multiple account domain server #2343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
e5e4f32
00bdefb
580378b
805f771
5275df9
cad9102
fa1f2f3
e1d1425
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,30 +77,52 @@ $(document).ready(function(){ | |
| if (!verifyAvatarHeights()) { | ||
| return false; | ||
| } | ||
|
|
||
| // check if we've set the basic http password | ||
| if (formJSON["security"]) { | ||
|
|
||
| var password = formJSON["security"]["http_password"]; | ||
| var verify_password = formJSON["security"]["verify_http_password"]; | ||
| // Check if we have updated the basic http authentication settings | ||
| if (formJSON["security"] && formJSON["security"]["http_authentication"]) { | ||
| // Keep a list of usernames while going though all accounts to see if there are any duplicates. | ||
| let usernameList = []; | ||
|
|
||
| // if they've only emptied out the default password field, we should go ahead and acknowledge | ||
| // the verify password field | ||
| if (password != undefined && verify_password == undefined) { | ||
| verify_password = ""; | ||
| } | ||
| for (loginIndex in formJSON["security"]["http_authentication"]) { | ||
| var loginPair = formJSON["security"]["http_authentication"][loginIndex]; | ||
|
|
||
| // if we have a password and its verification, convert it to sha256 for comparison | ||
| if (password != undefined && verify_password != undefined) { | ||
| formJSON["security"]["http_password"] = sha256_digest(password); | ||
| formJSON["security"]["verify_http_password"] = sha256_digest(verify_password); | ||
| var username = loginPair["http_username"].trim() || null; | ||
| var password = loginPair["http_password"] || null; | ||
| var passwordVerify = loginPair["http_password_verify"] || null; | ||
|
|
||
| if (password == verify_password) { | ||
| delete formJSON["security"]["verify_http_password"]; | ||
| } else { | ||
| if (!username) { | ||
| // Account does not have a user name, don't allow blank username. | ||
| bootbox.alert({ "message": "Account must have a username", "title": "Username Error" }); | ||
| return false; | ||
| } | ||
|
|
||
| if (usernameList.includes(username)) { | ||
| // Account already exists with this username, don't allow duplicate usernames. | ||
| bootbox.alert({ "message": `Account already exists with the username "${username}"`, "title": "Username Error" }); | ||
| return false; | ||
| } | ||
|
|
||
| if (passwordVerify && password !== passwordVerify) { | ||
| // Tried to change the password, but the password does not match. | ||
| bootbox.alert({ "message": "Passwords must match!", "title": "Password Error" }); | ||
| return false; | ||
| } | ||
|
|
||
| if (passwordVerify && password == passwordVerify) { | ||
| // If password and password verify match, we are changing the password for an account. | ||
| console.log(`Changing ${loginPair["http_username_multi"]}'s password`); | ||
|
|
||
| // Hash the new password | ||
| password = sha256_digest(password); | ||
| } | ||
|
|
||
| usernameList.push(username); | ||
|
|
||
| // Delete the verification field so we don't try and save it. | ||
| delete formJSON["security"]["http_authentication"][loginIndex]["http_password_verify"]; | ||
|
|
||
| // Set the form password value to the new hashed value of that password | ||
| formJSON["security"]["http_authentication"][loginIndex]["http_password"] = password; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If password doesn't verify, I believe we leave the password in clear text |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2834,8 +2834,7 @@ std::pair<bool, QString> DomainServer::isAuthenticatedRequest(HTTPConnection* c | |
| static const QByteArray HTTP_COOKIE_HEADER_KEY = "Cookie"; | ||
| static const QString ADMIN_USERS_CONFIG_KEY = "oauth.admin-users"; | ||
| static const QString ADMIN_ROLES_CONFIG_KEY = "oauth.admin-roles"; | ||
| static const QString BASIC_AUTH_USERNAME_KEY_PATH = "security.http_username"; | ||
| static const QString BASIC_AUTH_PASSWORD_KEY_PATH = "security.http_password"; | ||
| static const QString BASIC_AUTH_MULTI_PATH = "security.http_authentication"; | ||
| const QString COOKIE_UUID_REGEX_STRING = HIFI_SESSION_COOKIE_KEY + "=([\\d\\w-]+)($|;)"; | ||
|
|
||
| const QByteArray UNAUTHENTICATED_BODY = "You do not have permission to access this domain-server."; | ||
|
|
@@ -2856,7 +2855,7 @@ std::pair<bool, QString> DomainServer::isAuthenticatedRequest(HTTPConnection* c | |
| cookieUUID = cookieUUIDRegex.cap(1); | ||
| } | ||
|
|
||
| if (_settingsManager.valueForKeyPath(BASIC_AUTH_USERNAME_KEY_PATH).isValid()) { | ||
| if (_settingsManager.valueForKeyPath(BASIC_AUTH_MULTI_PATH).isValid()) { | ||
| qDebug() << "Config file contains web admin settings for OAuth and basic HTTP authentication." | ||
| << "These cannot be combined - using OAuth for authentication."; | ||
| } | ||
|
|
@@ -2926,7 +2925,7 @@ std::pair<bool, QString> DomainServer::isAuthenticatedRequest(HTTPConnection* c | |
| // we don't know about this user yet, so they are not yet authenticated | ||
| return { false, QString() }; | ||
| } | ||
| } else if (_settingsManager.valueForKeyPath(BASIC_AUTH_USERNAME_KEY_PATH).isValid()) { | ||
| } else if (_settingsManager.valueForKeyPath(BASIC_AUTH_MULTI_PATH).isValid()) { | ||
| // config file contains username and password combinations for basic auth | ||
| const QByteArray BASIC_AUTH_HEADER_KEY = "Authorization"; | ||
|
|
||
|
|
@@ -2944,9 +2943,29 @@ std::pair<bool, QString> DomainServer::isAuthenticatedRequest(HTTPConnection* c | |
| QString headerUsername = credentialList[0]; | ||
| QString headerPassword = credentialList[1]; | ||
|
|
||
| QString settingsUsername; | ||
| QVariant settingsPasswordVariant; | ||
|
|
||
| // we've pulled a username and password - now check if there is a match in our basic auth hash | ||
| QString settingsUsername = _settingsManager.valueForKeyPath(BASIC_AUTH_USERNAME_KEY_PATH).toString(); | ||
| QVariant settingsPasswordVariant = _settingsManager.valueForKeyPath(BASIC_AUTH_PASSWORD_KEY_PATH); | ||
| QVariant allAccounts = _settingsManager.valueForKeyPath(BASIC_AUTH_MULTI_PATH); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got to check whether you can lock yourself out -- I think it may be possible to delete every account. |
||
| QList<QVariant> accountList = allAccounts.toList(); | ||
|
|
||
| for (const QVariant &account : accountList) { | ||
| // Convert QVariant to QMap | ||
| QMap<QString, QVariant> accountMap = account.toMap(); | ||
|
|
||
| // Retrieve the values of the properties | ||
| QString httpUsername = accountMap.value("http_username").toString(); | ||
| QString httpPassword = accountMap.value("http_password").toString(); | ||
|
|
||
| if (httpUsername == headerUsername) { | ||
| // Found the username we are looking for | ||
| settingsUsername = httpUsername; | ||
| settingsPasswordVariant = httpPassword; | ||
|
|
||
| break; | ||
| } | ||
| } | ||
|
|
||
| QString settingsPassword = settingsPasswordVariant.isValid() ? settingsPasswordVariant.toString() : ""; | ||
| QString hexHeaderPassword = headerPassword.isEmpty() ? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -570,6 +570,33 @@ void DomainServerSettingsManager::setupConfigMap(const QString& userConfigFilena | |
| packPermissions(); | ||
| } | ||
|
|
||
| if (oldVersion < 2.8) { | ||
| // Turn HTTP authentication into the new array format that allows multiple accounts. | ||
| // https://github.com/overte-org/overte/pull/1350 | ||
| const QString HTTP_USERNAME = "security.http_username"; | ||
| const QString HTTP_PASSWORD = "security.http_password"; | ||
| const QString HTTP_AUTHENTICATION = "security.http_authentication"; | ||
|
|
||
| QVariant* httpUsernameValue = _configMap.valueForKeyPath(HTTP_USERNAME); | ||
| QVariant* httpPasswordValue = _configMap.valueForKeyPath(HTTP_PASSWORD); | ||
| QVariant* httpAuthentication = _configMap.valueForKeyPath(HTTP_AUTHENTICATION, true); | ||
|
|
||
| if (httpUsernameValue && httpUsernameValue->canConvert(QMetaType::QString)) { | ||
| qDebug() << "Migrating domain account to multi account friendly system."; | ||
| QJsonArray accountList; | ||
| QJsonObject accountListObject; | ||
|
|
||
| accountListObject.insert("http_username", httpUsernameValue->toString()); | ||
| accountListObject.insert("http_password", httpPasswordValue->toString()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this sends the hashed password to the UI of the domain server. This means that anyone with access can read the hashes of other users, and potentially break them. This might be used for nefarious ends. For instance, a current admin could obtain password hashes, crack them, and then log in as another user even after having their access removed. |
||
|
|
||
| // Append the existing account to the account list object | ||
| accountList.append(accountListObject); | ||
|
|
||
| // Set the array as the new value for http_authentication | ||
| *httpAuthentication = accountList; | ||
| } | ||
| } | ||
|
|
||
| // write the current description version to our settings | ||
| *versionVariant = _descriptionVersion; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should be
===