upnp: make upnp an account setting

Refs #64935

Change-Id: I8322d459466c4e0be27d760363a87058d49b89eb

daemon/src/account.cpp

@@ -83,6 +83,7 @@ const char * const Account::DEFAULT_USER_AGENT          = PACKAGE_NAME "/" PACKA
 const char * const Account::USER_AGENT_KEY              = "useragent";
 const char * const Account::HAS_CUSTOM_USER_AGENT_KEY   = "hasCustomUserAgent";
 const char * const Account::PRESENCE_MODULE_ENABLED_KEY = "presenceModuleEnabled";
+const char * const Account::UPNP_ENABLED_KEY            = "upnpEnabled";
 
 using std::map;
 using std::string;
@@ -106,6 +107,7 @@ Account::Account(const string &accountID)
     , userAgent_(DEFAULT_USER_AGENT)
     , hasCustomUserAgent_(false)
     , mailBox_()
+    , upnpEnabled_(false)
 {
     std::random_device rdev;
     std::seed_seq seed {rdev(), rdev()};
@@ -194,6 +196,7 @@ void Account::serialize(YAML::Emitter &out)
     out << YAML::Key << USERNAME_KEY << YAML::Value << username_;
     out << YAML::Key << DISPLAY_NAME_KEY << YAML::Value << displayName_;
     out << YAML::Key << HOSTNAME_KEY << YAML::Value << hostname_;
+    out << YAML::Key << UPNP_ENABLED_KEY << YAML::Value << upnpEnabled_;
 }
 
 void Account::unserialize(const YAML::Node &node)
@@ -218,6 +221,8 @@ void Account::unserialize(const YAML::Node &node)
     parseValue(node, USER_AGENT_KEY, userAgent_);
     parseValue(node, RINGTONE_PATH_KEY, ringtonePath_);
     parseValue(node, RINGTONE_ENABLED_KEY, ringtoneEnabled_);
+
+    parseValue(node, UPNP_ENABLED_KEY, upnpEnabled_);
 }
 
 void Account::setAccountDetails(const std::map<std::string, std::string> &details)
@@ -237,6 +242,7 @@ void Account::setAccountDetails(const std::map<std::string, std::string> &detail
         parseString(details, Conf::CONFIG_ACCOUNT_USERAGENT, userAgent_);
     else
         userAgent_ = DEFAULT_USER_AGENT;
+    parseBool(details, Conf::CONFIG_UPNP_ENABLED, upnpEnabled_);
 }
 
 std::map<std::string, std::string> Account::getAccountDetails() const
@@ -260,6 +266,7 @@ std::map<std::string, std::string> Account::getAccountDetails() const
     a[Conf::CONFIG_ACCOUNT_AUTOANSWER] = autoAnswerEnabled_ ? TRUE_STR : FALSE_STR;
     a[Conf::CONFIG_RINGTONE_ENABLED] = ringtoneEnabled_ ? TRUE_STR : FALSE_STR;
     a[Conf::CONFIG_RINGTONE_PATH] = ringtonePath_;
+    a[Conf::CONFIG_UPNP_ENABLED] = upnpEnabled_ ? TRUE_STR : FALSE_STR;
 
     return a;
 }
@@ -459,26 +466,41 @@ Account::parseBool(const std::map<std::string, std::string> &details, const char
 
 #undef find_iter
 
-void
-Account::setUseUPnP(bool useUPnP)
+/**
+ * Checks whether the upnp settings is enabled in the account.
+ * If so, tries to get a controller with a valid IGD.
+ * If not, destroys the controller.
+ *
+ * Returns whether or not there is a controller with a valid IGD to use.
+ */
+bool
+Account::checkUPnP()
 {
     std::unique_lock<std::mutex> lk(upnp_mtx);
 
-    if (useUPnP == static_cast<bool>(upnp_))
-        return;
+    if (upnpEnabled_ != (bool)upnp_) {
+        if (upnpEnabled_){
+            upnp_.reset(new upnp::Controller());
+            if (upnp_->hasValidIGD())
+                upnpIp_ = upnp_->getExternalIP();
+        } else {
+            upnp_.reset();
+            upnpIp_ = IpAddr{};
+        }
+    }
 
-    if (useUPnP){
-        upnp_.reset(new upnp::Controller());
-        upnpIp_ = upnp_->getExternalIP();
-    } else
-        upnp_.reset();
+    return upnp_ and upnp_->hasValidIGD();
 }
 
+/**
+ * Returns whether or not the upnp controller has a valid IGD
+ * with which to make port mappings
+ */
 bool
 Account::getUseUPnP() const
 {
     std::unique_lock<std::mutex> lk(upnp_mtx);
-    return static_cast<bool>(upnp_);
+    return upnp_ and upnp_->hasValidIGD();
 }
 
 } // namespace ring

daemon/src/account.h

@@ -255,12 +255,6 @@ class Account : public Serializable, public std::enable_shared_from_this<Account
         static const char * const VIDEO_CODEC_BITRATE;
 
         /**
-         * Set whether or not to use UPnP
-         */
-        void setUseUPnP(bool useUPnP);
-
-        /**
-         * Get whether UPnP is used.
          * @return bool Flag which determines if UPnP is used or not.
          */
         bool getUseUPnP() const;
@@ -313,6 +307,7 @@ class Account : public Serializable, public std::enable_shared_from_this<Account
         static const char * const HAS_CUSTOM_USER_AGENT_KEY;
         static const char * const DEFAULT_USER_AGENT;
         static const char * const PRESENCE_MODULE_ENABLED_KEY;
+        static const char * const UPNP_ENABLED_KEY;
 
         static std::string mapStateNumberToString(RegistrationState state);
 
@@ -414,6 +409,23 @@ class Account : public Serializable, public std::enable_shared_from_this<Account
         IpAddr upnpIp_ {};
         std::unique_ptr<ring::upnp::Controller> upnp_;
         mutable std::mutex upnp_mtx {};
+
+        /**
+         * Whether or not UPnP is enabled for this account
+         * Note that this is only used to store the account setting,
+         * getUseUPnP() should be used to check if UPnP is being use
+         * ie: if the UPnP controller has been initialized
+         */
+        bool upnpEnabled_;
+
+        /**
+         * Checks whether the upnp settings is enabled in the account.
+         * If so, tries to get a controller with a valid IGD.
+         * If not, destroys the controller.
+         *
+         * Returns whether or not there is a controller with a valid IGD to use.
+         */
+        bool checkUPnP();
 };
 
 } // namespace ring

daemon/src/account_schema.h

@@ -74,6 +74,7 @@ static const char *const CONFIG_PUBLISHED_SAMEAS_LOCAL          = "Account.publi
 static const char *const CONFIG_LOCAL_PORT                      = "Account.localPort";
 static const char *const CONFIG_PUBLISHED_PORT                  = "Account.publishedPort";
 static const char *const CONFIG_PUBLISHED_ADDRESS               = "Account.publishedAddress";
+static const char *const CONFIG_UPNP_ENABLED                    = "Account.upnpEnabled";
 
 // SIP specific parameters
 static const char *const CONFIG_STUN_SERVER                     = "STUN.server";

daemon/src/managerimpl.cpp

@@ -243,10 +243,10 @@ ManagerImpl::init(const std::string &config_file)
 
     history_.load(preferences.getHistoryLimit());
 
-    if (preferences.getUseUPnP()) {
-        RING_DBG("Remove any old UPnP mappings for RING mapped to this IP");
-        upnp::Controller().removeMappingsByLocalIPAndDescription();
-    }
+    /* TODO: Is this necessary? This will require looking for a UPnP enabled IGD at each start
+     * of the daemon... */
+    RING_DBG("Remove any old UPnP mappings for RING mapped to this IP");
+    upnp::Controller().removeMappingsByLocalIPAndDescription();
 
     registerAccounts();
 }
@@ -295,10 +295,9 @@ ManagerImpl::finish()
         RING_ERR("%s", err.what());
     }
 
-    if (preferences.getUseUPnP()) {
-        RING_DBG("Remove any remaning ports mapped to this client for RING");
-        upnp::Controller().removeMappingsByLocalIPAndDescription();
-    }
+
+    RING_DBG("Remove any remaning ports mapped to this client for RING");
+    upnp::Controller().removeMappingsByLocalIPAndDescription();
 }
 
 bool
@@ -2407,7 +2406,6 @@ ManagerImpl::setAccountDetails(const std::string& accountID,
         saveConfig();
 
         if (account->isEnabled()) {
-            account->setUseUPnP(preferences.getUseUPnP());
             account->doRegister();
         } else
             account->doUnregister();
@@ -2454,8 +2452,6 @@ ManagerImpl::addAccount(const std::map<std::string, std::string>& details)
 
     preferences.addAccount(newAccountID);
 
-    newAccount->setUseUPnP(preferences.getUseUPnP());
-
     newAccount->doRegister();
 
     saveConfig();
@@ -2715,7 +2711,6 @@ ManagerImpl::registerAccounts()
         a->loadConfig();
 
         if (a->isEnabled()) {
-            a->setUseUPnP(preferences.getUseUPnP());
             a->doRegister();
         }
     }
@@ -2743,7 +2738,6 @@ ManagerImpl::sendRegister(const std::string& accountID, bool enable)
     Manager::instance().saveConfig();
 
     if (acc->isEnabled()) {
-        acc->setUseUPnP(preferences.getUseUPnP());
         acc->doRegister();
     } else
         acc->doUnregister();

daemon/src/preferences.cpp

@@ -77,7 +77,6 @@ static const char * const ZONE_TONE_CHOICE_KEY = "zoneToneChoice";
 static const char * const PORT_NUM_KEY = "portNum";
 static const char * const SEARCH_BAR_DISPLAY_KEY = "searchBarDisplay";
 static const char * const MD5_HASH_KEY = "md5Hash";
-static const char * const USE_UPNP = "useUPnP";
 
 // voip preferences
 constexpr const char * const VoipPreference::CONFIG_LABEL;
@@ -138,7 +137,6 @@ Preferences::Preferences() :
     , portNum_(5060)
     , searchBarDisplay_(true)
     , md5Hash_(false)
-    , useUPnP_(false)
 {}
 
 void Preferences::verifyAccountOrder(const std::vector<std::string> &accountIDs)
@@ -197,7 +195,6 @@ void Preferences::serialize(YAML::Emitter &out)
     out << YAML::Key << REGISTRATION_EXPIRE_KEY << YAML::Value << registrationExpire_;
     out << YAML::Key << SEARCH_BAR_DISPLAY_KEY << YAML::Value << searchBarDisplay_;
     out << YAML::Key << ZONE_TONE_CHOICE_KEY << YAML::Value << zoneToneChoice_;
-    out << YAML::Key << USE_UPNP << YAML::Value << useUPnP_;
     out << YAML::EndMap;
 }
 
@@ -213,7 +210,6 @@ void Preferences::unserialize(const YAML::Node &in)
     parseValue(node, PORT_NUM_KEY, portNum_);
     parseValue(node, SEARCH_BAR_DISPLAY_KEY, searchBarDisplay_);
     parseValue(node, MD5_HASH_KEY, md5Hash_);
-    parseValue(node, USE_UPNP, useUPnP_);
 }
 
 VoipPreference::VoipPreference() :

daemon/src/preferences.h

@@ -126,13 +126,6 @@ class Preferences : public Serializable {
             md5Hash_ = md5;
         }
 
-        bool getUseUPnP() const {
-            return useUPnP_;
-        }
-        void setUseUPnP(bool upnp) {
-            useUPnP_ = upnp;
-        }
-
     private:
         std::string accountOrder_;
         int historyLimit_;
@@ -142,7 +135,6 @@ class Preferences : public Serializable {
         int portNum_;
         bool searchBarDisplay_;
         bool md5Hash_;
-        bool useUPnP_;
         constexpr static const char * const CONFIG_LABEL = "preferences";
 };
 

daemon/src/ringdht/ringaccount.cpp

@@ -603,7 +603,9 @@ void RingAccount::doRegister()
         return;
     }
 
-    if (not mapPortUPnP())
+    /* check UPnP to see if the account setting is enabled and try to get a
+     * UPnP controller; if this succeeds then try to map the account port */
+    if (not (checkUPnP() and mapPortUPnP()) )
         RING_WARN("Could not successfully map DHT port with UPnP, continuing with account registration anyways.");
 
     try {

daemon/src/sip/sipaccount.cpp

@@ -744,7 +744,9 @@ void SIPAccount::doRegister()
 
     RING_DBG("doRegister %s", hostname_.c_str());
 
-    if (not mapPortUPnP())
+    /* check UPnP to see if the account setting is enabled and try to get a
+     * UPnP controller; if this succeeds then try to map the account port */
+    if (not (checkUPnP() and mapPortUPnP()) )
         RING_WARN("Could not successfully map SIP port with UPnP, continuing with account registration anyways.");
 
     if (hostname_.empty() || isIP2IP()) {

daemon/src/upnp/upnp.cpp

@@ -88,7 +88,6 @@ IGD& IGD::operator=(IGD&& other)
 IGD::~IGD()
 {
     /* free the URLs */
-    RING_WARN("UPnP : deleting IGD");
     FreeUPNPUrls(&urls_);
 }
 
@@ -145,6 +144,19 @@ Controller::~Controller()
     removeMappings();
 }
 
+/**
+ * Return whether or not this controller has a valid IGD,
+ * if 'flase' then all requests will fail
+ */
+bool
+Controller::hasValidIGD()
+{
+#if HAVE_UPNP
+    return not defaultIGD_->isEmpty();
+#endif
+    return false;
+}
+
 /**
  * tries to add mapping
  */

daemon/src/upnp/upnp.h

@@ -162,6 +162,12 @@ public:
     /* destructor */
     ~Controller();
 
+    /**
+     * Return whether or not this controller has a valid IGD,
+     * if 'flase' then all requests will fail
+     */
+    bool hasValidIGD();
+
     /**
      * tries to add mapping from and to the port_desired
      * if unique == true, makes sure the client is not using this port already