diff --git a/tutoraspects/plugin.py b/tutoraspects/plugin.py index 281918c25..171d01275 100644 --- a/tutoraspects/plugin.py +++ b/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. diff --git a/tutoraspects/templates/aspects/apps/superset/pythonpath/openedx_jinja_filters.py b/tutoraspects/templates/aspects/apps/superset/pythonpath/openedx_jinja_filters.py index 809ddedef..fc61d4fab 100644 --- a/tutoraspects/templates/aspects/apps/superset/pythonpath/openedx_jinja_filters.py +++ b/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: diff --git a/tutoraspects/templates/aspects/apps/superset/pythonpath/openedx_sso_security_manager.py b/tutoraspects/templates/aspects/apps/superset/pythonpath/openedx_sso_security_manager.py index a39afecec..4a28afbf7 100644 --- a/tutoraspects/templates/aspects/apps/superset/pythonpath/openedx_sso_security_manager.py +++ b/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 diff --git a/tutoraspects/templates/aspects/apps/superset/pythonpath/superset_config.py b/tutoraspects/templates/aspects/apps/superset/pythonpath/superset_config.py index a96471091..c8a8cf111 100644 --- a/tutoraspects/templates/aspects/apps/superset/pythonpath/superset_config.py +++ b/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, } diff --git a/tutoraspects/templates/aspects/apps/superset/pythonpath/superset_config_docker.py b/tutoraspects/templates/aspects/apps/superset/pythonpath/superset_config_docker.py index f70f71abf..aa572b1b6 100644 --- a/tutoraspects/templates/aspects/apps/superset/pythonpath/superset_config_docker.py +++ b/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 = {