PM-48081 Only open /lti/start in embeds, else always open /lti/login

kialo · Feb 12, 2025 · 34caea2 · 34caea2
1 parent 754f3bb
commit 34caea2
Show file tree

Hide file tree

Showing 5 changed files with 53 additions and 19 deletions.
diff --git a/classes/kialo_config.php b/classes/kialo_config.php
@@ -144,16 +144,16 @@ public function get_platform(): Platform {
 
     /**
      * Returns the LTI tool interface representing kialo-edu.com.
-     * @param bool|null $deeplink Whether the tool is being used in a deeplink flow, defaults to false.
+     * @param bool|null $embedded Whether the tool is being shown in an embed within Moodle or in a new tab.
      * @return Tool
      */
-    public function get_tool(?bool $deeplink = false): Tool {
+    public function get_tool(?bool $embedded = false): Tool {
         $toolurl = $this->get_tool_url();
         return new Tool(
             'kialo-edu',                // Identifier.
             'Kialo Edu',                // Name.
             $toolurl,                   // Audience.
-            $toolurl . ($deeplink ? "/lti/login" : "/lti/start"),    // OIDC initiation url.
+            $toolurl . ($embedded ? "/lti/start" : "/lti/login"),    // OIDC initiation url.
             $toolurl . '/lti/launch',   // Launch url.
             $toolurl . '/lti/deeplink'  // Deep linking url.
         );
@@ -162,12 +162,12 @@ public function get_tool(?bool $deeplink = false): Tool {
     /**
      * Creates a new LTI tool registration for Kialo and one specific deployment id.
      * @param string|null $deploymentid The deployment id to use, or null, if it's not relevant.
-     * @param bool|null $deeplink Whether the registration is being created for a deeplink flow, defaults to false.
+     * @param bool|null $embedded Whether to display the Kialo app embedded in Moodle or in a new window.
      * @return Registration
      * @throws \dml_exception
      */
-    public function create_registration(?string $deploymentid = null, ?bool $deeplink = false): Registration {
-        $tool = $this->get_tool($deeplink);
+    public function create_registration(?string $deploymentid = null, ?bool $embedded = true): Registration {
+        $tool = $this->get_tool($embedded);
         $platformjwksurl = (new moodle_url('/mod/kialo/lti_jwks.php'))->out();
         $tooljwksurl = $this->get_tool_url() . "/lti/jwks.json";
         $deploymentids = $deploymentid ? [$deploymentid] : [];

diff --git a/classes/lti_flow.php b/classes/lti_flow.php
@@ -92,9 +92,9 @@ class lti_flow {
      * @param string $deploymentid The unique deployment ID of this activity (used to link the discussion on Kialo's side).
      * @param string $moodleuserid The Moodle user ID of the user that is launching the activity.
      * @param int $courseid The Moodle course ID of the course that the activity is in.
+     * @param bool $embedded Whether to display the Kialo app embedded in Moodle or in a new window.
      * @param array $roles The LTI roles to assign to the user, e.g. Instructor or Learner.
      * @param array $optionalclaims Optional claims to include in the LTI message.
-     * @param bool|null $deeplink Whether this is a deep linking flow. Defaults to false.
      * @return LtiMessageInterface The LTI message that can be used to launch Kialo.
      * @throws LtiExceptionInterface
      * @throws \dml_exception
@@ -105,12 +105,12 @@ private static function build_platform_originating_launch(
         string $deploymentid,
         string $moodleuserid,
         int $courseid,
+        bool $embedded,
         array $roles,
-        array $optionalclaims,
-        ?bool $deeplink = false
+        array $optionalclaims
     ): LtiMessageInterface {
         $kialoconfig = kialo_config::get_instance();
-        $registration = $kialoconfig->create_registration($deploymentid, $deeplink);
+        $registration = $kialoconfig->create_registration($deploymentid, $embedded);
 
         // In lti_auth.php we require the user to be logged into Moodle and have permissions on the course.
         // We also assert that it's the same moodle user that was used in the first step.
@@ -197,6 +197,7 @@ public static function init_resource_link(
         string $deploymentid,
         string $moodleuserid,
         string $discussionurl,
+        bool $embedded,
         ?string $groupid = null,
         ?string $groupname = null
     ): LtiMessageInterface {
@@ -216,6 +217,7 @@ public static function init_resource_link(
             $deploymentid,
             $moodleuserid,
             $courseid,
+            $embedded,
             $roles,
             [
                 // See https://www.imsglobal.org/spec/lti/v1p3#resource-link-claim.
@@ -333,6 +335,7 @@ public static function init_deep_link(int $courseid, string $moodleuserid, strin
             $deploymentid,
             $moodleuserid,
             $courseid,
+            false,
             ['http://purl.imsglobal.org/vocab/lis/v2/membership#Instructor'], // Only teachers can deeplink.
             [
                 new DeepLinkingSettingsClaim(
@@ -353,7 +356,6 @@ public static function init_deep_link(int $courseid, string $moodleuserid, strin
                     "id" => $courseid,
                 ],
             ],
-            true,
         );
     }
 

diff --git a/tests/classes/kialo_config_test.php b/tests/classes/kialo_config_test.php
@@ -111,16 +111,17 @@ public function test_get_platform(): void {
 
     /**
      * Tests the tool configuration.
-     * @param bool $usedeeplink Whether to use deep linking.
+     *
+     * @param bool $embedded Whether to display within embed.
      * @param string $expectedoidcinitiationurl The expected OIDC initiation URL.
      * @covers \mod_kialo\kialo_config::get_instance::get_tool
      * @dataProvider tool_provider
      */
-    public function test_get_tool_parametrized(bool $usedeeplink, string $expectedoidcinitiationurl): void {
+    public function test_get_tool_parametrized(bool $embedded, string $expectedoidcinitiationurl): void {
         // Ensure no environment variable interferes.
         putenv("TARGET_KIALO_URL=");
         // Get the tool with the parameter.
-        $tool = kialo_config::get_instance()->get_tool($usedeeplink);
+        $tool = kialo_config::get_instance()->get_tool($embedded);
 
         // Assert the common values.
         $this->assertEquals("kialo-edu", $tool->getIdentifier());
@@ -140,9 +141,9 @@ public function test_get_tool_parametrized(bool $usedeeplink, string $expectedoi
     public static function tool_provider(): array {
         return [
             // When not using deep linking.
-            'normal tool' => [false, "https://www.kialo-edu.com/lti/start"],
+            'embedded' => [true, "https://www.kialo-edu.com/lti/start"],
             // When using deep linking.
-            'deeplink tool' => [true, "https://www.kialo-edu.com/lti/login"],
+            'new window' => [false, "https://www.kialo-edu.com/lti/login"],
         ];
     }
 

diff --git a/tests/classes/lti_flow_test.php b/tests/classes/lti_flow_test.php
@@ -336,6 +336,29 @@ public function test_assign_lti_roles(array $moodleroles, array $expectedltirole
         $this->assertEquals($expectedltiroles, $role);
     }
 
+    /**
+     * Tests the platform configuration.
+     *
+     * @covers \mod_kialo\lti_flow::init_resource_link
+     */
+    public function test_init_non_embedded_resource_link(): void {
+        $this->getDataGenerator()->enrol_user($this->user->id, $this->course->id, "teacher");
+
+        // Construct the initial LTI message sent to Kialo when the user clicks on the activity.
+        $deploymentid = "random-string-123";
+        $discussionurl = "random-discussion-url.com";
+        $message = lti_flow::init_resource_link(
+            $this->course->id,
+            $this->cmid,
+            $deploymentid,
+            $this->user->id,
+            $discussionurl,
+            false
+        );
+
+        $this->assertStringContainsString('/lti/login', $message->getUrl());
+    }
+
     /**
      * Tests the initial LTI flow step, when Moodle redirects the user to Kialo.
      *
@@ -352,11 +375,15 @@ public function test_init_resource_link(): void {
             $this->cmid,
             $deploymentid,
             $this->user->id,
-            $discussionurl
+            $discussionurl,
+            true
         );
         $this->assertNotNull($message);
 
         $params = $message->getParameters()->jsonSerialize();
+
+        $this->assertStringContainsString('/lti/start', $message->getUrl());
+
         $this->assertEquals('https://www.example.com/moodle/mod/kialo', $params['iss']);
         $this->assertEquals($this->course->id . "/" . $this->user->id, $params['login_hint']);
         $this->assertEquals($discussionurl, $params['target_link_uri']);
@@ -421,6 +448,7 @@ public function test_init_resource_link_with_groups(): void {
             $deploymentid,
             $this->user->id,
             $discussionurl,
+            true,
             $groupinfo->groupid,
             $groupinfo->groupname
         );
@@ -477,6 +505,7 @@ public function test_init_resource_link_with_grouping(): void {
             $deploymentid,
             $this->user->id,
             $discussionurl,
+            true,
             $groupinfo->groupid,
             $groupinfo->groupname
         );

diff --git a/view.php b/view.php
@@ -54,7 +54,6 @@
 }
 
 $context = context_module::instance($cm->id);
-
 require_login($course, false, $cm);
 require_capability('mod/kialo:view', $context);
 
@@ -64,13 +63,16 @@
 
 $groupinfo = kialo_view::get_current_group_info($cm, $course);
 
+$embedded = $moduleinstance->display === MOD_KIALO_DISPLAY_IN_NEW_WINDOW;
+
 try {
     $message = lti_flow::init_resource_link(
         $course->id,
         $cm->id,
         KIALO_LTI_DEPLOYMENT_ID,
         $USER->id,
         $moduleinstance->discussion_url,
+        $embedded,
         $groupinfo->groupid,
         $groupinfo->groupname,
     );
@@ -79,7 +81,7 @@
     $PAGE->set_url('/mod/kialo/view.php', ['id' => $cm->id]);
     $PAGE->set_title($moduleinstance->name);
 
-    if ($moduleinstance->display === MOD_KIALO_DISPLAY_IN_NEW_WINDOW) {
+    if ($embedded) {
         echo $output->render(new loading_page(
             get_string("redirect_title", "mod_kialo"),
             get_string("redirect_loading", "mod_kialo"),