[RFC PATCH] REST: enable token authentication

[Philippe Pepiot]

On 05/25/2017 10:47 AM, Andrew Donnellan wrote:
> Token authentication is generally viewed as a more secure option for API
> authentication than storing a username and password.
> 
> Django REST Framework gives us a TokenAuthentication class and an authtoken
> app that we can use to generate random tokens and authenticate to API
> endpoints.
> 
> Enable DRF's token support and add options to the user profile view to view
> or regenerate tokens.
> 
> Signed-off-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>
> 
> ---
> 
> This is an RFC; I haven't written any tests or documentation, UI's a bit
> ugly, need to split patches.
> ---

Hi Andrew,

Thanks for adding this ! Aside my comments below, I tested a bit and it
worked fine.


>  patchwork/settings/base.py                 |  6 ++++++
>  patchwork/signals.py                       | 10 ++++++++++
>  patchwork/templates/patchwork/profile.html | 23 +++++++++++++++++++++--
>  patchwork/urls.py                          |  4 ++++
>  patchwork/views/bundle.py                  | 12 ++++++++----
>  patchwork/views/user.py                    | 19 +++++++++++++++++++
>  6 files changed, 68 insertions(+), 6 deletions(-)
> 
> diff --git a/patchwork/settings/base.py b/patchwork/settings/base.py
> index 26c75c9..6fd98a7 100644
> --- a/patchwork/settings/base.py
> +++ b/patchwork/settings/base.py
> @@ -143,6 +143,7 @@ try:
>  
>      INSTALLED_APPS += [
>          'rest_framework',
> +        'rest_framework.authtoken',
>          'django_filters',
>      ]
>  except ImportError:
> @@ -158,6 +159,11 @@ REST_FRAMEWORK = {
>          'rest_framework.filters.SearchFilter',
>          'rest_framework.filters.OrderingFilter',
>      ),
> +    'DEFAULT_AUTHENTICATION_CLASSES': (
> +        'rest_framework.authentication.SessionAuthentication',
> +        'rest_framework.authentication.BasicAuthentication',
> +        'rest_framework.authentication.TokenAuthentication',
> +    ),
>      'SEARCH_PARAM': 'q',
>      'ORDERING_PARAM': 'order',
>  }
> diff --git a/patchwork/signals.py b/patchwork/signals.py
> index 208685c..f335525 100644
> --- a/patchwork/signals.py
> +++ b/patchwork/signals.py
> @@ -19,6 +19,7 @@
>  
>  from datetime import datetime as dt
>  
> +from django.conf import settings
>  from django.db.models.signals import post_save
>  from django.db.models.signals import pre_save
>  from django.dispatch import receiver
> @@ -239,3 +240,12 @@ def create_series_completed_event(sender, instance, created, **kwargs):
>  
>      if instance.series.received_all:
>          create_event(instance.series)
> +
> +
> +if settings.ENABLE_REST_API:
> +    from rest_framework.authtoken.models import Token
> +    @receiver(post_save, sender=settings.AUTH_USER_MODEL)
> +    def create_user_created_event(sender, instance=None, created=False,
> +                                  **kwargs):
> +        if created:
> +            Token.objects.create(user=instance)
> diff --git a/patchwork/templates/patchwork/profile.html b/patchwork/templates/patchwork/profile.html
> index f976195..c7be044 100644
> --- a/patchwork/templates/patchwork/profile.html
> +++ b/patchwork/templates/patchwork/profile.html
> @@ -133,8 +133,27 @@ address.</p>
>  </div>
>  
>  <div class="box">
> -<h2>Authentication</h2>
> -<a href="{% url 'password_change' %}">Change password</a>
> +  <h2>Authentication</h2>
> +  <form method="post" action="{%url 'generate_token' %}">
> +    {% csrf_token %}
> +    <table>
> +      <tr>
> +	<th>Password:</th>
> +	<td><a href="{% url 'password_change' %}">Change password</a>
> +      </tr>
> +      <tr>
> +	<th>API Token:</th>
> +	<td>
> +	  {% if api_token %}
> +	  {{ api_token }}
> +	  <input type="submit" value="Regenerate token" />
> +	  {% else %}
> +	  <input type="submit" value="Generate token" />
> +	  {% endif %}
> +	</td>
> +      </tr>
> +    </table>
> +  </form>
>  </div>
>  
>  </div>
> diff --git a/patchwork/urls.py b/patchwork/urls.py
> index 1871c9a..aa49b4d 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -101,6 +101,10 @@ urlpatterns = [
>          auth_views.password_reset_complete,
>          name='password_reset_complete'),
>  
> +    # token change
> +    url(r'^user/generate-token/$', user_views.generate_token,
> +        name='generate_token'),
> +
>      # login/logout
>      url(r'^user/login/$', auth_views.login,
>          {'template_name': 'patchwork/login.html'},
> diff --git a/patchwork/views/bundle.py b/patchwork/views/bundle.py
> index 387b7c6..bb331f4 100644
> --- a/patchwork/views/bundle.py
> +++ b/patchwork/views/bundle.py
> @@ -36,17 +36,21 @@ from patchwork.views import generic_list
>  from patchwork.views.utils import bundle_to_mbox
>  
>  if settings.ENABLE_REST_API:
> -    from rest_framework.authentication import BasicAuthentication  # noqa
> +    from rest_framework.authentication import SessionAuthentication
>      from rest_framework.exceptions import AuthenticationFailed
> +    from rest_framework.settings import api_settings
>  
>  
>  def rest_auth(request):
>      if not settings.ENABLE_REST_API:
>          return request.user
>      try:
> -        auth_result = BasicAuthentication().authenticate(request)
> -        if auth_result:
> -            return auth_result[0]
> +        for auth in api_settings.DEFAULT_AUTHENTICATION_CLASSES:
> +            if auth == SessionAuthentication:
> +                continue
> +            auth_result = auth().authenticate(request)
> +            if auth_result:
> +                return auth_result[0]
>      except AuthenticationFailed:
>          pass


I think the try/except block should be inside the loop when calling
authenticate().

>      return request.user
> diff --git a/patchwork/views/user.py b/patchwork/views/user.py
> index 375d3d9..53e2ea8 100644
> --- a/patchwork/views/user.py
> +++ b/patchwork/views/user.py
> @@ -42,6 +42,8 @@ from patchwork.models import Project
>  from patchwork.models import State
>  from patchwork.views import generic_list
>  
> +if settings.ENABLE_REST_API:
> +    from rest_framework.authtoken.models import Token
>  
>  def register(request):
>      context = {}
> @@ -126,6 +128,11 @@ def profile(request):
>          .extra(select={'is_optout': optout_query})
>      context['linked_emails'] = people
>      context['linkform'] = EmailForm()
> +    if settings.ENABLE_REST_API:
> +        try:
> +            context['api_token'] = Token.objects.get(user=request.user)
> +        except Token.DoesNotExist:
> +            pass
>  
>      return render(request, 'patchwork/profile.html', context)
>  
> @@ -232,3 +239,15 @@ def todo_list(request, project_id):
>      context['action_required_states'] = \
>          State.objects.filter(action_required=True).all()
>      return render(request, 'patchwork/todo-list.html', context)
> +
> + at login_required
> +def generate_token(request):
> +    if not settings.ENABLE_REST_API:
> +        raise RuntimeError('REST API not enabled')

What about disabling the url in patchwork/urls.py instead ?

> +    try:
> +        t = Token.objects.get(user=request.user)
> +        t.delete()
> +    except Token.DoesNotExist:
> +        pass

This block could be replaced by:

Token.objects.filter(user=request.user).delete()


> +    Token.objects.create(user=request.user)
> +    return HttpResponseRedirect(reverse('user-profile'))
>