-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add clear expired tokens task in ol-social-auth #778
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
Changes from 6 commits
3e70bc1
70d4e25
086597d
cd07aeb
b443211
e831d54
dce4d4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| """ol_social_auth Django application initialization.""" | ||
|
|
||
| from django.apps import AppConfig | ||
| from edx_django_utils.plugins import PluginSettings | ||
| from openedx.core.djangoapps.plugins.constants import ProjectType, SettingsType | ||
|
|
||
|
|
||
| class OLSocialAuthConfig(AppConfig): | ||
| name = "ol_social_auth" | ||
| verbose_name = "OL Social Auth" | ||
|
|
||
| plugin_app = { | ||
| PluginSettings.CONFIG: { | ||
| ProjectType.LMS: { | ||
| SettingsType.COMMON: { | ||
| PluginSettings.RELATIVE_PATH: "settings.common", | ||
| }, | ||
| SettingsType.PRODUCTION: { | ||
| PluginSettings.RELATIVE_PATH: "settings.production", | ||
| }, | ||
| }, | ||
| }, | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,17 @@ | ||||||
| """Common settings for the ol-social-auth plugin.""" | ||||||
|
|
||||||
| from celery.schedules import crontab | ||||||
|
|
||||||
|
|
||||||
| def plugin_settings(settings): | ||||||
| """Settings for the ol-social-auth plugin.""" # noqa: D401 | ||||||
| settings.OAUTH2_PROVIDER["REFRESH_TOKEN_EXPIRE_SECONDS"] = ( | ||||||
| 30 * 24 * 60 * 60 # 30 days | ||||||
| ) | ||||||
|
Anas12091101 marked this conversation as resolved.
|
||||||
| # Add ol_clear_expired_tokens to the Celery beat schedule. | ||||||
| if not hasattr(settings, "CELERY_BEAT_SCHEDULE"): | ||||||
| settings.CELERY_BEAT_SCHEDULE = {} | ||||||
| settings.CELERY_BEAT_SCHEDULE["ol_clear_expired_tokens"] = { | ||||||
| "task": "ol_social_auth.tasks.ol_clear_expired_tokens", | ||||||
| "schedule": crontab(hour=9, minute=0, day_of_week="monday"), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not midnight?
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m not entirely sure, but this task is scheduled for 9 AM in our other applications as well; for example, MITxOnline and MITxPRO @pdpinch, what are your thoughts on this?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's a task that we care about succeeding, I prefer to do it during business hours, so we have a better chance of noticing a problem and fixing it. For this task, like this, really, any time would be fine.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was able to test it manually via Django shell, but I wasn't able to test it via the beat schedule and the task being run in auto mode. Were you able to test it? Also, Open edX has
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. I’ve updated the setting name in dce4d4b and verified that the tasks are running as expected according to the beat schedule. |
||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| """Production settings for the ol-social-auth plugin.""" | ||
|
|
||
|
|
||
| def plugin_settings(settings): | ||
| """Production overrides for ol-social-auth plugin.""" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| """Celery tasks for ol-social-auth plugin.""" | ||
|
|
||
| import logging | ||
|
|
||
| from celery import shared_task | ||
| from oauth2_provider.models import clear_expired | ||
|
|
||
| log = logging.getLogger(__name__) | ||
| oauth2_logger = logging.getLogger("oauth2_provider") | ||
|
|
||
|
|
||
| @shared_task(acks_late=True) | ||
| def ol_clear_expired_tokens(): | ||
| """Clear expired OAuth2 access, refresh, and ID tokens.""" | ||
| log.info("Starting ol_clear_expired_tokens...") | ||
| # Suppress debug-level logs from oauth2_provider during cleanup. | ||
| # Its batch_delete debug logs lack the 'userid' field expected by | ||
| # Open edX's custom log formatter, causing noisy ValueError tracebacks. | ||
|
Comment on lines
+16
to
+18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain this a little bit more? I am having a hard time getting it. Did we copy it from Open edX?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wasn't copied from Open edX — it's a workaround for an incompatibility between The problem: The fix: Before calling To reproduce the issue, run the clear_expired() fn without suppressing the logs. |
||
| original_level = oauth2_logger.level | ||
| oauth2_logger.setLevel(logging.INFO) | ||
| try: | ||
| clear_expired() | ||
| finally: | ||
| oauth2_logger.setLevel(original_level) | ||
| log.info("Finished ol_clear_expired_tokens.") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| [tool:pytest] | ||
| pep8maxlinelength = 119 | ||
| DJANGO_SETTINGS_MODULE = lms.envs.test | ||
| addopts = --nomigrations --reuse-db --durations=20 | ||
| filterwarnings = | ||
| default | ||
| ignore::xblock.exceptions.FieldDataDeprecationWarning | ||
| ignore::pytest.PytestConfigWarning | ||
| ignore:No request passed to the backend, unable to rate-limit:UserWarning | ||
| ignore:Flags not at the start of the expression:DeprecationWarning | ||
| ignore:Using or importing the ABCs from 'collections' instead of from 'collections.abc':DeprecationWarning | ||
| ignore:invalid escape sequence:DeprecationWarning | ||
| ignore:`formatargspec` is deprecated since Python 3.5:DeprecationWarning | ||
| ignore:the imp module is deprecated in favour of importlib:DeprecationWarning | ||
| ignore:"is" with a literal:SyntaxWarning | ||
| ignore:defusedxml.lxml is no longer supported:DeprecationWarning | ||
| ignore: `np.int` is a deprecated alias for the builtin `int`.:DeprecationWarning | ||
| ignore: `np.float` is a deprecated alias for the builtin `float`.:DeprecationWarning | ||
| ignore: `np.complex` is a deprecated alias for the builtin `complex`.:DeprecationWarning | ||
| ignore: 'etree' is deprecated. Use 'xml.etree.ElementTree' instead.:DeprecationWarning | ||
| ignore: defusedxml.cElementTree is deprecated, import from defusedxml.ElementTree instead.:DeprecationWarning | ||
|
|
||
| junit_family = xunit2 | ||
| norecursedirs = .* *.egg build conf dist node_modules test_root cms/envs lms/envs | ||
| python_classes = | ||
| python_files = tests.py test_*.py tests_*.py *_tests.py __init__.py |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| """Tests for ol_social_auth tasks.""" | ||
|
|
||
| from ol_social_auth import tasks | ||
|
|
||
|
|
||
| def test_ol_clear_expired_tokens(mocker): | ||
| """Test that ol_clear_expired_tokens calls the clear_expired function.""" | ||
| patched_clear_expired = mocker.patch("ol_social_auth.tasks.clear_expired") | ||
|
|
||
| tasks.ol_clear_expired_tokens.delay() | ||
| patched_clear_expired.assert_called_once_with() | ||
|
|
||
|
|
||
| def test_ol_clear_expired_tokens_logging(mocker): | ||
| """Test that ol_clear_expired_tokens logs start and finish messages.""" | ||
| mocker.patch("ol_social_auth.tasks.clear_expired") | ||
| patched_log_info = mocker.patch("ol_social_auth.tasks.log.info") | ||
|
|
||
| tasks.ol_clear_expired_tokens() | ||
|
|
||
| expected_log_call_count = 2 | ||
| assert patched_log_info.call_count == expected_log_call_count # noqa: S101 | ||
| patched_log_info.assert_any_call("Starting ol_clear_expired_tokens...") | ||
| patched_log_info.assert_any_call("Finished ol_clear_expired_tokens.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest checking with @blarghmatey this time range as well, since we're using service tokens before merging the PR. Since DevOps creates service tokens, I'm not sure what expiry time they set. I want to make sure they will not be deleted automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OAuth service accounts that we rely on are using client credentials grants so we're not worried about token expiration. Thanks for checking.