From a8831e89ff7a1af077bac1335e458b2f5aa4c25a Mon Sep 17 00:00:00 2001 From: David Kelly Date: Thu, 16 Feb 2017 09:50:56 -0700 Subject: [PATCH 1/4] Ban only by machine fingerprint, when possible --- .../src/DomainServerSettingsManager.cpp | 104 +++++++++--------- 1 file changed, 51 insertions(+), 53 deletions(-) diff --git a/domain-server/src/DomainServerSettingsManager.cpp b/domain-server/src/DomainServerSettingsManager.cpp index 23f663817d..661a6213b8 100644 --- a/domain-server/src/DomainServerSettingsManager.cpp +++ b/domain-server/src/DomainServerSettingsManager.cpp @@ -30,6 +30,7 @@ #include #include #include //for KillAvatarReason +#include #include "DomainServerNodeData.h" const QString SETTINGS_DESCRIPTION_RELATIVE_PATH = "/resources/describe-settings.json"; @@ -439,7 +440,7 @@ bool DomainServerSettingsManager::unpackPermissionsForKeypath(const QString& key foreach (QVariant permsHash, permissionsList) { NodePermissionsPointer perms { new NodePermissions(permsHash.toMap()) }; QString id = perms->getID(); - + NodePermissionsKey idKey = perms->getKey(); if (mapPointer->contains(idKey)) { @@ -484,7 +485,7 @@ void DomainServerSettingsManager::unpackPermissions() { // make sure that this permission row is for a non-empty hardware if (perms->getKey().first.isEmpty()) { _macPermissions.remove(perms->getKey()); - + // we removed a row from the MAC permissions, we'll need a re-pack needPack = true; } @@ -555,7 +556,7 @@ void DomainServerSettingsManager::unpackPermissions() { QList> permissionsSets; permissionsSets << _standardAgentPermissions.get() << _agentPermissions.get() << _groupPermissions.get() << _groupForbiddens.get() - << _ipPermissions.get() << _macPermissions.get() + << _ipPermissions.get() << _macPermissions.get() << _machineFingerprintPermissions.get(); foreach (auto permissionSet, permissionsSets) { @@ -668,71 +669,68 @@ void DomainServerSettingsManager::processNodeKickRequestPacket(QSharedPointerclear(NodePermissions::Permission::canConnectToDomain); } else { - // otherwise we apply the kick to the IP from active socket for this node and the MAC address - - // remove connect permissions for the IP (falling back to the public socket if not yet active) - auto& kickAddress = matchingNode->getActiveSocket() - ? matchingNode->getActiveSocket()->getAddress() - : matchingNode->getPublicSocket().getAddress(); - - // probably isLoopback covers it, as whenever I try to ban an agent on same machine as the domain-server - // it is always 127.0.0.1, but looking at the public and local addresses just to be sure - // TODO: soon we will have feedback (in the form of a message to the client) after we kick. When we - // do, we will have a success flag, and perhaps a reason for failure. For now, just don't do it. - if (kickAddress == limitedNodeList->getPublicSockAddr().getAddress() || - kickAddress == limitedNodeList->getLocalSockAddr().getAddress() || - kickAddress.isLoopback() ) { - qWarning() << "attempt to kick node running on same machine as domain server, ignoring KickRequest"; - return; - } - NodePermissionsKey ipAddressKey(kickAddress.toString(), QUuid()); - - // check if there were already permissions for the IP - bool hadIPPermissions = hasPermissionsForIP(kickAddress); - - // grab or create permissions for the given IP address - auto ipPermissions = _ipPermissions[ipAddressKey]; - - if (!hadIPPermissions || ipPermissions->can(NodePermissions::Permission::canConnectToDomain)) { - newPermissions = true; - - ipPermissions->clear(NodePermissions::Permission::canConnectToDomain); - } - - // potentially remove connect permissions for the MAC address and machine fingerprint + // remove connect permissions for the machine fingerprint DomainServerNodeData* nodeData = static_cast(matchingNode->getLinkedData()); if (nodeData) { - // mac address first - NodePermissionsKey macAddressKey(nodeData->getHardwareAddress(), 0); + // get this machine's fingerprint + auto domainServerFingerprint = FingerprintUtils::getMachineFingerprint(); - bool hadMACPermissions = hasPermissionsForMAC(nodeData->getHardwareAddress()); - - auto macPermissions = _macPermissions[macAddressKey]; - - if (!hadMACPermissions || macPermissions->can(NodePermissions::Permission::canConnectToDomain)) { - newPermissions = true; - - macPermissions->clear(NodePermissions::Permission::canConnectToDomain); + if (nodeData->getMachineFingerprint() == domainServerFingerprint) { + qWarning() << "attempt to kick node running on same machine as domain server (by fingerprint), ignoring KickRequest"; + return; } - - // now for machine fingerprint NodePermissionsKey machineFingerprintKey(nodeData->getMachineFingerprint().toString(), 0); - + + // check if there were already permissions for the fingerprint bool hadFingerprintPermissions = hasPermissionsForMachineFingerprint(nodeData->getMachineFingerprint()); - + + // grab or create permissions for the given fingerprint auto fingerprintPermissions = _machineFingerprintPermissions[machineFingerprintKey]; - + + // write them if (!hadFingerprintPermissions || fingerprintPermissions->can(NodePermissions::Permission::canConnectToDomain)) { newPermissions = true; fingerprintPermissions->clear(NodePermissions::Permission::canConnectToDomain); } + } else { + // if no node data, all we can do is IP address + auto& kickAddress = matchingNode->getActiveSocket() + ? matchingNode->getActiveSocket()->getAddress() + : matchingNode->getPublicSocket().getAddress(); + + // probably isLoopback covers it, as whenever I try to ban an agent on same machine as the domain-server + // it is always 127.0.0.1, but looking at the public and local addresses just to be sure + // TODO: soon we will have feedback (in the form of a message to the client) after we kick. When we + // do, we will have a success flag, and perhaps a reason for failure. For now, just don't do it. + if (kickAddress == limitedNodeList->getPublicSockAddr().getAddress() || + kickAddress == limitedNodeList->getLocalSockAddr().getAddress() || + kickAddress.isLoopback() ) { + qWarning() << "attempt to kick node running on same machine as domain server, ignoring KickRequest"; + return; + } + + + NodePermissionsKey ipAddressKey(kickAddress.toString(), QUuid()); + + // check if there were already permissions for the IP + bool hadIPPermissions = hasPermissionsForIP(kickAddress); + + // grab or create permissions for the given IP address + auto ipPermissions = _ipPermissions[ipAddressKey]; + + if (!hadIPPermissions || ipPermissions->can(NodePermissions::Permission::canConnectToDomain)) { + newPermissions = true; + + ipPermissions->clear(NodePermissions::Permission::canConnectToDomain); + } } } - // if we are here, then we kicked them, so send the KillAvatar message to everyone + + // if we are here, then we kicked them, so send the KillAvatar message auto packet = NLPacket::create(PacketType::KillAvatar, NUM_BYTES_RFC4122_UUID + sizeof(KillAvatarReason), true); packet->write(nodeUUID.toRfc4122()); packet->writePrimitive(KillAvatarReason::NoReason); - + // send to avatar mixer, it sends the kill to everyone else limitedNodeList->broadcastToNodes(std::move(packet), NodeSet() << NodeType::AvatarMixer); @@ -743,7 +741,7 @@ void DomainServerSettingsManager::processNodeKickRequestPacket(QSharedPointer Date: Thu, 16 Feb 2017 10:53:02 -0800 Subject: [PATCH 2/4] cleanup some accumulated cruft in pal code --- interface/resources/qml/hifi/Pal.qml | 4 +-- scripts/system/pal.js | 40 +++++++++++----------------- 2 files changed, 18 insertions(+), 26 deletions(-) diff --git a/interface/resources/qml/hifi/Pal.qml b/interface/resources/qml/hifi/Pal.qml index 0c7104fba5..c1fea7c09b 100644 --- a/interface/resources/qml/hifi/Pal.qml +++ b/interface/resources/qml/hifi/Pal.qml @@ -360,7 +360,7 @@ Rectangle { TextMetrics { id: displayNameHeaderMetrics text: displayNameHeader.title - font: displayNameHeader.font + // font: displayNameHeader.font // was this always undefined? giving error now... } // This Rectangle refers to the [?] popup button next to "NAMES" Rectangle { @@ -426,7 +426,6 @@ Rectangle { onExited: adminHelpText.color = hifi.colors.redHighlight } } - } HifiControls.Keyboard { id: keyboard @@ -438,6 +437,7 @@ Rectangle { right: parent.right } } + } // Timer used when selecting table rows that aren't yet present in the model // (i.e. when selecting avatars using edit.js or sphere overlays) diff --git a/scripts/system/pal.js b/scripts/system/pal.js index 2e07a2d431..57648da79a 100644 --- a/scripts/system/pal.js +++ b/scripts/system/pal.js @@ -290,17 +290,14 @@ function populateUserList(selectData) { userName: '', sessionId: id || '', audioLevel: 0.0, - admin: false + admin: false, + personalMute: !!id && Users.getPersonalMuteStatus(id), // expects proper boolean, not null + ignore: !!id && Users.getIgnoreStatus(id) // ditto }; - // Request the username, fingerprint, and admin status from the given UUID - // Username and fingerprint returns default constructor output if the requesting user isn't an admin - Users.requestUsernameFromID(id); - // Request personal mute status and ignore status - // from NodeList (as long as we're not requesting it for our own ID) if (id) { - avatarPalDatum['personalMute'] = Users.getPersonalMuteStatus(id); - avatarPalDatum['ignore'] = Users.getIgnoreStatus(id); addAvatarNode(id); // No overlay for ourselves + // Everyone needs to see admin status. Username and fingerprint returns default constructor output if the requesting user isn't an admin. + Users.requestUsernameFromID(id); } data.push(avatarPalDatum); print('PAL data:', JSON.stringify(avatarPalDatum)); @@ -314,20 +311,13 @@ function populateUserList(selectData) { // The function that handles the reply from the server function usernameFromIDReply(id, username, machineFingerprint, isAdmin) { - var data; - // If the ID we've received is our ID... - if (MyAvatar.sessionUUID === id) { - // Set the data to contain specific strings. - data = ['', username, isAdmin]; - } else if (Users.canKick) { - // Set the data to contain the ID and the username (if we have one) - // or fingerprint (if we don't have a username) string. - data = [id, username || machineFingerprint, isAdmin]; - } else { - // Set the data to contain specific strings. - data = [id, '', isAdmin]; - } - print('Username Data:', JSON.stringify(data)); + var data = [ + (MyAvatar.sessionUUID === id) ? '' : id, // Pal.qml recognizes empty id specially. + // If we get username (e.g., if in future we receive it when we're friends), use it. + // Otherwise, use valid machineFingerprint (which is not valid when not an admin). + username || (Users.canKick && machineFingerprint) || '', + isAdmin + ]; // Ship the data off to QML sendToQml({ method: 'updateUsername', params: data }); } @@ -339,13 +329,15 @@ function updateOverlays() { if (!id) { return; // don't update ourself } - + var avatar = AvatarList.getAvatar(id); + if (!avatar) { + return; // will be deleted below if there had been an overlay. + } var overlay = ExtendedOverlay.get(id); if (!overlay) { // For now, we're treating this as a temporary loss, as from the personal space bubble. Add it back. print('Adding non-PAL avatar node', id); overlay = addAvatarNode(id); } - var avatar = AvatarList.getAvatar(id); var target = avatar.position; var distance = Vec3.distance(target, eye); var offset = 0.2; From 07e97a0cabefde7f739d116849e3c58fd91a3f03 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 16 Feb 2017 21:40:53 +0000 Subject: [PATCH 3/4] unqueue from front for audio packets --- assignment-client/src/audio/AudioMixerClientData.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assignment-client/src/audio/AudioMixerClientData.cpp b/assignment-client/src/audio/AudioMixerClientData.cpp index fe43e6f730..d5e06504a6 100644 --- a/assignment-client/src/audio/AudioMixerClientData.cpp +++ b/assignment-client/src/audio/AudioMixerClientData.cpp @@ -61,7 +61,7 @@ void AudioMixerClientData::processPackets() { _packetQueue.node.clear(); while (!_packetQueue.empty()) { - auto& packet = _packetQueue.back(); + auto& packet = _packetQueue.front(); switch (packet->getType()) { case PacketType::MicrophoneAudioNoEcho: From 04712e374448736f4821ff3879bd81ec28346ff2 Mon Sep 17 00:00:00 2001 From: howard-stearns Date: Thu, 16 Feb 2017 16:43:16 -0800 Subject: [PATCH 4/4] fix rquestsDomainListData --- assignment-client/src/avatars/AvatarMixer.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index bf85918145..45b04c4189 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -410,7 +410,7 @@ void AvatarMixer::broadcastAvatarData() { bool isInView = nodeData->otherAvatarInView(otherNodeBox); // this throttles the extra data to only be sent every Nth message - if (!isInView && getsOutOfView && (lastSeqToReceiver % EXTRA_AVATAR_DATA_FRAME_RATIO > 0)) { + if (!isInView && !getsOutOfView && (lastSeqToReceiver % EXTRA_AVATAR_DATA_FRAME_RATIO > 0)) { return; } @@ -572,6 +572,7 @@ void AvatarMixer::handleRequestsDomainListDataPacket(QSharedPointerreadPrimitive(&isRequesting); nodeData->setRequestsDomainListData(isRequesting); + qDebug() << "node" << nodeData->getNodeID() << "requestsDomainListData" << isRequesting; } } }