Repository: modoboa/modoboa (language: python)

Issue:
Traceback: check_mx
# Impacted versions

* Modoboa: 1.13.1
* installer used: Yes
* Webserver: Nginx

# Steps to reproduce

/srv/modoboa/env/bin/python /srv/modoboa/instance/manage.py modo check_mx

Traceback (most recent call last):
  File "/srv/modoboa/instance/manage.py", line 22, in <module>
    execute_from_command_line(sys.argv)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 364, in execute_from_command_line
    utility.execute()
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 356, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/subcommand/base.py", line 53, in run_from_argv
    return super(SubcommandCommand, self).run_from_argv(argv)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/django/core/management/base.py", line 283, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/django/core/management/base.py", line 330, in execute
    output = self.handle(*args, **options)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/subcommand/base.py", line 84, in handle
    return command.run_from_argv(argv)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/django/core/management/base.py", line 283, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/django/core/management/base.py", line 330, in execute
    output = self.handle(*args, **options)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/modoboa/admin/management/commands/subcommands/_mx.py", line 264, in handle
    self.check_domain(domain, **options)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/modoboa/admin/management/commands/subcommands/_mx.py", line 195, in check_domain
    self.check_valid_mx(domain, mx_list, **options)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/modoboa/admin/management/commands/subcommands/_mx.py", line 154, in check_valid_mx
    valid_mxs = self.valid_mxs
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/django/utils/functional.py", line 35, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/modoboa/admin/management/commands/subcommands/_mx.py", line 48, in valid_mxs
    for v in valid_mxs.split() if v.strip()]
  File "/srv/modoboa/env/local/lib/python2.7/site-packages/ipaddress.py", line 204, in ip_network
    address)
ValueError: u'mail.secnux.net' does not appear to be an IPv4 or IPv6 network


Discussion / hints:
@cremesk The "valid mxs" option should only contain ip addresses, not domain names.
Which is kinda weird, considering mx records are defined in hostnames, not IP addresses..?
@tuxis-ie The comparison is done after name resolution.
I think in this case we can add a better error message instead of traceback

Fix patch:
diff --git a/modoboa/admin/api/v2/serializers.py b/modoboa/admin/api/v2/serializers.py
index 8ffcf3807..62fe5fb51 100644
--- a/modoboa/admin/api/v2/serializers.py
+++ b/modoboa/admin/api/v2/serializers.py
@@ -1,5 +1,6 @@
 """Admin API v2 serializers."""
 
+import ipaddress
 import os
 
 from django.conf import settings
@@ -230,6 +231,21 @@ def validate_dkim_keys_storage_dir(self, value):
                 )
         return value
 
+    def validate_valid_mxs(self, value):
+        """Make sure it only contains IP addresses."""
+        if not value:
+            return value
+        try:
+            ip_addresses = [
+                ipaddress.ip_network(v.strip())
+                for v in value.split() if v.strip()
+            ]
+        except ValueError:
+            raise serializers.ValidationError(
+                _("This field only allows valid IP addresses (or networks)")
+            )
+        return value
+
     def validate(self, data):
         """Check MX options."""
         condition = (
diff --git a/modoboa/admin/app_settings.py b/modoboa/admin/app_settings.py
index 184e3b5e2..1b763c0ad 100644
--- a/modoboa/admin/app_settings.py
+++ b/modoboa/admin/app_settings.py
@@ -1,6 +1,7 @@
 """Modoboa admin settings."""
 
 import collections
+import ipaddress
 import os
 
 from django import forms
@@ -196,7 +197,7 @@ class AdminParametersForm(param_forms.AdminParametersForm):
     }
 
     def __init__(self, *args, **kwargs):
-        super(AdminParametersForm, self).__init__(*args, **kwargs)
+        super().__init__(*args, **kwargs)
         self.field_widths = {
             "default_domain_quota": 2,
             "default_mailbox_quota": 2
@@ -260,6 +261,24 @@ def clean_dkim_keys_storage_dir(self):
                 )
         return storage_dir
 
+    def clean_valid_mxs(self):
+        """Make sure it only contains IP addresses."""
+        value = self.cleaned_data.get("valid_mxs")
+        if not value:
+            return value
+        try:
+            ip_addresses = [
+                ipaddress.ip_network(v.strip())
+                for v in value.split() if v.strip()
+            ]
+        except ValueError:
+            raise forms.ValidationError(
+                ugettext_lazy(
+                    "This field only allows valid IP addresses (or networks)"
+                )
+            )
+        return value
+
     def clean(self):
         """Check MX options."""
         cleaned_data = super(AdminParametersForm, self).clean()
diff --git a/modoboa/core/api/v2/views.py b/modoboa/core/api/v2/views.py
index 214affd20..c6ca853ff 100644
--- a/modoboa/core/api/v2/views.py
+++ b/modoboa/core/api/v2/views.py
@@ -54,7 +54,7 @@ def post(self, request, *args, **kwargs):
                         _("Password scheme mismatch. Updating %s password"),
                         user.username
                     )
-                    user.set_password(serializer.data["password"])
+                    user.set_password(request.data["password"])
                     user.save()
                 if pwhash.needs_rehash(user.password):
                     logger.info(


Test patch:
diff --git a/modoboa/parameters/api/v2/tests.py b/modoboa/parameters/api/v2/tests.py
index 0d3dbba7f..0803b650d 100644
--- a/modoboa/parameters/api/v2/tests.py
+++ b/modoboa/parameters/api/v2/tests.py
@@ -46,5 +46,45 @@ def test_update(self):
             "deflt_domain_mailbox_aliases_limit": 0
         }
         url = reverse("v2:parameter-detail", args=["limits"])
-        resp = self.client.put(url, json=data)
+        resp = self.client.put(url, data, format="json")
         self.assertEqual(resp.status_code, 200)
+
+    def test_update_admin_settings(self):
+        data = {
+            "default_domain_quota": -1,
+            "default_mailbox_quota": -1,
+            "valid_mxs": "mail.test.com",
+            "custom_dns_server": "",
+            "dkim_keys_storage_dir": "/wrong"
+        }
+        url = reverse("v2:parameter-detail", args=["admin"])
+        resp = self.client.put(url, data, format="json")
+        self.assertEqual(resp.status_code, 400)
+        errors = resp.json()
+        self.assertEqual(
+            errors["valid_mxs"],
+            ["This field only allows valid IP addresses (or networks)"]
+        )
+        self.assertEqual(
+            errors["default_domain_quota"], ["Must be a positive integer"]
+        )
+        self.assertEqual(
+            errors["default_mailbox_quota"], ["Must be a positive integer"]
+        )
+        self.assertEqual(
+            errors["dkim_keys_storage_dir"], ["Directory not found."]
+        )
+
+        data = {
+            "enable_mx_checks": True,
+            "domains_must_have_authorized_mx": True,
+            "valid_mxs": "",
+            "custom_dns_server": "",
+            "dkim_keys_storage_dir": ""
+        }
+        resp = self.client.put(url, data, format="json")
+        self.assertEqual(resp.status_code, 400)
+        self.assertEqual(
+            resp.json()["valid_mxs"],
+            ["Define at least one authorized network / address"]
+        )