From 03e4afdb39febc75ffc663b0b2abbe22a61bbac2 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Wed, 26 Apr 2023 14:27:21 +0800 Subject: [PATCH] MDL-78132 badges: move apiBase consumption to backpack The logic to create the issuer has been moved to the backpack form in order to improve the workflow and update the apiBase with the proper value comming from the badgeconnect.json manifest file. So, as part of this change in the workflow, the following changes has been also implemented (to make the UI easier for users): - The "Open Badges" oAuth issuer button has been removed from the "OAuth Services" admin page. As they are created/updated when a backpack is saved, this button is not required anymore. - The "OAuth2 services" and "Backpack API URL" parameters have been removed from the Manage backpacks form, because they are created on the fly each time the backpack is saved. --- admin/tool/oauth2/issuers.php | 6 ---- .../oauth2/tests/behat/basic_settings.feature | 34 ------------------ badges/classes/form/external_backpack.php | 36 +++++++++---------- badges/mybackpack.php | 2 +- badges/tests/behat/backpack.feature | 10 ++++-- lib/badgeslib.php | 34 ++++++++++++++++++ 6 files changed, 60 insertions(+), 62 deletions(-) diff --git a/admin/tool/oauth2/issuers.php b/admin/tool/oauth2/issuers.php index 79d04c87f98..35156a77bcc 100644 --- a/admin/tool/oauth2/issuers.php +++ b/admin/tool/oauth2/issuers.php @@ -210,12 +210,6 @@ if ($mform && $mform->is_cancelled()) { $addurl = new moodle_url('/admin/tool/oauth2/issuers.php', $params); echo $renderer->single_button($addurl, get_string('nextcloud_service', 'tool_oauth2')); - // IMS Open Badges Connect template. - $docs = 'admin/tool/oauth2/issuers/imsobv2p1'; - $params = ['action' => 'edittemplate', 'type' => 'imsobv2p1', 'sesskey' => sesskey(), 'docslink' => $docs]; - $addurl = new moodle_url('/admin/tool/oauth2/issuers.php', $params); - echo $renderer->single_button($addurl, get_string('imsobv2p1_service', 'tool_oauth2')); - // Linkedin template. $docs = 'admin/tool/oauth2/issuers/linkedin'; $params = ['action' => 'edittemplate', 'type' => 'linkedin', 'sesskey' => sesskey(), 'docslink' => $docs]; diff --git a/admin/tool/oauth2/tests/behat/basic_settings.feature b/admin/tool/oauth2/tests/behat/basic_settings.feature index 565cc356d16..f45a2c21019 100644 --- a/admin/tool/oauth2/tests/behat/basic_settings.feature +++ b/admin/tool/oauth2/tests/behat/basic_settings.feature @@ -142,40 +142,6 @@ Feature: Basic OAuth2 functionality And I should see "Identity issuer deleted" And I should not see "Testing service modified" - Scenario: Create, edit and delete standard service for Open Badges - Given I press "Open Badges" - And I should see "Create new service: Open Badges" - And I set the following fields to these values: - | Client ID | thisistheclientid | - | Client secret | supersecret | - | Service base URL | https://dc.imsglobal.org/ | - When I press "Save changes" - Then I should see "Changes saved" - And I should see "IMS Global Reference Implementation" - And I click on "Edit" "link" in the "IMS Global Reference Implementation" "table_row" - And I set the following fields to these values: - | Name | IMS Global | - And I press "Save changes" - And I should see "Changes saved" - And I should see "IMS Global" - And "Allow services" "icon" should exist in the "IMS Global" "table_row" - And "Do not allow login" "icon" should exist in the "IMS Global" "table_row" - And "Service discovery successful" "icon" should exist in the "IMS Global" "table_row" - And the "src" attribute of "table.admintable th img" "css_element" should contain "IMS-Global-Logo.png" - And I click on "Configure endpoints" "link" in the "IMS Global" "table_row" - And I should see "https://dc.imsglobal.org/.well-known/badgeconnect.json" in the "discovery_endpoint" "table_row" - And I should see "authorization_endpoint" - And I navigate to "Server > OAuth 2 services" in site administration - And I click on "Configure user field mappings" "link" in the "IMS Global" "table_row" - And I should not see "given_name" - And I should not see "middle_name" - And I navigate to "Server > OAuth 2 services" in site administration - And I click on "Delete" "link" in the "IMS Global" "table_row" - And I should see "Are you sure you want to delete the identity issuer \"IMS Global\"?" - And I press "Continue" - And I should see "Identity issuer deleted" - And I should not see "IMS Global" - Scenario: Create, edit and delete valid custom OIDC service Given I press "Custom" And I should see "Create new service: Custom" diff --git a/badges/classes/form/external_backpack.php b/badges/classes/form/external_backpack.php index 3d632760df8..06ea4a49135 100644 --- a/badges/classes/form/external_backpack.php +++ b/badges/classes/form/external_backpack.php @@ -53,21 +53,20 @@ class external_backpack extends \moodleform { $mform->addElement('hidden', 'action', 'edit'); $mform->setType('action', PARAM_ALPHA); - $mform->addElement('text', 'backpackapiurl', get_string('backpackapiurl', 'core_badges')); - $mform->setType('backpackapiurl', PARAM_URL); - $mform->addRule('backpackapiurl', null, 'required', null, 'client'); - $mform->addRule('backpackapiurl', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); + $apiversions = badges_get_badge_api_versions(); + $mform->addElement('select', 'apiversion', get_string('apiversion', 'core_badges'), $apiversions); + $mform->setType('apiversion', PARAM_RAW); + $mform->setDefault('apiversion', OPEN_BADGES_V2P1); + $mform->addRule('apiversion', null, 'required', null, 'client'); $mform->addElement('text', 'backpackweburl', get_string('backpackweburl', 'core_badges')); $mform->setType('backpackweburl', PARAM_URL); $mform->addRule('backpackweburl', null, 'required', null, 'client'); $mform->addRule('backpackweburl', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); - $apiversions = badges_get_badge_api_versions(); - $mform->addElement('select', 'apiversion', get_string('apiversion', 'core_badges'), $apiversions); - $mform->setType('apiversion', PARAM_RAW); - $mform->setDefault('apiversion', OPEN_BADGES_V2P1); - $mform->addRule('apiversion', null, 'required', null, 'client'); + $mform->addElement('text', 'backpackapiurl', get_string('backpackapiurl', 'core_badges')); + $mform->setType('backpackapiurl', PARAM_URL); + $mform->addRule('backpackapiurl', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); $mform->addElement('hidden', 'id', ($backpack->id ?? null)); $mform->setType('id', PARAM_INT); @@ -86,11 +85,6 @@ class external_backpack extends \moodleform { $issuercontact = $CFG->badges_defaultissuercontact; $this->add_auth_fields($issuercontact); - $oauth2options = badges_get_oauth2_service_options(); - $mform->addElement('select', 'oauth2_issuerid', get_string('oauth2issuer', 'core_badges'), $oauth2options); - $mform->setType('oauth2_issuerid', PARAM_INT); - $mform->hideIf('oauth2_issuerid', 'apiversion', 'neq', '2.1'); - if ($backpack) { $this->set_data($backpack); } @@ -99,7 +93,8 @@ class external_backpack extends \moodleform { $mform->hideIf('backpackemail', 'includeauthdetails'); $mform->hideIf('backpackemail', 'apiversion', 'in', [OPEN_BADGES_V2P1]); $mform->hideIf('password', 'includeauthdetails'); - $mform->hideIf('password', 'apiversion', 'in', [1, OPEN_BADGES_V2P1]); + $mform->hideIf('password', 'apiversion', 'in', [OPEN_BADGES_V1, OPEN_BADGES_V2P1]); + $mform->hideIf('backpackapiurl', 'apiversion', 'in', [OPEN_BADGES_V1, OPEN_BADGES_V2P1]); // Disable short forms. $mform->setDisableShortforms(); @@ -117,9 +112,14 @@ class external_backpack extends \moodleform { public function validation($data, $files) { $errors = parent::validation($data, $files); - // Ensure backpackapiurl and are valid URLs. - if (!empty($data['backpackapiurl']) && !preg_match('@^https?://.+@', $data['backpackapiurl'])) { - $errors['backpackapiurl'] = get_string('invalidurl', 'badges'); + // Ensure backpackapiurl and backpackweburl are valid URLs. + $isobv21 = isset($data['apiversion']) && $data['apiversion'] == OPEN_BADGES_V2P1; + if (!$isobv21) { + if (empty($data['backpackapiurl'])) { + $errors['backpackapiurl'] = get_string('err_required', 'form'); + } else if (!preg_match('@^https?://.+@', $data['backpackapiurl'])) { + $errors['backpackapiurl'] = get_string('invalidurl', 'badges'); + } } if (!empty($data['backpackweburl']) && !preg_match('@^https?://.+@', $data['backpackweburl'])) { $errors['backpackweburl'] = get_string('invalidurl', 'badges'); diff --git a/badges/mybackpack.php b/badges/mybackpack.php index 4220f5eaf9a..dfae36b4dca 100644 --- a/badges/mybackpack.php +++ b/badges/mybackpack.php @@ -129,7 +129,7 @@ if ($backpack) { // User input username/email/password on the backpack site // After confirm the scopes. redirect(new moodle_url('/badges/backpack-connect.php', ['backpackid' => $data->externalbackpackid])); - } else if ($data = $form->get_data()) { + } else { // The form may have been submitted under one of the following circumstances: // 1. After clicking 'Connect to backpack'. We'll have $data->email. // 2. After clicking 'Resend verification email'. We'll have $data->email. diff --git a/badges/tests/behat/backpack.feature b/badges/tests/behat/backpack.feature index 3f584b6d1f0..15e4f673faa 100644 --- a/badges/tests/behat/backpack.feature +++ b/badges/tests/behat/backpack.feature @@ -120,12 +120,15 @@ Feature: Backpack badges And I log in as "admin" And I navigate to "Badges > Manage backpacks" in site administration When I press "Add a new backpack" - And I set the field "backpackapiurl" to "http://backpackapiurl.cat" + And I set the field "apiversion" to "2" And I set the field "backpackweburl" to "aaa" And I press "Save changes" And I should see "Invalid URL" And I set the field "backpackweburl" to "http://backpackweburl.cat" And I press "Save changes" + And I should see "You must supply a value here" + And I set the field "backpackapiurl" to "http://backpackapiurl.cat" + And I press "Save changes" Then I should see "http://backpackweburl.cat" And "Delete" "icon" should exist in the "http://backpackweburl.cat" "table_row" And "Edit settings" "icon" should exist in the "http://backpackweburl.cat" "table_row" @@ -163,9 +166,9 @@ Feature: Backpack badges And I log in as "admin" And I navigate to "Badges > Manage backpacks" in site administration When I press "Add a new backpack" - And I set the field "backpackapiurl" to "http://backpackapiurl.cat" - And I set the field "backpackweburl" to "http://backpackweburl.cat" And I set the field "apiversion" to "2.1" + And I set the field "backpackweburl" to "http://backpackweburl.cat" + And I should not see "Backpack API URL" Then "Include authentication details with the backpack" "checkbox" should not be visible And I should not see "Badge issuer email address" And I should not see "Badge issuer password" @@ -180,6 +183,7 @@ Feature: Backpack badges And I should see "Badge issuer password" And I set the field "backpackemail" to "test@test.com" And I set the field "password" to "123456" + And I set the field "backpackapiurl" to "http://backpackapiurl.cat" And I press "Save changes" And I click on "Edit" "link" in the "http://backpackweburl.cat" "table_row" And the field "Include authentication details with the backpack" matches value "1" diff --git a/lib/badgeslib.php b/lib/badgeslib.php index d1f4d5ff5f0..f7225ec1b3f 100644 --- a/lib/badgeslib.php +++ b/lib/badgeslib.php @@ -798,6 +798,40 @@ function badges_delete_site_backpack($id) { */ function badges_save_external_backpack(stdClass $data) { global $DB; + if ($data->apiversion == OPEN_BADGES_V2P1) { + // Check if there is an existing issuer for the given backpackapiurl. + foreach (core\oauth2\api::get_all_issuers() as $tmpissuer) { + if ($data->backpackweburl == $tmpissuer->get('baseurl')) { + $issuer = $tmpissuer; + break; + } + } + + // Create the issuer if it doesn't exist yet. + if (empty($issuer)) { + $issuer = new \core\oauth2\issuer(0, (object) [ + 'name' => $data->backpackweburl, + 'baseurl' => $data->backpackweburl, + // Note: This is required because the DB schema is broken and does not accept a null value when it should. + 'image' => '', + ]); + $issuer->save(); + } + + // This can't be run from PHPUNIT because testing platforms need real URLs. + // In the future, this request can be moved to the moodle-exttests repository. + if (!PHPUNIT_TEST) { + // Create/update the endpoints for the issuer. + \core\oauth2\discovery\imsbadgeconnect::create_endpoints($issuer); + $data->oauth2_issuerid = $issuer->get('id'); + + $apibase = \core\oauth2\endpoint::get_record([ + 'issuerid' => $data->oauth2_issuerid, + 'name' => 'apiBase', + ]); + $data->backpackapiurl = $apibase->get('url'); + } + } $backpack = new stdClass(); $backpack->apiversion = $data->apiversion;