From 3878c854403bd415aced15dbd1ebd58dc0aa212f Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Wed, 24 May 2023 11:15:35 +0200 Subject: [PATCH] MDL-78102 badges: Check empty backpack email/password For backpack connection using OBv2.0, email and password can't be blank. This patch adds some extra checks to validate this and displays an error to the users before trying to connect to the backpack. Besides, the error displayed when any error is returned by the backpack (like invalid credentials), has also been improved, to make it clearer for the users. --- badges/backpackemailverify.php | 2 +- badges/classes/form/backpack.php | 47 +++++++++++++++++------------ badges/mybackpack.php | 3 +- badges/tests/behat/backpack.feature | 15 +++++++++ lang/en/badges.php | 4 ++- 5 files changed, 49 insertions(+), 22 deletions(-) diff --git a/badges/backpackemailverify.php b/badges/backpackemailverify.php index 73edaa49701..365331c28a2 100644 --- a/badges/backpackemailverify.php +++ b/badges/backpackemailverify.php @@ -50,7 +50,7 @@ if (!is_null($storedsecret)) { // Make sure we have all the required information before trying to save the connection. $backpackuid = $bp->authenticate(); if (empty($backpackuid) || !empty($backpackuid->error)) { - redirect(new moodle_url($redirect), get_string('backpackconnectionunexpectedresult', 'badges'), + redirect(new moodle_url($redirect), get_string('backpackconnectionunexpectedresult', 'badges', $backpackuid->error), null, \core\output\notification::NOTIFY_ERROR); } diff --git a/badges/classes/form/backpack.php b/badges/classes/form/backpack.php index e7341543b3a..917f65a6887 100644 --- a/badges/classes/form/backpack.php +++ b/badges/classes/form/backpack.php @@ -125,29 +125,38 @@ class backpack extends external_backpack { * Validates form data */ public function validation($data, $files) { - global $CFG; - - $errors = parent::validation($data, $files); if (badges_open_badges_backpack_api() == OPEN_BADGES_V2P1) { + return []; + } + + // We don't need to verify the email address if we're clearing a pending email verification attempt. + if (isset($data['revertbutton'])) { + return []; + } + + $errors = []; + // Email and password can't be blank. + if (empty($data['backpackemail'])) { + $errors['backpackemail'] = get_string('backpackemail_required', 'badges'); + } + if (empty($data['password'])) { + $errors['password'] = get_string('password_required', 'badges'); + } + if (!empty($errors)) { return $errors; } - // We don't need to verify the email address if we're clearing a pending email verification attempt. - if (!isset($data['revertbutton'])) { - $check = new stdClass(); - $check->email = $data['backpackemail']; - $check->password = $data['password']; - $sitebackpack = badges_get_site_backpack($data['externalbackpackid']); - $bp = new \core_badges\backpack_api($sitebackpack, $check); - $result = $bp->authenticate(); - if ($result === false || !empty($result->error)) { - $errors['backpackemail'] = get_string('backpackconnectionunexpectedresult', 'badges'); - $msg = $bp->get_authentication_error(); - if (!empty($msg)) { - $errors['backpackemail'] .= '

'; - $errors['backpackemail'] .= get_string('backpackconnectionunexpectedmessage', 'badges', $msg); - } - } + // Check the given credentials (email and password) are valid for this backpack. + $check = new stdClass(); + $check->email = $data['backpackemail']; + $check->password = $data['password']; + $sitebackpack = badges_get_site_backpack($data['externalbackpackid']); + $bp = new \core_badges\backpack_api($sitebackpack, $check); + + $result = $bp->authenticate(); + if ($result === false || !empty($result->error)) { + $msg = $bp->get_authentication_error(); + $errors['backpackemail'] = get_string('backpackconnectionunexpectedresult', 'badges', $msg); } return $errors; } diff --git a/badges/mybackpack.php b/badges/mybackpack.php index dfae36b4dca..809aafbcb0c 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 { + } else if ($data = $form->get_data()) { // 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. @@ -139,6 +139,7 @@ if ($backpack) { badges_disconnect_user_backpack($USER->id); redirect(new moodle_url('/badges/mybackpack.php')); } else if (isset($data->backpackemail)) { + // There are no errors, so the verification email can be sent. if (badges_send_verification_email($data->backpackemail, $data->externalbackpackid, $data->password)) { $a = get_user_preferences('badges_email_verify_backpackid'); redirect(new moodle_url('/badges/mybackpack.php'), diff --git a/badges/tests/behat/backpack.feature b/badges/tests/behat/backpack.feature index 0e6bf02b627..4376a1d3508 100644 --- a/badges/tests/behat/backpack.feature +++ b/badges/tests/behat/backpack.feature @@ -185,3 +185,18 @@ Feature: Backpack badges And I set the field "externalbackpackid" to "https://test.com/" And I should see "Email address" And I should see "Password" + + @javascript + Scenario: Check backpack form validation as a student + Given I log in as "student1" + And I follow "Preferences" in the user menu + And I follow "Backpack settings" + And I set the field "externalbackpackid" to "https://test.com/" + And I set the field "password" to "" + When I click on "Connect to backpack" "button" + Then I should see "Password can't be blank" + And I should not see "Email address can't be blank" + And I set the field "backpackemail" to "" + And I click on "Connect to backpack" "button" + And I should see "Email address can't be blank" + And I should see "Password can't be blank" diff --git a/lang/en/badges.php b/lang/en/badges.php index 463eb8b4c51..d1bef30e66a 100644 --- a/lang/en/badges.php +++ b/lang/en/badges.php @@ -105,11 +105,12 @@ $string['backpackconnection_help'] = 'Connecting to a backpack enables you to sh $string['backpackconnectioncancelattempt'] = 'Connect using a different email address'; $string['backpackconnectionconnect'] = 'Connect to backpack'; $string['backpackconnectionresendemail'] = 'Resend verification email'; -$string['backpackconnectionunexpectedresult'] = 'There was a problem connecting to your backpack. Please check the credentials and try again.'; +$string['backpackconnectionunexpectedresult'] = 'There was a problem connecting to your backpack. Your backpack provider is sending this message: \'{$a}\'.'; $string['backpackconnectionunexpectedmessage'] = 'The backpack returned the error: "{$a}".'; $string['backpackdetails'] = 'Backpack settings'; $string['backpackdisconnected'] = 'Backpack is disconnected'; $string['backpackemail'] = 'Email address'; +$string['backpackemail_required'] = 'Email address can\'t be blank'; $string['backpackemail_help'] = 'The email address associated with your backpack. While you are connected, any badges earned on this site will be associated with this email address.'; $string['backpackemailverificationpending'] = 'Verification pending'; $string['backpackemailverifyemailbody'] = 'Hi, @@ -458,6 +459,7 @@ $string['openbadgesv1'] = 'Open Badges v1.0'; $string['openbadgesv2'] = 'Open Badges v2.0'; $string['openbadgesv2p1'] = 'Open Badges v2.1'; $string['othernavigation'] = 'Other navigation ...'; +$string['password_required'] = 'Password can\'t be blank'; $string['potentialrecipients'] = 'Potential badge recipients'; $string['preferences'] = 'Badge preferences'; $string['privacy:metadata:backpack'] = 'A record of user\'s backpacks';