email: Documentation, code cleanup

- Audit: finalize MainCfDiagnosis API
- Doc: document Diagnosis models
- Doc: remove hacky imports and replace them with autodoc mock imports
- Views: add `email_` prefix to email server templates
- Apache: delete unnecessary directives (robots tag, cookie header etc.)

plinth/modules/email_server/audit/ldap.py

@@ -87,15 +87,12 @@ def action_set_up():
 
 def check_sasl():
     diagnosis = models.MainCfDiagnosis('Postfix-Dovecot SASL integration')
-    current = postconf.get_many_unsafe(default_config.keys())
+    diagnosis.compare(default_config, postconf.get_many_unsafe)
-    diagnosis.compare_and_advise(current=current, default=default_config)
     return diagnosis
 
 
 def fix_sasl(diagnosis):
-    diagnosis.assert_resolved()
+    diagnosis.apply_changes(postconf.set_many_unsafe)
-    logger.info('Setting postconf: %r', diagnosis.advice)
-    postconf.set_many_unsafe(diagnosis.advice)
 
 
 def action_set_sasl():
@@ -133,29 +130,26 @@ def check_alias_maps():
 
 
 def fix_alias_maps(diagnosis):
-    unresolved_issues = list(diagnosis.unresolved_issues())
+    diagnosis.repair('alias_maps', rearrange_alias_maps)
-    if 'alias_maps' in unresolved_issues:
+    diagnosis.apply_changes(postconf.set_many_unsafe)
-        analysis = diagnosis.user['alias_maps']
-        # Delete *all* references to BEFORE_ALIASES and AFTER_ALIASES
-        for i in range(len(analysis.parsed)):
-            if analysis.parsed[i] in (BEFORE_ALIASES, AFTER_ALIASES):
-                analysis.parsed[i] = ''
-        # Does hash:/etc/aliases exist in list?
-        if analysis.isystem >= 0:
-            # Put the maps around hash:/etc/aliases
-            val = '%s %s %s' % (BEFORE_ALIASES, ETC_ALIASES, AFTER_ALIASES)
-            analysis.parsed[analysis.isystem] = val
-        else:
-            # To the end
-            analysis.parsed.append(BEFORE_ALIASES)
-            analysis.parsed.append(AFTER_ALIASES)
-        # List -> string
-        fixed = ' '.join(filter(None, analysis.parsed))
-        diagnosis.advice['alias_maps'] = fixed
 
-    diagnosis.assert_resolved()
+
-    logging.info('Setting postfix config: %r', diagnosis.advice)
+def rearrange_alias_maps(analysis):
-    postconf.set_many_unsafe(diagnosis.advice)
+    # Delete *all* references to BEFORE_ALIASES and AFTER_ALIASES
+    for i in range(len(analysis.parsed)):
+        if analysis.parsed[i] in (BEFORE_ALIASES, AFTER_ALIASES):
+            analysis.parsed[i] = ''
+    # Does hash:/etc/aliases exist in list?
+    if analysis.isystem >= 0:
+        # Put the maps around hash:/etc/aliases
+        val = '%s %s %s' % (BEFORE_ALIASES, ETC_ALIASES, AFTER_ALIASES)
+        analysis.parsed[analysis.isystem] = val
+    else:
+        # To the end
+        analysis.parsed.append(BEFORE_ALIASES)
+        analysis.parsed.append(AFTER_ALIASES)
+    # List -> string
+    return ' '.join(filter(None, analysis.parsed))
 
 
 def check_local_recipient_maps():
@@ -180,9 +174,7 @@ def check_local_recipient_maps():
 
 
 def fix_local_recipient_maps(diagnosis):
-    diagnosis.assert_resolved()
+    diagnosis.apply_changes(postconf.set_many_unsafe)
-    logging.info('Setting postfix config: %r', diagnosis.advice)
-    postconf.set_many_unsafe(diagnosis.advice)
 
 
 def action_set_ulookup():

plinth/modules/email_server/audit/models.py

@@ -1,5 +1,6 @@
-"""Audit models"""
 # SPDX-License-Identifier: AGPL-3.0-or-later
+"""Models of the audit module"""
+
 import dataclasses
 import logging
 import typing
@@ -12,7 +13,10 @@ class UnresolvedIssueError(AssertionError):
 
 
 class Diagnosis:
+    """Records a diagnosis: what went wrong and how to fix them"""
+
     def __init__(self, title):
+        """Class constructor"""
         self.title = title
         self.fails = []
         self.errors = []
@@ -53,12 +57,28 @@ class Diagnosis:
 
 
 class MainCfDiagnosis(Diagnosis):
+    """Diagnosis for a set of main.cf configuration keys"""
+
     def __init__(self, title):
+        """Class constructor"""
         super().__init__(title)
         self.advice = {}
         self.user = {}
 
     def flag(self, key, corrected_value=None, user=None):
+        """Flag a problematic key.
+
+        If `corrected_value` is a str, the specified value is assumed to be
+        correct.
+
+        :type key: str
+        :param key: main.cf key
+        :type corrected_value: str or None
+        :param corrected_value: corrected value
+        :type user: Any
+        :param user: customized data (see the :meth:`.repair` method)
+        :raises ValueError: if the key has been flagged
+        """
         if key in self.advice:
             raise ValueError('Key has been flagged')
         else:
@@ -66,16 +86,23 @@ class MainCfDiagnosis(Diagnosis):
             self.user[key] = user
 
     def flag_once(self, key, **kwargs):
+        """Flag a problematic key. If the key has been flagged, do nothing.
+
+        See :meth:`.flag` for the function signature.
+        """
         if key not in self.advice:
             self.flag(key, **kwargs)
 
     def unresolved_issues(self):
-        """Returns an interator of dictionary keys"""
+        """Return the iterator of all keys that do not have a corrected value.
+
+        :return: an iterator of keys
+        """
         for key, value in self.advice.items():
             if value is None:
                 yield key
 
-    def compare_and_advise(self, current, default):
+    def _compare_and_advise(self, current, default):
         if len(current) > len(default):
             raise ValueError('Sanity check failed: dictionary sizes')
         for key, value in default.items():
@@ -83,9 +110,63 @@ class MainCfDiagnosis(Diagnosis):
                 self.flag(key, corrected_value=value)
                 self.critical('%s must equal %s', key, value)
 
+    def compare(self, expected, getter):
+        """Check the current Postfix configuration. Flag and correct all keys
+        that have an unexpected value.
+
+        :type expected: dict[str, str]
+        :param expected: a dictionary specifying the set of keys to be checked
+          and their expected values
+        :type getter: Iterator[str] -> dict[str, str]
+        :param getter: a function that fetches the current postfix config; it
+          takes an iterator of strings and returns a str-to-str dictionary.
+        """
+        current = getter(expected.keys())
+        self._compare_and_advise(current, expected)
+
+    def repair(self, key, repair_function):
+        """Repair the key if its value has not been corrected by other means.
+
+        `repair_function` will not be called if the key has had a corrected
+        value or the key does not need attention.
+
+        In case `repair_function` is called, we will pass in the `user` data
+        associated with `key`.
+
+        The job of `repair_function` is to return a corrected value. It should
+        not modify any Postfix configuration in any way. It may read
+        configuration files, but pending Postconf changes are not visible.
+
+        If `repair_function` could not solve the problem, it may return `None`
+        as an alternative to raising an exception. Using this feature, you may
+        implement fallback strategies.
+
+        :type key: str
+        :param key: the key to be repaired
+        :type repair_function: Any -> Union[str, None]
+        :param repair_function: a function that returns the corrected value
+          for `key`
+        """
+        if key in self.advice and self.advice[key] is None:
+            self.advice[key] = repair_function(self.user[key])
+
+    def apply_changes(self, setter):
+        """Apply changes by calling the `setter` with a dictionary of corrected
+        keys and values.
+
+        :type setter: dict[str, str] -> None
+        :param setter: configuration changing function that takes a str-to-str
+          dictionary.
+        :raises UnresolvedIssueError: if the diagnosis contains an uncorrected
+          key
+        """
+        self.assert_resolved()
+        logger.info('Setting postconf: %r', self.advice)
+        setter(self.advice)
+
     def assert_resolved(self):
-        """Raises an UnresolvedIssueError if the diagnosis report contains an
+        """Raises an :class:`.UnresolvedIssueError` if the diagnosis report
-        unresolved issue"""
+        contains an unresolved issue (i.e. an uncorrected key)"""
         if None in self.advice.values():
             raise UnresolvedIssueError('Assertion failed')
 

plinth/modules/email_server/audit/spam.py

@@ -86,15 +86,12 @@ def repair():
 
 def check_filter():
     diagnosis = models.MainCfDiagnosis('Inbound and outbound mail filters')
-    current = postconf.get_many_unsafe(milter_config.keys())
+    diagnosis.compare(milter_config, postconf.get_many_unsafe)
-    diagnosis.compare_and_advise(current=current, default=milter_config)
     return diagnosis
 
 
 def fix_filter(diagnosis):
-    diagnosis.assert_resolved()
+    diagnosis.apply_changes(postconf.set_many_unsafe)
-    logger.info('Setting postconf: %r', diagnosis.advice)
-    postconf.set_many_unsafe(diagnosis.advice)
 
 
 def action_set_filter():

plinth/modules/email_server/audit/tls.py

@@ -76,15 +76,12 @@ def repair():
 
 def check_tls():
     diagnosis = models.MainCfDiagnosis('Postfix TLS')
-    current = postconf.get_many_unsafe(list(postfix_config.keys()))
+    diagnosis.compare(postfix_config, postconf.get_many_unsafe)
-    diagnosis.compare_and_advise(current=current, default=postfix_config)
     return diagnosis
 
 
 def repair_tls(diagnosis):
-    diagnosis.assert_resolved()
+    diagnosis.apply_changes(postconf.set_many_unsafe)
-    logger.info('Setting postconf: %r', diagnosis.advice)
-    postconf.set_many_unsafe(diagnosis.advice)
 
 
 def try_set_up_certificates():

plinth/modules/email_server/data/etc/apache2/conf-available/email-server-freedombox.conf

@@ -1,6 +1,5 @@
 <LocationMatch "^/rspamd$">
     Redirect "/rspamd/"
-    Include includes/freedombox-robots.conf
 </LocationMatch>
 
 <Location "/rspamd/">
@@ -25,17 +24,9 @@
     </IfModule>
 </Location>
 
-<Location "/plinth/apps/email_server/*.xml">
+<Location "/.well-known/autoconfig/mail/config-v1.1.xml">
-    # Require SSO
+    <IfModule mod_rewrite.c>
-    Include includes/freedombox-single-sign-on.conf
+        RewriteEngine On
-    <IfModule mod_auth_pubtkt.c>
+        RewriteRule ^ /plinth/apps/email_server/config.xml [PT]
-        TKTAuthToken "admin"
     </IfModule>
 </Location>
-
-<Location "/.well-known/autoconfig/mail/config-v1.1.xml">
-    ProxyPass http://127.0.0.1:8000/plinth/apps/email_server/config.xml
-    RequestHeader unset Cookie
-    Header unset Set-Cookie
-    Include includes/freedombox-robots.conf
-</Location>

plinth/modules/email_server/templates/email_alias.html

@@ -1,5 +1,5 @@
 {# SPDX-License-Identifier: AGPL-3.0-or-later #}
-{% extends "form_base.html" %}
+{% extends "email_form_base.html" %}
 
 {% load bootstrap %}
 {% load i18n %}

plinth/modules/email_server/templates/email_domains.html

@@ -1,5 +1,5 @@
 {# SPDX-License-Identifier: AGPL-3.0-or-later #}
-{% extends "form_base.html" %}
+{% extends "email_form_base.html" %}
 {% load bootstrap %}
 {% load i18n %}
 

plinth/modules/email_server/templates/email_security.html

@@ -1,5 +1,5 @@
 {# SPDX-License-Identifier: AGPL-3.0-or-later #}
-{% extends "form_base.html" %}
+{% extends "email_form_base.html" %}
 
 {% load i18n %}
 

plinth/modules/email_server/templates/my_mail.html

@@ -1,5 +1,5 @@
 {# SPDX-License-Identifier: AGPL-3.0-or-later #}
-{% extends "form_base.html" %}
+{% extends "email_form_base.html" %}
 
 {% load bootstrap %}
 {% load i18n %}

plinth/modules/email_server/urls.py

@@ -7,7 +7,7 @@ from . import views
 
 urlpatterns = [
     path('apps/email_server/', views.EmailServerView.as_view(), name='index'),
-    path('apps/email_server/email_security', views.TLSView.as_view()),
+    path('apps/email_server/security', views.TLSView.as_view()),
     path('apps/email_server/domains', views.DomainView.as_view()),
 
     path('apps/email_server/my_mail',

plinth/modules/email_server/views.py

@@ -21,7 +21,7 @@ class TabMixin(View):
         ('', _('Home')),
         ('my_mail', _('My Mail')),
         ('my_aliases', _('My Aliases')),
-        ('email_security', _('Security')),
+        ('security', _('Security')),
         ('domains', _('Domains'))
     ]
 
@@ -163,7 +163,7 @@ class AliasView(TabMixin, TemplateView):
                 self.cleaned_data['alias'] = lst
                 return True
 
-    template_name = 'alias.html'
+    template_name = 'email_alias.html'
     form_classes = (forms.AliasCreationForm, Checkboxes)
 
     def get_context_data(self, *args, **kwargs):
@@ -222,7 +222,7 @@ class TLSView(TabMixin, TemplateView):
 
 
 class DomainView(TabMixin, TemplateView):
-    template_name = 'domains.html'
+    template_name = 'email_domains.html'
 
     def get_context_data(self, *args, **kwargs):
         context = super().get_context_data(*args, **kwargs)
@@ -248,12 +248,12 @@ class DomainView(TabMixin, TemplateView):
 
 
 class XmlView(TemplateView):
-    template_name = 'config.xml'
+    template_name = 'email_autoconfig.xml'
 
     def render_to_response(self, *args, **kwargs):
-        if 200 <= kwargs.get('status', 200) < 300:
+        kwargs['content_type'] = 'text/xml; charset=utf-8'
-            kwargs['content_type'] = 'text/xml; charset=utf-8'
         response = super().render_to_response(*args, **kwargs)
+        response['X-Robots-Tag'] = 'noindex, nofollow, noarchive'
         return response
 
     def get_context_data(self, **kwargs):