Skip to content

Commit 47aac8e

Browse files
pascalseelandmjansenDatabay
pascalseeland
authored and
mjansenDatabay
committed
Fixing findings of the PHP8-Review
1 parent ca5b36b commit 47aac8e

9 files changed

+90
-62
lines changed

‎Services/Init/classes/class.ilStartUpGUI.php

+5-1
Original file line numberDiff line numberDiff line change
@@ -1929,7 +1929,11 @@ protected function doOpenIdConnectAuthentication()
19291929
switch ($status->getStatus()) {
19301930
case ilAuthStatus::STATUS_AUTHENTICATED:
19311931
$this->logger->debug('Authentication successful; Redirecting to starting page.');
1932-
ilInitialisation::redirectToStartingPage();
1932+
if ($credentials->getRedirectionTarget()) {
1933+
ilInitialisation::redirectToStartingPage($credentials->getRedirectionTarget());
1934+
} else {
1935+
ilInitialisation::redirectToStartingPage();
1936+
}
19331937
return;
19341938

19351939
case ilAuthStatus::STATUS_AUTHENTICATION_FAILED:

‎Services/OpenIdConnect/classes/class.ilAuthFrontendCredentialsOpenIdConnect.php

+9-3
Original file line numberDiff line numberDiff line change
@@ -20,22 +20,29 @@
2020
class ilAuthFrontendCredentialsOpenIdConnect extends ilAuthFrontendCredentials
2121
{
2222
private const SESSION_TARGET = 'oidc_target';
23+
private const QUERY_PARAM_TARGET = 'target';
2324

2425
private ilOpenIdConnectSettings $settings;
2526
private ?string $target = null;
2627

2728
public function __construct()
2829
{
30+
global $DIC;
31+
2932
parent::__construct();
3033
$this->settings = ilOpenIdConnectSettings::getInstance();
34+
$httpquery = $DIC->http()->wrapper()->query();
35+
if ($httpquery->has(self::QUERY_PARAM_TARGET)) {
36+
$this->target = $httpquery->retrieve(self::QUERY_PARAM_TARGET, $DIC->refinery()->to()->string());
37+
}
3138
}
3239

3340
protected function getSettings() : ilOpenIdConnectSettings
3441
{
3542
return $this->settings;
3643
}
3744

38-
public function getRedirectionTarget() : string
45+
public function getRedirectionTarget() : ?string
3946
{
4047
return $this->target;
4148
}
@@ -50,8 +57,7 @@ public function initFromRequest() : void
5057

5158
protected function parseRedirectionTarget() : void
5259
{
53-
if (!empty($_GET['target'])) { // TODO PHP8-REVIEW Please eliminate this.
54-
$this->target = $_GET['target']; // TODO PHP8-REVIEW Please eliminate this.
60+
if ($this->target) {
5561
ilSession::set(self::SESSION_TARGET, $this->target);
5662
} elseif (ilSession::get(self::SESSION_TARGET)) {
5763
$this->target = ilSession::get(self::SESSION_TARGET);

‎Services/OpenIdConnect/classes/class.ilAuthProviderOpenIdConnect.php

+26-20
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,18 @@
2323
class ilAuthProviderOpenIdConnect extends ilAuthProvider
2424
{
2525
private ilOpenIdConnectSettings $settings;
26+
/** @var array $body */
27+
private $body;
28+
private ilLogger $logger;
2629

2730
public function __construct(ilAuthCredentials $credentials)
2831
{
32+
global $DIC;
2933
parent::__construct($credentials);
34+
35+
$this->logger = $DIC->logger()->auth();
3036
$this->settings = ilOpenIdConnectSettings::getInstance();
37+
$this->body = $DIC->http()->request()->getParsedBody();
3138
}
3239

3340
public function handleLogout() : void
@@ -37,7 +44,7 @@ public function handleLogout() : void
3744
}
3845

3946
$auth_token = ilSession::get('oidc_auth_token');
40-
$this->getLogger()->debug('Using token: ' . $auth_token);
47+
$this->logger->debug('Using token: ' . $auth_token);
4148

4249
if (isset($auth_token) && $auth_token !== '') {
4350
ilSession::set('oidc_auth_token', '');
@@ -65,7 +72,7 @@ public function doAuthentication(ilAuthStatus $status) : bool
6572
$oidc->setHttpProxy($host);
6673
}
6774

68-
$this->getLogger()->debug(
75+
$this->logger->debug(
6976
'Redirect url is: ' .
7077
$oidc->getRedirectURL()
7178
);
@@ -79,32 +86,31 @@ public function doAuthentication(ilAuthStatus $status) : bool
7986

8087
$oidc->addScope($this->settings->getAllScopes());
8188
$oidc->addAuthParam(['response_mode' => 'form_post']);
82-
switch ($this->settings->getLoginPromptType()) {
83-
case ilOpenIdConnectSettings::LOGIN_ENFORCE:
84-
$oidc->addAuthParam(['prompt' => 'login']);
85-
break;
89+
if ($this->settings->getLoginPromptType() === ilOpenIdConnectSettings::LOGIN_ENFORCE) {
90+
$oidc->addAuthParam(['prompt' => 'login']);
8691
}
8792
$oidc->setAllowImplicitFlow(true);
8893

8994
$oidc->authenticate();
9095
// user is authenticated, otherwise redirected to authorization endpoint or exception
91-
$this->getLogger()->dump($_REQUEST, ilLogLevel::DEBUG);
96+
$this->logger->dump($this->body, ilLogLevel::DEBUG);
9297

9398
$claims = $oidc->getVerifiedClaims(null);
94-
$this->getLogger()->dump($claims, ilLogLevel::DEBUG);
99+
$this->logger->dump($claims, ilLogLevel::DEBUG);
95100
$status = $this->handleUpdate($status, $claims);
96101

97102
// @todo : provide a general solution for all authentication methods
98-
$_GET['target'] = (string) $this->getCredentials()->getRedirectionTarget();// TODO PHP8-REVIEW Please eliminate this. Mutating the request is not allowed and will not work in ILIAS 8.
103+
//$_GET['target'] = $this->getCredentials()->getRedirectionTarget();// TODO PHP8-REVIEW Please eliminate this. Mutating the request is not allowed and will not work in ILIAS 8.
99104

100-
if ($this->settings->getLogoutScope() === ilOpenIdConnectSettings::LOGOUT_SCOPE_GLOBAL) {
101-
$token = $oidc->requestClientCredentialsToken();
102-
ilSession::set('oidc_auth_token', $token->access_token);
103-
}
105+
//TODO fix this. There is a PR and it is broken in 7 as well
106+
//if ($this->settings->getLogoutScope() === ilOpenIdConnectSettings::LOGOUT_SCOPE_GLOBAL) {
107+
//$token = $oidc->requestClientCredentialsToken();
108+
//ilSession::set('oidc_auth_token', $token->access_token);
109+
//}
104110
return true;
105111
} catch (Exception $e) {
106-
$this->getLogger()->warning($e->getMessage());
107-
$this->getLogger()->warning($e->getCode());
112+
$this->logger->warning($e->getMessage());
113+
$this->logger->warning((string) $e->getCode());
108114
$status->setStatus(ilAuthStatus::STATUS_AUTHENTICATION_FAILED);
109115
$status->setTranslatedReason($e->getMessage());
110116
return false;
@@ -120,17 +126,17 @@ public function doAuthentication(ilAuthStatus $status) : bool
120126
private function handleUpdate(ilAuthStatus $status, $user_info) : ilAuthStatus
121127
{
122128
if (!is_object($user_info)) {
123-
$this->getLogger()->error('Received invalid user credentials: ');
124-
$this->getLogger()->dump($user_info, ilLogLevel::ERROR);
129+
$this->logger->error('Received invalid user credentials: ');
130+
$this->logger->dump($user_info, ilLogLevel::ERROR);
125131
$status->setStatus(ilAuthStatus::STATUS_AUTHENTICATION_FAILED);
126132
$status->setReason('err_wrong_login');
127133
return $status;
128134
}
129135

130136
$uid_field = $this->settings->getUidField();
131-
$ext_account = $user_info->$uid_field;
137+
$ext_account = $user_info->{$uid_field};
132138

133-
$this->getLogger()->debug('Authenticated external account: ' . $ext_account);
139+
$this->logger->debug('Authenticated external account: ' . $ext_account);
134140

135141

136142
$int_account = ilObjUser::_checkExternalAuthAccount(
@@ -154,7 +160,7 @@ private function handleUpdate(ilAuthStatus $status, $user_info) : ilAuthStatus
154160
$status->setAuthenticatedUserId($user_id);
155161
$status->setStatus(ilAuthStatus::STATUS_AUTHENTICATED);
156162

157-
$_GET['target'] = (string) $this->getCredentials()->getRedirectionTarget();// TODO PHP8-REVIEW Please eliminate this. Mutating the request is not allowed and will not work in ILIAS 8.
163+
//$_GET['target'] = $this->getCredentials()->getRedirectionTarget();// TODO PHP8-REVIEW Please eliminate this. Mutating the request is not allowed and will not work in ILIAS 8.
158164
} catch (ilOpenIdConnectSyncForbiddenException $e) {
159165
$status->setStatus(ilAuthStatus::STATUS_AUTHENTICATION_FAILED);
160166
$status->setReason('err_wrong_login');

‎Services/OpenIdConnect/classes/class.ilOpenIdConnectAppEventListener.php

+5-8
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,11 @@ protected function handleLogoutFor(int $user_id) : void
3232
*/
3333
public static function handleEvent(string $a_component, string $a_event, array $a_parameter) : void
3434
{
35-
ilLoggerFactory::getLogger('root')->info($a_component . ' : ' . $a_event);
36-
if ($a_component === 'Services/Authentication') {
37-
ilLoggerFactory::getLogger('root')->info($a_component . ' : ' . $a_event);
38-
if ($a_event === 'beforeLogout') {
39-
ilLoggerFactory::getLogger('root')->info($a_component . ' : ' . $a_event);
40-
$listener = new self();
41-
$listener->handleLogoutFor($a_parameter['user_id']);
42-
}
35+
global $DIC;
36+
$DIC->logger()->auth()->debug($a_component . ' : ' . $a_event);
37+
if (($a_component === 'Services/Authentication') && $a_event === 'beforeLogout') {
38+
$listener = new self();
39+
$listener->handleLogoutFor($a_parameter['user_id']);
4340
}
4441
}
4542
}

‎Services/OpenIdConnect/classes/class.ilOpenIdConnectSettings.php

+13-15
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,7 @@ public function getRoleMappingUpdateForId(int $a_role_id) : bool
312312

313313
public function validateScopes(string $provider, array $custom_scopes) : array
314314
{
315+
$result = [];
315316
try {
316317
$curl = new ilCurlConnection($provider . '/.well-known/openid-configuration');
317318
$curl->init();
@@ -322,20 +323,17 @@ public function validateScopes(string $provider, array $custom_scopes) : array
322323

323324
$response = json_decode($curl->exec(), false, 512, JSON_THROW_ON_ERROR);
324325

325-
if ($curl->getInfo(CURLINFO_RESPONSE_CODE) !== 200) {
326-
return array();
327-
}
328-
329-
$available_scopes = $response->scopes_supported;
330-
array_unshift($custom_scopes, self::DEFAULT_SCOPE);
326+
if ($curl->getInfo(CURLINFO_RESPONSE_CODE) === 200) {
327+
$available_scopes = $response->scopes_supported;
328+
array_unshift($custom_scopes, self::DEFAULT_SCOPE);
331329

332-
$result = array_diff($custom_scopes, $available_scopes);
333-
} catch (ilCurlConnectionException $e) {
334-
throw $e;
330+
$result = array_diff($custom_scopes, $available_scopes);
331+
}
335332
} finally {
336-
$curl->close();
333+
if (isset($curl)) {
334+
$curl->close();
335+
}
337336
}
338-
339337
return $result;
340338
}
341339

@@ -384,11 +382,11 @@ protected function load() : void
384382
$this->setLoginElementType((int) $this->storage->get('le_type'));
385383
$this->setLoginPromptType((int) $this->storage->get('prompt_type', (string) self::LOGIN_ENFORCE));
386384
$this->setLogoutScope((int) $this->storage->get('logout_scope', (string) self::LOGOUT_SCOPE_GLOBAL));
387-
$this->useCustomSession((bool) $this->storage->get('custom_session'), '0');
385+
$this->useCustomSession((bool) $this->storage->get('custom_session', '0'));
388386
$this->setSessionDuration((int) $this->storage->get('session_duration', '60'));
389-
$this->allowSync((bool) $this->storage->get('allow_sync'), '0');
390-
$this->setRole((int) $this->storage->get('role'), '0');
391-
$this->setUidField((string) $this->storage->get('uid'), '');
387+
$this->allowSync((bool) $this->storage->get('allow_sync', '0'));
388+
$this->setRole((int) $this->storage->get('role', '0'));
389+
$this->setUidField((string) $this->storage->get('uid', ''));
392390
$this->setRoleMappings((array) unserialize(
393391
$this->storage->get('role_mappings', serialize([])),
394392
['allowed_classes' => false]

‎Services/OpenIdConnect/classes/class.ilOpenIdConnectSettingsGUI.php

+9-5
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@ class ilOpenIdConnectSettingsGUI
2727
private const STAB_ROLES = 'roles';
2828

2929
private const DEFAULT_CMD = 'settings';
30-
private int $ref_id = 0;
30+
private int $ref_id;
31+
/** @var array $body */
32+
private $body;
3133
private ilOpenIdConnectSettings $settings;
3234
private ilLanguage $lng;
3335
private ilCtrl $ctrl;
@@ -57,7 +59,7 @@ public function __construct(int $a_ref_id)
5759
$this->review = $DIC->rbac()->review();
5860
$this->error = $DIC['ilErr'];
5961
$this->upload = $DIC->upload();
60-
62+
$this->body = $DIC->http()->request()->getParsedBody();
6163
$this->settings = ilOpenIdConnectSettings::getInstance();
6264
}
6365

@@ -540,7 +542,7 @@ protected function saveRoles() : void
540542
$this->checkAccess('write');
541543
$form = $this->initRolesForm();
542544
if ($form->checkInput()) {
543-
$this->logger->dump($_POST, ilLogLevel::DEBUG); // TODO PHP8-REVIEW Please Use the `getParsedBody()` method of the HTTP request instead
545+
$this->logger->dump($this->body, ilLogLevel::DEBUG);
544546

545547

546548
$role_settings = [];
@@ -554,11 +556,13 @@ protected function saveRoles() : void
554556
$this->logger->dump($role_params, ilLogLevel::DEBUG);
555557

556558
if (count($role_params) !== 2) {
557-
$form->getItemByPostVar('role_map_' . $role_id)->setAlert($this->lng->txt('msg_wrong_format'));
559+
if ($form->getItemByPostVar('role_map_' . $role_id)) {
560+
$form->getItemByPostVar('role_map_' . $role_id)->setAlert($this->lng->txt('msg_wrong_format'));
561+
}
558562
$role_valid = false;
559563
continue;
560564
}
561-
$role_settings[(int) $role_id]['update'] = (bool) !$form->getInput('role_map_update_' . $role_id);
565+
$role_settings[(int) $role_id]['update'] = !$form->getInput('role_map_update_' . $role_id);
562566
$role_settings[(int) $role_id]['value'] = (string) $form->getInput('role_map_' . $role_id);
563567
}
564568

‎Services/OpenIdConnect/classes/class.ilOpenIdConnectUserSync.php

+2-3
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ protected function parseRoleAssignments() : array
204204
}
205205

206206
if (is_array($this->user_info->{$role_attribute})) {
207-
if (!in_array($role_value, $this->user_info->{$role_attribute})) {
207+
if (!in_array($role_value, $this->user_info->{$role_attribute}, true)) {
208208
$this->logger->debug('User account has no ' . $role_value);
209209
continue;
210210
}
@@ -256,7 +256,6 @@ protected function valueFrom(string $connect_name) : string
256256
return '';
257257
}
258258

259-
$val = $this->user_info->{$connect_name};
260-
return $val;
259+
return $this->user_info->{$connect_name};
261260
}
262261
}

‎composer.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@
5858
"league/flysystem": "^1.0",
5959
"james-heinrich/getid3": "^1.9",
6060
"phpoffice/phpspreadsheet": "^1.18.0",
61-
"jumbojett/openid-connect-php": "^0.9.2",
61+
"jumbojett/openid-connect-php": "^0.9.5",
6262
"sabre/dav": "^4.1",
6363
"symfony/console" : "^5.0",
6464
"slim/slim": "^3.11",

‎openidconnect.php

+20-6
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,25 @@
11
<?php
2-
include_once './Services/Context/classes/class.ilContext.php';
2+
/******************************************************************************
3+
*
4+
* This file is part of ILIAS, a powerful learning management system.
5+
*
6+
* ILIAS is licensed with the GPL-3.0, you should have received a copy
7+
* of said license along with the source code.
8+
*
9+
* If this is not the case or you just want to try ILIAS, you'll find
10+
* us at:
11+
* https://www.ilias.de
12+
* https://github.com/ILIAS-eLearning
13+
*
14+
*****************************************************************************/
15+
/** @noRector */
16+
require_once("libs/composer/vendor/autoload.php");
317
ilContext::init(ilContext::CONTEXT_SHIBBOLETH);
4-
5-
require_once("Services/Init/classes/class.ilInitialisation.php");
618
ilInitialisation::initILIAS();
19+
global $DIC;
20+
721

822
// authentication is done here ->
9-
$ilCtrl->setCmd('doOpenIdConnectAuthentication');
10-
$ilCtrl->setTargetScript('ilias.php');
11-
$ilCtrl->callBaseClass('ilStartUpGUI');
23+
$DIC->ctrl()->setCmd('doOpenIdConnectAuthentication');
24+
$DIC->ctrl()->setTargetScript('ilias.php');
25+
$DIC->ctrl()->callBaseClass(ilStartUpGUI::class);

0 commit comments

Comments
 (0)