From dad6ebade58d14eece8cfee6d9c2cad166066ca5 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Tue, 30 Jul 2024 09:48:01 -0700 Subject: [PATCH 1/4] Add more edge case handling for FacilityUser and Facility getting deleted during an active browser session. --- kolibri/core/auth/middleware.py | 10 ++++++++-- kolibri/core/auth/permissions/general.py | 9 ++++++--- kolibri/core/tasks/permissions.py | 8 -------- kolibri/plugins/user_profile/tasks.py | 19 ++++++++++++++++++- 4 files changed, 32 insertions(+), 14 deletions(-) diff --git a/kolibri/core/auth/middleware.py b/kolibri/core/auth/middleware.py index 4bea3591a9c..d7bd69f34a2 100644 --- a/kolibri/core/auth/middleware.py +++ b/kolibri/core/auth/middleware.py @@ -6,6 +6,7 @@ from django.contrib.sessions.middleware import SessionMiddleware from django.core.cache import cache from django.core.exceptions import ImproperlyConfigured +from django.db.models.signals import post_delete from django.db.models.signals import post_save from django.utils.functional import SimpleLazyObject @@ -64,12 +65,17 @@ def _get_user(request): return request._cached_user -def clear_user_cache(sender, instance, created, **kwargs): +def clear_user_cache_on_delete(sender, instance, **kwargs): + cache.delete(USER_SESSION_CACHE_KEY.format(instance.id)) + + +def clear_user_cache_on_save(sender, instance, created, **kwargs): if not created: cache.delete(USER_SESSION_CACHE_KEY.format(instance.id)) -post_save.connect(clear_user_cache, sender=settings.AUTH_USER_MODEL) +post_delete.connect(clear_user_cache_on_delete, sender=settings.AUTH_USER_MODEL) +post_save.connect(clear_user_cache_on_save, sender=settings.AUTH_USER_MODEL) class CustomAuthenticationMiddleware(AuthenticationMiddleware): diff --git a/kolibri/core/auth/permissions/general.py b/kolibri/core/auth/permissions/general.py index 3e5e38cdb08..3e3c0216e1b 100644 --- a/kolibri/core/auth/permissions/general.py +++ b/kolibri/core/auth/permissions/general.py @@ -126,9 +126,12 @@ def _user_is_admin_for_own_facility(user, obj=None): if obj: if not hasattr(obj, "dataset_id") or not user.dataset_id == obj.dataset_id: return False - - facility = Facility.objects.get(dataset_id=user.dataset_id) - return user.has_role_for_collection(role_kinds.ADMIN, facility) + try: + facility = Facility.objects.get(dataset_id=user.dataset_id) + return user.has_role_for_collection(role_kinds.ADMIN, facility) + except Facility.DoesNotExist: + # Handle the edge case where a facility has been deleted + return False class IsAdminForOwnFacility(BasePermissions): diff --git a/kolibri/core/tasks/permissions.py b/kolibri/core/tasks/permissions.py index 851b06d8f45..b5afe6236dd 100644 --- a/kolibri/core/tasks/permissions.py +++ b/kolibri/core/tasks/permissions.py @@ -177,14 +177,6 @@ def __init__(self): super(IsAdminForJob, self).__init__(IsSuperAdmin(), IsFacilityAdminForJob()) -class IsSelf(BasePermission): - def user_can_run_job(self, user, job): - return user.id == job.kwargs.get("local_user_id", None) - - def user_can_read_job(self, user, job): - return user.id == job.kwargs.get("local_user_id", None) - - class NotProvisioned(BasePermission): def user_can_run_job(self, user, job): from kolibri.core.device.utils import device_provisioned diff --git a/kolibri/plugins/user_profile/tasks.py b/kolibri/plugins/user_profile/tasks.py index d51567d9c26..95158f2a7df 100644 --- a/kolibri/plugins/user_profile/tasks.py +++ b/kolibri/plugins/user_profile/tasks.py @@ -8,6 +8,7 @@ from .utils import TokenGenerator from kolibri.core import error_constants from kolibri.core.auth.constants import role_kinds +from kolibri.core.auth.middleware import clear_user_cache_on_delete from kolibri.core.auth.models import FacilityUser from kolibri.core.auth.tasks import PeerImportSingleSyncJobValidator from kolibri.core.auth.utils.delete import delete_facility @@ -19,8 +20,8 @@ from kolibri.core.tasks.decorators import register_task from kolibri.core.tasks.job import JobStatus from kolibri.core.tasks.job import Priority +from kolibri.core.tasks.permissions import BasePermission from kolibri.core.tasks.permissions import IsFacilityAdmin -from kolibri.core.tasks.permissions import IsSelf from kolibri.core.tasks.permissions import IsSuperAdmin from kolibri.core.tasks.permissions import PermissionsFromAny from kolibri.core.tasks.utils import get_current_job @@ -112,6 +113,18 @@ def start_soud_sync(user_id): enqueue_soud_sync_processing() +class IsSelf(BasePermission): + def user_can_run_job(self, user, job): + return user.id == job.kwargs.get( + "local_user_id", None + ) or user.id == job.kwargs.get("remote_user_pk", None) + + def user_can_read_job(self, user, job): + return user.id == job.kwargs.get( + "local_user_id", None + ) or user.id == job.kwargs.get("remote_user_pk", None) + + @register_task( queue="soud", validator=MergeUserValidator, @@ -203,6 +216,10 @@ def mergeuser( user=remote_user, is_superuser=True, can_manage_content=True ) delete_facility(local_user.facility) + # Because delete_facility disables post delete signals + # we have to manually call the clear_user_cache_on_delete signal + # to ensure we don't leave around references to the deleted user + clear_user_cache_on_delete(None, local_user) set_device_settings(default_facility=remote_user.facility) else: local_user.delete() From 8eaaacd8d90c7f711841099b9a68541ef90c690b Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Tue, 30 Jul 2024 10:24:17 -0700 Subject: [PATCH 2/4] Add setuptools to dev and test requires to ensure proper installation of setup.py --- requirements/dev.txt | 1 + requirements/test.txt | 1 + 2 files changed, 2 insertions(+) diff --git a/requirements/dev.txt b/requirements/dev.txt index 01844a7e425..4a947f4d48f 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -1,3 +1,4 @@ +setuptools -r base.txt ipdb==0.13.2 flake8==3.8.3 diff --git a/requirements/test.txt b/requirements/test.txt index 872fb52727d..210570a1bcc 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -1,3 +1,4 @@ +setuptools # These are for testing beautifulsoup4==4.8.2 factory-boy==2.7.0 From 23d1316233bb043912728785d78437c89ad78ce1 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Tue, 30 Jul 2024 14:52:35 -0700 Subject: [PATCH 3/4] Create token in advance to avoid permissions issues. Handle 403s from successful user deletion as task success. --- .../views/ChangeFacility/MergeFacility.vue | 37 ++++++++++++++----- kolibri/plugins/user_profile/tasks.py | 18 ++++----- kolibri/plugins/user_profile/utils.py | 7 ++++ kolibri/plugins/user_profile/viewsets.py | 2 +- 4 files changed, 43 insertions(+), 21 deletions(-) diff --git a/kolibri/plugins/user_profile/assets/src/views/ChangeFacility/MergeFacility.vue b/kolibri/plugins/user_profile/assets/src/views/ChangeFacility/MergeFacility.vue index 33f2bfddece..8442769dc62 100644 --- a/kolibri/plugins/user_profile/assets/src/views/ChangeFacility/MergeFacility.vue +++ b/kolibri/plugins/user_profile/assets/src/views/ChangeFacility/MergeFacility.vue @@ -228,16 +228,33 @@ isPolling = false; }); } else { - TaskResource.fetchModel({ id: taskId.value, force: true }).then(startedTask => { - task.value = startedTask; - if (startedTask.status == TaskStatuses.COMPLETED) { + TaskResource.fetchModel({ id: taskId.value, force: true }) + .then(startedTask => { + task.value = startedTask; + if (startedTask.status == TaskStatuses.COMPLETED) { + isPolling = false; + } else if (startedTask.status === TaskStatuses.FAILED) { + TaskResource.clear(taskId.value); // start a new one + isTaskRequested = false; + taskError.value = true; + } + }) + .catch(err => { + if (err?.response?.status === 403 && task.value) { + // If we get a 403, it means that our currently logged in user + // does not have permission to access the task. This can happen + // if the user has been deleted by the task as intended, so assume + // the task has completed successfully. + task.value = { + ...task.value, + status: TaskStatuses.COMPLETED, + }; + } else { + // if the request is bad, we can't do anything + taskError.value = true; + } isPolling = false; - } else if (startedTask.status === TaskStatuses.FAILED) { - TaskResource.clear(taskId.value); // start a new one - isTaskRequested = false; - taskError.value = true; - } - }); + }); } if (isPolling) { @@ -265,7 +282,7 @@ method: 'POST', data: params, }).then(() => { - redirectBrowser(urls['kolibri:kolibri.plugins.learn:learn']()); + redirectBrowser(); }); } diff --git a/kolibri/plugins/user_profile/tasks.py b/kolibri/plugins/user_profile/tasks.py index 95158f2a7df..48498bc9404 100644 --- a/kolibri/plugins/user_profile/tasks.py +++ b/kolibri/plugins/user_profile/tasks.py @@ -59,6 +59,12 @@ def validate(self, data): job_data["args"].append(data["local_user_id"].id) job_data["extra_metadata"].update(user_fullname=data["local_user_id"].full_name) + # create token to validate user in the new facility + # after it's deleted in the current facility: + remote_user_pk = job_data["kwargs"]["user"] + token = TokenGenerator().make_token(remote_user_pk) + job_data["extra_metadata"]["token"] = token + job_data["extra_metadata"]["remote_user_pk"] = remote_user_pk if data.get("new_superuser_id"): job_data["kwargs"]["new_superuser_id"] = data["new_superuser_id"].id if data.get("set_as_super_user"): @@ -190,16 +196,6 @@ def mergeuser( user=new_superuser, is_superuser=True, can_manage_content=True ) - # create token to validate user in the new facility - # after it's deleted in the current facility: - remote_user_pk = kwargs["user"] - remote_user = FacilityUser.objects.get(pk=remote_user_pk) - token = TokenGenerator().make_token(remote_user) - job.extra_metadata["token"] = token - job.extra_metadata["remote_user_pk"] = remote_user_pk - job.save_meta() - job.update_progress(1.0, 1.0) - try: # If the local user is associated with an OSUser # then transfer to the new remote user to maintain @@ -226,3 +222,5 @@ def mergeuser( # queue up this new user for syncing with any other devices start_soud_sync(remote_user.id) + + job.update_progress(1.0, 1.0) diff --git a/kolibri/plugins/user_profile/utils.py b/kolibri/plugins/user_profile/utils.py index 22557e4b0c7..44cff53f9bb 100644 --- a/kolibri/plugins/user_profile/utils.py +++ b/kolibri/plugins/user_profile/utils.py @@ -13,6 +13,13 @@ class TokenGenerator(PasswordResetTokenGenerator): - expires the token after some seconds, instead of one day """ + def _make_hash_value(self, user_id, timestamp): + """ + Override the hash value to only need the user_id and timestamp + to allow us to calculate before we have imported the remote user. + """ + return f"{user_id}{timestamp}" + def make_token(self, user): """ Returns a token that can be used for TOKEN_EXPIRE_LIMIT seconds diff --git a/kolibri/plugins/user_profile/viewsets.py b/kolibri/plugins/user_profile/viewsets.py index 63d596538a9..a501bb149e2 100644 --- a/kolibri/plugins/user_profile/viewsets.py +++ b/kolibri/plugins/user_profile/viewsets.py @@ -90,7 +90,7 @@ def post(self, request): pk = request.data.get("pk", None) token = request.data.get("token", None) new_user = FacilityUser.objects.get(pk=pk) - if not TokenGenerator().check_token(new_user, token): + if not TokenGenerator().check_token(new_user.id, token): return Response({"error": "Unauthorized"}, status=401) login(request, new_user) return Response({"success": True}) From 904ca1cba83cb0df701884293ba19fadbb438a15 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Tue, 30 Jul 2024 14:53:01 -0700 Subject: [PATCH 4/4] Update our inClasses boolean when we fetch classes from the backend to avoid mismatches in state. --- kolibri/plugins/learn/assets/src/views/HomePage/index.vue | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/kolibri/plugins/learn/assets/src/views/HomePage/index.vue b/kolibri/plugins/learn/assets/src/views/HomePage/index.vue index c8cd0161da5..2eb4cfdec26 100644 --- a/kolibri/plugins/learn/assets/src/views/HomePage/index.vue +++ b/kolibri/plugins/learn/assets/src/views/HomePage/index.vue @@ -63,7 +63,7 @@