refactor/improve /editor/finalize with more JS and REST

src/c3nav/api/authentication.py

@@ -0,0 +1,12 @@
+from rest_framework.authentication import SessionAuthentication
+
+
+class ForceCSRFCheckSessionAuthentication(SessionAuthentication):
+
+    def authenticate(self, request):
+        result = super().authenticate(request)
+
+        if result is None:
+            self.enforce_csrf(request)
+
+        return result

src/c3nav/api/urls.py

@@ -1,7 +1,7 @@
 from django.conf.urls import include, url
 from rest_framework.routers import DefaultRouter
 
-from c3nav.editor.api import HosterViewSet
+from c3nav.editor.api import HosterViewSet, SubmitTaskViewSet
 from c3nav.mapdata.api import FeatureTypeViewSet, FeatureViewSet, LevelViewSet, PackageViewSet, SourceViewSet
 
 router = DefaultRouter()
@@ -11,6 +11,7 @@ router.register(r'sources', SourceViewSet)
 router.register(r'featuretypes', FeatureTypeViewSet, base_name='featuretype')
 router.register(r'features', FeatureViewSet)
 router.register(r'hosters', HosterViewSet, base_name='hoster')
+router.register(r'submittask', SubmitTaskViewSet, base_name='submittask')
 
 
 urlpatterns = [

src/c3nav/editor/api.py

@@ -1,10 +1,17 @@
+from collections import OrderedDict
+
+from django.core import signing
+from django.core.signing import BadSignature
 from django.http import Http404
 from rest_framework.decorators import detail_route
+from rest_framework.exceptions import ValidationError
 from rest_framework.response import Response
 from rest_framework.viewsets import ViewSet
 
-from c3nav.editor.hosters import hosters
+from c3nav.editor.hosters import get_hoster_for_package, hosters
-from c3nav.editor.serializers import HosterSerializer
+from c3nav.editor.serializers import HosterSerializer, TaskSerializer
+from c3nav.editor.tasks import submit_edit_task
+from c3nav.mapdata.models.package import Package
 
 
 class HosterViewSet(ViewSet):
@@ -25,4 +32,74 @@ class HosterViewSet(ViewSet):
     def state(self, request, pk=None, version=None):
         if pk not in hosters:
             raise Http404
-        return Response(hosters[pk].get_state(request))
+
+        hoster = hosters[pk]
+        state = hoster.get_state(request)
+        error = hoster.get_error(request) if state == 'logged_out' else None
+
+        return Response(OrderedDict((
+            ('state', state),
+            ('error', error),
+        )))
+
+    @detail_route(methods=['post'])
+    def auth_uri(self, request, pk=None, version=None):
+        if pk not in hosters:
+            raise Http404
+        return Response({
+            'auth_uri': hosters[pk].get_auth_uri(request)
+        })
+
+    @detail_route(methods=['post'])
+    def submit(self, request, pk=None, version=None):
+        if pk not in hosters:
+            raise Http404
+        hoster = hosters[pk]
+
+        if 'data' not in request.POST:
+            raise ValidationError('Missing POST parameter: data')
+
+        if 'commit_msg' not in request.POST:
+            raise ValidationError('Missing POST parameter: commit_msg')
+
+        data = request.POST['data']
+        commit_msg = request.POST['commit_msg'].strip()
+
+        if not commit_msg:
+            raise ValidationError('POST parameter may not be empty: commit_msg')
+
+        try:
+            data = signing.loads(data)
+        except BadSignature:
+            raise ValidationError('Bad data signature.')
+
+        if data['type'] != 'editor.edit':
+            raise ValidationError('Wrong data type.')
+
+        package = Package.objects.filter(name=data['package_name']).first()
+        data_hoster = None
+        if package is not None:
+            data_hoster = get_hoster_for_package(package)
+
+        if hoster != data_hoster:
+            raise ValidationError('Wrong hoster.')
+
+        task = hoster.submit_edit(request, data)
+
+        serializer = TaskSerializer(task)
+        return Response(serializer.data)
+
+
+class SubmitTaskViewSet(ViewSet):
+    """
+    Get Submit Tasks
+    """
+    def retrieve(self, request, pk=None, version=None):
+        task = submit_edit_task.AsyncResult(task_id=pk)
+        try:
+            task.ready()
+        except:
+            raise Http404
+
+        serializer = TaskSerializer(task)
+        return Response(serializer.data)

src/c3nav/editor/hosters/base.py

@@ -45,21 +45,6 @@ class Hoster(ABC):
         if 'error' in session_data:
             return session_data.pop('error')
 
-    def set_tmp_data(self, request, data):
-        """
-        Save data before redirecting to the OAuth Provider.
-        """
-        self._get_session_data(request)['tmp_data'] = data
-
-    def get_tmp_data(self, request):
-        """
-        Get and forget data that was saved before redirecting to the OAuth Provider.
-        """
-        data = self._get_session_data(request)
-        if 'tmp_data' not in data:
-            return None
-        return data.pop('tmp_data')
-
     def get_state(self, request):
         """
         Get current hoster state for this user.

src/c3nav/editor/serializers.py

@@ -8,3 +8,30 @@ class HosterSerializer(serializers.Serializer):
 
     def get_packages(self, obj):
         return tuple(obj.get_packages().values_list('name', flat=True))
+
+
+class TaskSerializer(serializers.Serializer):
+    id = serializers.CharField()
+    started = serializers.SerializerMethodField()
+    done = serializers.SerializerMethodField()
+    success = serializers.SerializerMethodField()
+    result = serializers.SerializerMethodField()
+    error = serializers.SerializerMethodField()
+
+    def get_started(self, obj):
+        return obj.status != 'PENDING'
+
+    def get_done(self, obj):
+        return obj.ready()
+
+    def get_success(self, obj):
+        return (obj.successful() and obj.result['success']) if obj.ready() else None
+
+    def get_result(self, obj):
+        return obj.result if obj.ready() and obj.successful() else None
+
+    def get_error(self, obj):
+        success = self.get_success(obj)
+        if success is not False:
+            return None
+        return 'Internal Error' if not obj.successful() else obj.result['error']

src/c3nav/editor/static/editor/css/finalize.css

@@ -0,0 +1,3 @@
+.hoster-state, .alert {
+    display:none;
+}

src/c3nav/editor/static/editor/js/editor.js

@@ -394,17 +394,3 @@ editor = {
 if ($('#mapeditlist').length) {
     editor.init();
 }
-$('form[name=redirect]').submit();
-
-function check_hoster_form() {
-    $('form[data-check-hoster]').each(function() {
-        $.getJSON('/api/v1/hosters/'+$(this).attr('data-check-hoster')+'/state/', function(state) {
-            if (state == 'checking') {
-                window.setTimeout(check_hoster_form, 700);
-            } else {
-                $('form[data-check-hoster]').submit();
-            }
-        });
-    });
-}
-check_hoster_form();

src/c3nav/editor/static/editor/js/finalize.js

@@ -0,0 +1,71 @@
+finalize = {
+    hoster: null,
+    state: 'checking',
+    init: function() {
+        finalize.hoster = $('#hoster').attr('data-name');
+        finalize._set_state('checking');
+        finalize._check_hoster();
+        sessionStorage.setItem('finalize-data', finalize.get_data());
+        $('button[data-oauth]').click(finalize._click_oauth_btn);
+        $('button[data-commit]').click(finalize._click_commit_btn);
+    },
+    get_data: function() {
+        return $('#data').val();
+    },
+    _check_hoster: function() {
+        $.getJSON('/api/v1/hosters/'+finalize.hoster+'/state/', function(data) {
+            if (data.state == 'checking') {
+                window.setTimeout(finalize._check_hoster, 700);
+            } else {
+                $('#error').text(data.error).toggle(data.error !== null);
+                finalize._set_state(data.state);
+            }
+        });
+    },
+    _set_state: function(state) {
+        finalize.state = state;
+        $('.hoster-state').hide().filter('[data-state='+state+']').show();
+    },
+    _click_oauth_btn: function() {
+        finalize._set_state('oauth');
+        $.ajax({
+            type: "POST",
+            url: '/api/v1/hosters/'+finalize.hoster+'/auth_uri/',
+            dataType: 'json',
+            headers: {'X-CSRFToken': $('[name=csrfmiddlewaretoken]').val()},
+            success: function(data) {
+                window.location = data.auth_uri;
+            }
+        });
+    },
+    _click_commit_btn: function() {
+        var commit_msg = $.trim($('#commit_msg').val());
+        if (commit_msg == '') return;
+        $('#error').hide();
+        finalize._set_state('progress');
+        $.ajax({
+            type: "POST",
+            url: '/api/v1/hosters/'+finalize.hoster+'/submit/',
+            data: {
+                'data': finalize.get_data(),
+                'commit_msg': commit_msg
+            },
+            dataType: 'json',
+            headers: {'X-CSRFToken': $('[name=csrfmiddlewaretoken]').val()},
+            success: finalize.handle_task_data
+        });
+    },
+    handle_task_data: function(data) {
+        if (data.done && !data.success) {
+            $('#error').text(data.error).show();
+            finalize._set_state('logged_in');
+        }
+    }
+};
+
+if ($('#hoster').length) {
+    finalize.init();
+}
+if ($('#finalize-redirect').length) {
+    $('form').append($('<input type="hidden" name="data">').val(sessionStorage.getItem('finalize-data'))).submit();
+}

src/c3nav/editor/templates/editor/base.html

@@ -8,9 +8,10 @@
     <meta name="viewport" content="width=device-width, initial-scale=1">
     <title>c3nav Map Editor</title>
     {% compress css %}
-    <link href="{% static 'bootstrap/css/bootstrap.css' %}" rel="stylesheet">
+        <link href="{% static 'bootstrap/css/bootstrap.css' %}" rel="stylesheet">
-    <link href="{% static 'leaflet/leaflet.css' %}" rel="stylesheet">
+        <link href="{% static 'leaflet/leaflet.css' %}" rel="stylesheet">
-    <link href="{% static 'editor/css/editor.css' %}" rel="stylesheet">
+        <link href="{% static 'editor/css/editor.css' %}" rel="stylesheet">
+        <link href="{% static 'editor/css/finalize.css' %}" rel="stylesheet">
     {% endcompress %}
   </head>
 
@@ -30,11 +31,12 @@
     </div>
 
     {% compress js %}
-    <script type="text/javascript" src="{% static 'jquery/jquery.js' %}"></script>
+        <script type="text/javascript" src="{% static 'jquery/jquery.js' %}"></script>
-    <script type="text/javascript" src="{% static 'bootstrap/js/bootstrap.js' %}"></script>
+        <script type="text/javascript" src="{% static 'bootstrap/js/bootstrap.js' %}"></script>
-    <script type="text/javascript" src="{% static 'leaflet/leaflet.js' %}"></script>
+        <script type="text/javascript" src="{% static 'leaflet/leaflet.js' %}"></script>
-    <script type="text/javascript" src="{% static 'leaflet/leaflet.editable.js' %}"></script>
+        <script type="text/javascript" src="{% static 'leaflet/leaflet.editable.js' %}"></script>
-    <script type="text/javascript" src="{% static 'editor/js/editor.js' %}"></script>
+        <script type="text/javascript" src="{% static 'editor/js/editor.js' %}"></script>
+        <script type="text/javascript" src="{% static 'editor/js/finalize.js' %}"></script>
     {% endcompress %}
   </body>
 </html>

src/c3nav/editor/templates/editor/feature_success.html

@@ -3,7 +3,6 @@
 <form action="{% url 'editor.finalize' %}" method="POST" name="redirect">
     {% csrf_token %}
     <input type="hidden" name="data" value="{{ data }}">
-    <input type="hidden" name="action" value="check">
     <img src="{% static 'img/loader.gif' %}">
     Redirecting…
 </form>

src/c3nav/editor/templates/editor/finalize.html

@@ -2,80 +2,67 @@
 {% load static %}
 {% load bootstrap3 %}
 {% block content %}
+    <input type="hidden" id="data" value="{{ data }}">
+    {% csrf_token %}
     {% if hoster %}
-        {% if not task %}
+        <div id="hoster" data-name="{{ hoster.name }}">
-
+            <div class="alert alert-danger" role="alert" id="error"></div>
-            {% if hoster_error %}
+            <noscript>
-                <div class="alert alert-danger" role="alert">
+                <h2>Please enable Javascript to propose your edit.</h2>
-                    <p>{{ hoster_error }}</p>
+            </noscript>
-                </div>
+            <div class="hoster-state" data-state="checking">
-            {% endif %}
+                <h2>Sign in with {{ hoster.title }}</h2>
-            {% if hoster_state == 'logged_in' %}
+                <p><img src="{% static 'img/loader.gif' %}"></p>
+                <p><em>Checking authentication, please wait…</em></p>
+            </div>
+            <div class="hoster-state" data-state="logged_out">
+                <h2>Sign in with {{ hoster.title }}</h2>
+                <p>Please sign in to continue and propose your edit.</p>
+                <p>
+                    <button class="btn btn-lg btn-primary" data-oauth>Sign in with {{ hoster.title }}</button><br>
+                    <small><em>{{ hoster.name }} – {{ hoster.base_url }}</em></small>
+                </p>
+            </div>
+            <div class="hoster-state" data-state="missing_permissions">
+                <h2>Missing {{ hoster.title }} Permissions</h2>
+                <p>c3nav is missing permissions that it needs to propose your edit.</p>
+                <p>Please click the button below to grant the missing permissions.</p>
+                <p>
+                    <button class="btn btn-lg btn-primary" data-oauth>Sign in with {{ hoster.title }}</button><br>
+                    <small><em>{{ hoster.name }} – {{ hoster.base_url }}</em></small>
+                </p>
+            </div>
+            <div class="hoster-state" data-state="oauth">
+                <h2>Redirecting…</h2>
+                <p><img src="{% static 'img/loader.gif' %}"></p>
+                <p><em>You will be redirected to {{ hoster.title }}…</em></p>
+            </div>
+            <div class="hoster-state" data-state="logged_in">
                 <h2>Propose Changes</h2>
                 <p>Please provide a short helpful title for your change.</p>
-                <form action="{% url 'editor.finalize' %}" method="POST">
+                <p>
-                    {% csrf_token %}
+                    <input class="form-control" id="commit_msg" maxlength="100" type="text" value="{{ commit_msg }}">
-                    <input type="hidden" name="data" value="{{ data }}">
+                </p>
-                    <input type="hidden" name="action" value="submit">
+                <p>
-                    <input type="hidden" name="editor_submit_token" value="{{ editor_submit_token }}">
+                    <button class="btn btn-lg btn-primary" data-commit>Create Pull Request</button><br>
-                    {% bootstrap_form commit_form %}
+                    <small><em>
-
+                        {{ hoster.name }} – {{ hoster.base_url }}
-                    {% buttons %}
+                    </em></small>
-                        <button type="submit" class="btn btn-lg btn-primary">Create Pull Request</button><br>
+                </p>
-                        <small><em>
+            </div>
-                            {{ hoster.name }} – {{ hoster.base_url }}
+            <div class="hoster-state" data-state="progress">
-                        </em></small>
+                <h2>Proposing Changes…</h2>
-                    {% endbuttons %}
+                <p><img src="{% static 'img/loader.gif' %}"></p>
-                </form>
+                <p><em>Proposing your changes, please wait…</em></p>
-
+            </div>
-            {% elif hoster_state == 'checking' %}
+            <div class="hoster-state" data-state="done">
-                <h2>Sign in with {{ hoster.title }}</h2>
+                <h2>Proposing Changes…</h2>
-                <form action="{% url 'editor.finalize' %}" method="POST" data-check-hoster="{{ hoster.name }}">
+                <p><img src="{% static 'img/loader.gif' %}"></p>
-                    {% csrf_token %}
+                <p><em>Proposing your changes, please wait…</em></p>
-                    <input type="hidden" name="data" value="{{ data }}">
+            </div>
-                    <p><img src="{% static 'img/loader.gif' %}"></p>
-                    <p><em>Checking authentication, please wait…</em></p>
-                </form>
-
-            {% else %}
-                {% if hoster_state == 'misssing_permissions' %}
-                    <h2>Missing {{ hoster.title }} Permissions</h2>
-                    <p>c3nav is missing permissions that it needs to propose your edit.</p>
-                    <p>Please click the button below to grant the missing permissions.</p>
-                {% else %}
-                    <h2>Sign in with {{ hoster.title }}</h2>
-                    <p>Please sign in to continue and propose your edit.</p>
-                {% endif %}
-                <form action="{% url 'editor.finalize' %}" method="POST">
-                    {% csrf_token %}
-                    <input type="hidden" name="data" value="{{ data }}">
-                    <input type="hidden" name="action" value="oauth">
-                    <p>
-                        <button type="submit" class="btn btn-lg btn-primary">Sign in with {{ hoster.title }}</button><br>
-                        <small><em>
-                            {{ hoster.name }} – {{ hoster.base_url }}
-                        </em></small>
-                    </p>
-                </form>
-            {% endif %}
             <p>Alternatively, you can copy your edit below and send it to the maps maintainer.</p>
-        {% else %}
+        </div>
-            {% if not task.ready or redirect %}
+     {% else %}
-                <h2>Creating Pull Request…</h2>
-                <form action="{% url 'editor.finalize' %}" method="POST"{% if redirect %} name="redirect"{% else %} data-check-task="{{ task.id }}"{% endif %}>
-                    {% csrf_token %}
-                    <input type="hidden" name="data" value="{{ data }}">
-                    <input type="hidden" name="action" value="result">
-                    <input type="hidden" name="task" value="{{ task.id }}">
-                    <p><img src="{% static 'img/loader.gif' %}"></p>
-                    <p><em>Creating Pull Request…</em></p>
-                </form>
-            {% else %}
-                <h2>Pull Request created</h2>
-                <p>You can find it here:</p>
-            {% endif %}
-        {% endif %}
-    {% else %}
         <h2>Copy your edit</h2>
         <p>In order to propose your edit, please copy it and send it to the maps maintainer.</p>
         <p><em>You are seeing this message because there is no hoster defined for this map package.</em></p>

src/c3nav/editor/templates/editor/finalize_redirect.html

@@ -0,0 +1,9 @@
+{% extends 'editor/base.html' %}
+{% load static %}
+{% block content %}
+<form action="{% url 'editor.finalize' %}" method="POST" id="finalize-redirect">
+    {% csrf_token %}
+    <img src="{% static 'img/loader.gif' %}">
+    Redirecting…
+</form>
+{% endblock %}

src/c3nav/editor/views.py

@@ -1,16 +1,12 @@
-import string
-
 from django.conf import settings
 from django.core import signing
 from django.core.exceptions import PermissionDenied, SuspiciousOperation
+from django.core.signing import BadSignature
 from django.http.response import Http404
-from django.shortcuts import get_object_or_404, redirect, render
+from django.shortcuts import get_object_or_404, render
-from django.utils.crypto import get_random_string
-from django.views.decorators.http import require_POST
 
-from c3nav.editor.forms import CommitForm, FeatureForm
+from c3nav.editor.forms import FeatureForm
 from c3nav.editor.hosters import get_hoster_for_package, hosters
-from c3nav.editor.tasks import submit_edit_task
 from c3nav.mapdata.models.feature import FEATURE_TYPES, Feature
 from c3nav.mapdata.models.package import Package
 from c3nav.mapdata.packageio.write import json_encode
@@ -100,12 +96,18 @@ def edit_feature(request, feature_type=None, name=None):
     })
 
 
-@require_POST
 def finalize(request):
+    if request.method != 'POST':
+        return render(request, 'editor/finalize_redirect.html', {})
+
     if 'data' not in request.POST:
         raise SuspiciousOperation('Missing data.')
     raw_data = request.POST['data']
-    data = signing.loads(raw_data)
+
+    try:
+        data = signing.loads(raw_data)
+    except BadSignature:
+        raise SuspiciousOperation('Bad Signature.')
 
     if data['type'] != 'editor.edit':
         raise SuspiciousOperation('Wrong data type.')
@@ -115,54 +117,15 @@ def finalize(request):
     if package is not None:
         hoster = get_hoster_for_package(package)
 
-    action = request.POST.get('action')
+    hoster.check_state(request)
-
-    if 'commit_msg' in request.POST or action == 'submit':
-        form = CommitForm(request.POST)
-    else:
-        form = CommitForm({'commit_msg': data['commit_msg']})
-
-    task = None
-    new_submit_token = False
-    if action == 'check':
-        hoster.check_state(request)
-    elif action == 'oauth':
-        hoster.set_tmp_data(request, raw_data)
-        return redirect(hoster.get_auth_uri(request))
-    elif action == 'submit' and hoster.get_state(request) == 'logged_in':
-        if request.POST.get('editor_submit_token', '') != request.session.get('editor_submit_token', None):
-            raise SuspiciousOperation('Invalid submit token.')
-        if form.is_valid():
-            new_submit_token = True
-            data['commit_msg'] = form.cleaned_data['commit_msg']
-            task = hoster.submit_edit(request, data)
-    elif action == 'result':
-        if 'task' not in request.POST:
-            raise SuspiciousOperation('Missing task id.')
-        task = submit_edit_task.AsyncResult(task_id=request.POST['task'])
-        try:
-            task.ready()
-        except:
-            raise Http404()
-
-    if 'editor_submit_token' not in request.session or new_submit_token:
-        request.session['editor_submit_token'] = get_random_string(42, string.ascii_letters + string.digits)
-
-    hoster_state = hoster.get_state(request)
-    hoster_error = hoster.get_error(request) if hoster_state == 'logged_out' else None
 
     return render(request, 'editor/finalize.html', {
+        'hoster': hoster,
         'data': raw_data,
         'action': data['action'],
         'commit_id': data['commit_id'],
-        'commit_form': form,
+        'commit_msg': data['commit_msg'],
         'package_name': data['package_name'],
-        'hoster': hoster,
-        'hoster_state': hoster_state,
-        'hoster_error': hoster_error,
-        'redirect': action == 'submit' and not settings.CELERY_ALWAYS_EAGER,
-        'editor_submit_token': request.session['editor_submit_token'],
-        'task': {'id': task.id, 'ready': task.ready(), 'result': task.result} if task is not None else None,
         'file_path': data['file_path'],
         'file_contents': data.get('content')
     })
@@ -173,7 +136,6 @@ def oauth_callback(request, hoster):
     if hoster is None:
         raise Http404
 
-    data = hoster.get_tmp_data(request)
     hoster.handle_callback_request(request)
 
-    return render(request, 'editor/oauth_callback.html', {'data': data})
+    return render(request, 'editor/finalize_redirect.html', {})

src/c3nav/settings.py

@@ -180,6 +180,9 @@ REST_FRAMEWORK = {
     'DEFAULT_VERSIONING_CLASS': 'rest_framework.versioning.URLPathVersioning',
     'ALLOWED_VERSIONS': ['v1'],
     'DEFAULT_VERSION': 'v1',
+    'DEFAULT_AUTHENTICATION_CLASSES': (
+        'c3nav.api.authentication.ForceCSRFCheckSessionAuthentication',
+    ),
     'DEFAULT_PERMISSION_CLASSES': (
         'c3nav.mapdata.permissions.LockedMapFeatures',
     ),