Handle invalid alert address in alert sending

python/nav/alertengine/dispatchers/__init__.py

@@ -101,6 +101,13 @@ class FatalDispatcherException(DispatcherException):
     pass
 
 
+class InvalidAlertAddressError(Exception):
+    """Raised when an alert address is invalid, which is determined by the
+    is_valid_address method of each dispatcher"""
+
+    pass
+
+
 def is_valid_email(address):
     """Validates a string as an e-mail address"""
     try:

python/nav/models/profiles.py

@@ -35,8 +35,11 @@
 import nav.buildconf
 import nav.pwhash
 from nav.config import getconfig as get_alertengine_config
-from nav.alertengine.dispatchers import DispatcherException
-from nav.alertengine.dispatchers import FatalDispatcherException
+from nav.alertengine.dispatchers import (
+    DispatcherException,
+    FatalDispatcherException,
+    InvalidAlertAddressError,
+)
 
 from nav.models.event import AlertQueue, AlertType, EventType
 from nav.models.manage import Arp, Cam, Category, Device, Location
@@ -406,6 +409,12 @@
     def __str__(self):
         return self.type.scheme() + self.address
 
+    def has_valid_address(self):
+        if not self.type.supported or not self.address:
+            return False
+        dispatcher = self.type.load_dispatcher_class()
+        return dispatcher.is_valid_address(self.address)
+
     @transaction.atomic
     def send(self, alert, subscription):
         """Handles sending of alerts to with defined alert notification types
@@ -417,10 +426,10 @@
         # Determine the right language for the user.
         lang = self.account.preferences.get(Account.PREFERENCE_KEY_LANGUAGE, 'en')
 
-        if not (self.address or '').strip():
+        if not self.has_valid_address():
             _logger.error(
-                'Ignoring alert %d (%s: %s)! Account %s does not have an '
-                'address set for the alertaddress with id %d, this needs '
+                'Ignoring alert %d (%s: %s)! Account %s does not have a '
+                'valid address for the alertaddress with id %d, this needs '
                 'to be fixed before the user will recieve any alerts.',
                 alert.id,
                 alert,
@@ -429,7 +438,7 @@
                 self.id,
             )
 
-            return True
+            raise InvalidAlertAddressError
 
         if self.type.is_blacklisted():
             _logger.debug(
@@ -505,7 +514,7 @@
         if not self.supported:
             raise FatalDispatcherException("{} is not supported".format(self.name))
         if self.handler not in self._handlers:
-            dispatcher_class = self._load_dispatcher_class()
+            dispatcher_class = self.load_dispatcher_class()
             dispatcher = dispatcher_class(
                 config=AlertSender.config.get(self.handler, {})
             )
@@ -516,7 +525,7 @@
         # Delegate sending of message
         return dispatcher.send(*args, **kwargs)
 
-    def _load_dispatcher_class(self):
+    def load_dispatcher_class(self):
         # Get config
         if not hasattr(AlertSender, 'config'):
             AlertSender.config = get_alertengine_config('alertengine.conf')
@@ -1358,7 +1367,7 @@
 
             super(AccountAlertQueue, self).delete()
             return False
-        except FatalDispatcherException:
+        except (FatalDispatcherException, InvalidAlertAddressError):
             self.delete()
             return False
 

tests/integration/alertengine_test.py

@@ -4,5 +4,5 @@
 
 def test_all_handlers_should_be_loadable():
     for sender in AlertSender.objects.filter(supported=True):
-        dispatcher = sender._load_dispatcher_class()
+        dispatcher = sender.load_dispatcher_class()
         assert issubclass(dispatcher, Dispatcher)

tests/integration/models/alertsubscription_test.py → tests/integration/models/alert_test.py

@@ -1,4 +1,6 @@
 from datetime import datetime
+
+from nav.alertengine.dispatchers import InvalidAlertAddressError
 from nav.models.profiles import (
     Account,
     AccountAlertQueue,
@@ -10,24 +12,46 @@
     TimePeriod,
 )
 from nav.models.event import AlertQueue, Subsystem
+
 import pytest
 
 
-def test_delete_alert_subscription(db, alert, alertsub):
-    account_queue = AccountAlertQueue(alert=alert, subscription=alertsub)
-    account_queue.save()
+def test_delete_alert_subscription(db, alert, alertsub, account_alert_queue):
     alertsub.delete()
-    assert not AccountAlertQueue.objects.filter(pk=account_queue.pk).exists()
+    assert not AccountAlertQueue.objects.filter(pk=account_alert_queue.pk).exists()
+    assert not AlertQueue.objects.filter(pk=alert.pk).exists()
+
+
+def test_sending_alert_to_alert_address_with_empty_address_will_raise_error(
+    db, alert_address, alert, alertsub
+):
+    with pytest.raises(InvalidAlertAddressError):
+        alert_address.send(alert, alertsub)
+
+
+def test_sending_alert_to_alert_address_with_invalid_address_will_raise_error(
+    db, alert_address, alert, alertsub
+):
+    alert_address.address = "abc"
+    alert_address.save()
+    with pytest.raises(InvalidAlertAddressError):
+        alert_address.send(alert, alertsub)
+
+
+def test_sending_alert_to_alert_address_with_invalid_address_will_delete_alert_and_fail(
+    db, alert, account_alert_queue
+):
+    assert not account_alert_queue.send()
     assert not AlertQueue.objects.filter(pk=alert.pk).exists()
 
 
 @pytest.fixture
-def account():
+def account(db):
     return Account.objects.get(pk=Account.ADMIN_ACCOUNT)
 
 
 @pytest.fixture
-def alert_address(account):
+def alert_address(db, account):
     addr = AlertAddress(
         account=account,
         type=AlertSender.objects.get(name=AlertSender.SMS),
@@ -39,7 +63,7 @@ def alert_address(account):
 
 
 @pytest.fixture
-def alert_profile(account):
+def alert_profile(db, account):
     profile = AlertProfile(account=account)
     profile.save()
     yield profile
@@ -48,7 +72,7 @@ def alert_profile(account):
 
 
 @pytest.fixture
-def time_period(alert_profile):
+def time_period(db, alert_profile):
     time_period = TimePeriod(profile=alert_profile)
     time_period.save()
     yield time_period
@@ -57,7 +81,7 @@ def time_period(alert_profile):
 
 
 @pytest.fixture
-def alertsub(alert_address, time_period):
+def alertsub(db, alert_address, time_period):
     alertsub = AlertSubscription(
         alert_address=alert_address,
         time_period=time_period,
@@ -70,11 +94,20 @@ def alertsub(alert_address, time_period):
 
 
 @pytest.fixture
-def alert():
+def alert(db):
     alert = AlertQueue(
         source=Subsystem.objects.first(), time=datetime.now(), value=1, severity=3
     )
     alert.save()
     yield alert
     if alert.pk:
         alert.delete()
+
+
+@pytest.fixture
+def account_alert_queue(db, alert, alertsub):
+    account_queue = AccountAlertQueue(alert=alert, subscription=alertsub)
+    account_queue.save()
+    yield account_queue
+    if account_queue.pk:
+        account_queue.delete()