Prevent adding existing or predefined services

and improved form error handling

actions/pagekite

@@ -98,7 +98,8 @@ def subcommand_is_running(_):
 def subcommand_start_and_enable(_):
     aug.remove(PATHS['abort_not_configured'])
     aug.save()
-    _service('start')
+    # 'start' alone sometimes fails, even if the service is not running
+    _service('restart')
     print 'enabled'
 
 

plinth/__main__.py

@@ -222,6 +222,9 @@ def configure_django():
             'django.middleware.clickjacking.XFrameOptionsMiddleware',
             'plinth.modules.first_boot.middleware.FirstBootMiddleware',
         ),
+        MESSAGE_TAGS = {
+            messages_constants.ERROR: 'danger'
+        },
         ROOT_URLCONF='plinth.urls',
         SECURE_PROXY_SSL_HEADER=secure_proxy_ssl_header,
         SESSION_ENGINE='django.contrib.sessions.backends.file',

plinth/modules/pagekite/forms.py

@@ -18,11 +18,13 @@
 from gettext import gettext as _
 import logging
 
+import copy
 from django import forms
 from django.contrib import messages
 from django.core import validators
 
 from actions.pagekite_util import convert_service_to_string
+from plinth.errors import ActionError
 from .util import PREDEFINED_SERVICES, _run, get_kite_details, KITE_NAME, \
     KITE_SECRET, BACKEND_HOST
 
@@ -52,7 +54,7 @@ class ConfigurationForm(forms.Form):
 
     kite_name = TrimmedCharField(
         label=_('Kite name'),
-        help_text=_('Example: mybox1-myacc.pagekite.me'),
+        help_text=_('Example: mybox.pagekite.me'),
         validators=[
             validators.RegexValidator(r'^[\w-]{1,63}(\.[\w-]{1,63})*$',
                                       _('Invalid kite name'))])
@@ -120,22 +122,23 @@ class DefaultServiceForm(forms.Form):
                                      .format(name=service_name))
 
 
-class CustomServiceForm(forms.Form):
+class BaseCustomServiceForm(forms.Form):
-    """Form to add/delete a custom service"""
+    """Basic form functionality to handle a custom service"""
     choices = [("http", "http"), ("https", "https"), ("raw", "raw")]
     protocol = forms.ChoiceField(choices=choices, label="protocol")
     frontend_port = forms.IntegerField(min_value=0, max_value=65535,
-                                       label="external (frontend) port")
+                                       label="external (frontend) port",
+                                       required=True)
     backend_port = forms.IntegerField(min_value=0, max_value=65535,
                                       label="internal (freedombox) port")
     subdomains = forms.BooleanField(label="Enable Subdomains", required=False)
 
-    def convert_form_data_to_service_string(self, formdata):
+    def convert_formdata_to_service(self, formdata):
-        """Prepare the user form input for being passed on to the action
+        """Add information to make a service out of the form data"""
+        # convert integers to str (to compare values with DEFAULT_SERVICES)
+        for field in ('frontend_port', 'backend_port'):
+            formdata[field] = str(formdata[field])
 
-        1. add all information that a 'service' is expected to have
-        2. convert the service to a service_string
-        """
         # set kitename and kitesecret if not already set
         if 'kitename' not in formdata:
             if 'subdomains' in formdata and formdata['subdomains']:
@@ -145,22 +148,70 @@ class CustomServiceForm(forms.Form):
         if 'secret' not in formdata:
             formdata['secret'] = KITE_SECRET
 
-        # merge protocol and frontend_port to one entry (protocol)
+        # merge protocol and frontend_port back to one entry (protocol)
         if 'frontend_port' in formdata:
-            if str(formdata['frontend_port']) not in formdata['protocol']:
+            if formdata['frontend_port'] not in formdata['protocol']:
                 formdata['protocol'] = "%s/%s" % (formdata['protocol'],
                                                   formdata['frontend_port'])
         if 'backend_host' not in formdata:
             formdata['backend_host'] = BACKEND_HOST
 
-        return convert_service_to_string(formdata)
+        return formdata
 
-    def save(self, request):
+
-        service = self.convert_form_data_to_service_string(self.cleaned_data)
+class DeleteCustomServiceForm(BaseCustomServiceForm):
-        _run(['add-service', '--service', service])
-        messages.success(request, _('Added custom service'))
 
     def delete(self, request):
-        service = self.convert_form_data_to_service_string(self.cleaned_data)
+        service = self.convert_formdata_to_service(self.cleaned_data)
-        _run(['remove-service', '--service', service])
+        service_string = convert_service_to_string(service)
+        _run(['remove-service', '--service', service_string])
         messages.success(request, _('Deleted custom service'))
+
+
+class AddCustomServiceForm(BaseCustomServiceForm):
+    """Adds the save() method and validation to not add predefined services"""
+
+    def matches_predefined_service(self, formdata):
+        """Returns whether the user input matches a predefined service"""
+        service = self.convert_formdata_to_service(formdata)
+        match_found = False
+        for predefined_service_obj in PREDEFINED_SERVICES.values():
+            # manually add the port to compare predefined with custom services
+            # that's due to the (sometimes) implicit port in the configuration
+            predefined_service = copy.copy(predefined_service_obj['params'])
+            if predefined_service['protocol'] == 'http':
+                predefined_service['protocol'] = 'http/80'
+            elif predefined_service['protocol'] == 'https':
+                predefined_service['protocol'] = 'https/443'
+
+            # The formdata service has additional keys, so we can't compare
+            # the dicts directly.
+            # instead look whether predefined_service is a subset of service
+            if all(service[k] == v for k, v in predefined_service.items()):
+                match_found = True
+                break
+        return match_found
+
+    def clean(self):
+        cleaned_data = super(AddCustomServiceForm, self).clean()
+        try:
+            is_predefined = self.matches_predefined_service(cleaned_data)
+        except KeyError:
+            is_predefined = False
+        if is_predefined:
+            msg = _("""This service is available as a default service. Please
+                    use the 'Default Services' page to enable it.""")
+            raise forms.ValidationError(msg)
+        return cleaned_data
+
+    def save(self, request):
+        service = self.convert_formdata_to_service(self.cleaned_data)
+        service_string = convert_service_to_string(service)
+        try:
+            _run(['add-service', '--service', service_string])
+            messages.success(request, _('Added custom service'))
+        except ActionError as exception:
+            if "already exists" in str(exception):
+                messages.error(request, _('This service already exists'))
+            else:
+                raise

plinth/modules/pagekite/templates/pagekite_custom_services.html

@@ -24,32 +24,43 @@
 
 {% block page_head %}
   <style type="text/css">
-    div.checkbox .help-block {
-      display: inline-block;
-      margin: 0px 10px;
-    }
     div.custom-services span.service {
       display: inline-block;
       padding-top: 6px;
     }
-    input.btn {
+    form.pull-right button {
-      margin: 10px 15px;
+      margin: 10px 5px;
+    }
+    .add-service input.btn {
+      margin: 10px 0px;
     }
   </style>
 {% endblock %}
 
 {% block content %}
 
-  <h3>Custom Services</h3>
   <div class="alert alert-warning" role="alert">
-    Warning: Your PageKite frontend server may not support all the
+    <b>Warning:</b><br>Your PageKite frontend server may not support all the
     protocol/port combinations that you are able to define here. For example,
     HTTPS on ports other than 443 is known to cause problems.
   </div>
 
   <div class="row custom-services">
 
-    <div class="col-lg-5">
+    <div class="col-lg-6">
+      <form class="form add-service" method="post">
+        <h4>Create a custom service</h4>
+        {{ form|bootstrap_horizontal:'col-lg-6' }}
+        {% csrf_token %}
+        <div class="form-group">
+          <div class=" col-lg-offset-6 col-lg-6">
+            <input type="submit" class="btn btn-primary" value="Add Service"/>
+          </div>
+        </div>
+      </form>
+    </div>
+
+    <div class="col-lg-5 col-lg-offset-1">
       <h4>Existing custom services</h4>
       {% if not custom_services %}
         <i>You don't have any Custom Services enabled</i>
@@ -65,13 +76,15 @@
                 {% else %}
                   {{ service_url }}
                 {% endif %}
+                <br>
+                connected to {{ service.backend_host }}:{{ service.backend_port }}
               </span>
             </span>
             <form class="form pull-right" method="post"
                   action="{% url 'pagekite:delete-custom-service' %}">
               <div style='display:none'>
                 {% csrf_token %}
-                {{ service.form }}
+                {{ service.form.as_p }}
               </div>
               <button type="submit" class="btn btn-default"
                       title="Delete this service">
@@ -84,15 +97,6 @@
       </div>
     </div>
 
-    <div class="col-lg-6 col-lg-offset-1">
-      <form class="form" method="post">
-        <h4>Create a custom service</h4>
-        {{ form|bootstrap_horizontal:'col-lg-6' }}
-        {% csrf_token %}
-        <input type="submit" class="btn btn-primary pull-right" value="Add Service"/>
-      </form>
-    </div>
-
   </div>
 
 {% endblock %}

plinth/modules/pagekite/templates/pagekite_default_services.html

@@ -38,12 +38,10 @@
 
 {% block content %}
 
-  <h3>Default Services</h3>
-
   <div class="alert alert-warning" role="alert">
-  <p>Exposing services makes them accessible and attackable from the evil
+  <b>Warning:</b><br>
-  internet.<p>
+  <p>Published services are accessible and attackable from the evil internet.<p>
-  <p><b>Exposing SSH with the default password for 'fbx' is a very bad idea.</b></p>
+  <p>Exposing SSH with the default password for 'fbx' is a VERY BAD idea.</p>
   </div>
 
   <form class="form predefined" method="post">

plinth/modules/pagekite/util.py

@@ -28,7 +28,11 @@ LOGGER = logging.getLogger(__name__)
 KITE_NAME = '@kitename'
 KITE_SECRET = '@kitesecret'
 BACKEND_HOST = 'localhost'
-# predefined services show up in the PredefinedServiceForm as checkbox
+
+# Predefined services are used to build the PredefinedServiceForm
+#
+# ATTENTION: When changing the params, make sure that the AddCustomServiceForm
+# still recognizes when you try to add a service equal to a predefined one
 PREDEFINED_SERVICES = {
     'http': {
         'params': {'protocol': 'http',

plinth/modules/pagekite/views.py

@@ -26,7 +26,9 @@ from django.views.generic.edit import FormView
 from plinth import package
 from .util import get_pagekite_config, get_pagekite_services, \
     get_kite_details, prepare_service_for_display
-from .forms import ConfigurationForm, DefaultServiceForm, CustomServiceForm
+from .forms import ConfigurationForm, DefaultServiceForm, \
+    AddCustomServiceForm, DeleteCustomServiceForm
+
 
 subsubmenu = [{'url': reverse_lazy('pagekite:index'),
                'text': _('About PageKite')},
@@ -65,7 +67,7 @@ class ContextMixin(object):
 
 class DeleteServiceView(ContextMixin, View):
     def post(self, request):
-        form = CustomServiceForm(request.POST)
+        form = DeleteCustomServiceForm(request.POST)
         if form.is_valid():
             form.delete(request)
         return HttpResponseRedirect(reverse('pagekite:custom-services'))
@@ -79,7 +81,7 @@ class CustomServiceView(ContextMixin, TemplateView):
                                                                   **kwargs)
         unused, custom_services = get_pagekite_services()
         for service in custom_services:
-            service['form'] = CustomServiceForm(initial=service)
+            service['form'] = AddCustomServiceForm(initial=service)
         context['custom_services'] = [prepare_service_for_display(service)
                                       for service in custom_services]
         context.update(get_kite_details())
@@ -87,15 +89,15 @@ class CustomServiceView(ContextMixin, TemplateView):
 
     def get(self, request, *args, **kwargs):
         context = self.get_context_data(**kwargs)
-        form = CustomServiceForm(prefix="custom")
+        form = AddCustomServiceForm(prefix="custom")
         context['form'] = form
         return self.render_to_response(context)
 
     def post(self, request):
-        form = CustomServiceForm(request.POST, prefix="custom")
+        form = AddCustomServiceForm(request.POST, prefix="custom")
         if form.is_valid():
             form.save(request)
-            form = CustomServiceForm(prefix="custom")
+            form = AddCustomServiceForm(prefix="custom")
 
         context = self.get_context_data()
         context['form'] = form