chore(cache): reset shared cache with node on request #2391
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new way to reset the shared cache on request by implementing a decorator for passcode or staff-based authorization and updating configuration to support the new passcode environment variable.
- Added a new decorator (passcode_or_staff_required) in sefaria/decorators.py to check a passcode or staff member status.
- Updated the rebuild_shared_cache view in sefaria/views.py to use the new decorator and trigger a shared cache rebuild.
- Configured STAFF_PASSCODE in multiple settings files and updated the Helm chart configuration.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| sefaria/decorators.py | Added decorator for passcode or staff authentication |
| sefaria/views.py | Applied the new decorator to the rebuild_shared_cache view and imported necessary settings |
| sefaria/local_settings_coolify.py, sefaria/local_settings_example.py, sefaria/local_settings_ci.py | Introduced STAFF_PASSCODE environment variable configuration |
| helm-chart/sefaria/templates/configmap/local-settings-file.yaml | Added STAFF_PASSCODE to the configuration file |
| sefaria/urls.py | Introduced a new URL pattern for resetting the shared cache |
Comments suppressed due to low confidence (1)
sefaria/views.py:767
- The identifier 'library' is used without an import in this file. Please ensure 'library' is properly imported before its usage in the rebuild_shared_cache view.
library.init_shared_cache(rebuild=True)
sefaria/local_settings_ci.py
Outdated
| @@ -226,3 +226,5 @@ | |||
| wrapper_class=structlog.stdlib.BoundLogger, | |||
| cache_logger_on_first_use=True, | |||
| ) | |||
|
|
|||
| STAFF_PASSCODE = os.getenv("STAFF_PASSCODE") | |||
There was a problem hiding this comment.
I don't think STAFF_PASSCODE is the right terminology here.
The decorator is passcode_or_staff_required - but the passcode should not be confused with giving access to anything but some very specific (and locked down!) endpoints.
Perhaps MAINTENANCE_URLS_PASSCODE?
In addition, since this passcode is passed as a GET parameter, it is highly unsecured and will be "leaked" all over the place (logs, etc.)
You've implemented a mechanism to allow for separate passcodes for every instance of the decorator - maybe we should use that already, and this should be a single-use SHARED_CACHE_REBUILD_PASSCODE
sefaria/views.py
Outdated
| from sefaria.client.util import jsonResponse, send_email, read_webpack_bundle | ||
| from sefaria.forms import SefariaNewUserForm, SefariaNewUserFormAPI, SefariaDeleteUserForm, SefariaDeleteSheet | ||
| from sefaria.settings import MAINTENANCE_MESSAGE, USE_VARNISH, MULTISERVER_ENABLED | ||
| from sefaria.settings import MAINTENANCE_MESSAGE, USE_VARNISH, MULTISERVER_ENABLED, STAFF_PASSCODE |
There was a problem hiding this comment.
Please modify to from django.conf import settings
and modify the use of the attributes to be
settings.STAFF_PASSCODE etc.
sefaria/decorators.py
Outdated
| import base64 | ||
| from django.contrib.admin.views.decorators import staff_member_required | ||
| from functools import wraps | ||
| from sefaria.settings import WEBHOOK_USERNAME, WEBHOOK_PASSWORD |
There was a problem hiding this comment.
Please change to -
from django.conf import settings
settings.WEBHOOK_USERNAME
etc.
| import pytest | ||
| import json | ||
|
|
||
| import sefaria.system.decorators as d | ||
| import sefaria.system.exceptions as e | ||
|
|
||
| import base64 |
There was a problem hiding this comment.
Both these imports (base64, pytest) are duplicated
| wrapped = webhook_auth_or_staff_required(dummy_view) | ||
| response = wrapped(request) | ||
| # Since staff_member_required redirects by default | ||
| assert response.status_code in [302, 401] # Depending on login settings |
There was a problem hiding this comment.
I don't understand this comment, nor how the status code could by 302 or 401. The test should be deterministic (and if there's a redirect, we should resolve it first, and then check the status code)
|
|
||
| wrapped = webhook_auth_or_staff_required(dummy_view) | ||
| response = wrapped(request) | ||
| assert response.status_code in [302, 401] |
There was a problem hiding this comment.
As above, the response should be single deterministic
This pull request introduces a new way to reset the shared cache on a url request, this request is protected with a passcode to allow webhooks to use it
New Feature: Passcode Protection for Admin Actions
sefaria/decorators.py: Added a new decoratorpasscode_or_staff_requiredthat checks for a passcode or staff member status before allowing access to certain views.sefaria/views.py: Added thepasscode_or_staff_requireddecorator to therebuild_shared_cacheview function and imported the decorator andSTAFF_PASSCODEfrom settings. [1] [2]Configuration Updates
helm-chart/sefaria/templates/configmap/local-settings-file.yaml: AddedSTAFF_PASSCODEenvironment variable to the configuration.sefaria/local_settings_ci.py,sefaria/local_settings_coolify.py,sefaria/local_settings_example.py: AddedSTAFF_PASSCODEenvironment variable to each of these settings files. [1] [2] [3]URL and View Updates
sefaria/urls.py: Added a new URL pattern for therebuild_shared_cacheview.