Re: [RFC PATCH] REST: enable token authentication

[Stephen Finucane]

On Thu, 2017-05-25 at 18:47 +1000, 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 

Yup, totally onboard with this, even for 2.0. I'd be in favor of
removing Basic Auth if we add this. I can take documentation if you
want to handle the other items. I imagine tests shouldn't be too
arduous either.

Think this is something you could wire up in the next few days? :)

Stephen

> ---
> 
> This is an RFC; I haven't written any tests or documentation, UI's a
> bit
> ugly, need to split patches.
> ---
>  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.
>  
>  
>  
> -Authentication
> -Change password
> +  Authentication
> +  
> +{% csrf_token %}
> +
> +  
> + Password:
> + Change
> password
> +  
> +  
> + API Token:
> + 
> +   {% if api_token %}
> +   {{ api_token }}
> +   
> +   {% else %}
> +   
> +   {% endif %}
> + 
> +  
> +
> +  
>  
>  
>  
> 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:
>  

[RFC PATCH] REST: enable token authentication

[Andrew Donnellan]

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 

---

This is an RFC; I haven't written any tests or documentation, UI's a bit
ugly, need to split patches.
---
 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.
 
 
 
-Authentication
-Change password
+  Authentication
+  
+{% csrf_token %}
+
+  
+   Password:
+   Change password
+  
+  
+   API Token:
+   
+ {% if api_token %}
+ {{ api_token }}
+ 
+ {% else %}
+ 
+ {% endif %}
+   
+  
+
+  
 
 
 
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
 return request.user
diff --git a/patchwork/views/user.py b/patchwork/views/user.py
index 375d3d9..53e2ea8 100644
--- a/patchwork/views/user.py
+++ 

[PATCH] docs/api: change POST to PATCH in REST API parameters example

[Andrew Donnellan]

api/rest.rst gives an example of how to POST parameters to the PatchDetail
view at api/patches/. However, the endpoint in question doesn't
support POST - you need to use PUT or PATCH. Change it to PATCH.

Signed-off-by: Andrew Donnellan 

---

I have no idea whether it's *meant* to support POST...
---
 docs/api/rest.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/api/rest.rst b/docs/api/rest.rst
index 1750913..8c8fd95 100644
--- a/docs/api/rest.rst
+++ b/docs/api/rest.rst
@@ -155,7 +155,7 @@ parameters should be passed as form-encoded data:
 
 .. code-block:: shell
 
-$ curl -X POST -F 'state=under-review' \
+$ curl -X PATCH -F 'state=under-review' \
   'https://patchwork.example.com/api/patches/123'
 
 Authentication
-- 
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

[PATCH 1/2] bundle: Fix use of basic auth for bundle mboxes

[Andrew Donnellan]

Commit 0b4f508a8438 ("views: Allow use of basic auth for bundle mboxes")
added support for using Django REST Framework's BasicAuthentication to
authenticate when accessing the bundle-mbox view.

To check the user's credentials, we call
BasicAuthentication.authenticate(), however, we don't check whether
the returned user is actually the bundle owner. This means that any user
can access any private bundle if they authenticate using basic
authentication.

Additionally, if invalid credentials are provided via a basic
authentication header, BasicAuthentication.authenticate() will throw an
AuthenticationFailed exception. We currently don't catch this, resulting in
an exception page being displayed rather than a 404.

Add a new helper, rest_auth(), that takes a request and returns a user.
Call this in bundle_mbox() and save the result into request.user before we
check whether request.user is actually the bundle owner.

Found by code inspection.

Fixes: 0b4f508a8438 ("views: Allow use of basic auth for bundle mboxes")
Signed-off-by: Andrew Donnellan 

---

Given this is a (very, very, very minor) security issue, this should land
in the 2.0 release.
---
 patchwork/views/bundle.py | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/patchwork/views/bundle.py b/patchwork/views/bundle.py
index dc9b74b..387b7c6 100644
--- a/patchwork/views/bundle.py
+++ b/patchwork/views/bundle.py
@@ -37,9 +37,19 @@ from patchwork.views.utils import bundle_to_mbox
 
 if settings.ENABLE_REST_API:
 from rest_framework.authentication import BasicAuthentication  # noqa
-basic_auth = BasicAuthentication()
-else:
-basic_auth = None
+from rest_framework.exceptions import AuthenticationFailed
+
+
+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]
+except AuthenticationFailed:
+pass
+return request.user
 
 
 @login_required
@@ -140,8 +150,8 @@ def bundle_mbox(request, username, bundlename):
 bundle = get_object_or_404(Bundle, owner__username=username,
name=bundlename)
 
-if not (request.user == bundle.owner or bundle.public or
-(basic_auth and basic_auth.authenticate(request))):
+request.user = rest_auth(request)
+if not (request.user == bundle.owner or bundle.public):
 return HttpResponseNotFound()
 
 response = HttpResponse(content_type='text/plain')
-- 
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

[PATCH 2/2] tests: Add tests for viewing private bundles

[Andrew Donnellan]

Add some tests to check that owners can view their private bundles while
other authenticated users can't.

Signed-off-by: Andrew Donnellan 

---

I'm not very familiar with writing Django tests, please flame away
---
 patchwork/tests/test_bundles.py | 58 +
 1 file changed, 58 insertions(+)

diff --git a/patchwork/tests/test_bundles.py b/patchwork/tests/test_bundles.py
index 0dc9165..e4082b2 100644
--- a/patchwork/tests/test_bundles.py
+++ b/patchwork/tests/test_bundles.py
@@ -19,6 +19,7 @@
 
 from __future__ import absolute_import
 
+import base64
 import datetime
 import unittest
 
@@ -283,6 +284,63 @@ class BundlePublicModifyTest(BundleTestBase):
 self.assertNotEqual(self.bundle.name, newname)
 
 
+class BundlePrivateViewTest(BundleTestBase):
+
+"""Ensure that non-owners can't view private bundles"""
+
+def setUp(self):
+super(BundlePrivateViewTest, self).setUp()
+self.bundle.public = False
+self.bundle.save()
+self.bundle.append_patch(self.patches[0])
+self.url = bundle_url(self.bundle)
+self.other_user = create_user()
+
+def test_private_bundle(self):
+# Check we can view as owner
+self.client.login(username=self.user.username,
+  password=self.user.username)
+response = self.client.get(self.url)
+self.assertEqual(response.status_code, 200)
+self.assertContains(response, self.patches[0].name)
+
+# Check we can't view as another user
+self.client.login(username=self.other_user.username,
+  password=self.other_user.username)
+response = self.client.get(self.url)
+self.assertEqual(response.status_code, 404)
+
+
+class BundlePrivateViewMboxTest(BundlePrivateViewTest):
+
+"""Ensure that non-owners can't view private bundle mboxes"""
+
+def setUp(self):
+super(BundlePrivateViewMboxTest, self).setUp()
+self.url = reverse('bundle-mbox', kwargs={
+'username': self.bundle.owner.username,
+'bundlename': self.bundle.name})
+
+def test_private_bundle_mbox_basic_auth(self):
+self.client.logout()
+
+# Check we can view as owner
+auth_string = 'Basic ' + base64.b64encode('%s:%s' %
+  (self.user.username,
+   self.user.username))
+response = self.client.get(self.url, HTTP_AUTHORIZATION=auth_string)
+
+self.assertEqual(response.status_code, 200)
+self.assertContains(response, self.patches[0].name)
+
+# Check we can't view as another user
+auth_string = 'Basic ' + base64.b64encode('%s:%s' %
+  (self.other_user.username,
+   self.other_user.username))
+response = self.client.get(self.url, HTTP_AUTHORIZATION=auth_string)
+self.assertEqual(response.status_code, 404)
+
+
 class BundleCreateFromListTest(BundleTestBase):
 
 def test_create_empty_bundle(self):
-- 
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] docker: increase database connection timeout

[Daniel Axtens]

Andrew Donnellan  writes:

> When starting the Docker environment, if the web container can't see the
> database immediately, it waits 5 seconds, tries again, then waits 15
> seconds more to account for first-time start-ups where it takes a bit
> longer for the database to be initialised.
>
> Some of us, unfortunately, have slow computers with slow mechanical hard
> drives which take just a bit longer. Increase the second timeout from 15
> seconds to 60 seconds, testing every 5 seconds.
>
> Cc: Daniel Axtens 
Acked-by: Daniel Axtens 

LGTM - thanks Andrew for the fix and the reminder we don't all have
PCI-connected NVMe :P

Regards,
Daniel
> Signed-off-by: Andrew Donnellan 
> ---
>  tools/docker/entrypoint.sh | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tools/docker/entrypoint.sh b/tools/docker/entrypoint.sh
> index 5a23fa3..949d8af 100755
> --- a/tools/docker/entrypoint.sh
> +++ b/tools/docker/entrypoint.sh
> @@ -53,8 +53,13 @@ if ! test_db_connection; then
>  sleep 5
>  if ! test_db_connection; then
>  echo "Still cannot connect to MySQL."
> -echo "Maybe you are starting the db for the first time. Waiting 15 
> seconds."
> -sleep 15
> +echo "Maybe you are starting the db for the first time. Waiting up 
> to 60 seconds."
> +for i in {0..9}; do
> +sleep 5
> +if test_db_connection; then
> +break
> +fi
> +done
>  if ! test_db_connection; then
>  echo "Still cannot connect to MySQL. Giving up."
>  echo "Are you using docker-compose? If not, have you set up the 
> link correctly?"
> -- 
> Andrew Donnellan  OzLabs, ADL Canberra
> andrew.donnel...@au1.ibm.com  IBM Australia Limited
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork