Handle concurrent verify operations better; other improvements

assignment-client/src/avatars/AvatarMixer.cpp

@@ -83,7 +83,7 @@ AvatarMixer::AvatarMixer(ReceivedMessage& message) :
     packetReceiver.registerListener(PacketType::BulkAvatarTraitsAck, this, "queueIncomingPacket");
     packetReceiver.registerListenerForTypes({ PacketType::OctreeStats, PacketType::EntityData, PacketType::EntityErase },
         this, "handleOctreePacket");
-    packetReceiver.registerListener(PacketType::ChallengeOwnership, this, "handleChallengeOwnership");
+    packetReceiver.registerListener(PacketType::ChallengeOwnership, this, "queueIncomingPacket");
 
     packetReceiver.registerListenerForTypes({
         PacketType::ReplicatedAvatarIdentity,
@@ -1141,7 +1141,7 @@ void AvatarMixer::handleChallengeOwnership(QSharedPointer<ReceivedMessage> messa
         auto clientData = static_cast<AvatarMixerClientData*>(senderNode->getLinkedData());
         auto avatar = clientData->getAvatarSharedPointer();
         if (avatar) {
-            avatar->handleChallengeResponse(message.data());
+            //avatar->handleChallengeResponse(message.data());
         }
     }
 }

assignment-client/src/avatars/AvatarMixerClientData.cpp

@@ -74,6 +74,9 @@ int AvatarMixerClientData::processPackets(const SlaveSharedData& slaveSharedData
             case PacketType::BulkAvatarTraitsAck:
                 processBulkAvatarTraitsAckMessage(*packet);
                 break;
+            case PacketType::ChallengeOwnership:
+                _avatar->handleChallengeResponse(*packet);
+                break;
             default:
                 Q_UNREACHABLE();
         }

assignment-client/src/avatars/MixerAvatar.cpp

@@ -33,7 +33,15 @@ MixerAvatar::~MixerAvatar() {
     }
 }
 
+const char* MixerAvatar::stateToName(VerifyState state) {
+    return QMetaEnum::fromType<VerifyState>().valueToKey(state);
+}
+
 void MixerAvatar::fetchAvatarFST() {
+    volatile auto enumName = stateToName(_verifyState);
+    if (_verifyState >= requestingFST && _verifyState <= challengeClient) {
+        qCDebug(avatars) << "WARNING: Avatar verification restarted; old state:" << stateToName(_verifyState);
+    }
     _verifyState = nonCertified;
 
     _pendingEvent = false;
@@ -80,7 +88,7 @@ void MixerAvatar::fetchAvatarFST() {
 void MixerAvatar::fstRequestComplete() {
     ResourceRequest* fstRequest = static_cast<ResourceRequest*>(QObject::sender());
     QMutexLocker certifyLocker(&_avatarCertifyLock);
-    if (fstRequest == _avatarRequest) {
+    if (_verifyState == requestingFST && fstRequest == _avatarRequest) {
         auto result = fstRequest->getResult();
         if (result != ResourceRequest::Success) {
             _verifyState = error;
@@ -90,11 +98,11 @@ void MixerAvatar::fstRequestComplete() {
             _verifyState = receivedFST;
             _pendingEvent = true;
         }
-        _avatarRequest->deleteLater();
         _avatarRequest = nullptr;
     } else {
-        qCDebug(avatars) << "Incorrect request for" << getDisplayName();
+        qCDebug(avatars) << "Incorrect or outdated FST request for" << getDisplayName();
     }
+    fstRequest->deleteLater();
 }
 
 bool MixerAvatar::generateFSTHash() {
@@ -108,7 +116,7 @@ bool MixerAvatar::generateFSTHash() {
     return true;
 }
 
-bool MixerAvatar::validateFSTHash(const QString& publicKey) {
+bool MixerAvatar::validateFSTHash(const QString& publicKey) const {
     // Guess we should refactor this stuff into a Authorization namespace ...
     return EntityItemProperties::verifySignature(publicKey, _certificateHash,
         QByteArray::fromBase64(_certificateIdFromFST.toUtf8()));
@@ -171,7 +179,9 @@ void MixerAvatar::ownerRequestComplete() {
     QMutexLocker certifyLocker(&_avatarCertifyLock);
     QNetworkReply* networkReply = static_cast<QNetworkReply*>(QObject::sender());
 
-    if (networkReply->error() == QNetworkReply::NoError) {
+    if (_verifyState != requestingOwner) {
+        qCDebug(avatars) << "WARNING: outdated avatar-owner information received in state" << stateToName(_verifyState);
+    } else if (networkReply->error() == QNetworkReply::NoError) {
         _dynamicMarketResponse = networkReply->readAll();
         _verifyState = ownerResponse;
         _pendingEvent = true;
@@ -259,7 +269,6 @@ void MixerAvatar::processCertifyEvents() {
                     }
                     sendOwnerChallenge();
                     _verifyState = challengeClient;
-                    _pendingEvent = true;
                 } else {
                     _verifyState = error;
                     qCDebug(avatars) << "Get owner status - couldn't parse response for" << getSessionUUID()
@@ -273,37 +282,6 @@ void MixerAvatar::processCertifyEvents() {
             break;
         }
 
-        case challengeResponse:
-        {
-            if (_challengeResponse.length() < 8) {
-                _verifyState = error;
-                _pendingEvent = false;
-                break;
-            }
-
-            int avatarIDLength;
-            int signedNonceLength;
-            {
-                QDataStream responseStream(_challengeResponse);
-                responseStream.setByteOrder(QDataStream::LittleEndian);
-                responseStream >> avatarIDLength >> signedNonceLength;
-            }
-            QByteArray avatarID(_challengeResponse.data() + 2 * sizeof(int), avatarIDLength);
-            QByteArray signedNonce(_challengeResponse.data() + 2 * sizeof(int) + avatarIDLength, signedNonceLength);
-
-            bool challengeResult = EntityItemProperties::verifySignature(_ownerPublicKey, _challengeNonceHash,
-                QByteArray::fromBase64(signedNonce));
-            _verifyState = challengeResult ? verificationSucceeded : verificationFailed;
-            _needsIdentityUpdate = true;
-            if (_verifyState == verificationFailed) {
-                qCDebug(avatars) << "Dynamic verification FAILED for " << getDisplayName() << getSessionUUID();
-            } else {
-                qCDebug(avatars) << "Dynamic verification SUCCEEDED for " << getDisplayName() << getSessionUUID();
-            }
-            _pendingEvent = false;
-            break;
-        }
-
         case requestingOwner:
         case challengeClient:
         {   // Qt networking done on this thread:
@@ -312,7 +290,7 @@ void MixerAvatar::processCertifyEvents() {
         }
 
         default:
-            qCDebug(avatars) << "Unexpected verify state" << _verifyState;
+            qCDebug(avatars) << "Unexpected verify state" << stateToName(_verifyState);
             break;
 
     }  // close switch
@@ -334,6 +312,7 @@ void MixerAvatar::sendOwnerChallenge() {
     QCryptographicHash nonceHash(QCryptographicHash::Sha256);
     nonceHash.addData(nonce);
     _challengeNonceHash = nonceHash.result();
+    _pendingEvent = false;
 
     static constexpr int CHALLENGE_TIMEOUT_MS = 10 * 1000;  // 10 s
     if (_challengeTimeout) {
@@ -348,18 +327,45 @@ void MixerAvatar::sendOwnerChallenge() {
             _verifyState = verificationFailed;
             _needsIdentityUpdate = true;
             qCDebug(avatars) << "Dynamic verification TIMED-OUT for " << getDisplayName() << getSessionUUID();
+        } else {
+            qCDebug(avatars) << "Ignoring timeout of avatar challenge";
         }
     });
     _challengeTimeout->start();
 }
 
-void MixerAvatar::handleChallengeResponse(ReceivedMessage* response) {
+void MixerAvatar::handleChallengeResponse(ReceivedMessage& response) {
     QByteArray avatarID;
-    QByteArray encryptedNonce;
     QMutexLocker certifyLocker(&_avatarCertifyLock);
     if (_verifyState == challengeClient) {
-        _challengeResponse = response->readAll();
+        QByteArray responseData = response.readAll();
-        _verifyState = challengeResponse;
+        if (responseData.length() < 8) {
-        _pendingEvent = true;
+            _verifyState = error;
+            qCDebug(avatars) << "Avatar challenge response packet too small, length:" << responseData.length();
+            return;
+        }
+
+        int avatarIDLength;
+        int signedNonceLength;
+        {
+            QDataStream responseStream(responseData);
+            responseStream.setByteOrder(QDataStream::LittleEndian);
+            responseStream >> avatarIDLength >> signedNonceLength;
+        }
+        QByteArray avatarID(responseData.data() + 2 * sizeof(int), avatarIDLength);
+        QByteArray signedNonce(responseData.data() + 2 * sizeof(int) + avatarIDLength, signedNonceLength);
+
+        bool challengeResult = EntityItemProperties::verifySignature(_ownerPublicKey, _challengeNonceHash,
+            QByteArray::fromBase64(signedNonce));
+        _verifyState = challengeResult ? verificationSucceeded : verificationFailed;
+        _needsIdentityUpdate = true;
+        if (_verifyState == verificationFailed) {
+            qCDebug(avatars) << "Dynamic verification FAILED for " << getDisplayName() << getSessionUUID();
+        } else {
+            qCDebug(avatars) << "Dynamic verification SUCCEEDED for" << getDisplayName() << getSessionUUID();
+        }
+
+    } else {
+        qCDebug(avatars) << "WARNING: Unexpected avatar challenge-response in state" << stateToName(_verifyState);
     }
 }

assignment-client/src/avatars/MixerAvatar.h

@@ -20,6 +20,7 @@
 class ResourceRequest;
 
 class MixerAvatar : public AvatarData {
+    Q_OBJECT
 public:
     ~MixerAvatar();
     bool getNeedsHeroCheck() const { return _needsHeroCheck; }
@@ -31,15 +32,18 @@ public:
     void setNeedsIdentityUpdate(bool value = true) { _needsIdentityUpdate = value; }
 
     void processCertifyEvents();
-    void handleChallengeResponse(ReceivedMessage* response);
+    void handleChallengeResponse(ReceivedMessage& response);
+
+    // Avatar certification/verification:
+    enum VerifyState {
+        nonCertified, requestingFST, receivedFST, staticValidation, requestingOwner, ownerResponse,
+        challengeClient, verified, verificationFailed, verificationSucceeded, error
+    };
+    Q_ENUM(VerifyState)
 
 private:
     bool _needsHeroCheck { false };
-
+    static const char* stateToName(VerifyState state);
-    // Avatar certification/verification:
-    enum VerifyState { nonCertified, requestingFST, receivedFST, staticValidation, requestingOwner, ownerResponse,
-        challengeClient, challengeResponse, verified, verificationFailed, verificationSucceeded, error };
-    Q_ENUM(VerifyState);
     VerifyState _verifyState { nonCertified };
     std::atomic<bool> _pendingEvent { false };
     QMutex _avatarCertifyLock;
@@ -53,12 +57,11 @@ private:
     QString _dynamicMarketResponse;
     QString _ownerPublicKey;
     QByteArray _challengeNonceHash;
-    QByteArray _challengeResponse;
     QTimer* _challengeTimeout { nullptr };
     bool _needsIdentityUpdate { false };
 
     bool generateFSTHash();
-    bool validateFSTHash(const QString& publicKey);
+    bool validateFSTHash(const QString& publicKey) const;
     QByteArray canonicalJson(const QString fstFile);
     void sendOwnerChallenge();