Merge pull request #12500 from rtibbles/safety_first

Add more error handling for facility user merging and facility deletion

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):

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):

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

kolibri/plugins/learn/assets/src/views/HomePage/index.vue

@@ -63,7 +63,7 @@
 <script>
 
   import { computed, getCurrentInstance } from 'kolibri.lib.vueCompositionApi';
-  import { get } from '@vueuse/core';
+  import { get, set } from '@vueuse/core';
   import client from 'kolibri.client';
   import urls from 'kolibri.urls';
   import useUser from 'kolibri.coreVue.composables.useUser';
@@ -75,6 +75,7 @@
     setResumableContentNodes,
   } from '../../composables/useLearnerResources';
   import { setContentNodeProgress } from '../../composables/useContentNodeProgress';
+  import { inClasses } from '../../composables/useCoreLearn';
   import { PageNames } from '../../constants';
   import AssignedLessonsCards from '../classes/AssignedLessonsCards';
   import AssignedQuizzesCards from '../classes/AssignedQuizzesCards';
@@ -171,6 +172,9 @@
         return client({ url: urls['kolibri:kolibri.plugins.learn:homehydrate']() }).then(
           response => {
             setClasses(response.data.classrooms);
+            // Update our hydrated class membership boolean in case it has changed
+            // since the learn page was opened.
+            set(inClasses, Boolean(response.data.classrooms.length));
             setResumableContentNodes(
               response.data.resumable_resources.results || [],
               response.data.resumable_resources.more || null,

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();
         });
       }
 

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
@@ -58,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"):
@@ -112,6 +119,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,
@@ -177,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
@@ -203,9 +212,15 @@ 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()
 
     # queue up this new user for syncing with any other devices
     start_soud_sync(remote_user.id)
+
+    job.update_progress(1.0, 1.0)

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

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})

requirements/dev.txt

@@ -1,3 +1,4 @@
+setuptools
 -r base.txt
 ipdb==0.13.2
 flake8==3.8.3

requirements/test.txt

@@ -1,3 +1,4 @@
+setuptools
 # These are for testing
 beautifulsoup4==4.8.2
 factory-boy==2.7.0