[v3,4/5] views: Allow use of basic auth for bundle mboxes

Submitted by Stephen Finucane on March 16, 2017, 10:07 p.m.

Details

Message ID 20170316220728.10817-5-stephen@that.guru
State Accepted
Headers show

Commit Message

Stephen Finucane March 16, 2017, 10:07 p.m.
API clients are going to talk using basic auth. We also need to do this
for bundles. The alternative is to provide another endpoint for bundles
in the API but that seems unnecessary.

Signed-off-by: Stephen Finucane <stephen@that.guru>
---
 patchwork/views/bundle.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Andy Doan March 17, 2017, 3:12 p.m.
On 03/16/2017 05:07 PM, Stephen Finucane wrote:
> API clients are going to talk using basic auth. We also need to do this
> for bundles. The alternative is to provide another endpoint for bundles
> in the API but that seems unnecessary.
>
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> ---
>  patchwork/views/bundle.py | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/patchwork/views/bundle.py b/patchwork/views/bundle.py
> index e717429..9f5f2b9 100644
> --- a/patchwork/views/bundle.py
> +++ b/patchwork/views/bundle.py
> @@ -19,6 +19,7 @@
>
>  from __future__ import absolute_import
>
> +from django.conf import settings
>  from django.contrib.auth.decorators import login_required
>  import django.core.urlresolvers
>  from django.http import (HttpResponse, HttpResponseRedirect,
> @@ -30,6 +31,12 @@ from patchwork.forms import BundleForm, DeleteBundleForm
>  from patchwork.models import Patch, Bundle, BundlePatch, Project
>  from patchwork.views import generic_list, patch_to_mbox, get_patch_ids
>
> +if settings.ENABLE_REST_API:
> +    from rest_framework.authentication import BasicAuthentication  # noqa
> +    basic_auth = BasicAuthentication()
> +else:
> +    basic_auth = None
> +
>
>  @login_required
>  def setbundle(request):
> @@ -193,7 +200,8 @@ def mbox(request, username, bundlename):
>      bundle = get_object_or_404(Bundle, owner__username=username,
>                                 name=bundlename)
>
> -    if not (request.user == bundle.owner or bundle.public):
> +    if not (request.user == bundle.owner or bundle.public or
> +            (basic_auth and BasicAuthentication().authenticate(request))):

This feels a little wierd since basic_auth is already instantiated 
above. Wouldn't it be more clean as:

     if not (request.user == bundle.owner or bundle.public or
             (basic_auth and basic_auth.authenticate(request))):

>          return HttpResponseNotFound()
>
>      mbox = '\n'.join([patch_to_mbox(p).as_string(True)
>
Stephen Finucane March 20, 2017, 9:50 a.m.
On Fri, 2017-03-17 at 10:12 -0500, Andy Doan wrote:
> On 03/16/2017 05:07 PM, Stephen Finucane wrote:
> > API clients are going to talk using basic auth. We also need to do
> > this
> > for bundles. The alternative is to provide another endpoint for
> > bundles
> > in the API but that seems unnecessary.
> > 
> > Signed-off-by: Stephen Finucane <stephen@that.guru>
> > ---
> >  patchwork/views/bundle.py | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/patchwork/views/bundle.py b/patchwork/views/bundle.py
> > index e717429..9f5f2b9 100644
> > --- a/patchwork/views/bundle.py
> > +++ b/patchwork/views/bundle.py
> > @@ -19,6 +19,7 @@
> > 
> >  from __future__ import absolute_import
> > 
> > +from django.conf import settings
> >  from django.contrib.auth.decorators import login_required
> >  import django.core.urlresolvers
> >  from django.http import (HttpResponse, HttpResponseRedirect,
> > @@ -30,6 +31,12 @@ from patchwork.forms import BundleForm,
> > DeleteBundleForm
> >  from patchwork.models import Patch, Bundle, BundlePatch, Project
> >  from patchwork.views import generic_list, patch_to_mbox,
> > get_patch_ids
> > 
> > +if settings.ENABLE_REST_API:
> > +    from rest_framework.authentication import
> > BasicAuthentication  # noqa
> > +    basic_auth = BasicAuthentication()
> > +else:
> > +    basic_auth = None
> > +
> > 
> >  @login_required
> >  def setbundle(request):
> > @@ -193,7 +200,8 @@ def mbox(request, username, bundlename):
> >      bundle = get_object_or_404(Bundle, owner__username=username,
> >                                 name=bundlename)
> > 
> > -    if not (request.user == bundle.owner or bundle.public):
> > +    if not (request.user == bundle.owner or bundle.public or
> > +            (basic_auth and
> > BasicAuthentication().authenticate(request))):
> 
> This feels a little wierd since basic_auth is already instantiated 
> above. Wouldn't it be more clean as:

It would and that was my intention. I'll fix when I merge.

Happy with this otherwise?

Stephen

>      if not (request.user == bundle.owner or bundle.public or
>              (basic_auth and basic_auth.authenticate(request))):
> 
> >          return HttpResponseNotFound()
> > 
> >      mbox = '\n'.join([patch_to_mbox(p).as_string(True)
> > 
> 
>
Andy Doan March 20, 2017, 2:02 p.m.
On 03/20/2017 04:50 AM, Stephen Finucane wrote:
> It would and that was my intention. I'll fix when I merge.
>
> Happy with this otherwise?

Yep:

Reviewed-by: Andy Doan <andy.doan@linaro.org>

Patch hide | download patch | download mbox

diff --git a/patchwork/views/bundle.py b/patchwork/views/bundle.py
index e717429..9f5f2b9 100644
--- a/patchwork/views/bundle.py
+++ b/patchwork/views/bundle.py
@@ -19,6 +19,7 @@ 
 
 from __future__ import absolute_import
 
+from django.conf import settings
 from django.contrib.auth.decorators import login_required
 import django.core.urlresolvers
 from django.http import (HttpResponse, HttpResponseRedirect,
@@ -30,6 +31,12 @@  from patchwork.forms import BundleForm, DeleteBundleForm
 from patchwork.models import Patch, Bundle, BundlePatch, Project
 from patchwork.views import generic_list, patch_to_mbox, get_patch_ids
 
+if settings.ENABLE_REST_API:
+    from rest_framework.authentication import BasicAuthentication  # noqa
+    basic_auth = BasicAuthentication()
+else:
+    basic_auth = None
+
 
 @login_required
 def setbundle(request):
@@ -193,7 +200,8 @@  def mbox(request, username, bundlename):
     bundle = get_object_or_404(Bundle, owner__username=username,
                                name=bundlename)
 
-    if not (request.user == bundle.owner or bundle.public):
+    if not (request.user == bundle.owner or bundle.public or
+            (basic_auth and BasicAuthentication().authenticate(request))):
         return HttpResponseNotFound()
 
     mbox = '\n'.join([patch_to_mbox(p).as_string(True)