From fc02628620721510afe336fa2a50e519fe19da2e Mon Sep 17 00:00:00 2001 From: David Matamoros Date: Mon, 29 Mar 2021 10:11:05 +0200 Subject: [PATCH] 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()); + } }