fix: Correctly cache user permissions

During the upgrade to Superset 3 the course cache accidentally became permanent due to incorrect Superset documentation. This cleans up caching of permissions in general as well as fixing some small localization bugs found in testing.

tutoraspects/plugin.py

@@ -316,6 +316,7 @@
                 "da",
                 "de_DE",
                 "el",
+                "en",
                 "es_419",
                 "es_ES",
                 "fr_CA",
@@ -333,6 +334,17 @@
             ],
         ),
         ("SUPERSET_EXTRA_JINJA_FILTERS", {}),
+        # This controls the cache time of the can_view_courses
+        # wrapper, which controls the course-based permissions.
+        # This includes the user roles and course list. This
+        # does not get cleared on login, and so should be kept
+        # short since mostly most of the savings comes from the
+        # course cache anyway.
+        ("SUPERSET_USER_PERMISSIONS_CACHE_TIMEOUT", 120),
+        # This controls the cache time of the user's course list
+        # only, limiting the number of LMS calls since they are
+        # rate limited. This can be cleared by logging back in.
+        ("SUPERSET_USER_COURSES_CACHE_TIMEOUT", 300),
         ("SUPERSET_BLOCK_STUDENT_ACCESS", True),
         # This setting allows Superset to run behind a reverse proxy in HTTPS and
         # redirect to the correct http/s based on the headers sent from the proxy.

tutoraspects/templates/aspects/apps/superset/pythonpath/openedx_jinja_filters.py

@@ -9,9 +9,13 @@
 NO_COURSES = "1 = 0"
 
 
-def can_view_courses(username, field_name="course_id"):
+def can_view_courses(username, field_name="course_id", **kwargs):
     """
-    Returns SQL WHERE clause which restricts access to the courses the current user has staff access to.
+    Returns SQL WHERE clause which restricts access to the courses the current user has
+    staff access to.
+
+    We accept kwargs for optional caching args, since this is memoized in
+    can_view_courses_wrapper.
     """
     user = security_manager.get_user_by_username(username)
     if user:

tutoraspects/templates/aspects/apps/superset/pythonpath/openedx_sso_security_manager.py

@@ -6,6 +6,7 @@
 from authlib.common.urls import add_params_to_qs, add_params_to_uri
 from authlib.integrations.flask_client import OAuthError
 from flask import current_app, session
+from superset.extensions import cache_manager
 from superset.security import SupersetSecurityManager
 
 log = logging.getLogger(__name__)
@@ -79,6 +80,9 @@ def oauth_user_info(self, provider, response=None):
                 user_profile.get("preferred_username"), locale_preference
             )
 
+            log.info(f"User roles for {user_profile['preferred_username']}:"
+                     f" {user_roles}")
+
             return {
                 "name": user_profile["name"],
                 "email": user_profile["email"],
@@ -161,12 +165,15 @@ def _get_user_roles(self, username, locale):
         decoded_access_token = self.decoded_user_info()
 
         if decoded_access_token.get("superuser", False):
-            return ["admin", "admin-{locale}"]
+            return ["admin", f"admin-{locale}"]
         elif decoded_access_token.get("administrator", False):
-            return ["alpha", "operator", "operator-{locale}"]
+            return ["alpha", "operator", f"operator-{locale}"]
         else:
-            # User has to have staff access to one or more courses to view any content here.
-            courses = self.get_courses(username)
+            # User has to have staff access to one or more courses to view any content
+            # here. Since this is only called on login, we take the opportunity
+            # to force refresh the user's courses from LMS. This allows them to bust
+            # the course permissions cache if necessary by logging out and back in.
+            courses = self.get_courses(username, force=True)
             if courses:
                 return ["instructor", f"instructor-{locale}"]
             else:
@@ -188,10 +195,24 @@ def extra_get_user_roles(self, username, decoded_access_token):
         {{patch("superset-sso-assignment-rules") | indent(8)}}
         return None
 
-    def get_courses(self, username, permission="staff", next_url=None):
+    def get_courses(self, username, permission="staff", next_url=None, force=False):
         """
         Returns the list of courses the current user has access to.
+
+        Calls itself recursively if the response is paginated.
+        Force will cause a force refresh of the cache, we do this on login to make
+        sure that the user always has an ability to update this data.
         """
+        cache = cache_manager.cache
+        cache_key = f"{username}+{permission}"
+
+        # Only return from the cache when we're at the top level
+        # (not in recursion)
+        if not next_url and not force:
+            obj = cache.get(cache_key)
+            if obj is not None:
+                return obj
+
         courses = []
         provider = session.get("oauth_provider")
         oauth_remote = self.oauth_remotes.get(provider)
@@ -209,7 +230,9 @@ def get_courses(self, username, permission="staff", next_url=None):
             username=username, permission=permission
         )
         url = next_url or courses_url
-        response = oauth_remote.get(url, token=token).json()
+        resp = oauth_remote.get(url, token=token)
+        resp.raise_for_status()
+        response = resp.json()
 
         for course in response.get("results", []):
             course_id = course.get("course_id")
@@ -224,4 +247,10 @@ def get_courses(self, username, permission="staff", next_url=None):
             for course_id in next_courses:
                 courses.append(course_id)
 
+        log.info(f"Courses for {username}: {courses}")
+
+        # Only cache the top level (non-recursive) response
+        if not next_url:
+            cache.set(cache_key, courses, timeout=300)
+
         return courses

tutoraspects/templates/aspects/apps/superset/pythonpath/superset_config.py

@@ -69,8 +69,9 @@ def get_env_variable(var_name: str, default: Optional[str] = None) -> str:
 
 REDIS_HOST = get_env_variable("REDIS_HOST")
 REDIS_PORT = get_env_variable("REDIS_PORT")
-REDIS_CELERY_DB = get_env_variable("REDIS_CELERY_DB", "0")
-REDIS_RESULTS_DB = get_env_variable("REDIS_RESULTS_DB", "1")
+REDIS_CELERY_DB = get_env_variable("REDIS_CELERY_DB", "3")
+REDIS_RESULTS_DB = get_env_variable("REDIS_RESULTS_DB", "4")
+REDIS_CACHE_DB = get_env_variable("REDIS_CACHE_DB", "5")
 REDIS_PASSWORD = get_env_variable("REDIS_PASSWORD", "")
 
 RESULTS_BACKEND = RedisCache(host=REDIS_HOST, port=REDIS_PORT, password=REDIS_PASSWORD, db=REDIS_RESULTS_DB, key_prefix='superset_results')
@@ -88,9 +89,9 @@ def get_env_variable(var_name: str, default: Optional[str] = None) -> str:
 
 # Cache for dashboard filter state
 FILTER_STATE_CACHE_CONFIG: CacheConfig = {
-    "CACHE_DEFAULT_TIMEOUT": int(timedelta(days=90).total_seconds()),
+    "CACHE_DEFAULT_TIMEOUT": 1200,
     # should the timeout be reset when retrieving a cached value
-    "REFRESH_TIMEOUT_ON_RETRIEVAL": True,
+    "REFRESH_TIMEOUT_ON_RETRIEVAL": False,
     **CACHE_CONFIG,
 }
 

tutoraspects/templates/aspects/apps/superset/pythonpath/superset_config_docker.py

@@ -97,10 +97,13 @@
 from openedx_jinja_filters import *
 
 def can_view_courses_wrapper(*args, **kwargs):
+    """
+    Wraps the can_view_courses call in a cache for performance.
+    """
     from superset.utils.cache import memoized_func
-    from superset.extensions import cache_manager
 
-    return memoized_func(key="{username}", cache=cache_manager.cache)(can_view_courses)(*args, **kwargs)
+    kwargs["cache_timeout"] = {{ SUPERSET_USER_PERMISSIONS_CACHE_TIMEOUT }}
+    return memoized_func()(can_view_courses)(*args, **kwargs)
 
 
 JINJA_CONTEXT_ADDONS = {