From 720d74ebeb320d92fd5f644363e9e4b4f0b09877 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Mon, 1 Mar 2021 15:33:57 +0100 Subject: [PATCH 1/4] MDL-71017 oauth2: configure some oauth2 services for login only --- admin/tool/oauth2/classes/form/issuer.php | 105 +++++++++++++----- admin/tool/oauth2/classes/output/renderer.php | 20 +++- admin/tool/oauth2/issuers.php | 2 +- admin/tool/oauth2/lang/en/tool_oauth2.php | 10 ++ auth/oauth2/classes/auth.php | 17 +-- auth/oauth2/classes/output/renderer.php | 4 +- auth/oauth2/linkedlogins.php | 9 +- auth/oauth2/login.php | 3 + lib/classes/oauth2/api.php | 10 +- lib/classes/oauth2/issuer.php | 41 ++++++- .../oauth2/refresh_system_tokens_task.php | 2 +- lib/classes/oauth2/service/facebook.php | 2 +- lib/classes/oauth2/service/google.php | 2 +- lib/classes/oauth2/service/microsoft.php | 2 +- lib/db/install.xml | 1 + lib/db/upgrade.php | 15 +++ repository/googledocs/tests/generator/lib.php | 2 +- version.php | 2 +- 18 files changed, 186 insertions(+), 63 deletions(-) diff --git a/admin/tool/oauth2/classes/form/issuer.php b/admin/tool/oauth2/classes/form/issuer.php index a5d6c971adc..f466509f46d 100644 --- a/admin/tool/oauth2/classes/form/issuer.php +++ b/admin/tool/oauth2/classes/form/issuer.php @@ -115,28 +115,6 @@ class issuer extends persistent { $mform->addElement('checkbox', 'basicauth', get_string('usebasicauth', 'tool_oauth2')); $mform->addHelpButton('basicauth', 'usebasicauth', 'tool_oauth2'); - // Login scopes. - $mform->addElement('text', 'loginscopes', get_string('issuerloginscopes', 'tool_oauth2')); - $mform->addRule('loginscopes', null, 'required', null, 'client'); - $mform->addRule('loginscopes', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); - $mform->addHelpButton('loginscopes', 'issuerloginscopes', 'tool_oauth2'); - - // Login scopes offline. - $mform->addElement('text', 'loginscopesoffline', get_string('issuerloginscopesoffline', 'tool_oauth2')); - $mform->addRule('loginscopesoffline', null, 'required', null, 'client'); - $mform->addRule('loginscopesoffline', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); - $mform->addHelpButton('loginscopesoffline', 'issuerloginscopesoffline', 'tool_oauth2'); - - // Login params. - $mform->addElement('text', 'loginparams', get_string('issuerloginparams', 'tool_oauth2')); - $mform->addRule('loginparams', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); - $mform->addHelpButton('loginparams', 'issuerloginparams', 'tool_oauth2'); - - // Login params offline. - $mform->addElement('text', 'loginparamsoffline', get_string('issuerloginparamsoffline', 'tool_oauth2')); - $mform->addRule('loginparamsoffline', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); - $mform->addHelpButton('loginparamsoffline', 'issuerloginparamsoffline', 'tool_oauth2'); - // Base Url. $mform->addElement('text', 'baseurl', get_string('issuerbaseurl', 'tool_oauth2')); $mform->addRule('baseurl', get_string('maximumchars', '', 1024), 'maxlength', 1024, 'client'); @@ -145,24 +123,61 @@ class issuer extends persistent { $mform->addRule('baseurl', null, 'required', null, 'client'); } - // Allowed Domains. - $mform->addElement('text', 'alloweddomains', get_string('issueralloweddomains', 'tool_oauth2')); - $mform->addRule('alloweddomains', get_string('maximumchars', '', 1024), 'maxlength', 1024, 'client'); - $mform->addHelpButton('alloweddomains', 'issueralloweddomains', 'tool_oauth2'); - // Image. $mform->addElement('text', 'image', get_string('issuerimage', 'tool_oauth2'), 'maxlength="1024"'); $mform->addRule('image', get_string('maximumchars', '', 1024), 'maxlength', 1024, 'client'); $mform->addHelpButton('image', 'issuername', 'tool_oauth2'); // Show on login page. - $mform->addElement('checkbox', 'showonloginpage', get_string('issuershowonloginpage', 'tool_oauth2')); - $mform->addHelpButton('showonloginpage', 'issuershowonloginpage', 'tool_oauth2'); + $options = [ + \core\oauth2\issuer::EVERYWHERE => get_string('issueruseineverywhere', 'tool_oauth2'), + \core\oauth2\issuer::LOGINONLY => get_string('issueruseinloginonly', 'tool_oauth2'), + \core\oauth2\issuer::SERVICEONLY => get_string('issueruseininternalonly', 'tool_oauth2'), + ]; + $mform->addElement('select', 'showonloginpage', get_string('issuerusein', 'tool_oauth2'), $options); + $mform->addHelpButton('showonloginpage', 'issuerusein', 'tool_oauth2'); + + // Name on login page. + $mform->addElement('text', 'loginpagename', get_string('issuerloginpagename', 'tool_oauth2')); + $mform->addRule('loginpagename', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); + $mform->addHelpButton('loginpagename', 'issuerloginpagename', 'tool_oauth2'); + $mform->hideIf('loginpagename', 'showonloginpage', 'eq', \core\oauth2\issuer::SERVICEONLY); + + // Login scopes. + $mform->addElement('text', 'loginscopes', get_string('issuerloginscopes', 'tool_oauth2')); + $mform->addRule('loginscopes', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); + $mform->addHelpButton('loginscopes', 'issuerloginscopes', 'tool_oauth2'); + $mform->hideIf('loginscopes', 'showonloginpage', 'eq', \core\oauth2\issuer::SERVICEONLY); + + // Login scopes offline. + $mform->addElement('text', 'loginscopesoffline', get_string('issuerloginscopesoffline', 'tool_oauth2')); + $mform->addRule('loginscopesoffline', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); + $mform->addHelpButton('loginscopesoffline', 'issuerloginscopesoffline', 'tool_oauth2'); + $mform->hideIf('loginscopesoffline', 'showonloginpage', 'eq', \core\oauth2\issuer::SERVICEONLY); + + // Login params. + $mform->addElement('text', 'loginparams', get_string('issuerloginparams', 'tool_oauth2')); + $mform->addRule('loginparams', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); + $mform->addHelpButton('loginparams', 'issuerloginparams', 'tool_oauth2'); + $mform->hideIf('loginparams', 'showonloginpage', 'eq', \core\oauth2\issuer::SERVICEONLY); + + // Login params offline. + $mform->addElement('text', 'loginparamsoffline', get_string('issuerloginparamsoffline', 'tool_oauth2')); + $mform->addRule('loginparamsoffline', get_string('maximumchars', '', 255), 'maxlength', 255, 'client'); + $mform->addHelpButton('loginparamsoffline', 'issuerloginparamsoffline', 'tool_oauth2'); + $mform->hideIf('loginparamsoffline', 'showonloginpage', 'eq', \core\oauth2\issuer::SERVICEONLY); + + // Allowed Domains. + $mform->addElement('text', 'alloweddomains', get_string('issueralloweddomains', 'tool_oauth2')); + $mform->addRule('alloweddomains', get_string('maximumchars', '', 1024), 'maxlength', 1024, 'client'); + $mform->addHelpButton('alloweddomains', 'issueralloweddomains', 'tool_oauth2'); + $mform->hideIf('alloweddomains', 'showonloginpage', 'eq', \core\oauth2\issuer::SERVICEONLY); if ($this->showrequireconfirm) { // Require confirmation email for new accounts. $mform->addElement('advcheckbox', 'requireconfirmation', get_string('issuerrequireconfirmation', 'tool_oauth2')); $mform->addHelpButton('requireconfirmation', 'issuerrequireconfirmation', 'tool_oauth2'); + $mform->hideIf('requireconfirmation', 'showonloginpage', 'eq', \core\oauth2\issuer::SERVICEONLY); } if ($this->type == 'imsobv2p1' || $issuer->get('servicetype') == 'imsobv2p1') { @@ -209,4 +224,36 @@ class issuer extends persistent { $mform->getElement('servicetype')->setValue($this->type); } } + + /** + * Define extra validation mechanims. + * + * The data here: + * - does not include {@link self::$fieldstoremove}. + * - does include {@link self::$foreignfields}. + * - was converted to map persistent-like data, e.g. array $description to string $description + int $descriptionformat. + * + * You can modify the $errors parameter in order to remove some validation errors should you + * need to. However, the best practice is to return new or overriden errors. Only modify the + * errors passed by reference when you have no other option. + * + * Do not add any logic here, it is only intended to be used by child classes. + * + * @param stdClass $data Data to validate. + * @param array $files Array of files. + * @param array $errors Currently reported errors. + * @return array of additional errors, or overridden errors. + */ + protected function extra_validation($data, $files, array &$errors) { + $errors = []; + if ($data->showonloginpage != \core\oauth2\issuer::SERVICEONLY) { + if (!strlen(trim($data->loginscopes))) { + $errors['loginscopes'] = get_string('required'); + } + if (!strlen(trim($data->loginscopesoffline))) { + $errors['loginscopesoffline'] = get_string('required'); + } + } + return $errors; + } } diff --git a/admin/tool/oauth2/classes/output/renderer.php b/admin/tool/oauth2/classes/output/renderer.php index b838d0af482..23c41b0033b 100644 --- a/admin/tool/oauth2/classes/output/renderer.php +++ b/admin/tool/oauth2/classes/output/renderer.php @@ -54,7 +54,9 @@ class renderer extends plugin_renderer_base { $table->head = [ get_string('name'), get_string('configuredstatus', 'tool_oauth2'), - get_string('loginissuer', 'tool_oauth2'), + get_string('issuerusedforlogin', 'tool_oauth2'), + get_string('issuerdisplayas', 'tool_oauth2'), + get_string('issuerusedforinternal', 'tool_oauth2'), get_string('discoverystatus', 'tool_oauth2') . ' ' . $this->help_icon('discovered', 'tool_oauth2'), get_string('systemauthstatus', 'tool_oauth2') . ' ' . $this->help_icon('systemaccountconnected', 'tool_oauth2'), get_string('edit'), @@ -93,12 +95,22 @@ class renderer extends plugin_renderer_base { $configuredstatuscell = new html_table_cell($configured); // Login issuer. - if (!empty($issuer->get('showonloginpage'))) { + if ((int)$issuer->get('showonloginpage') != issuer::SERVICEONLY) { + $loginissuer = $this->pix_icon('yes', get_string('loginissuer', 'tool_oauth2'), 'tool_oauth2'); + $logindisplayas = s($issuer->get_display_name()); + } else { + $loginissuer = $this->pix_icon('no', get_string('notloginissuer', 'tool_oauth2'), 'tool_oauth2'); + $logindisplayas = ''; + } + $loginissuerstatuscell = new html_table_cell($loginissuer); + + // Internal services issuer. + if ((int)$issuer->get('showonloginpage') != issuer::LOGINONLY) { $loginissuer = $this->pix_icon('yes', get_string('loginissuer', 'tool_oauth2'), 'tool_oauth2'); } else { $loginissuer = $this->pix_icon('no', get_string('notloginissuer', 'tool_oauth2'), 'tool_oauth2'); } - $loginissuerstatuscell = new html_table_cell($loginissuer); + $internalissuerstatuscell = new html_table_cell($loginissuer); // Discovered. if (!empty($issuer->get('scopessupported'))) { @@ -188,6 +200,8 @@ class renderer extends plugin_renderer_base { $namecell, $configuredstatuscell, $loginissuerstatuscell, + $logindisplayas, + $internalissuerstatuscell, $discoverystatuscell, $systemauthstatuscell, $editcell, diff --git a/admin/tool/oauth2/issuers.php b/admin/tool/oauth2/issuers.php index 6383ef2c46b..ad720c4d5d1 100644 --- a/admin/tool/oauth2/issuers.php +++ b/admin/tool/oauth2/issuers.php @@ -195,7 +195,7 @@ if ($mform && $mform->is_cancelled()) { echo $OUTPUT->header(); echo $OUTPUT->heading(get_string('pluginname', 'tool_oauth2')); echo $OUTPUT->doc_link('OAuth2_Services', get_string('serviceshelp', 'tool_oauth2')); - $issuers = core\oauth2\api::get_all_issuers(); + $issuers = core\oauth2\api::get_all_issuers(true); echo $renderer->issuers_table($issuers); echo $renderer->container_start(); diff --git a/admin/tool/oauth2/lang/en/tool_oauth2.php b/admin/tool/oauth2/lang/en/tool_oauth2.php index 9ff13af03a4..c37bda1a97b 100644 --- a/admin/tool/oauth2/lang/en/tool_oauth2.php +++ b/admin/tool/oauth2/lang/en/tool_oauth2.php @@ -70,6 +70,8 @@ $string['issuerdisabled'] = 'Identity issuer disabled'; $string['issuerenabled'] = 'Identity issuer enabled'; $string['issuerimage_help'] = 'An image URL used to show a logo for this issuer. May be displayed on login page.'; $string['issuerimage'] = 'Logo URL'; +$string['issuerloginpagename'] = 'Name displayed on the login page'; +$string['issuerloginpagename_help'] = 'If specified, this name will be used on the login page instead of the service name above'; $string['issuerloginparams'] = 'Additional parameters included in a login request.'; $string['issuerloginparams_help'] = 'Some systems require additional parameters for a login request in order to read the user\'s basic profile.'; $string['issuerloginparamsoffline'] = 'Additional parameters included in a login request for offline access.'; @@ -85,6 +87,14 @@ $string['issuershowonloginpage'] = 'Show on login page'; $string['issuerrequireconfirmation_help'] = 'Require that all users verify their email address before they can log in with OAuth. This applies to newly created accounts as part of the login process, or when an existing Moodle account is connected to an OAuth login via matching email addresses.'; $string['issuerrequireconfirmation'] = 'Require email verification'; $string['issuers'] = 'Issuers'; +$string['issuerusein'] = 'This service will be used'; +$string['issuerusein_help'] = 'OAuth 2 services can be used in some internal services, on the login page, or both, if needed'; +$string['issueruseineverywhere'] = 'Login page and internal services'; +$string['issueruseininternalonly'] = 'Internal services only'; +$string['issueruseinloginonly'] = 'Login page only'; +$string['issuerusedforlogin'] = 'Login'; +$string['issuerusedforinternal'] = 'Services'; +$string['issuerdisplayas'] = 'Display as'; $string['loginissuer'] = 'Allow login'; $string['microsoft_service'] = 'Microsoft'; $string['nextcloud_service'] = 'Nextcloud'; diff --git a/auth/oauth2/classes/auth.php b/auth/oauth2/classes/auth.php index 41527292b70..251adbff50a 100644 --- a/auth/oauth2/classes/auth.php +++ b/auth/oauth2/classes/auth.php @@ -166,17 +166,6 @@ class auth extends \auth_plugin_base { return false; } - /** - * Do some checks on the identity provider before showing it on the login page. - * @param core\oauth2\issuer $issuer - * @return boolean - */ - private function is_ready_for_login_page(\core\oauth2\issuer $issuer) { - return $issuer->get('enabled') && - $issuer->is_configured() && - !empty($issuer->get('showonloginpage')); - } - /** * Return a list of identity providers to display on the login page. * @@ -184,17 +173,17 @@ class auth extends \auth_plugin_base { * @return array List of arrays with keys url, iconurl and name. */ public function loginpage_idp_list($wantsurl) { - $providers = \core\oauth2\api::get_all_issuers(); + $providers = \core\oauth2\api::get_all_issuers(true); $result = []; if (empty($wantsurl)) { $wantsurl = '/'; } foreach ($providers as $idp) { - if ($this->is_ready_for_login_page($idp)) { + if ($idp->is_available_for_login()) { $params = ['id' => $idp->get('id'), 'wantsurl' => $wantsurl, 'sesskey' => sesskey()]; $url = new moodle_url('/auth/oauth2/login.php', $params); $icon = $idp->get('image'); - $result[] = ['url' => $url, 'iconurl' => $icon, 'name' => $idp->get('name')]; + $result[] = ['url' => $url, 'iconurl' => $icon, 'name' => $idp->get_display_name()]; } } return $result; diff --git a/auth/oauth2/classes/output/renderer.php b/auth/oauth2/classes/output/renderer.php index 12435603891..07a0428afe6 100644 --- a/auth/oauth2/classes/output/renderer.php +++ b/auth/oauth2/classes/output/renderer.php @@ -28,7 +28,7 @@ use html_table; use html_table_cell; use html_table_row; use html_writer; -use auth\oauth2\linked_login; +use auth_oauth2\linked_login; use moodle_url; defined('MOODLE_INTERNAL') || die(); @@ -43,7 +43,7 @@ class renderer extends plugin_renderer_base { /** * This function will render one beautiful table with all the linked_logins. * - * @param \auth\oauth2\linked_login[] $linkedlogins - list of all linked logins. + * @param linked_login[] $linkedlogins - list of all linked logins. * @return string HTML to output. */ public function linked_logins_table($linkedlogins) { diff --git a/auth/oauth2/linkedlogins.php b/auth/oauth2/linkedlogins.php index 146e261857e..30df5d44d7b 100644 --- a/auth/oauth2/linkedlogins.php +++ b/auth/oauth2/linkedlogins.php @@ -45,7 +45,7 @@ if ($action == 'new') { $issuerid = required_param('issuerid', PARAM_INT); $issuer = \core\oauth2\api::get_issuer($issuerid); - if (!$issuer->is_authentication_supported() || !$issuer->get('showonloginpage') || !$issuer->get('enabled')) { + if (!$issuer->is_available_for_login()) { throw new \moodle_exception('issuernologin', 'auth_oauth2'); } @@ -89,19 +89,20 @@ $linkedlogin = null; auth_oauth2\api::clean_orphaned_linked_logins(); -$issuers = \core\oauth2\api::get_all_issuers(); +$issuers = \core\oauth2\api::get_all_issuers(true); $anyshowinloginpage = false; $issuerbuttons = array(); foreach ($issuers as $issuer) { - if (!$issuer->is_authentication_supported() || !$issuer->get('showonloginpage') || !$issuer->get('enabled')) { + if (!$issuer->is_available_for_login()) { continue; } $anyshowinloginpage = true; $addparams = ['action' => 'new', 'issuerid' => $issuer->get('id'), 'sesskey' => sesskey(), 'logout' => true]; $addurl = new moodle_url('/auth/oauth2/linkedlogins.php', $addparams); - $issuerbuttons[$issuer->get('id')] = $renderer->single_button($addurl, get_string('createnewlinkedlogin', 'auth_oauth2', s($issuer->get('name')))); + $issuerbuttons[$issuer->get('id')] = $renderer->single_button($addurl, get_string('createnewlinkedlogin', 'auth_oauth2', + s($issuer->get_display_name()))); } if (!$anyshowinloginpage) { diff --git a/auth/oauth2/login.php b/auth/oauth2/login.php index 9abb2a32cdf..949b5a10e4c 100644 --- a/auth/oauth2/login.php +++ b/auth/oauth2/login.php @@ -37,6 +37,9 @@ if (!\auth_oauth2\api::is_enabled()) { } $issuer = new \core\oauth2\issuer($issuerid); +if (!$issuer->is_available_for_login()) { + throw new \moodle_exception('issuernologin', 'auth_oauth2'); +} $returnparams = ['wantsurl' => $wantsurl, 'sesskey' => sesskey(), 'id' => $issuerid]; $returnurl = new moodle_url('/auth/oauth2/login.php', $returnparams); diff --git a/lib/classes/oauth2/api.php b/lib/classes/oauth2/api.php index 6219c8dc98c..ed6b4fd935d 100644 --- a/lib/classes/oauth2/api.php +++ b/lib/classes/oauth2/api.php @@ -110,10 +110,16 @@ class api { /** * List all the issuers, ordered by the sortorder field + * + * @param bool $showall also include issues that are configured to be shown only on login page * @return \core\oauth2\issuer[] */ - public static function get_all_issuers() { - return issuer::get_records([], 'sortorder'); + public static function get_all_issuers(bool $showall = false) { + if ($showall) { + return issuer::get_records([], 'sortorder'); + } else { + return array_values(issuer::get_records_select('showonloginpage<>?', [issuer::LOGINONLY], 'sortorder')); + } } /** diff --git a/lib/classes/oauth2/issuer.php b/lib/classes/oauth2/issuer.php index ce4d32f384b..c2e06c0d630 100644 --- a/lib/classes/oauth2/issuer.php +++ b/lib/classes/oauth2/issuer.php @@ -36,6 +36,13 @@ use lang_string; */ class issuer extends persistent { + /** @var int Issuer is displayed on both login page and in the services lists */ + const EVERYWHERE = 1; + /** @var int Issuer is displayed on the login page only */ + const LOGINONLY = 2; + /** @var int Issuer is displayed only in the services lists and can not be used for login */ + const SERVICEONLY = 0; + const TABLE = 'oauth2_issuer'; /** @@ -70,8 +77,8 @@ class issuer extends persistent { 'default' => true ), 'showonloginpage' => array( - 'type' => PARAM_BOOL, - 'default' => false + 'type' => PARAM_INT, + 'default' => self::SERVICEONLY, ), 'basicauth' => array( 'type' => PARAM_BOOL, @@ -115,6 +122,11 @@ class issuer extends persistent { 'null' => NULL_ALLOWED, 'default' => null, ), + 'loginpagename' => array( + 'type' => PARAM_TEXT, + 'null' => NULL_ALLOWED, + 'default' => null, + ), ); } @@ -173,9 +185,25 @@ class issuer extends persistent { * @return boolean */ public function is_authentication_supported() { + debugging('Method is_authentication_supported() is deprecated, please use is_available_for_login()', + DEBUG_DEVELOPER); return (!empty($this->get_endpoint_url('userinfo'))); } + /** + * Is this issue fully configured and enabled and can be used for login/signup + * + * @return bool + * @throws \coding_exception + */ + public function is_available_for_login() { + return $this->get('id') && + $this->is_configured() && + $this->get('showonloginpage') != issuer::SERVICEONLY && + $this->get('enabled') && + !empty($this->get_endpoint_url('userinfo')); + } + /** * Return true if this issuer looks like it has been configured. * @@ -231,4 +259,13 @@ class issuer extends persistent { } return true; } + + /** + * Display name for the issuers used on the login page + * + * @return string + */ + public function get_display_name() { + return $this->get('loginpagename') ? $this->get('loginpagename') : $this->get('name'); + } } diff --git a/lib/classes/oauth2/refresh_system_tokens_task.php b/lib/classes/oauth2/refresh_system_tokens_task.php index 1249cdf3405..00f247a20c6 100644 --- a/lib/classes/oauth2/refresh_system_tokens_task.php +++ b/lib/classes/oauth2/refresh_system_tokens_task.php @@ -84,7 +84,7 @@ class refresh_system_tokens_task extends scheduled_task { * Throw exceptions on errors (the job will be retried). */ public function execute() { - $issuers = \core\oauth2\api::get_all_issuers(); + $issuers = \core\oauth2\api::get_all_issuers(true); $tasksuccess = true; foreach ($issuers as $issuer) { if ($issuer->is_system_account_connected()) { diff --git a/lib/classes/oauth2/service/facebook.php b/lib/classes/oauth2/service/facebook.php index 92ec1f513d2..29af16e4e61 100644 --- a/lib/classes/oauth2/service/facebook.php +++ b/lib/classes/oauth2/service/facebook.php @@ -42,7 +42,7 @@ class facebook extends openidconnect implements issuer_interface { 'baseurl' => '', 'loginscopes' => 'public_profile email', 'loginscopesoffline' => 'public_profile email', - 'showonloginpage' => true, + 'showonloginpage' => issuer::EVERYWHERE, 'servicetype' => 'facebook', ]; diff --git a/lib/classes/oauth2/service/google.php b/lib/classes/oauth2/service/google.php index 77d27cc9a2b..253c6d36527 100644 --- a/lib/classes/oauth2/service/google.php +++ b/lib/classes/oauth2/service/google.php @@ -39,7 +39,7 @@ class google extends openidconnect implements issuer_interface { 'image' => 'https://accounts.google.com/favicon.ico', 'baseurl' => 'https://accounts.google.com/', 'loginparamsoffline' => 'access_type=offline&prompt=consent', - 'showonloginpage' => true, + 'showonloginpage' => issuer::EVERYWHERE, 'servicetype' => 'google', ]; $issuer = new issuer(0, $record); diff --git a/lib/classes/oauth2/service/microsoft.php b/lib/classes/oauth2/service/microsoft.php index d5d6e7ff7d2..3a891a3c1c9 100644 --- a/lib/classes/oauth2/service/microsoft.php +++ b/lib/classes/oauth2/service/microsoft.php @@ -42,7 +42,7 @@ class microsoft extends openidconnect implements issuer_interface { 'baseurl' => '', 'loginscopes' => 'openid profile email user.read', 'loginscopesoffline' => 'openid profile email user.read offline_access', - 'showonloginpage' => true, + 'showonloginpage' => issuer::EVERYWHERE, 'servicetype' => 'microsoft', ]; diff --git a/lib/db/install.xml b/lib/db/install.xml index fdeba8596f2..3d5113f6c3b 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -3824,6 +3824,7 @@ + diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 188906cf131..2e0508cfc1b 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -2564,5 +2564,20 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2021041300.01); } + if ($oldversion < 2021041600.01) { + + // Define field loginpagename to be added to oauth2_issuer. + $table = new xmldb_table('oauth2_issuer'); + $field = new xmldb_field('loginpagename', XMLDB_TYPE_CHAR, '255', null, null, null, null, 'servicetype'); + + // Conditionally launch add field loginpagename. + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + + // Main savepoint reached. + upgrade_main_savepoint(true, 2021041600.01); + } + return true; } diff --git a/repository/googledocs/tests/generator/lib.php b/repository/googledocs/tests/generator/lib.php index 294fcbdf690..cc875a48575 100644 --- a/repository/googledocs/tests/generator/lib.php +++ b/repository/googledocs/tests/generator/lib.php @@ -49,7 +49,7 @@ class repository_googledocs_generator extends testing_repository_generator { 'image' => 'https://accounts.google.com/favicon.ico', 'baseurl' => 'https://accounts.google.com/', 'loginparamsoffline' => 'access_type=offline&prompt=consent', - 'showonloginpage' => true + 'showonloginpage' => issuer::EVERYWHERE ]; $issuer = new issuer(0, $issuerrecord); diff --git a/version.php b/version.php index d03ea843071..adca8434e0b 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2021041600.00; // 20201109 = branching date YYYYMMDD - do not modify! +$version = 2021041600.01; // 20201109 = branching date YYYYMMDD - do not modify! // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '3.11dev+ (Build: 20210416)';// Human-friendly version name From 8cc1bcb123c9be35a34fb42daeb10cb13dccdf8f Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Fri, 26 Mar 2021 09:25:53 +0100 Subject: [PATCH 2/4] MDL-71017 tool_oauth2: show not configured services --- admin/tool/oauth2/classes/output/renderer.php | 32 ++++++++----------- admin/tool/oauth2/pix/notconfigured.svg | 14 ++++++++ 2 files changed, 28 insertions(+), 18 deletions(-) create mode 100644 admin/tool/oauth2/pix/notconfigured.svg diff --git a/admin/tool/oauth2/classes/output/renderer.php b/admin/tool/oauth2/classes/output/renderer.php index 23c41b0033b..df007828a79 100644 --- a/admin/tool/oauth2/classes/output/renderer.php +++ b/admin/tool/oauth2/classes/output/renderer.php @@ -53,7 +53,6 @@ class renderer extends plugin_renderer_base { $table = new html_table(); $table->head = [ get_string('name'), - get_string('configuredstatus', 'tool_oauth2'), get_string('issuerusedforlogin', 'tool_oauth2'), get_string('issuerdisplayas', 'tool_oauth2'), get_string('issuerusedforinternal', 'tool_oauth2'), @@ -86,31 +85,29 @@ class renderer extends plugin_renderer_base { $namecell = new html_table_cell($name); $namecell->header = true; - // Configured. - if ($issuer->is_configured()) { - $configured = $this->pix_icon('yes', get_string('configured', 'tool_oauth2'), 'tool_oauth2'); - } else { - $configured = $this->pix_icon('no', get_string('notconfigured', 'tool_oauth2'), 'tool_oauth2'); - } - $configuredstatuscell = new html_table_cell($configured); - // Login issuer. - if ((int)$issuer->get('showonloginpage') != issuer::SERVICEONLY) { - $loginissuer = $this->pix_icon('yes', get_string('loginissuer', 'tool_oauth2'), 'tool_oauth2'); - $logindisplayas = s($issuer->get_display_name()); - } else { + if ((int)$issuer->get('showonloginpage') == issuer::SERVICEONLY) { $loginissuer = $this->pix_icon('no', get_string('notloginissuer', 'tool_oauth2'), 'tool_oauth2'); $logindisplayas = ''; + } else { + $logindisplayas = s($issuer->get_display_name()); + if ($issuer->is_available_for_login()) { + $loginissuer = $this->pix_icon('yes', get_string('loginissuer', 'tool_oauth2'), 'tool_oauth2'); + } else { + $loginissuer = $this->pix_icon('notconfigured', get_string('notconfigured', 'tool_oauth2'), 'tool_oauth2'); + } } $loginissuerstatuscell = new html_table_cell($loginissuer); // Internal services issuer. - if ((int)$issuer->get('showonloginpage') != issuer::LOGINONLY) { - $loginissuer = $this->pix_icon('yes', get_string('loginissuer', 'tool_oauth2'), 'tool_oauth2'); + if ((int)$issuer->get('showonloginpage') == issuer::LOGINONLY) { + $serviceissuer = $this->pix_icon('no', get_string('notloginissuer', 'tool_oauth2'), 'tool_oauth2'); + } else if ($issuer->is_configured() && $issuer->get('enabled')) { + $serviceissuer = $this->pix_icon('yes', get_string('loginissuer', 'tool_oauth2'), 'tool_oauth2'); } else { - $loginissuer = $this->pix_icon('no', get_string('notloginissuer', 'tool_oauth2'), 'tool_oauth2'); + $serviceissuer = $this->pix_icon('caution', get_string('notconfigured', 'tool_oauth2'), 'tool_oauth2'); } - $internalissuerstatuscell = new html_table_cell($loginissuer); + $internalissuerstatuscell = new html_table_cell($serviceissuer); // Discovered. if (!empty($issuer->get('scopessupported'))) { @@ -198,7 +195,6 @@ class renderer extends plugin_renderer_base { $row = new html_table_row([ $namecell, - $configuredstatuscell, $loginissuerstatuscell, $logindisplayas, $internalissuerstatuscell, diff --git a/admin/tool/oauth2/pix/notconfigured.svg b/admin/tool/oauth2/pix/notconfigured.svg new file mode 100644 index 00000000000..300198e2a30 --- /dev/null +++ b/admin/tool/oauth2/pix/notconfigured.svg @@ -0,0 +1,14 @@ + + + + + + From fc02628620721510afe336fa2a50e519fe19da2e Mon Sep 17 00:00:00 2001 From: David Matamoros Date: Mon, 29 Mar 2021 10:11:05 +0200 Subject: [PATCH 3/4] MDL-71017 tool_oauth2: Small fixes on Oauth 2 services UI and tests --- admin/tool/oauth2/classes/form/issuer.php | 4 +- admin/tool/oauth2/classes/output/renderer.php | 16 ++-- admin/tool/oauth2/lang/en/tool_oauth2.php | 6 +- .../oauth2/tests/behat/basic_settings.feature | 76 +++++++++++++++--- lib/classes/oauth2/issuer.php | 2 +- lib/tests/oauth2_test.php | 77 +++++++++++++++++++ 6 files changed, 160 insertions(+), 21 deletions(-) diff --git a/admin/tool/oauth2/classes/form/issuer.php b/admin/tool/oauth2/classes/form/issuer.php index f466509f46d..84b06696ca9 100644 --- a/admin/tool/oauth2/classes/form/issuer.php +++ b/admin/tool/oauth2/classes/form/issuer.php @@ -229,8 +229,8 @@ class issuer extends persistent { * Define extra validation mechanims. * * The data here: - * - does not include {@link self::$fieldstoremove}. - * - does include {@link self::$foreignfields}. + * - does not include {@see self::$fieldstoremove}. + * - does include {@see self::$foreignfields}. * - was converted to map persistent-like data, e.g. array $description to string $description + int $descriptionformat. * * You can modify the $errors parameter in order to remove some validation errors should you diff --git a/admin/tool/oauth2/classes/output/renderer.php b/admin/tool/oauth2/classes/output/renderer.php index df007828a79..3eb41b00384 100644 --- a/admin/tool/oauth2/classes/output/renderer.php +++ b/admin/tool/oauth2/classes/output/renderer.php @@ -54,7 +54,7 @@ class renderer extends plugin_renderer_base { $table->head = [ get_string('name'), get_string('issuerusedforlogin', 'tool_oauth2'), - get_string('issuerdisplayas', 'tool_oauth2'), + get_string('logindisplay', 'tool_oauth2'), get_string('issuerusedforinternal', 'tool_oauth2'), get_string('discoverystatus', 'tool_oauth2') . ' ' . $this->help_icon('discovered', 'tool_oauth2'), get_string('systemauthstatus', 'tool_oauth2') . ' ' . $this->help_icon('systemaccountconnected', 'tool_oauth2'), @@ -91,7 +91,7 @@ class renderer extends plugin_renderer_base { $logindisplayas = ''; } else { $logindisplayas = s($issuer->get_display_name()); - if ($issuer->is_available_for_login()) { + if ($issuer->get('id') && $issuer->is_configured() && !empty($issuer->get_endpoint_url('userinfo'))) { $loginissuer = $this->pix_icon('yes', get_string('loginissuer', 'tool_oauth2'), 'tool_oauth2'); } else { $loginissuer = $this->pix_icon('notconfigured', get_string('notconfigured', 'tool_oauth2'), 'tool_oauth2'); @@ -101,11 +101,11 @@ class renderer extends plugin_renderer_base { // Internal services issuer. if ((int)$issuer->get('showonloginpage') == issuer::LOGINONLY) { - $serviceissuer = $this->pix_icon('no', get_string('notloginissuer', 'tool_oauth2'), 'tool_oauth2'); - } else if ($issuer->is_configured() && $issuer->get('enabled')) { - $serviceissuer = $this->pix_icon('yes', get_string('loginissuer', 'tool_oauth2'), 'tool_oauth2'); + $serviceissuer = $this->pix_icon('no', get_string('issuersservicesnotallow', 'tool_oauth2'), 'tool_oauth2'); + } else if ($issuer->get('id') && $issuer->is_configured() && !empty($issuer->get_endpoint_url('userinfo'))) { + $serviceissuer = $this->pix_icon('yes', get_string('issuersservicesallow', 'tool_oauth2'), 'tool_oauth2'); } else { - $serviceissuer = $this->pix_icon('caution', get_string('notconfigured', 'tool_oauth2'), 'tool_oauth2'); + $serviceissuer = $this->pix_icon('notconfigured', get_string('notconfigured', 'tool_oauth2'), 'tool_oauth2'); } $internalissuerstatuscell = new html_table_cell($serviceissuer); @@ -203,6 +203,10 @@ class renderer extends plugin_renderer_base { $editcell, ]); + if (!$issuer->get('enabled')) { + $row->attributes['class'] = 'dimmed_text'; + } + $data[] = $row; $index++; } diff --git a/admin/tool/oauth2/lang/en/tool_oauth2.php b/admin/tool/oauth2/lang/en/tool_oauth2.php index c37bda1a97b..b929911c2f9 100644 --- a/admin/tool/oauth2/lang/en/tool_oauth2.php +++ b/admin/tool/oauth2/lang/en/tool_oauth2.php @@ -87,14 +87,16 @@ $string['issuershowonloginpage'] = 'Show on login page'; $string['issuerrequireconfirmation_help'] = 'Require that all users verify their email address before they can log in with OAuth. This applies to newly created accounts as part of the login process, or when an existing Moodle account is connected to an OAuth login via matching email addresses.'; $string['issuerrequireconfirmation'] = 'Require email verification'; $string['issuers'] = 'Issuers'; +$string['issuersservicesallow'] = 'Allow services'; +$string['issuersservicesnotallow'] = 'Do not allow services'; $string['issuerusein'] = 'This service will be used'; $string['issuerusein_help'] = 'OAuth 2 services can be used in some internal services, on the login page, or both, if needed'; $string['issueruseineverywhere'] = 'Login page and internal services'; $string['issueruseininternalonly'] = 'Internal services only'; $string['issueruseinloginonly'] = 'Login page only'; $string['issuerusedforlogin'] = 'Login'; -$string['issuerusedforinternal'] = 'Services'; -$string['issuerdisplayas'] = 'Display as'; +$string['issuerusedforinternal'] = 'Internal services'; +$string['logindisplay'] = 'Display on login page as'; $string['loginissuer'] = 'Allow login'; $string['microsoft_service'] = 'Microsoft'; $string['nextcloud_service'] = 'Nextcloud'; diff --git a/admin/tool/oauth2/tests/behat/basic_settings.feature b/admin/tool/oauth2/tests/behat/basic_settings.feature index 429d3713e1b..16a4e924195 100644 --- a/admin/tool/oauth2/tests/behat/basic_settings.feature +++ b/admin/tool/oauth2/tests/behat/basic_settings.feature @@ -18,8 +18,8 @@ Feature: Basic OAuth2 functionality When I press "Save changes" Then I should see "Changes saved" And I should see "Testing service" - And "Configured" "icon" should exist in the "Testing service" "table_row" And "Allow login" "icon" should exist in the "Testing service" "table_row" + And "Allow services" "icon" should exist in the "Testing service" "table_row" And "Service discovery successful" "icon" should exist in the "Testing service" "table_row" And I click on "Configure endpoints" "link" in the "Testing service" "table_row" And I should see "https://accounts.google.com/.well-known/openid-configuration" in the "discovery_endpoint" "table_row" @@ -51,8 +51,8 @@ Feature: Basic OAuth2 functionality When I press "Save changes" Then I should see "Changes saved" And I should see "Testing service" - And "Configured" "icon" should exist in the "Testing service" "table_row" And "Allow login" "icon" should exist in the "Testing service" "table_row" + And "Allow services" "icon" should exist in the "Testing service" "table_row" And I should see "-" in the "Testing service" "table_row" And I click on "Configure endpoints" "link" in the "Testing service" "table_row" And I should see "authorization_endpoint" @@ -83,8 +83,8 @@ Feature: Basic OAuth2 functionality When I press "Save changes" Then I should see "Changes saved" And I should see "Testing service" - And "Configured" "icon" should exist in the "Testing service" "table_row" And "Allow login" "icon" should exist in the "Testing service" "table_row" + And "Allow services" "icon" should exist in the "Testing service" "table_row" And I should see "-" in the "Testing service" "table_row" And I click on "Configure endpoints" "link" in the "Testing service" "table_row" And I should see "authorization_endpoint" @@ -120,8 +120,8 @@ Feature: Basic OAuth2 functionality When I press "Save changes" Then I should see "Changes saved" And I should see "Testing service" - And "Configured" "icon" should exist in the "Testing service" "table_row" And "Do not allow login" "icon" should exist in the "Testing service" "table_row" + And "Allow services" "icon" should exist in the "Testing service" "table_row" And I should see "-" in the "Testing service" "table_row" And I click on "Configure endpoints" "link" in the "Testing service" "table_row" And I should see "authorization_endpoint" @@ -152,7 +152,7 @@ Feature: Basic OAuth2 functionality When I press "Save changes" Then I should see "Changes saved" And I should see "OpenBadges" - And "Configured" "icon" should exist in the "OpenBadges" "table_row" + And "Not configured" "icon" should exist in the "OpenBadges" "table_row" And "Do not allow login" "icon" should exist in the "OpenBadges" "table_row" And "Service discovery successful" "icon" should exist in the "OpenBadges" "table_row" And the "src" attribute of "table.admintable th img" "css_element" should contain "IMS-Global-Logo.png" @@ -187,8 +187,8 @@ Feature: Basic OAuth2 functionality When I press "Save changes" Then I should see "Changes saved" And I should see "Google custom" - And "Configured" "icon" should exist in the "Google custom" "table_row" And "Do not allow login" "icon" should exist in the "Google custom" "table_row" + And "Allow services" "icon" should exist in the "Google custom" "table_row" And "Service discovery successful" "icon" should exist in the "Google custom" "table_row" And the "src" attribute of "table.admintable th img" "css_element" should contain "favicon.ico" And I click on "Configure endpoints" "link" in the "Google custom" "table_row" @@ -222,8 +222,8 @@ Feature: Basic OAuth2 functionality When I press "Save changes" Then I should see "Could not discover end points for identity issuer: Invalid custom service" And I should see "URL: https://dc.imsglobal.org/.well-known/openid-configuration" - And "Configured" "icon" should exist in the "Invalid custom service" "table_row" And "Do not allow login" "icon" should exist in the "Invalid custom service" "table_row" + And "Not configured" "icon" should exist in the "Invalid custom service" "table_row" And I should see "-" in the "Invalid custom service" "table_row" And I click on "Configure endpoints" "link" in the "Invalid custom service" "table_row" And I should not see "discovery_endpoint" @@ -237,8 +237,8 @@ Feature: Basic OAuth2 functionality | Name | Valid custom service | | Service base URL | https://accounts.google.com/ | And I press "Save changes" - And "Configured" "icon" should exist in the "Valid custom" "table_row" And "Do not allow login" "icon" should exist in the "Valid custom" "table_row" + And "Allow services" "icon" should exist in the "Valid custom" "table_row" And "Service discovery successful" "icon" should exist in the "Valid custom" "table_row" And I click on "Edit" "link" in the "Valid custom service" "table_row" And I set the following fields to these values: @@ -263,8 +263,8 @@ Feature: Basic OAuth2 functionality When I press "Save changes" And I should see "Changes saved" And I should see "Empty custom service" - And "Configured" "icon" should exist in the "Empty custom service" "table_row" And "Do not allow login" "icon" should exist in the "Empty custom service" "table_row" + And "Not configured" "icon" should exist in the "Empty custom service" "table_row" And I should see "-" in the "Empty custom service" "table_row" And I click on "Configure endpoints" "link" in the "Empty custom service" "table_row" And I should not see "discovery_endpoint" @@ -279,8 +279,8 @@ Feature: Basic OAuth2 functionality | Name | Valid custom service | | Service base URL | https://accounts.google.com | And I press "Save changes" - And "Configured" "icon" should exist in the "Valid custom" "table_row" And "Do not allow login" "icon" should exist in the "Valid custom" "table_row" + And "Allow services" "icon" should exist in the "Valid custom" "table_row" And "Service discovery successful" "icon" should exist in the "Valid custom" "table_row" And I click on "Edit" "link" in the "Valid custom service" "table_row" And I set the following fields to these values: @@ -301,3 +301,59 @@ Feature: Basic OAuth2 functionality And I press "Continue" And I should see "Identity issuer deleted" And I should not see "Empty custom service" + + Scenario: Create a standard service for Google and test form and UI for login only, services only and both + Given I press "Google" + And I should see "Create new service: Google" + # Create using 'Login page only' option. + And I set the following fields to these values: + | Name | Testing service | + | Client ID | thisistheclientid | + | Client secret | supersecret | + | This service will be used | Login page only | + When I press "Save changes" + Then I should see "Changes saved" + And I should see "Testing service" + And "Allow login" "icon" should exist in the "Testing service" "table_row" + And "Do not allow services" "icon" should exist in the "Testing service" "table_row" + And "Service discovery successful" "icon" should exist in the "Testing service" "table_row" + # Change to 'Internal services only'. + And I click on "Edit" "link" in the "Testing service" "table_row" + And I set the following fields to these values: + | This service will be used | Internal services only | + And I press "Save changes" + And I should see "Changes saved" + And "Do not allow login" "icon" should exist in the "Testing service" "table_row" + And "Allow services" "icon" should exist in the "Testing service" "table_row" + # Change to 'Login page and internal services' and add a display name. + And I click on "Edit" "link" in the "Testing service" "table_row" + And I set the following fields to these values: + | This service will be used | Login page and internal services | + | Name displayed on the login page | Google new display name | + And I press "Save changes" + And I should see "Changes saved" + And "Allow login" "icon" should exist in the "Testing service" "table_row" + And "Allow services" "icon" should exist in the "Testing service" "table_row" + And I should see "Google new display name" in the "Testing service" "table_row" + + Scenario: Create a login page only custom OIDC service + Given I press "Custom" + And I should see "Create new service: Custom" + And I set the following fields to these values: + | Name | Empty custom service | + | Client ID | thisistheclientid | + | Client secret | supersecret | + | This service will be used | Login page only | + | Name displayed on the login page | Custom display name | + When I press "Save changes" + And I should see "Changes saved" + And I should see "Empty custom service" + And I should see "Custom display name" in the "Empty custom service" "table_row" + And "Not configured" "icon" should exist in the "Empty custom service" "table_row" + And "Do not allow services" "icon" should exist in the "Empty custom service" "table_row" + And I click on "Edit" "link" in the "Empty custom service" "table_row" + And I set the following fields to these values: + | Service base URL | https://accounts.google.com | + And I press "Save changes" + And "Allow login" "icon" should exist in the "Empty custom service" "table_row" + And "Do not allow services" "icon" should exist in the "Empty custom service" "table_row" diff --git a/lib/classes/oauth2/issuer.php b/lib/classes/oauth2/issuer.php index c2e06c0d630..758f693c7df 100644 --- a/lib/classes/oauth2/issuer.php +++ b/lib/classes/oauth2/issuer.php @@ -199,7 +199,7 @@ class issuer extends persistent { public function is_available_for_login() { return $this->get('id') && $this->is_configured() && - $this->get('showonloginpage') != issuer::SERVICEONLY && + $this->get('showonloginpage') != self::SERVICEONLY && $this->get('enabled') && !empty($this->get_endpoint_url('userinfo')); } diff --git a/lib/tests/oauth2_test.php b/lib/tests/oauth2_test.php index 09595adf9d5..4da5d1e56b0 100644 --- a/lib/tests/oauth2_test.php +++ b/lib/tests/oauth2_test.php @@ -366,4 +366,81 @@ class core_oauth2_testcase extends advanced_testcase { ]; } + /** + * Test for get all issuers. + */ + public function test_get_all_issuers() { + $this->resetAfterTest(); + $this->setAdminUser(); + $googleissuer = core\oauth2\api::create_standard_issuer('google'); + core\oauth2\api::create_standard_issuer('facebook'); + core\oauth2\api::create_standard_issuer('microsoft'); + + // Set Google issuer to be shown only on login page. + $record = $googleissuer->to_record(); + $record->showonloginpage = $googleissuer::LOGINONLY; + core\oauth2\api::update_issuer($record); + + $issuers = \core\oauth2\api::get_all_issuers(); + $this->assertCount(2, $issuers); + $expected = ['Microsoft', 'Facebook']; + $this->assertEqualsCanonicalizing($expected, [$issuers[0]->get_display_name(), $issuers[1]->get_display_name()]); + + $issuers = \core\oauth2\api::get_all_issuers(true); + $this->assertCount(3, $issuers); + $expected = ['Google', 'Microsoft', 'Facebook']; + $this->assertEqualsCanonicalizing($expected, + [$issuers[0]->get_display_name(), $issuers[1]->get_display_name(), $issuers[2]->get_display_name()]); + } + + /** + * Test for is available for login. + */ + public function test_is_available_for_login() { + $this->resetAfterTest(); + $this->setAdminUser(); + $googleissuer = core\oauth2\api::create_standard_issuer('google'); + + // Set Google issuer to be shown only on login page. + $record = $googleissuer->to_record(); + $record->showonloginpage = $googleissuer::LOGINONLY; + core\oauth2\api::update_issuer($record); + + $this->assertFalse($googleissuer->is_available_for_login()); + + // Set a clientid and clientsecret. + $googleissuer->set('clientid', 'clientid'); + $googleissuer->set('clientsecret', 'secret'); + $googleissuer->update(); + + $this->assertTrue($googleissuer->is_available_for_login()); + + // Set showonloginpage to service only. + $googleissuer->set('showonloginpage', \core\oauth2\issuer::SERVICEONLY); + $googleissuer->update(); + + $this->assertFalse($googleissuer->is_available_for_login()); + + // Set showonloginpage to everywhere (service and login) and disable issuer. + $googleissuer->set('showonloginpage', \core\oauth2\issuer::EVERYWHERE); + $googleissuer->set('enabled', 0); + $googleissuer->update(); + + $this->assertFalse($googleissuer->is_available_for_login()); + + // Enable issuer. + $googleissuer->set('enabled', 1); + $googleissuer->update(); + + $this->assertTrue($googleissuer->is_available_for_login()); + + // Remove userinfo endpoint from issuer. + $endpoint = core\oauth2\endpoint::get_record([ + 'issuerid' => $googleissuer->get('id'), + 'name' => 'userinfo_endpoint' + ]); + \core\oauth2\api::delete_endpoint($endpoint->get('id')); + + $this->assertFalse($googleissuer->is_available_for_login()); + } } From 444a4a8c3e1493130e9580323d4da53037935dd4 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Tue, 20 Apr 2021 12:07:54 +0200 Subject: [PATCH 4/4] MDL-71017 oath2: configuration check fix and return types --- admin/tool/oauth2/classes/output/renderer.php | 2 +- admin/tool/oauth2/tests/behat/basic_settings.feature | 6 +++--- lib/classes/oauth2/api.php | 7 ++++--- lib/classes/oauth2/issuer.php | 4 ++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/admin/tool/oauth2/classes/output/renderer.php b/admin/tool/oauth2/classes/output/renderer.php index 3eb41b00384..cb79978ab10 100644 --- a/admin/tool/oauth2/classes/output/renderer.php +++ b/admin/tool/oauth2/classes/output/renderer.php @@ -102,7 +102,7 @@ class renderer extends plugin_renderer_base { // Internal services issuer. if ((int)$issuer->get('showonloginpage') == issuer::LOGINONLY) { $serviceissuer = $this->pix_icon('no', get_string('issuersservicesnotallow', 'tool_oauth2'), 'tool_oauth2'); - } else if ($issuer->get('id') && $issuer->is_configured() && !empty($issuer->get_endpoint_url('userinfo'))) { + } else if ($issuer->get('id') && $issuer->is_configured()) { $serviceissuer = $this->pix_icon('yes', get_string('issuersservicesallow', 'tool_oauth2'), 'tool_oauth2'); } else { $serviceissuer = $this->pix_icon('notconfigured', get_string('notconfigured', 'tool_oauth2'), 'tool_oauth2'); diff --git a/admin/tool/oauth2/tests/behat/basic_settings.feature b/admin/tool/oauth2/tests/behat/basic_settings.feature index 16a4e924195..1a258b3bfd7 100644 --- a/admin/tool/oauth2/tests/behat/basic_settings.feature +++ b/admin/tool/oauth2/tests/behat/basic_settings.feature @@ -152,7 +152,7 @@ Feature: Basic OAuth2 functionality When I press "Save changes" Then I should see "Changes saved" And I should see "OpenBadges" - And "Not configured" "icon" should exist in the "OpenBadges" "table_row" + And "Allow services" "icon" should exist in the "OpenBadges" "table_row" And "Do not allow login" "icon" should exist in the "OpenBadges" "table_row" And "Service discovery successful" "icon" should exist in the "OpenBadges" "table_row" And the "src" attribute of "table.admintable th img" "css_element" should contain "IMS-Global-Logo.png" @@ -222,8 +222,8 @@ Feature: Basic OAuth2 functionality When I press "Save changes" Then I should see "Could not discover end points for identity issuer: Invalid custom service" And I should see "URL: https://dc.imsglobal.org/.well-known/openid-configuration" + And "Allow services" "icon" should exist in the "Invalid custom service" "table_row" And "Do not allow login" "icon" should exist in the "Invalid custom service" "table_row" - And "Not configured" "icon" should exist in the "Invalid custom service" "table_row" And I should see "-" in the "Invalid custom service" "table_row" And I click on "Configure endpoints" "link" in the "Invalid custom service" "table_row" And I should not see "discovery_endpoint" @@ -263,8 +263,8 @@ Feature: Basic OAuth2 functionality When I press "Save changes" And I should see "Changes saved" And I should see "Empty custom service" + And "Allow services" "icon" should exist in the "Empty custom service" "table_row" And "Do not allow login" "icon" should exist in the "Empty custom service" "table_row" - And "Not configured" "icon" should exist in the "Empty custom service" "table_row" And I should see "-" in the "Empty custom service" "table_row" And I click on "Configure endpoints" "link" in the "Empty custom service" "table_row" And I should not see "discovery_endpoint" diff --git a/lib/classes/oauth2/api.php b/lib/classes/oauth2/api.php index ed6b4fd935d..c083deff69d 100644 --- a/lib/classes/oauth2/api.php +++ b/lib/classes/oauth2/api.php @@ -111,11 +111,12 @@ class api { /** * List all the issuers, ordered by the sortorder field * - * @param bool $showall also include issues that are configured to be shown only on login page + * @param bool $includeloginonly also include issuers that are configured to be shown only on login page, + * By default false, in this case the method returns all issuers that can be used in services * @return \core\oauth2\issuer[] */ - public static function get_all_issuers(bool $showall = false) { - if ($showall) { + public static function get_all_issuers(bool $includeloginonly = false) { + if ($includeloginonly) { return issuer::get_records([], 'sortorder'); } else { return array_values(issuer::get_records_select('showonloginpage<>?', [issuer::LOGINONLY], 'sortorder')); diff --git a/lib/classes/oauth2/issuer.php b/lib/classes/oauth2/issuer.php index 758f693c7df..89beafefb94 100644 --- a/lib/classes/oauth2/issuer.php +++ b/lib/classes/oauth2/issuer.php @@ -196,7 +196,7 @@ class issuer extends persistent { * @return bool * @throws \coding_exception */ - public function is_available_for_login() { + public function is_available_for_login(): bool { return $this->get('id') && $this->is_configured() && $this->get('showonloginpage') != self::SERVICEONLY && @@ -265,7 +265,7 @@ class issuer extends persistent { * * @return string */ - public function get_display_name() { + public function get_display_name(): string { return $this->get('loginpagename') ? $this->get('loginpagename') : $this->get('name'); } }