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

Using token/get_scope instead of verify #27

Closed
wants to merge 7 commits into from
Closed

Conversation

iambibhas
Copy link
Contributor

Work In Progress

if 'access_token' in request.values:
raise LastuserTokenAuthException(u"Access token specified in both header and body.")
raise LastuserTokenAuthException(
u"Access token specified in both header and body.")
Copy link
Member

Choose a reason for hiding this comment

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

Technically both are in the header, so this message is incorrect. Change it to:

Access token specified in both Authorization header and query or form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

elif not header_only:
# Is there an access token in the form or query?
token = request.values.get('access_token')
else:
return
return None
Copy link
Member

Choose a reason for hiding this comment

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

return None is the same thing as return. The else clause is entirely unnecessary though as we've already specified token = None. Just return token should do.

return token

def get_lastuser_cache_key(self, token):
return u'lastuser/tokenscope/{token}'.format(token=token)
Copy link
Member

Choose a reason for hiding this comment

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

Rename to get_tokenscope_cache_key

def get_lastuser_cache_key(self, token):
return u'lastuser/tokenscope/{token}'.format(token=token)

def get_lastuser_cache(self, header_only):
Copy link
Member

Choose a reason for hiding this comment

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

Rename to get_and_cache_token_scope


def token_auth(self, resource='*', header_only=False):
if not self.lastuser.cache:
raise LastuserException(u"No caching backend found")
Copy link
Member

Choose a reason for hiding this comment

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

Move this test into get_and_cache_token_scope.


if not user:
g.lastuser_cookie.pop('userid', None)
g.lastuser_cookie.pop('sessionid', None)

g.user = user
lastuser_cache = self.get_lastuser_cache(header_only=True)
Copy link
Member

Choose a reason for hiding this comment

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

You've already called this method in the token_auth method, so it's now going to hit cache twice in the same request.

Copy link
Member

Choose a reason for hiding this comment

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

Return it from the chain of method calls instead. Don't hit cache for in-thread data, and don't make cache a mandatory requirement.

if user:
g.access_scope = ['*'] # TODO: In future, restrict to access token's scope
g.access_scope = lastuser_cache['scope'] # TODO: In future, restrict to access token's scope
Copy link
Member

Choose a reason for hiding this comment

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

If you don't have an access token but have a cookie user, lastuser_cache will be None and the scope should be *.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jace we can just check in required_login etc that if the specified scope is in g.access_scope mentioned here, right? no need to check anything else then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After making the changes you mentioned above.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that is what you need to be doing. Cookie login sets g.access_scope to ['*'] while token login sets a specific scope list.

@@ -400,14 +423,15 @@ def after_request(self, response):
httponly=True) # Don't allow reading this from JS.
return response

def requires_login(self, f):
def requires_login(self, f, scope="*"):
Copy link
Member

Choose a reason for hiding this comment

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

This won't work. A decorator receives a single parameter, the decorated function. It can't take a second. The API currently looks like this:

def requires_login(f):
    @wraps(f)
    def decorated_function(*args, **kwargs):
        # Insert login check here
        return f(*args, **kwargs)
    return decorated_function

@requires_login
def view_that_requires_login():
    pass

To add a scope parameter to the decorator, you either need to make a new decorator with an incompatible API, or do some jugaad:

def requires_login(scope='*'):
    def decorator(f):
        @wraps(f)
        def decorated_function(*args, **kwargs):
            # Insert login check here
            return f(*args, **kwargs)
        return decorated_function
    if callable(scope):  # We got a function instead of a scope
        return decorator(scope)
    else:
        return decorator

# Now we can use it with both APIs. Old (backward compatible):
@requires_login
def view_that_requires_login():
    pass

# New (with scope specified):
@requires_login(scope='image')
def view_that_requires_scope():
    pass

Copy link
Contributor Author

@iambibhas iambibhas Nov 7, 2017

Choose a reason for hiding this comment

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

this was an oversight from my side. Just read a little more on decorators. One question -

make a new decorator with an incompatible API

But what you did above makes things backward compatible, right? I think the same is done for requires_permission below this decorator.

"""
Decorator for functions that require login.
"""
@wraps(f)
def decorated_function(*args, **kwargs):
g.login_required = True
if hasattr(g, 'lastuserinfo') and g.lastuserinfo is None:
user, user_from_token, token_error = self.get_logged_in_user(scope=scope)
Copy link
Member

Choose a reason for hiding this comment

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

Does this hit cache again?

@jace
Copy link
Member

jace commented Nov 7, 2017

get_scope not get_skope.

@iambibhas iambibhas changed the title Using token/get_skope instead of verify Using token/get_scope instead of verify Nov 7, 2017
@iambibhas
Copy link
Contributor Author

@jace review this once more.

@jace
Copy link
Member

jace commented Nov 27, 2017

Could you sync this branch with master?

@jace
Copy link
Member

jace commented Apr 1, 2020

Closing as this has been abandoned for a while.

@jace jace closed this Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify scope in requires_login decorator
2 participants