Opened 11 years ago

Closed 11 years ago

#548 closed enhancement (fixed)

Add support for token refresh to the OAuth plugin

Reported by: nyergler Owned by:
Priority: major Milestone: 0.4.0
Component: programming Keywords: oauth review
Cc: joar, nyergler Parent Tickets:

Description

The OAuth plugin currently does not support token refresh, as described in the OAuth specification. This is needed to allow Android clients (among others) to refresh their tokens as they expire. It appears there was some planning for this, as a refresh_token field exists in the OAuthToken model, but it is currently unpopulated.

This work may overlap with work on #517.

Change History (6)

comment:1 by joar, 11 years ago

Keywords: needsreview added

I've pushed a fix for this in the oauth/refresh_tokens branch at git@github.com:joar/mediagoblin.git

  • mediagoblin/plugins/oauth/__init__.py 

    diff --git a/mediagoblin/plugins/oauth/__init__.py b/mediagoblin/plugins/oauth/__init__.py
index 4714d95..5762379 100644
    a b def setup_plugin():  
    3434    _log.debug('OAuth config: {0}'.format(config))
    3535
    3636    routes = [
    37        ('mediagoblin.plugins.oauth.authorize',
     37        ('mediagoblin.plugins.oauth.authorize',
    3838            '/oauth/authorize',
    3939            'mediagoblin.plugins.oauth.views:authorize'),
    4040        ('mediagoblin.plugins.oauth.authorize_client',

  • mediagoblin/plugins/oauth/migrations.py 

    diff --git a/mediagoblin/plugins/oauth/migrations.py b/mediagoblin/plugins/oauth/migrations.py
index 6aa0d7c..f70a2e8 100644
    a b class OAuthCode_v0(declarative_base()):  
    102102    client_id = Column(Integer, ForeignKey(OAuthClient_v0.id), nullable=False)
    103103
    104104
     105class OAuthRefreshToken_v0(declarative_base()):
     106    __tablename__ = 'oauth__refresh_tokens'
     107
     108    id = Column(Integer, primary_key=True)
     109    created = Column(DateTime, nullable=False,
     110                     default=datetime.now)
     111
     112    token = Column(Unicode, index=True)
     113
     114    user_id = Column(Integer, ForeignKey(User.id), nullable=False,
     115            index=True)
     116
     117    # XXX: Is it OK to use OAuthClient_v0.id in this way?
     118    client_id = Column(Integer, ForeignKey(OAuthClient_v0.id), nullable=False)
     119
     120
    105121@RegisterMigration(1, MIGRATIONS)
    106122def remove_and_replace_token_and_code(db):
    107123    metadata = MetaData(bind=db.bind)
    … … def remove_and_replace_token_and_code(db):  
    122138    OAuthCode_v0.__table__.create(db.bind)
    123139
    124140    db.commit()
     141
     142
     143@RegisterMigration(2, MIGRATIONS)
     144def remove_refresh_token_field(db):
     145    metadata = MetaData(bind=db.bind)
     146
     147    token_table = Table('oauth__tokens', metadata, autoload=True,
     148                        autoload_with=db.bind)
     149
     150    refresh_token = token_table.columns['refresh_token']
     151
     152    refresh_token.drop()
     153    db.commit()
     154
     155@RegisterMigration(3, MIGRATIONS)
     156def create_refresh_token_table(db):
     157    OAuthRefreshToken_v0.__table__.create(db.bind)
     158
     159    db.commit()

  • mediagoblin/plugins/oauth/models.py 

    diff --git a/mediagoblin/plugins/oauth/models.py b/mediagoblin/plugins/oauth/models.py
index 695dad3..28735dd 100644
    a b import bcrypt  
    1919
    2020from datetime import datetime, timedelta
    2121
    22 from mediagoblin.db.base import Base
    23 from mediagoblin.db.models import User
    2422
    2523from sqlalchemy import (
    2624        Column, Unicode, Integer, DateTime, ForeignKey, Enum)
    2725from sqlalchemy.orm import relationship
     26from mediagoblin.db.base import Base
     27from mediagoblin.db.models import User
     28from mediagoblin.plugins.oauth.tools import generate_identifier, \
     29    generate_secret, generate_token, generate_code, generate_refresh_token
    2830
    2931# Don't remove this, I *think* it applies sqlalchemy-migrate functionality onto
    3032# the models.
    … … class OAuthClient(Base):  
    4143    name = Column(Unicode)
    4244    description = Column(Unicode)
    4345
    44     identifier = Column(Unicode, unique=True, index=True)
    45     secret = Column(Unicode, index=True)
     46    identifier = Column(Unicode, unique=True, index=True,
     47                        default=generate_identifier)
     48    secret = Column(Unicode, index=True, default=generate_secret)
    4649
    4750    owner_id = Column(Integer, ForeignKey(User.id))
    4851    owner = relationship(User, backref='registered_clients')
    … … class OAuthClient(Base):  
    5457        u'public',
    5558        name=u'oauth__client_type'))
    5659
    57     def generate_identifier(self):
    58         self.identifier = unicode(uuid.uuid4())
    59 
    60     def generate_secret(self):
    61         self.secret = unicode(
    62                 bcrypt.hashpw(
    63                     unicode(uuid.uuid4()),
    64                     bcrypt.gensalt()))
     60    def update_secret(self):
     61        self.secret = generate_secret()
    6562
    6663    def __repr__(self):
    6764        return '<{0} {1}:{2} ({3})>'.format(
    … … class OAuthUserClient(Base):  
    7673    id = Column(Integer, primary_key=True)
    7774
    7875    user_id = Column(Integer, ForeignKey(User.id))
    79     user = relationship(User, backref='oauth_clients')
     76    user = relationship(User, backref='oauth_client_relations')
    8077
    8178    client_id = Column(Integer, ForeignKey(OAuthClient.id))
    82     client = relationship(OAuthClient, backref='users')
     79    client = relationship(OAuthClient, backref='oauth_user_relations')
    8380
    8481    state = Column(Enum(
    8582        u'approved',
    … … class OAuthToken(Base):  
    103100            default=datetime.now)
    104101    expires = Column(DateTime, nullable=False,
    105102            default=lambda: datetime.now() + timedelta(days=30))
    106     token = Column(Unicode, index=True)
    107     refresh_token = Column(Unicode, index=True)
     103    token = Column(Unicode, index=True, default=generate_token)
    108104
    109105    user_id = Column(Integer, ForeignKey(User.id), nullable=False,
    110106            index=True)
    … … class OAuthToken(Base):  
    121117                self.user,
    122118                self.client)
    123119
     120class OAuthRefreshToken(Base):
     121    __tablename__ = 'oauth__refresh_tokens'
     122
     123    id = Column(Integer, primary_key=True)
     124    created = Column(DateTime, nullable=False,
     125                     default=datetime.now)
     126
     127    token = Column(Unicode, index=True,
     128                   default=generate_refresh_token)
     129
     130    user_id = Column(Integer, ForeignKey(User.id), nullable=False,
     131            index=True)
     132
     133    user = relationship(User)
     134
     135    client_id = Column(Integer, ForeignKey(OAuthClient.id), nullable=False)
     136    client = relationship(OAuthClient)
     137
     138    def __repr__(self):
     139        return '<{0} #{1} [{3}, {4}]>'.format(
     140                self.__class__.__name__,
     141                self.id,
     142                self.user,
     143                self.client)
     144
    124145
    125146class OAuthCode(Base):
    126147    __tablename__ = 'oauth__codes'
    … … class OAuthCode(Base):  
    130151            default=datetime.now)
    131152    expires = Column(DateTime, nullable=False,
    132153            default=lambda: datetime.now() + timedelta(minutes=5))
    133     code = Column(Unicode, index=True)
     154    code = Column(Unicode, index=True, default=generate_code)
    134155
    135156    user_id = Column(Integer, ForeignKey(User.id), nullable=False,
    136157            index=True)
    … … class OAuthCode(Base):  
    150171
    151172MODELS = [
    152173        OAuthToken,
     174        OAuthRefreshToken,
    153175        OAuthCode,
    154176        OAuthClient,
    155177        OAuthUserClient]

  • mediagoblin/plugins/oauth/tools.py 

    diff --git a/mediagoblin/plugins/oauth/tools.py b/mediagoblin/plugins/oauth/tools.py
index d21c8a5..25d0977 100644
    a b  
     1# -*- coding: utf-8 -*-
    12# GNU MediaGoblin -- federated, autonomous media hosting
    23# Copyright (C) 2011, 2012 MediaGoblin contributors.  See AUTHORS.
    34#
    … …  
    1415# You should have received a copy of the GNU Affero General Public License
    1516# along with this program.  If not, see <http://www.gnu.org/licenses/>.
    1617
     18import uuid
     19import bcrypt
     20
     21from datetime import datetime
     22
    1723from functools import wraps
    1824
    19 from mediagoblin.plugins.oauth.models import OAuthClient
    2025from mediagoblin.plugins.api.tools import json_response
    2126
    2227
    2328def require_client_auth(controller):
     29    '''
     30    View decorator
     31
     32    - Requires the presence of ``?client_id``
     33    '''
     34    # Avoid circular import
     35    from mediagoblin.plugins.oauth.models import OAuthClient
     36
    2437    @wraps(controller)
    2538    def wrapper(request, *args, **kw):
    2639        if not request.GET.get('client_id'):
    … … def require_client_auth(controller):  
    4154        return controller(request, client)
    4255
    4356    return wrapper
     57
     58
     59def create_token(client, user):
     60    '''
     61    Create an OAuthToken and an OAuthRefreshToken entry in the database
     62
     63    Returns the data structure expected by the OAuth clients.
     64    '''
     65    from mediagoblin.plugins.oauth.models import OAuthToken, OAuthRefreshToken
     66
     67    token = OAuthToken()
     68    token.user = user
     69    token.client = client
     70    token.save()
     71
     72    refresh_token = OAuthRefreshToken()
     73    refresh_token.user = user
     74    refresh_token.client = client
     75    refresh_token.save()
     76
     77    # expire time of token in full seconds
     78    # timedelta.total_seconds is python >= 2.7 or we would use that
     79    td = token.expires - datetime.now()
     80    exp_in = 86400*td.days + td.seconds # just ignore µsec
     81
     82    return {'access_token': token.token, 'token_type': 'bearer',
     83            'refresh_token': refresh_token.token, 'expires_in': exp_in}
     84
     85
     86def generate_identifier():
     87    ''' Generates a ``uuid.uuid4()`` '''
     88    return unicode(uuid.uuid4())
     89
     90
     91def generate_token():
     92    ''' Uses generate_identifier '''
     93    return generate_identifier()
     94
     95
     96def generate_refresh_token():
     97    ''' Uses generate_identifier '''
     98    return generate_identifier()
     99
     100
     101def generate_code():
     102    ''' Uses generate_identifier '''
     103    return generate_identifier()
     104
     105
     106def generate_secret():
     107    '''
     108    Generate a long string of pseudo-random characters
     109    '''
     110    # XXX: We might not want it to use bcrypt, since bcrypt takes its time to
     111    # generate the result.
     112    return unicode(
     113            bcrypt.hashpw(
     114                unicode(uuid.uuid4()),
     115                bcrypt.gensalt()))
     116

  • mediagoblin/plugins/oauth/views.py 

    diff --git a/mediagoblin/plugins/oauth/views.py b/mediagoblin/plugins/oauth/views.py
index c7b2a33..ad8ea8f 100644
    a b  
    1616# along with this program.  If not, see <http://www.gnu.org/licenses/>.
    1717
    1818import logging
    19 import json
    2019
    2120from urllib import urlencode
    22 from uuid import uuid4
    23 from datetime import datetime
     21
     22from werkzeug.exceptions import BadRequest
    2423
    2524from mediagoblin.tools.response import render_to_response, redirect
    2625from mediagoblin.decorators import require_active_login
    27 from mediagoblin.messages import add_message, SUCCESS, ERROR
     26from mediagoblin.messages import add_message, SUCCESS
    2827from mediagoblin.tools.translate import pass_to_ugettext as _
    29 from mediagoblin.plugins.oauth.models import OAuthCode, OAuthToken, \
    30         OAuthClient, OAuthUserClient
     28from mediagoblin.plugins.oauth.models import OAuthCode, OAuthClient, \
     29        OAuthUserClient, OAuthRefreshToken
    3130from mediagoblin.plugins.oauth.forms import ClientRegistrationForm, \
    3231        AuthorizationForm
    33 from mediagoblin.plugins.oauth.tools import require_client_auth
     32from mediagoblin.plugins.oauth.tools import require_client_auth, \
     33        create_token
    3434from mediagoblin.plugins.api.tools import json_response
    3535
    3636_log = logging.getLogger(__name__)
    … … def register_client(request):  
    5151        client.owner_id = request.user.id
    5252        client.redirect_uri = unicode(request.form['redirect_uri'])
    5353
    54         client.generate_identifier()
    55         client.generate_secret()
    56 
    5754        client.save()
    5855
    5956        add_message(request, SUCCESS, _('The client {0} has been registered!')\
    … … def authorize_client(request):  
    9289        form.client_id.data).first()
    9390
    9491    if not client:
    95         _log.error('''No such client id as received from client authorization
    96                 form.''')
     92        _log.error('No such client id as received from client authorization \
     93form.')
    9794        return BadRequest()
    9895
    9996    if form.validate():
    … … def authorize_client(request):  
    105102        elif form.deny.data:
    106103            relation.state = u'rejected'
    107104        else:
    108             return BadRequest
     105            return BadRequest()
    109106
    110107        relation.save()
    111108
    … … def authorize(request, client):  
    136133                return json_response({
    137134                    'status': 400,
    138135                    'errors':
    139                         [u'Public clients MUST have a redirect_uri pre-set']},
     136                        [u'Public clients should have a redirect_uri pre-set.']},
    140137                        _disable_cors=True)
    141138
    142139            redirect_uri = client.redirect_uri
    … … def authorize(request, client):  
    146143            if not redirect_uri:
    147144                return json_response({
    148145                    'status': 400,
    149                     'errors': [u'Can not find a redirect_uri for client: {0}'\
    150                             .format(client.name)]}, _disable_cors=True)
     146                    'errors': [u'No redirect_uri supplied!']},
     147                    _disable_cors=True)
    151148
    152149        code = OAuthCode()
    153         code.code = unicode(uuid4())
    154150        code.user = request.user
    155151        code.client = client
    156152        code.save()
    … … def authorize(request, client):  
    180176
    181177
    182178def access_token(request):
     179    '''
     180    Access token endpoint provides access tokens to any clients that have the
     181    right grants/credentials
     182    '''
     183
     184    client = None
     185    user = None
     186
    183187    if request.GET.get('code'):
     188        # Validate the code arg, then get the client object from the db.
    184189        code = OAuthCode.query.filter(OAuthCode.code ==
    185190                request.GET.get('code')).first()
    186191
    187         if code:
    188             if code.client.type == u'confidential':
    189                 client_identifier = request.GET.get('client_id')
    190 
    191                 if not client_identifier:
    192                     return json_response({
    193                         'error': 'invalid_request',
    194                         'error_description':
    195                             'Missing client_id in request'})
    196 
    197                 client_secret = request.GET.get('client_secret')
    198 
    199                 if not client_secret:
    200                     return json_response({
    201                         'error': 'invalid_request',
    202                         'error_description':
    203                             'Missing client_secret in request'})
    204 
    205                 if not client_secret == code.client.secret or \
    206                         not client_identifier == code.client.identifier:
    207                     return json_response({
    208                         'error': 'invalid_client',
    209                         'error_description':
    210                             'The client_id or client_secret does not match the'
    211                             ' code'})
    212 
    213             token = OAuthToken()
    214             token.token = unicode(uuid4())
    215             token.user = code.user
    216             token.client = code.client
    217             token.save()
    218 
    219             # expire time of token in full seconds
    220             # timedelta.total_seconds is python >= 2.7 or we would use that
    221             td = token.expires - datetime.now()
    222             exp_in = 86400*td.days + td.seconds # just ignore µsec
    223 
    224             access_token_data = {
    225                 'access_token': token.token,
    226                 'token_type': 'bearer',
    227                 'expires_in': exp_in}
    228             return json_response(access_token_data, _disable_cors=True)
    229         else:
     192        if not code:
    230193            return json_response({
    231194                'error': 'invalid_request',
    232195                'error_description':
    233                     'Invalid code'})
    234     else:
    235         return json_response({
    236             'error': 'invalid_request',
    237             'error_descriptin':
    238                 'Missing `code` parameter in request'})
     196                    'Invalid code.'})
     197
     198        client = code.client
     199        user = code.user
     200
     201    elif request.args.get('refresh_token'):
     202        # Validate a refresh token, then get the client object from the db.
     203        refresh_token = OAuthRefreshToken.query.filter(
     204            OAuthRefreshToken.token ==
     205            request.args.get('refresh_token')).first()
     206
     207        if not refresh_token:
     208            return json_response({
     209                'error': 'invalid_request',
     210                'error_description':
     211                    'Invalid refresh token.'})
     212
     213        client = refresh_token.client
     214        user = refresh_token.user
     215
     216    if client:
     217        client_identifier = request.GET.get('client_id')
     218
     219        if not client_identifier:
     220            return json_response({
     221                'error': 'invalid_request',
     222                'error_description':
     223                    'Missing client_id in request.'})
     224
     225        if not client_identifier == client.identifier:
     226            return json_response({
     227                'error': 'invalid_client',
     228                'error_description':
     229                    'Mismatching client credentials.'})
     230
     231        if client.type == u'confidential':
     232            client_secret = request.GET.get('client_secret')
     233
     234            if not client_secret:
     235                return json_response({
     236                    'error': 'invalid_request',
     237                    'error_description':
     238                        'Missing client_secret in request.'})
     239
     240            if not client_secret == client.secret:
     241                return json_response({
     242                    'error': 'invalid_client',
     243                    'error_description':
     244                        'Mismatching client credentials.'})
     245
     246
     247        access_token_data = create_token(client, user)
     248
     249        return json_response(access_token_data, _disable_cors=True)
     250
     251    return json_response({
     252        'error': 'invalid_request',
     253        'error_description':
     254            'Missing `code` or `refresh_token` parameter in request.'})

  • mediagoblin/tests/test_oauth.py 

    diff --git a/mediagoblin/tests/test_oauth.py b/mediagoblin/tests/test_oauth.py
index 94ba5da..f036569 100644
    a b  
    1616
    1717import json
    1818import logging
     19import urllib
    1920
    2021from urlparse import parse_qs, urlparse
    2122
    2223from mediagoblin import mg_globals
    2324from mediagoblin.tools import template, pluginapi
    24 from mediagoblin.tests.tools import get_app, fixture_add_user
     25from mediagoblin.tests.tools import get_app, fixture_add_user, expect_failure
    2526
    2627
    2728_log = logging.getLogger(__name__)
    … … class TestOAuth(object):  
    7071        assert response.status_int == 200
    7172
    7273        # Should display an error
    73         assert ctx['form'].redirect_uri.errors
     74        assert len(ctx['form'].redirect_uri.errors)
    7475
    7576        # Should not pass through
    7677        assert not client
    … … class TestOAuth(object):  
    7879    def test_2_successful_public_client_registration(self):
    7980        ''' Successfully register a public client '''
    8081        self.login()
     82        uri = 'http://foo.example'
    8183        self.register_client(u'OMGOMG', 'public', 'OMG!',
    82                 'http://foo.example')
     84                uri)
    8385
    8486        client = self.db.OAuthClient.query.filter(
    8587                self.db.OAuthClient.name == u'OMGOMG').first()
    8688
     89        # redirect_uri should be set
     90        assert client.redirect_uri == uri
     91
    8792        # Client should have been registered
    8893        assert client
    8994
    … … class TestOAuth(object):  
    111116        redirect_uri = 'https://foo.example'
    112117        response = self.app.get('/oauth/authorize', {
    113118                'client_id': client.identifier,
    114                 'scope': 'admin',
     119                'scope': 'all',
    115120                'redirect_uri': redirect_uri})
    116121
    117122        # User-agent should NOT be redirected
    … … class TestOAuth(object):  
    137142        return authorization_response, client_identifier
    138143
    139144    def get_code_from_redirect_uri(self, uri):
     145        ''' Get the value of ?code= from an URI '''
    140146        return parse_qs(urlparse(uri).query)['code'][0]
    141147
    142148    def test_token_endpoint_successful_confidential_request(self):
    … … code={1}&client_secret={2}'.format(client_id, code, client.secret))  
    162168        assert type(token_data['expires_in']) == int
    163169        assert token_data['expires_in'] > 0
    164170
     171        # There should be a refresh token provided in the token data
     172        assert len(token_data['refresh_token'])
     173
     174        return client_id, token_data
     175
    165176    def test_token_endpont_missing_id_confidential_request(self):
    166177        ''' Unsuccessful request against token endpoint, missing client_id '''
    167178        code_redirect, client_id = self.test_4_authorize_confidential_client()
    … … code={0}&client_secret={1}'.format(code, client.secret))  
    181192        assert 'error' in token_data
    182193        assert not 'access_token' in token_data
    183194        assert token_data['error'] == 'invalid_request'
    184         assert token_data['error_description'] == 'Missing client_id in request'
     195        assert len(token_data['error_description'])
     196
     197    def test_refresh_token(self):
     198        ''' Try to get a new access token using the refresh token '''
     199        # Get an access token and a refresh token
     200        client_id, token_data =\
     201            self.test_token_endpoint_successful_confidential_request()
     202
     203        client = self.db.OAuthClient.query.filter(
     204            self.db.OAuthClient.identifier == client_id).first()
     205
     206        token_res = self.app.get('/oauth/access_token',
     207                     {'refresh_token': token_data['refresh_token'],
     208                      'client_id': client_id,
     209                      'client_secret': client.secret
     210                      })
     211
     212        assert token_res.status_int == 200
     213
     214        new_token_data = json.loads(token_res.body)
     215
     216        assert not 'error' in new_token_data
     217        assert 'access_token' in new_token_data
     218        assert 'token_type' in new_token_data
     219        assert 'expires_in' in new_token_data
     220        assert type(new_token_data['expires_in']) == int
     221        assert new_token_data['expires_in'] > 0

  • mediagoblin/tests/tools.py 

    diff --git a/mediagoblin/tests/tools.py b/mediagoblin/tests/tools.py
index cc4a7ad..71954e0 100644
    a b import os  
    1919import pkg_resources
    2020import shutil
    2121
     22import nose
     23
    2224from functools import wraps
    2325
    2426from paste.deploy import loadapp
    … … def fixture_media_entry(title=u"Some title", slug=None,  
    235237    entry.slug = slug
    236238    entry.uploader = uploader or fixture_add_user().id
    237239    entry.media_type = u'image'
    238    
     240
    239241    if gen_slug:
    240242        entry.generate_slug()
    241243    if save:
    … … def fixture_add_collection(name=u"My first Collection", user=None):  
    263265    Session.expunge(coll)
    264266
    265267    return coll
     268
     269
     270def expect_failure(test):
     271    @wraps(test)
     272    def wrapper(*args, **kwargs):
     273        try:
     274            test(*args, **kwargs)
     275        except Exception:
     276            raise nose.SkipTest
     277        else:
     278            raise AssertionError(
     279                'Test is expected to fail, did you add a feature?')
     280    return wrapper
Version 0, edited 11 years ago by joar (next)

comment:2 by joar, 11 years ago

Keywords: review added; needsreview removed

comment:3 by joar, 11 years ago

Milestone: → 0.4.0

comment:4 by Christopher Allan Webber, 11 years ago

Cc: nyergler added

Yeowch, that's a huge diff and is kind of making this bug report long! I wonder if we should be careful about that...

anyway, adding nyergler as CC'ed. I don't think I'm well qualified to review this. Maybe Nathan is?

If it comes close to 0.4.0 and nobody else has time to review I can do a rough review and we can just merge it but I'm not really well qualified at all.

comment:5 by nyergler, 11 years ago

I read ​https://github.com/joar/mediagoblin/compare/master...oauth;refresh_tokens carefully and believe this is merge-able. I haven't tried executing it, but have don't see anything glaring. The relational model for our OAuth plugin is a little weird (foreign keys to both client and user, when I believe a client is specific to a user already), but this patch is consistent with the existing code.

I say merge it!

Note: See TracTickets for help on using tickets.