Canvas Sync: Add SIS viewing permissions check (#610)

ekassos · web-flow · commit 6f30aa389f5b · 2024-11-15T16:57:06.000-05:00
Closes #605 by introducing a permissions check right before users select a class to connect with Canvas. This check is introduced in `verify_access` under the `canvas` module, and is run twice: once when the user selects the class, and once when they save the class. If `missing_sso_ids` is true when syncing, the user will need to reconnect their account.
diff --git a/pingpong/canvas.py b/pingpong/canvas.py
@@ -466,8 +466,15 @@ async def _roster_access_check(self, course_id: str) -> bool:
         request_url = f"/api/v1/courses/{course_id}/users"
 
         async for result in self._request_all_pages(request_url):
-            return bool(result)
-
+            if not result:
+                return False
+            for user in result:
+                if self.config.sso_target and not user.get(self.config.sso_target):
+                    raise CanvasException(
+                        code=403,
+                        detail="You do not have permission to access SIS information for this class. Please ask another privileged user to set up Canvas Sync. If you're still facing issues, contact your Canvas administrator.",
+                    )
+                return True
         return False
 
     async def verify_access(self, course_id: str) -> None:
@@ -562,12 +569,12 @@ def _process_users(
                 if enrollment["type"] in ["TeacherEnrollment", "TaEnrollment"]:
                     is_teacher = True
                     break
-            if not user.get(self.config.sso_target):
+            if self.config.sso_target and not user.get(self.config.sso_target):
                 logging.warning(
-                    f"User {user['email']} does not have an SSO ID in the Canvas response. Skipping."
+                    f"User {user['email']} does not have an SSO ID in the Canvas response. Marking class as having a sync error."
                 )
                 self.missing_sso_ids = True
-                continue
+                break
             yield CreateUserClassRole(
                 email=user["email"],
                 sso_id=user.get(self.config.sso_target),
@@ -625,8 +632,12 @@ async def sync_roster(self) -> None:
             lms_tenant=self.config.tenant,
             lms_type=self.config.type,
             sso_tenant=self.config.sso_tenant,
-            missing_some_sso_ids=self.missing_sso_ids,
         )
+        if self.missing_sso_ids:
+            raise CanvasException(
+                code=403,
+                detail="Some users in the Canvas class do not have SIS information. Please ask another privileged user to set up Canvas Sync. If you're still facing issues, contact your Canvas administrator.",
+            )
         await self._update_user_roles()
         await Class.update_last_synced(self.db, self.class_id)
 
diff --git a/pingpong/schemas.py b/pingpong/schemas.py
@@ -536,7 +536,6 @@ class CreateUserClassRoles(BaseModel):
     lms_tenant: str | None = None
     lms_type: LMSType | None = None
     sso_tenant: str | None = None
-    missing_some_sso_ids: bool = False
 
 
 class LMSClass(BaseModel):
diff --git a/pingpong/server.py b/pingpong/server.py
@@ -951,6 +951,60 @@ async def get_canvas_classes(class_id: str, tenant: str, request: Request):
             ) from e
 
 
+@v1.post(
+    "/class/{class_id}/canvas/{tenant}/classes/{canvas_class_id}/verify",
+    dependencies=[Depends(Authz("can_edit_info", "class:{class_id}"))],
+    response_model=schemas.GenericStatus,
+)
+async def verify_canvas_class_permissions(
+    class_id: str, tenant: str, canvas_class_id: str, request: Request
+):
+    canvas_settings = get_canvas_config(tenant)
+    async with LightweightCanvasClient(
+        canvas_settings,
+        int(class_id),
+        request,
+    ) as client:
+        try:
+            await client.verify_access(canvas_class_id)
+            return {"status": "ok"}
+        except CanvasException as e:
+            logger.exception(
+                "verify_canvas_class_permissions: CanvasException occurred"
+            )
+            raise HTTPException(
+                status_code=e.code or 500,
+                detail=e.detail
+                or "We faced an error while verifying your access to this Canvas class.",
+            )
+        except CanvasWarning as e:
+            logger.warning(
+                "verify_canvas_class_permissions: CanvasWarning occurred: %s", e.detail
+            )
+            raise HTTPException(
+                status_code=e.code or 500,
+                detail=e.detail
+                or "We faced an error while verifying your access to this Canvas class.",
+            ) from e
+        except ClientResponseError as e:
+            # If we get a 401 error, mark the class as having a sync error.
+            # Otherwise, just display an error message.
+            if e.code == 401:
+                await models.Class.mark_lms_sync_error(request.state.db, int(class_id))
+            logger.exception(
+                "verify_canvas_class_permissions: ClientResponseError occurred"
+            )
+            raise HTTPException(
+                status_code=e.code, detail="Canvas returned an error: " + e.message
+            )
+        except Exception:
+            logger.exception("verify_canvas_class_permissions: Exception occurred")
+            raise HTTPException(
+                status_code=500,
+                detail="We faced an internal error while verifying your access to this Canvas class.",
+            )
+
+
 @v1.post(
     "/class/{class_id}/canvas/{tenant}/classes/{canvas_class_id}",
     dependencies=[Depends(Authz("can_edit_info", "class:{class_id}"))],
@@ -1025,6 +1079,8 @@ async def sync_canvas_class(
                 status_code=e.code, detail="Canvas returned an error: " + e.message
             )
         except (CanvasException, AddUserException) as e:
+            if e.code == 403:
+                await models.Class.mark_lms_sync_error(request.state.db, int(class_id))
             logger.exception(
                 "sync_canvas_class: CanvasException or AddUserException occurred"
             )
diff --git a/pingpong/users.py b/pingpong/users.py
@@ -403,8 +403,7 @@ async def add_new_users(self) -> schemas.CreateUserResults:
             self.send_invites()
 
         if self.new_ucr.lms_tenant:
-            if not self.new_ucr.missing_some_sso_ids:
-                await self._remove_deleted_users()
+            await self._remove_deleted_users()
             await self._merge_accounts()
 
         await self.client.write_safe(grant=grants, revoke=self.revokes)
diff --git a/web/pingpong/src/lib/api.ts b/web/pingpong/src/lib/api.ts
@@ -2043,6 +2043,16 @@ export const saveCanvasClass = async (
   return await POST<never, GenericStatus>(f, url);
 };
 
+export const verifyCanvasClass = async (
+  f: Fetcher,
+  classId: number,
+  tenant: string,
+  canvasClassId: string
+) => {
+  const url = `class/${classId}/canvas/${tenant}/classes/${canvasClassId}/verify`;
+  return await POST<never, GenericStatus>(f, url);
+};
+
 export const syncCanvasClass = async (f: Fetcher, classId: number, tenant: string) => {
   const url = `class/${classId}/canvas/${tenant}/sync`;
   return await POST<never, GenericStatus>(f, url);
diff --git a/web/pingpong/src/routes/group/[classId]/manage/+page.svelte b/web/pingpong/src/routes/group/[classId]/manage/+page.svelte
@@ -51,7 +51,8 @@
     UserRemoveSolid,
     FileLinesOutline,
     ExclamationCircleOutline,
-    LockSolid
+    LockSolid,
+    CheckCircleOutline
   } from 'flowbite-svelte-icons';
   import { sadToast, happyToast } from '$lib/toast';
   import { humanSize } from '$lib/size';
@@ -429,9 +430,13 @@
   }, {});
   $: selectedClassName = classNameDict[selectedClass] || 'Select a class...';
 
-  const updateSelectedClass = (classValue: string) => {
+  const updateSelectedClass = async (classValue: string) => {
+    canvasClassVerified = false;
+    canvasClassBeingVerified = true;
+    canvasClassVerificationError = '';
     classSelectDropdownOpen = false;
     selectedClass = classValue;
+    await verifyCanvasClass();
   };
 
   const saveSelectedClass = async () => {
@@ -448,6 +453,24 @@
     }
   };
 
+  let canvasClassVerified = false;
+  let canvasClassBeingVerified = false;
+  let canvasClassVerificationError = '';
+
+  const verifyCanvasClass = async () => {
+    canvasClassBeingVerified = true;
+    const result = await api.verifyCanvasClass(fetch, data.class.id, 'harvard', selectedClass);
+    const response = api.expandResponse(result);
+    if (response.error) {
+      canvasClassVerificationError =
+        response.error.detail ||
+        'There was an issue while trying to verify your access to the class roster. Try again later.';
+    } else {
+      canvasClassVerified = true;
+    }
+    canvasClassBeingVerified = false;
+  };
+
   let syncingCanvasClass = false;
   const syncClass = async () => {
     syncingCanvasClass = true;
@@ -975,27 +998,67 @@
                     />
                   </DropdownContainer>
                   <div class="flex gap-2 items-center">
+                    {#if canvasClassBeingVerified}
+                      <Spinner color="yellow" class="w-6 h-6" />
+                      <Tooltip
+                        defaultClass="text-wrap py-2 px-3 mr-5 text-sm font-light shadow-sm"
+                        arrow={false}
+                        >We're verifying your access to the class roster. This shouldn't take long.</Tooltip
+                      >
+                    {:else if canvasClassVerified}
+                      <CheckCircleOutline class="w-6 h-6 text-amber-800" />
+                      <Tooltip
+                        defaultClass="text-wrap py-2 px-3 mr-5 text-sm font-light shadow-sm"
+                        arrow={false}>Your access to the class roster has been verified.</Tooltip
+                      >
+                    {:else if canvasClassVerificationError}
+                      <ExclamationCircleOutline class="w-6 h-6 text-amber-800" />
+                      <Tooltip
+                        defaultClass="text-wrap py-2 px-3 mr-5 text-sm font-light shadow-sm"
+                        arrow={false}>{canvasClassVerificationError}</Tooltip
+                      >
+                    {:else if !canvasClassVerified}
+                      <CheckCircleOutline class="w-6 h-6 text-amber-800 text-opacity-25" />
+                      <Tooltip
+                        defaultClass="text-wrap py-2 px-3 mr-5 text-sm font-light shadow-sm"
+                        arrow={false}
+                        >We'll verify your permissions to access the class roster. Select a class to
+                        begin.</Tooltip
+                      >
+                    {/if}
                     <Button
                       pill
                       size="xs"
                       class="shrink-0 max-h-fit border border-amber-900 bg-gradient-to-t from-amber-900 to-amber-800 text-white hover:from-amber-800 hover:to-amber-700"
                       on:click={saveSelectedClass}
                       on:touchstart={saveSelectedClass}
-                      disabled={loadingCanvasClasses || !selectedClass}
+                      disabled={loadingCanvasClasses ||
+                        !selectedClass ||
+                        canvasClassBeingVerified ||
+                        !canvasClassVerified}
                     >
                       Save</Button
                     >
                     <Button
                       pill
                       size="xs"
                       class="shrink-0 max-h-fit border border-gray-400 bg-gradient-to-t from-gray-100 to-gray-100 text-gray-800 hover:from-gray-200 hover:to-gray-100"
+                      disabled={loadingCanvasClasses || canvasClassBeingVerified}
                       on:click={() => {
                         $loadedCanvasClasses = [];
                         selectedClass = '';
+                        canvasClassVerified = false;
+                        canvasClassBeingVerified = false;
+                        canvasClassVerificationError = '';
+                        classSelectDropdownOpen = false;
                       }}
                       on:touchstart={() => {
                         $loadedCanvasClasses = [];
                         selectedClass = '';
+                        canvasClassVerified = false;
+                        canvasClassBeingVerified = false;
+                        canvasClassVerificationError = '';
+                        classSelectDropdownOpen = false;
                       }}
                     >
                       Cancel</Button
@@ -1188,16 +1251,16 @@
                 <span class="text-lg font-medium">Important: Reconnect your Canvas account</span>
               </div>
               <p class="mt-2 text-sm">
-                We faced an issue when trying to connect to your Canvas account. Use the
-                reconnection button below to reauthorize Pingpong to access your Canvas account and
-                ensure uninterrupted syncing of your class roster.
+                We faced an issue when trying to get the class roster from your Canvas account. Use
+                the reconnection button below to reauthorize Pingpong to access your Canvas account
+                and ensure uninterrupted syncing of your class roster.
               </p>
               <p class="mt-2 mb-4 text-sm">
                 Last sync: {data.class.lms_last_synced
                   ? dayjs.utc(data.class.lms_last_synced).fromNow()
                   : 'never'}
               </p>
-              <div class="flex gap-2">
+              <div class="flex flex-row justify-between items-center">
                 <Button
                   pill
                   size="xs"
@@ -1207,7 +1270,23 @@
                 >
                   <RefreshOutline class="w-4 h-4 me-2" />Reconnect Canvas account</Button
                 >
+                <Button
+                  pill
+                  size="xs"
+                  class="border border-red-900 hover:bg-red-900 text-red-900 hover:bg-gradient-to-t hover:from-red-800 hover:to-red-700 hover:text-white"
+                  on:click={() => (disconnectCanvas = true)}
+                  on:touchstart={() => (disconnectCanvas = true)}
+                >
+                  <UserRemoveSolid class="w-4 h-4 me-2" />Disconnect Canvas account</Button
+                >
               </div>
+              <Modal bind:open={disconnectCanvas} size="sm" autoclose>
+                <CanvasDisconnectModal
+                  canvasCourseCode={data.class.lms_class?.course_code || ''}
+                  on:keep={() => removeCanvasConnection(true)}
+                  on:remove={() => removeCanvasConnection(false)}
+                />
+              </Modal>
             </div>
           </Alert>
         {:else if data.class.lms_status === 'error'}
