From 6753907df51d6dc370bc21af4d817c312e3f288d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laura=20Kl=C3=BCnder?= Date: Wed, 5 Oct 2016 18:20:06 +0200 Subject: [PATCH] refactor/improve /editor/finalize with more JS and REST --- src/c3nav/api/authentication.py | 12 ++ src/c3nav/api/urls.py | 3 +- src/c3nav/editor/api.py | 83 +++++++++++- src/c3nav/editor/hosters/base.py | 15 --- src/c3nav/editor/serializers.py | 27 ++++ .../editor/static/editor/css/finalize.css | 3 + src/c3nav/editor/static/editor/js/editor.js | 14 -- src/c3nav/editor/static/editor/js/finalize.js | 71 ++++++++++ src/c3nav/editor/templates/editor/base.html | 18 +-- .../templates/editor/feature_success.html | 1 - .../editor/templates/editor/finalize.html | 127 ++++++++---------- .../templates/editor/finalize_redirect.html | 9 ++ src/c3nav/editor/views.py | 68 +++------- src/c3nav/settings.py | 3 + 14 files changed, 289 insertions(+), 165 deletions(-) create mode 100644 src/c3nav/api/authentication.py create mode 100644 src/c3nav/editor/static/editor/css/finalize.css create mode 100644 src/c3nav/editor/static/editor/js/finalize.js create mode 100644 src/c3nav/editor/templates/editor/finalize_redirect.html diff --git a/src/c3nav/api/authentication.py b/src/c3nav/api/authentication.py new file mode 100644 index 00000000..426b3774 --- /dev/null +++ b/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 diff --git a/src/c3nav/api/urls.py b/src/c3nav/api/urls.py index b27ae7f8..46dede36 100644 --- a/src/c3nav/api/urls.py +++ b/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 = [ diff --git a/src/c3nav/editor/api.py b/src/c3nav/editor/api.py index 06405c9c..1b1b6f8b 100644 --- a/src/c3nav/editor/api.py +++ b/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.serializers import HosterSerializer +from c3nav.editor.hosters import get_hoster_for_package, hosters +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) diff --git a/src/c3nav/editor/hosters/base.py b/src/c3nav/editor/hosters/base.py index b294fed4..8357a0aa 100644 --- a/src/c3nav/editor/hosters/base.py +++ b/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. diff --git a/src/c3nav/editor/serializers.py b/src/c3nav/editor/serializers.py index bc72ec91..6b6f9330 100644 --- a/src/c3nav/editor/serializers.py +++ b/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'] diff --git a/src/c3nav/editor/static/editor/css/finalize.css b/src/c3nav/editor/static/editor/css/finalize.css new file mode 100644 index 00000000..a849be9f --- /dev/null +++ b/src/c3nav/editor/static/editor/css/finalize.css @@ -0,0 +1,3 @@ +.hoster-state, .alert { + display:none; +} diff --git a/src/c3nav/editor/static/editor/js/editor.js b/src/c3nav/editor/static/editor/js/editor.js index b933b24b..9e851c09 100644 --- a/src/c3nav/editor/static/editor/js/editor.js +++ b/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(); diff --git a/src/c3nav/editor/static/editor/js/finalize.js b/src/c3nav/editor/static/editor/js/finalize.js new file mode 100644 index 00000000..393410d3 --- /dev/null +++ b/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($('').val(sessionStorage.getItem('finalize-data'))).submit(); +} diff --git a/src/c3nav/editor/templates/editor/base.html b/src/c3nav/editor/templates/editor/base.html index d9f7b02b..8c84d5f4 100644 --- a/src/c3nav/editor/templates/editor/base.html +++ b/src/c3nav/editor/templates/editor/base.html @@ -8,9 +8,10 @@ c3nav Map Editor {% compress css %} - - - + + + + {% endcompress %} @@ -30,11 +31,12 @@ {% compress js %} - - - - - + + + + + + {% endcompress %} diff --git a/src/c3nav/editor/templates/editor/feature_success.html b/src/c3nav/editor/templates/editor/feature_success.html index c2a9ebae..9bfadbe5 100644 --- a/src/c3nav/editor/templates/editor/feature_success.html +++ b/src/c3nav/editor/templates/editor/feature_success.html @@ -3,7 +3,6 @@
{% csrf_token %} - Redirecting…
diff --git a/src/c3nav/editor/templates/editor/finalize.html b/src/c3nav/editor/templates/editor/finalize.html index 87dda68b..05b5a8f6 100644 --- a/src/c3nav/editor/templates/editor/finalize.html +++ b/src/c3nav/editor/templates/editor/finalize.html @@ -2,80 +2,67 @@ {% load static %} {% load bootstrap3 %} {% block content %} + + {% csrf_token %} {% if hoster %} - {% if not task %} - - {% if hoster_error %} - - {% endif %} - {% if hoster_state == 'logged_in' %} +
+ + +
+

Sign in with {{ hoster.title }}

+

+

Checking authentication, please wait…

+
+
+

Sign in with {{ hoster.title }}

+

Please sign in to continue and propose your edit.

+

+
+ {{ hoster.name }} – {{ hoster.base_url }} +

+
+
+

Missing {{ hoster.title }} Permissions

+

c3nav is missing permissions that it needs to propose your edit.

+

Please click the button below to grant the missing permissions.

+

+
+ {{ hoster.name }} – {{ hoster.base_url }} +

+
+
+

Redirecting…

+

+

You will be redirected to {{ hoster.title }}…

+
+

Propose Changes

Please provide a short helpful title for your change.

-
- {% csrf_token %} - - - - {% bootstrap_form commit_form %} - - {% buttons %} -
- - {{ hoster.name }} – {{ hoster.base_url }} - - {% endbuttons %} -
- - {% elif hoster_state == 'checking' %} -

Sign in with {{ hoster.title }}

-
- {% csrf_token %} - -

-

Checking authentication, please wait…

-
- - {% else %} - {% if hoster_state == 'misssing_permissions' %} -

Missing {{ hoster.title }} Permissions

-

c3nav is missing permissions that it needs to propose your edit.

-

Please click the button below to grant the missing permissions.

- {% else %} -

Sign in with {{ hoster.title }}

-

Please sign in to continue and propose your edit.

- {% endif %} -
- {% csrf_token %} - - -

-
- - {{ hoster.name }} – {{ hoster.base_url }} - -

-
- {% endif %} +

+ +

+

+
+ + {{ hoster.name }} – {{ hoster.base_url }} + +

+
+
+

Proposing Changes…

+

+

Proposing your changes, please wait…

+
+
+

Proposing Changes…

+

+

Proposing your changes, please wait…

+

Alternatively, you can copy your edit below and send it to the maps maintainer.

- {% else %} - {% if not task.ready or redirect %} -

Creating Pull Request…

-
- {% csrf_token %} - - - -

-

Creating Pull Request…

-
- {% else %} -

Pull Request created

-

You can find it here:

- {% endif %} - {% endif %} - {% else %} +
+ {% else %}

Copy your edit

In order to propose your edit, please copy it and send it to the maps maintainer.

You are seeing this message because there is no hoster defined for this map package.

diff --git a/src/c3nav/editor/templates/editor/finalize_redirect.html b/src/c3nav/editor/templates/editor/finalize_redirect.html new file mode 100644 index 00000000..e365af54 --- /dev/null +++ b/src/c3nav/editor/templates/editor/finalize_redirect.html @@ -0,0 +1,9 @@ +{% extends 'editor/base.html' %} +{% load static %} +{% block content %} +
+ {% csrf_token %} + + Redirecting… +
+{% endblock %} diff --git a/src/c3nav/editor/views.py b/src/c3nav/editor/views.py index 3119210a..fde66f6f 100644 --- a/src/c3nav/editor/views.py +++ b/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.utils.crypto import get_random_string -from django.views.decorators.http import require_POST +from django.shortcuts import get_object_or_404, render -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') - - 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 + hoster.check_state(request) 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', {}) diff --git a/src/c3nav/settings.py b/src/c3nav/settings.py index cc32ad4a..643a149a 100644 --- a/src/c3nav/settings.py +++ b/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', ),