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

Should get_secure_cookie set token_authenticated property? #566

Closed
Zsailer opened this issue Aug 2, 2021 · 5 comments
Closed

Should get_secure_cookie set token_authenticated property? #566

Zsailer opened this issue Aug 2, 2021 · 5 comments
Labels

Comments

@Zsailer
Copy link
Member

Zsailer commented Aug 2, 2021

Should this branch set handler._token_authenticated = True.

Technically it is "cookie authenticated", however, the cookie is based on an earlier token.

I ask because it can be useful for a handler to know when token authorisation is in use, for example:

class MyExtensionHandler(ExtensionHandlerMixin, JupyterHandler):  

    @tornado.web.authenticated
    def get(self):
        user = self.get_current_user()                                           
        if self.token_authenticated:
            user = getpass.getuser()
        self.write(f'hello {user}') 

Originally posted by @oliver-sanders in #562 (comment)

@vidartf
Copy link
Member

vidartf commented Aug 12, 2021

This docstring seems explanatory of intent:

@classmethod
def is_token_authenticated(cls, handler):
"""Returns True if handler has been token authenticated. Otherwise, False.
Login with a token is used to signal certain things, such as:
- permit access to REST API
- xsrf protection
- skip origin-checks for scripts

@vidartf
Copy link
Member

vidartf commented Aug 12, 2021

So if no one is complaining about the current behavior I would argue for leaving token_authenticated as it currently is (changing it would make things more permissible in certain scenarios, so would need to be carefully reviewed) and adding another property if needed.

@vidartf
Copy link
Member

vidartf commented Aug 12, 2021

For the branch where the comment were made:

if user_id is None:
get_secure_cookie_kwargs = handler.settings.get('get_secure_cookie_kwargs', {})
user_id = handler.get_secure_cookie(handler.cookie_name, **get_secure_cookie_kwargs )
if user_id:
user_id = user_id.decode()

I don't think we can assume that the source of the secure cookie is always from a token? Or am I missing something?

@oliver-sanders
Copy link
Contributor

Thanks all for looking into this and your comments, I have convinced myself the answer to this question is no, please close this issue.

The root cause for raising this was handling the difference between hub and token authentication.

With "hubless" token authentication get_current_user returns the token as a string (or as string or bytes in earlier versions of Jupyter Server).

Note: get_current_user can also return the string anonymous so testing if the returned user is a string isn't a secure way of determining if the request was token authenticated?

With hub authentication get_current_user returns a dictionary containing the following fields:

  • kind
  • name
  • admin
  • groups
  • server
  • pending
  • created
  • last_activity
  • servers

The "name" field is relevant to hubless authentication, the "server" field might be, once we have authorisation in the "kind" and "groups" fields might be relevant too.

We could potentially smooth over the difference by returning a dict in these circumstances something like:

return {
    'kind': 'user',
    'user': getpass.getuser()
}

@Zsailer
Copy link
Member Author

Zsailer commented Aug 24, 2021

Thank you for further investigating this, @oliver-sanders. Closing this for now, but I'm guessing we will revisit this further as we move to make the server more multi-user.

@Zsailer Zsailer closed this as completed Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants