Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TokenAuthentication: Allow custom keyword in the header #4097

Merged
merged 2 commits into from May 4, 2016

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented May 3, 2016

This allows subclassing TokenAuthentication and setting custom keyword,
thus allowing the Authorization header to be for example:

Bearer 956e252a-513c-48c5-92dd-bfddc364e812

It doesn't change the behavior of TokenAuthentication itself,
it simply allows to reuse the logic of TokenAuthentication without
the need of copy pasting the class and changing one hardcoded string.

Related: #4080

@codecov-io
Copy link

codecov-io commented May 3, 2016

Current coverage is 91.39%

Merging #4097 into master will increase coverage by +<.01%

@@             master      #4097   diff @@
==========================================
  Files            51         51          
  Lines          5472       5473     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5001       5002     +1   
  Misses          471        471          
  Partials          0          0          

Powered by Codecov. Last updated by 37b1ce5...c7c7726

@hroncok hroncok force-pushed the keywordtoken branch 2 times, most recently from 6360db2 to a4c8e56 Compare May 3, 2016
@tomchristie
Copy link
Member

tomchristie commented May 4, 2016

This isn't unreasonable, but I'd still rather just have end-users customize the class if they want differing behavior.

@tomchristie tomchristie closed this May 4, 2016
@hroncok
Copy link
Contributor Author

hroncok commented May 4, 2016

I don't understand the objection against this, so I'll try to explain my reason even further.

This is the customization I would like to be able to do (In my own code/project/app) and this pull request makes that possible:

from rest_framework import authentication


class BearerAuthentication(authentication.TokenAuthentication):
    '''
    Simple token based authentication using utvsapitoken.

    Clients should authenticate by passing the token key in the 'Authorization'
    HTTP header, prepended with the string 'Bearer '.  For example:

        Authorization: Bearer 956e252a-513c-48c5-92dd-bfddc364e812
    '''
    keyword = 'Bearer'

Unfortunately, this is what has to be done at this point (without this pull request):

from rest_framework import authentication, exceptions
from django.utils.translation import ugettext_lazy as _


class BearerAuthentication(authentication.TokenAuthentication):
    '''
    Simple token based authentication using utvsapitoken.

    Clients should authenticate by passing the token key in the 'Authorization'
    HTTP header, prepended with the string 'Bearer '.  For example:

        Authorization: Bearer 956e252a-513c-48c5-92dd-bfddc364e812
    '''
    def authenticate(self, request):
        auth = authentication.get_authorization_header(request).split()

        if not auth or auth[0].lower() != b'bearer':
            return None

        if len(auth) == 1:
            msg = _('Invalid token header. No credentials provided.')
            raise exceptions.AuthenticationFailed(msg)
        elif len(auth) > 2:
            msg = _('Invalid token header. Token string should not contain spaces.')
            raise exceptions.AuthenticationFailed(msg)

        try:
            token = auth[1].decode()
        except UnicodeError:
            msg = _('Invalid token header. Token string should not contain invalid characters.')
            raise exceptions.AuthenticationFailed(msg)

        return self.authenticate_credentials(token)

    def authenticate_header(self, request):
        return 'Bearer'

This is no customization, this is copy pasting code form Django REST framework.
This has many disadvantages, every time the code is changed in DRF (let's say for security fixes etc.), I would need to keep tack of such change and change my copy pasted code.

This PR simply allows easier customization. Would you please reconsider, or provide more verbose explanation? I'm willing to shape this PR a bit (make keyword a method, rename it, etc.). Thanks

@xordoquy
Copy link
Collaborator

xordoquy commented May 4, 2016

I'd join in this request on the principle.
This change doesn't make things harder to maintain and will ease overriding the class.

@@ -150,6 +150,7 @@ class TokenAuthentication(BaseAuthentication):
Authorization: Token 401f7ac837da42b97f613d789819ff93537bee6a
"""

keyword = 'token'
Copy link
Member

@tomchristie tomchristie May 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest we use the proper casing here, and instead do .lower() in the comparison against the auth header.

Copy link
Contributor Author

@hroncok hroncok May 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I was considering that as well, don't have strong opinion. Will do.

Copy link
Contributor Author

@hroncok hroncok May 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@tomchristie
Copy link
Member

tomchristie commented May 4, 2016

Given @xordoquy's take let's reopen and have this in.

I don't understand the objection against this.

In the absence of any other factors the tendency is always for classes such as this to accumulate more and more interface. I'm generally in favor of pushing back against that with "put some code in your codebase" wherever possible.

@tomchristie tomchristie reopened this May 4, 2016
hroncok added 2 commits May 4, 2016
This allows subclassing TokenAuthentication and setting custom keyword,
thus allowing the Authorization header to be for example:

    Bearer 956e252a-513c-48c5-92dd-bfddc364e812

It doesn't change the behavior of TokenAuthentication itself,
it simply allows to reuse the logic of TokenAuthentication without
the need of copy pasting the class and changing one hardcoded string.

Related: encode#4080
@tomchristie tomchristie added this to the 3.3.4 Release milestone May 4, 2016
@hroncok
Copy link
Contributor Author

hroncok commented May 4, 2016

I can see. Well, I'm replacing the token validation with my own logic, so I'm definitely putting some code in my own codebase, I was simply against copy pasting the parts that should be inheritable.

@tomchristie tomchristie merged commit ffdac0d into encode:master May 4, 2016
@tomchristie
Copy link
Member

tomchristie commented May 4, 2016

👍 S'all good. Thanks @hroncok, @xordoquy, @jpocentek.

@hroncok
Copy link
Contributor Author

hroncok commented May 4, 2016

@xordoquy
Copy link
Collaborator

xordoquy commented May 4, 2016

Thanks @tomchristie for taking the time to reconsider this and thanks @hroncok for the PR.

@jpadilla
Copy link
Member

jpadilla commented May 4, 2016

I'm a bit late on the conv, but I don't really think keyword is descriptive. Specs usually call this authentication scheme.

@hroncok
Copy link
Contributor Author

hroncok commented May 4, 2016

@kevin-brown
Copy link
Member

kevin-brown commented May 4, 2016

I'm a bit late on the conv, but I don't really think keyword is descriptive.

I'm going to second this sentiment.

In the HTTP spec, this is usually referred to as the "scheme" [1, 2, 3]. My only problem with using "keyword" is that it's only slightly more descriptive than "thing" or "indicator".

@hroncok
Copy link
Contributor Author

hroncok commented May 4, 2016

You are quite right. Shall I craft another PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants