code review

domain-server/src/DomainGatekeeper.cpp

@@ -315,8 +315,6 @@ SharedNodePointer DomainGatekeeper::processAgentConnectRequest(const NodeConnect
         sendConnectionTokenPacket(username, nodeConnection.senderSockAddr);
         // ask for their public key right now to make sure we have it
         requestUserPublicKey(username);
-        getGroupMemberships(username); // optimistically get started on group memberships
-        getDomainOwnerFriendsList();
         return SharedNodePointer();
     }
 
@@ -326,7 +324,6 @@ SharedNodePointer DomainGatekeeper::processAgentConnectRequest(const NodeConnect
         userPerms.setVerifiedUserName(username);
         verifiedUsername = username;
         getGroupMemberships(username);
-        getDomainOwnerFriendsList();
     } else if (!username.isEmpty()) {
         // they sent us a username, but it didn't check out
         requestUserPublicKey(username);
@@ -725,6 +722,14 @@ void DomainGatekeeper::getGroupMemberships(const QString& username) {
     // loop through the groups mentioned on the settings page and ask if this user is in each.  The replies
     // will be received asynchronously and permissions will be updated as the answers come in.
 
+    // if we've already asked, wait for the answer before asking again
+    QString lowerUsername = username.toLower();
+    if (_inFlightGroupMembershipsRequests.contains(lowerUsername)) {
+        // public-key request for this username is already flight, not rerequesting
+        return;
+    }
+    _inFlightGroupMembershipsRequests += lowerUsername;
+
     QJsonObject json;
     QSet<QString> groupIDSet;
     foreach (QUuid groupID, _server->_settingsManager.getGroupIDs() + _server->_settingsManager.getBlacklistGroupIDs()) {
@@ -747,7 +752,16 @@ void DomainGatekeeper::getGroupMemberships(const QString& username) {
 
 }
 
-
+QString extractUsernameFromGroupMembershipsReply(QNetworkReply& requestReply) {
+    // extract the username from the request url
+    QString username;
+    const QString GROUP_MEMBERSHIPS_URL_REGEX_STRING = "api\\/v1\\/groups\\/members\\/([A-Za-z0-9_\\.]+)";
+    QRegExp usernameRegex(GROUP_MEMBERSHIPS_URL_REGEX_STRING);
+    if (usernameRegex.indexIn(requestReply.url().toString()) != -1) {
+        username = usernameRegex.cap(1);
+    }
+    return username.toLower();
+}
 
 void DomainGatekeeper::getIsGroupMemberJSONCallback(QNetworkReply& requestReply) {
     // {
@@ -781,10 +795,13 @@ void DomainGatekeeper::getIsGroupMemberJSONCallback(QNetworkReply& requestReply)
     } else {
         qDebug() << "getIsGroupMember api call returned:" << QJsonDocument(jsonObject).toJson(QJsonDocument::Compact);
     }
+
+    _inFlightGroupMembershipsRequests.remove(extractUsernameFromGroupMembershipsReply(requestReply));
 }
 
 void DomainGatekeeper::getIsGroupMemberErrorCallback(QNetworkReply& requestReply) {
     qDebug() << "getIsGroupMember api call failed:" << requestReply.error();
+    _inFlightGroupMembershipsRequests.remove(extractUsernameFromGroupMembershipsReply(requestReply));
 }
 
 void DomainGatekeeper::getDomainOwnerFriendsList() {
@@ -825,7 +842,6 @@ void DomainGatekeeper::refreshGroupsCache() {
     // if agents are connected to this domain, refresh our cached information about groups and memberships in such.
     getDomainOwnerFriendsList();
 
-    int agentCount = 0;
     auto nodeList = DependencyManager::get<LimitedNodeList>();
     nodeList->eachNode([&](const SharedNodePointer& node) {
         if (!node->getPermissions().isAssignment) {
@@ -834,13 +850,10 @@ void DomainGatekeeper::refreshGroupsCache() {
             if (verifiedUserName.isEmpty()) {
                 getGroupMemberships(verifiedUserName);
             }
-            agentCount++;
         }
     });
 
-    if (agentCount > 0) {
+    _server->_settingsManager.apiRefreshGroupInformation();
-        _server->_settingsManager.apiRefreshGroupInformation();
-    }
 
     updateNodePermissions();
 

domain-server/src/DomainGatekeeper.h

@@ -104,6 +104,7 @@ private:
     QHash<QString, QByteArray> _userPublicKeys;
     QSet<QString> _inFlightPublicKeyRequests; // keep track of which we've already asked for
     QSet<QString> _domainOwnerFriends; // keep track of friends of the domain owner
+    QSet<QString> _inFlightGroupMembershipsRequests; // keep track of which we've already asked for
 
     NodePermissions applyPermissionsForUser(bool isLocalUser, NodePermissions userPerms, QString verifiedUsername);
     void getGroupMemberships(const QString& username);

domain-server/src/DomainServerSettingsManager.cpp

@@ -346,7 +346,7 @@ void DomainServerSettingsManager::initializeGroupPermissions(NodePermissionsMap&
             continue;
         }
         permissionsRows[nameKey]->setAll(false);
-        permissionsRows[nameKey] |= perms;
+        *(permissionsRows[nameKey]) |= *perms;
     }
 }
 
@@ -480,7 +480,7 @@ void DomainServerSettingsManager::unpackPermissions() {
         foundFriends |= (idKey == NodePermissions::standardNameFriends);
         if (_standardAgentPermissions.contains(idKey)) {
             qDebug() << "duplicate name in standard permissions table: " << id;
-            _standardAgentPermissions[idKey] |= perms;
+            *(_standardAgentPermissions[idKey]) |= *perms;
             needPack = true;
         } else {
             _standardAgentPermissions[idKey] = perms;
@@ -494,7 +494,7 @@ void DomainServerSettingsManager::unpackPermissions() {
         NodePermissionsKey idKey = NodePermissionsKey(id, 0);
         if (_agentPermissions.contains(idKey)) {
             qDebug() << "duplicate name in permissions table: " << id;
-            _agentPermissions[idKey] |= perms;
+            *(_agentPermissions[idKey]) |= *perms;
             needPack = true;
         } else {
             _agentPermissions[idKey] = perms;
@@ -508,10 +508,10 @@ void DomainServerSettingsManager::unpackPermissions() {
         NodePermissionsKey idKey = perms->getKey();
         if (_groupPermissions.contains(idKey)) {
             qDebug() << "duplicate name in group permissions table: " << id;
-            _groupPermissions[idKey] |= perms;
+            *(_groupPermissions[idKey]) |= *perms;
             needPack = true;
         } else {
-            _groupPermissions[idKey] = perms;
+            *(_groupPermissions[idKey]) = *perms;
         }
         if (perms->isGroup()) {
             // the group-id was cached.  hook-up the uuid in the uuid->group hash
@@ -527,7 +527,7 @@ void DomainServerSettingsManager::unpackPermissions() {
         NodePermissionsKey idKey = perms->getKey();
         if (_groupForbiddens.contains(idKey)) {
             qDebug() << "duplicate name in group forbiddens table: " << id;
-            _groupForbiddens[idKey] |= perms;
+            *(_groupForbiddens[idKey]) |= *perms;
             needPack = true;
         } else {
             _groupForbiddens[idKey] = perms;
@@ -1367,7 +1367,7 @@ void DomainServerSettingsManager::apiGetGroupRanksJSONCallback(QNetworkReply& re
             QHash<QUuid, bool> idsFromThisUpdate;
 
             for (int rankIndex = 0; rankIndex < ranks.size(); rankIndex++) {
-                QJsonObject rank = ranks.at(rankIndex).toObject();
+                QJsonObject rank = ranks[rankIndex].toObject();
 
                 QUuid rankID = QUuid(rank["id"].toString());
                 int rankOrder = rank["order"].toInt();

libraries/networking/src/GroupRank.h

@@ -14,14 +14,14 @@
 
 class GroupRank {
 public:
-    GroupRank() : id(QUuid()), order(-1), name(""), membersCount(-1) {}
+    GroupRank() {}
     GroupRank(QUuid id, unsigned int order, QString name, unsigned int membersCount) :
         id(id), order(order), name(name), membersCount(membersCount) {}
 
     QUuid id;
-    int order;
+    int order { -1 };
     QString name;
-    int membersCount;
+    int membersCount { -1 };
 };
 
 inline bool operator==(const GroupRank& lhs, const GroupRank& rhs) {

libraries/networking/src/NodePermissions.cpp

@@ -77,47 +77,11 @@ NodePermissions& NodePermissions::operator|=(const NodePermissions& rhs) {
     permissions |= rhs.permissions;
     return *this;
 }
-NodePermissions& NodePermissions::operator|=(const NodePermissionsPointer& rhs) {
-    if (rhs) {
-        *this |= *rhs.get();
-    }
-    return *this;
-}
-NodePermissionsPointer& operator|=(NodePermissionsPointer& lhs, const NodePermissionsPointer& rhs) {
-    if (lhs && rhs) {
-        *lhs.get() |= rhs;
-    }
-    return lhs;
-}
-NodePermissionsPointer& operator|=(NodePermissionsPointer& lhs, NodePermissions::Permission rhs) {
-    if (lhs) {
-        lhs.get()->permissions |= rhs;
-    }
-    return lhs;
-}
 
 NodePermissions& NodePermissions::operator&=(const NodePermissions& rhs) {
     permissions &= rhs.permissions;
     return *this;
 }
-NodePermissions& NodePermissions::operator&=(const NodePermissionsPointer& rhs) {
-    if (rhs) {
-        *this &= *rhs.get();
-    }
-    return *this;
-}
-NodePermissionsPointer& operator&=(NodePermissionsPointer& lhs, const NodePermissionsPointer& rhs) {
-    if (lhs && rhs) {
-        *lhs.get() &= rhs;
-    }
-    return lhs;
-}
-NodePermissionsPointer& operator&=(NodePermissionsPointer& lhs, NodePermissions::Permission rhs) {
-    if (lhs) {
-        lhs.get()->permissions &= rhs;
-    }
-    return lhs;
-}
 
 NodePermissions NodePermissions::operator~() {
     NodePermissions result = *this;
@@ -125,15 +89,6 @@ NodePermissions NodePermissions::operator~() {
     return result;
 }
 
-NodePermissionsPointer operator~(NodePermissionsPointer& lhs) {
-    if (lhs) {
-        NodePermissionsPointer result { new NodePermissions };
-        (*result.get()) = ~(*lhs.get());
-        return result;
-    }
-    return lhs;
-}
-
 QDataStream& operator<<(QDataStream& out, const NodePermissions& perms) {
     out << (uint)perms.permissions;
     return out;

libraries/networking/src/NodePermissions.h

@@ -72,9 +72,7 @@ public:
     void setAll(bool value);
 
     NodePermissions& operator|=(const NodePermissions& rhs);
-    NodePermissions& operator|=(const NodePermissionsPointer& rhs);
     NodePermissions& operator&=(const NodePermissions& rhs);
-    NodePermissions& operator&=(const NodePermissionsPointer& rhs);
     NodePermissions operator~();
     friend QDataStream& operator<<(QDataStream& out, const NodePermissions& perms);
     friend QDataStream& operator>>(QDataStream& in, NodePermissions& perms);
@@ -128,8 +126,6 @@ const NodePermissions DEFAULT_AGENT_PERMISSIONS;
 
 QDebug operator<<(QDebug debug, const NodePermissions& perms);
 QDebug operator<<(QDebug debug, const NodePermissionsPointer& perms);
-NodePermissionsPointer& operator|=(NodePermissionsPointer& lhs, const NodePermissionsPointer& rhs);
-NodePermissionsPointer& operator|=(NodePermissionsPointer& lhs, NodePermissions::Permission rhs);
 NodePermissionsPointer& operator&=(NodePermissionsPointer& lhs, const NodePermissionsPointer& rhs);
 NodePermissionsPointer& operator&=(NodePermissionsPointer& lhs, NodePermissions::Permission rhs);
 NodePermissionsPointer operator~(NodePermissionsPointer& lhs);