From 5a9c7e5077e34f941774e5af79fbb48760f1cfd4 Mon Sep 17 00:00:00 2001 From: fliu <10025-fliu@users.noreply.salsa.debian.org> Date: Sun, 15 Aug 2021 02:45:57 +0000 Subject: [PATCH] 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 | 52 +++++------ plinth/modules/email_server/audit/models.py | 91 ++++++++++++++++++- plinth/modules/email_server/audit/spam.py | 7 +- plinth/modules/email_server/audit/tls.py | 7 +- .../email-server-freedombox.conf | 17 +--- .../{alias.html => email_alias.html} | 2 +- .../{config.xml => email_autoconfig.xml} | 0 .../{domains.html => email_domains.html} | 2 +- .../{form_base.html => email_form_base.html} | 0 .../templates/email_security.html | 2 +- .../email_server/templates/my_mail.html | 2 +- plinth/modules/email_server/urls.py | 2 +- plinth/modules/email_server/views.py | 12 +-- 13 files changed, 127 insertions(+), 69 deletions(-) rename plinth/modules/email_server/templates/{alias.html => email_alias.html} (96%) rename plinth/modules/email_server/templates/{config.xml => email_autoconfig.xml} (100%) rename plinth/modules/email_server/templates/{domains.html => email_domains.html} (96%) rename plinth/modules/email_server/templates/{form_base.html => email_form_base.html} (100%) diff --git a/plinth/modules/email_server/audit/ldap.py b/plinth/modules/email_server/audit/ldap.py index 412828531..8293120b9 100644 --- a/plinth/modules/email_server/audit/ldap.py +++ b/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_and_advise(current=current, default=default_config) + diagnosis.compare(default_config, postconf.get_many_unsafe) return diagnosis def fix_sasl(diagnosis): - diagnosis.assert_resolved() - logger.info('Setting postconf: %r', diagnosis.advice) - postconf.set_many_unsafe(diagnosis.advice) + diagnosis.apply_changes(postconf.set_many_unsafe) def action_set_sasl(): @@ -133,29 +130,26 @@ def check_alias_maps(): def fix_alias_maps(diagnosis): - unresolved_issues = list(diagnosis.unresolved_issues()) - if 'alias_maps' in unresolved_issues: - 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.repair('alias_maps', rearrange_alias_maps) + diagnosis.apply_changes(postconf.set_many_unsafe) - diagnosis.assert_resolved() - logging.info('Setting postfix config: %r', diagnosis.advice) - postconf.set_many_unsafe(diagnosis.advice) + +def rearrange_alias_maps(analysis): + # 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() - logging.info('Setting postfix config: %r', diagnosis.advice) - postconf.set_many_unsafe(diagnosis.advice) + diagnosis.apply_changes(postconf.set_many_unsafe) def action_set_ulookup(): diff --git a/plinth/modules/email_server/audit/models.py b/plinth/modules/email_server/audit/models.py index 9debf504e..0d5795e92 100644 --- a/plinth/modules/email_server/audit/models.py +++ b/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 - unresolved issue""" + """Raises an :class:`.UnresolvedIssueError` if the diagnosis report + contains an unresolved issue (i.e. an uncorrected key)""" if None in self.advice.values(): raise UnresolvedIssueError('Assertion failed') diff --git a/plinth/modules/email_server/audit/spam.py b/plinth/modules/email_server/audit/spam.py index 8c7472cc4..ca2f6caf3 100644 --- a/plinth/modules/email_server/audit/spam.py +++ b/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_and_advise(current=current, default=milter_config) + diagnosis.compare(milter_config, postconf.get_many_unsafe) return diagnosis def fix_filter(diagnosis): - diagnosis.assert_resolved() - logger.info('Setting postconf: %r', diagnosis.advice) - postconf.set_many_unsafe(diagnosis.advice) + diagnosis.apply_changes(postconf.set_many_unsafe) def action_set_filter(): diff --git a/plinth/modules/email_server/audit/tls.py b/plinth/modules/email_server/audit/tls.py index 72736a9d3..ceb48b3d6 100644 --- a/plinth/modules/email_server/audit/tls.py +++ b/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_and_advise(current=current, default=postfix_config) + diagnosis.compare(postfix_config, postconf.get_many_unsafe) return diagnosis def repair_tls(diagnosis): - diagnosis.assert_resolved() - logger.info('Setting postconf: %r', diagnosis.advice) - postconf.set_many_unsafe(diagnosis.advice) + diagnosis.apply_changes(postconf.set_many_unsafe) def try_set_up_certificates(): diff --git a/plinth/modules/email_server/data/etc/apache2/conf-available/email-server-freedombox.conf b/plinth/modules/email_server/data/etc/apache2/conf-available/email-server-freedombox.conf index b7ae302a2..218087e8a 100644 --- a/plinth/modules/email_server/data/etc/apache2/conf-available/email-server-freedombox.conf +++ b/plinth/modules/email_server/data/etc/apache2/conf-available/email-server-freedombox.conf @@ -1,6 +1,5 @@ Redirect "/rspamd/" - Include includes/freedombox-robots.conf @@ -25,17 +24,9 @@ - - # Require SSO - Include includes/freedombox-single-sign-on.conf - - TKTAuthToken "admin" + + + RewriteEngine On + RewriteRule ^ /plinth/apps/email_server/config.xml [PT] - - - 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 - diff --git a/plinth/modules/email_server/templates/alias.html b/plinth/modules/email_server/templates/email_alias.html similarity index 96% rename from plinth/modules/email_server/templates/alias.html rename to plinth/modules/email_server/templates/email_alias.html index 1980b66f6..fab376a96 100644 --- a/plinth/modules/email_server/templates/alias.html +++ b/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 %} diff --git a/plinth/modules/email_server/templates/config.xml b/plinth/modules/email_server/templates/email_autoconfig.xml similarity index 100% rename from plinth/modules/email_server/templates/config.xml rename to plinth/modules/email_server/templates/email_autoconfig.xml diff --git a/plinth/modules/email_server/templates/domains.html b/plinth/modules/email_server/templates/email_domains.html similarity index 96% rename from plinth/modules/email_server/templates/domains.html rename to plinth/modules/email_server/templates/email_domains.html index bab811f11..5a588b658 100644 --- a/plinth/modules/email_server/templates/domains.html +++ b/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 %} diff --git a/plinth/modules/email_server/templates/form_base.html b/plinth/modules/email_server/templates/email_form_base.html similarity index 100% rename from plinth/modules/email_server/templates/form_base.html rename to plinth/modules/email_server/templates/email_form_base.html diff --git a/plinth/modules/email_server/templates/email_security.html b/plinth/modules/email_server/templates/email_security.html index a84bed457..0a65916f2 100644 --- a/plinth/modules/email_server/templates/email_security.html +++ b/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 %} diff --git a/plinth/modules/email_server/templates/my_mail.html b/plinth/modules/email_server/templates/my_mail.html index 7bb928151..720ba77fe 100644 --- a/plinth/modules/email_server/templates/my_mail.html +++ b/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 %} diff --git a/plinth/modules/email_server/urls.py b/plinth/modules/email_server/urls.py index 1ae111ee4..5e12fefa1 100644 --- a/plinth/modules/email_server/urls.py +++ b/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', diff --git a/plinth/modules/email_server/views.py b/plinth/modules/email_server/views.py index 3bf378b8c..fb64d9b9a 100644 --- a/plinth/modules/email_server/views.py +++ b/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):