From ce9d9fd26dbba3909b7ba45304511a2aca2e36e4 Mon Sep 17 00:00:00 2001
From: Min RK <benjaminrk@gmail.com>
Date: Thu, 13 Sep 2018 10:28:10 +0200
Subject: [PATCH] clarify docstrings, comments in refresh_user

- refresh_user may return True in the common case, identifying that everything is up-to-date
- return False for "needs login"
- return auth_data dict when an update can be performed without logging in again
---
 jupyterhub/auth.py          | 34 +++++++++++++++++++++-------------
 jupyterhub/handlers/base.py | 33 +++++++++++++++++++++++++--------
 2 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py
index 8a7bc0ba..b301330e 100644
--- a/jupyterhub/auth.py
+++ b/jupyterhub/auth.py
@@ -300,21 +300,27 @@ class Authenticator(LoggingConfigurable):
         Args:
             user (User): the user to refresh
         Returns:
-            auth_data (dict or None):
-                The same return value as `.authenticate`.
-                Any values here will refresh the value
-                for the user.
-                This can include updating `.admin` fields
-                or updating `.auth_state`.
-                Return None if the user's auth data has expired,
+            auth_data (bool or dict):
+                Return **True** if auth data for the user is up-to-date
+                and no updates are required.
+
+                Return **False** if the user's auth data has expired,
                 and they should be required to login again.
+
+                Return a **dict** of auth data if some values should be updated.
+                This dict should have the same structure as that returned
+                by :meth:`.authenticate()` when it returns a dict.
+                Any fields present will refresh the value for the user.
+                Any fields not present will be left unchanged.
+                This can include updating `.admin` or `.auth_state` fields.
         """
-        return {'name': user.name}
+        return True
 
     async def authenticate(self, handler, data):
         """Authenticate a user with login form data
 
-        This must be a tornado gen.coroutine.
+        This must be a coroutine.
+
         It must return the username on successful authentication,
         and return None on failed authentication.
 
@@ -328,12 +334,14 @@ class Authenticator(LoggingConfigurable):
             data (dict): The formdata of the login form.
                          The default form has 'username' and 'password' fields.
         Returns:
-            user (str or dict or None): The username of the authenticated user,
+            user (str or dict or None):
+                The username of the authenticated user,
                 or None if Authentication failed.
+
                 The Authenticator may return a dict instead, which MUST have a
-                key 'name' holding the username, and may have two optional keys
-                set - 'auth_state', a dictionary of of auth state that will be
-                persisted; and 'admin', the admin setting value for the user.
+                key `name` holding the username, and MAY have two optional keys
+                set: `auth_state`, a dictionary of of auth state that will be
+                persisted; and `admin`, the admin setting value for the user.
         """
 
     def pre_spawn_start(self, user, spawner):
diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py
index 175cb424..f1ec8cc4 100644
--- a/jupyterhub/handlers/base.py
+++ b/jupyterhub/handlers/base.py
@@ -54,8 +54,14 @@ class BaseHandler(RequestHandler):
     async def prepare(self):
         """Identify the user during the prepare stage of each request
 
-        During requests, the current user may be retrieved via
-        self.current_user
+        `.prepare()` runs prior to all handler methods,
+        e.g. `.get()`, `.post()`.
+
+        Checking here allows `.get_current_user` to be async without requiring
+        every current user check to be made async.
+
+        The current user (None if not logged in) may be accessed
+        via the `self.current_user` property during the handling of any request.
         """
         try:
             await self.get_current_user()
@@ -234,7 +240,8 @@ class BaseHandler(RequestHandler):
             user (User): the user whose auth info is to be refreshed
             force (bool): force a refresh instead of checking last refresh time
         Returns:
-            user (User): the user having been refreshed, or None if
+            user (User): the user having been refreshed,
+                or None if the user must login again to refresh auth info.
         """
         if not force: # TODO: and it's sufficiently recent
             return user
@@ -249,15 +256,25 @@ class BaseHandler(RequestHandler):
 
         self.log.debug("Refreshing auth for %s", user.name)
         auth_info = await self.authenticator.refresh_user(user)
-        if auth_info is None:
-            self.log.warning("User %s has stale auth info. Login required to refresh.", user.name)
+
+        if not auth_info:
+            self.log.warning(
+                "User %s has stale auth info. Login is required to refresh.",
+                user.name,
+            )
             return
 
-        if isinstance(auth_info, str):
-            auth_info = {'name': auth_info}
+        if auth_info == True:
+            # refresh_user confirmed that it's up-to-date,
+            # nothing to refresh
+            return user
+
+        # Ensure name field is set. It cannot be updated.
+        auth_info['name'] = user.name
+
         if 'auth_state' not in auth_info:
             # refresh didn't specify auth_state,
-            # so preserve previous value to avoid clearing
+            # so preserve previous value to avoid clearing it
             auth_info['auth_state'] = await user.get_auth_state()
         return await self.auth_to_user(auth_info, user)