From 9a26c37ffd3b9d98a3422963d7925383d39efed8 Mon Sep 17 00:00:00 2001 From: odain Date: Tue, 16 Jan 2024 18:23:02 +0100 Subject: [PATCH 1/4] =?UTF-8?q?N=C2=B07147=20-=20Error=20HTTP=20500=20due?= =?UTF-8?q?=20to=20access=5Ftoken=20not=20URL=20decoded?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../TwigBase/Controller/Controller.php | 21 +++++- .../application/twigbase/ControllerTest.php | 75 +++++++++++++++++++ .../application/twigbase/FakeController.php | 5 ++ 3 files changed, 97 insertions(+), 4 deletions(-) create mode 100644 tests/php-unit-tests/unitary-tests/application/twigbase/ControllerTest.php create mode 100644 tests/php-unit-tests/unitary-tests/application/twigbase/FakeController.php diff --git a/sources/Application/TwigBase/Controller/Controller.php b/sources/Application/TwigBase/Controller/Controller.php index 326048812..256db1c79 100644 --- a/sources/Application/TwigBase/Controller/Controller.php +++ b/sources/Application/TwigBase/Controller/Controller.php @@ -256,6 +256,7 @@ abstract class Controller extends AbstractController } /** + * @since 3.0.0 N°3606 - Adapt TwigBase Controller for combodo-monitoring extension * @throws \Exception */ protected function CheckAccess() @@ -271,12 +272,24 @@ abstract class Controller extends AbstractController if (empty($sExecModule) || empty($sConfiguredAccessTokenValue)){ LoginWebPage::DoLogin($this->m_bMustBeAdmin); - }else { + } else { //token mode without login required - $sPassedToken = utils::ReadParam($this->m_sAccessTokenConfigParamId, null); - if ($sPassedToken !== $sConfiguredAccessTokenValue){ + //N°7147 - Error HTTP 500 due to access_token not URL decoded + $sPassedToken = utils::ReadPostedParam($this->m_sAccessTokenConfigParamId, null, false, 'raw_data'); + if (is_null($sPassedToken)){ + $sPassedToken = utils::ReadParam($this->m_sAccessTokenConfigParamId, null, false, 'raw_data'); + } + + $sDecodedPassedToken = urldecode($sPassedToken); + var_dump([$sPassedToken, $sDecodedPassedToken]); + if ($sDecodedPassedToken !== $sConfiguredAccessTokenValue){ $sMsg = "Invalid token passed under '$this->m_sAccessTokenConfigParamId' http param to reach '$sExecModule' page."; - IssueLog::Error($sMsg); + IssueLog::Error($sMsg, null, + [ + 'sHtmlDecodedToken' => $sDecodedPassedToken, + 'conf param ID' => $this->m_sAccessTokenConfigParamId + ] + ); throw new Exception("Invalid token"); } } diff --git a/tests/php-unit-tests/unitary-tests/application/twigbase/ControllerTest.php b/tests/php-unit-tests/unitary-tests/application/twigbase/ControllerTest.php new file mode 100644 index 000000000..9a6c82804 --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/application/twigbase/ControllerTest.php @@ -0,0 +1,75 @@ +RequireOnceUnitTestFile('FakeController.php'); + } + + public function CheckAccessProvider() { + return [ + 'simple token access OK' => [ + 'access_token' => 'toto123', + 'http_access_token' => 'toto123', + 'bSuccess' => true, + ], + 'simple token access OK sent by POST' => [ + 'access_token' => 'toto123', + 'http_access_token' => 'toto123', + 'bSuccess' => true, + 'bPost' => true, + ], + 'simple token access FAILED' => [ + 'access_token' => 'toto123', + 'http_access_token' => 'toto124', + 'bSuccess' => false, + ], + 'url encoded token access OK' => [ + 'access_token' => 'rfb4j"E?7}-ZJq4T^B*26pk8{;zxem', + 'http_access_token' => 'rfb4j%22E%3F7%7D-ZJq4T%5EB%2A26pk8%7B%3Bzxem', + 'bSuccess' => true, + ], + ]; + } + + /** + * Fix N°7147 + * @dataProvider CheckAccessProvider + */ + public function testCheckAccess($sConfiguredAccessToken, $sHttpAccessToken, $bSuccess, $bPost=false){ + $sModuleName = "MyModule"; + $sTokenParamName = "access_token_conf_param"; + + $_SESSION = []; + $_POST = []; + $_REQUEST = []; + + $_REQUEST['exec_module'] = $sModuleName; + if ($bPost){ + $_POST[$sTokenParamName] = $sHttpAccessToken; + } else { + $_REQUEST[$sTokenParamName] = $sHttpAccessToken; + } + + $oController = new FakeController(); + $oController->SetAccessTokenConfigParamId($sTokenParamName); + + MetaModel::GetConfig()->SetModuleSetting($sModuleName, $sTokenParamName, $sConfiguredAccessToken); + + if (! $bSuccess){ + $this->expectExceptionMessage("Invalid token"); + } + + $this->InvokeNonPublicMethod(FakeController::class, "CheckAccess", $oController); + + if ($bSuccess){ + $this->assertTrue(true, "no issue encountered"); + } + } +} diff --git a/tests/php-unit-tests/unitary-tests/application/twigbase/FakeController.php b/tests/php-unit-tests/unitary-tests/application/twigbase/FakeController.php new file mode 100644 index 000000000..262e0d8dd --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/application/twigbase/FakeController.php @@ -0,0 +1,5 @@ + Date: Tue, 23 Jan 2024 20:45:35 +0100 Subject: [PATCH 2/4] =?UTF-8?q?N=C2=B07147=20-=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- sources/Application/TwigBase/Controller/Controller.php | 1 - 1 file changed, 1 deletion(-) diff --git a/sources/Application/TwigBase/Controller/Controller.php b/sources/Application/TwigBase/Controller/Controller.php index 256db1c79..9e6ad611e 100644 --- a/sources/Application/TwigBase/Controller/Controller.php +++ b/sources/Application/TwigBase/Controller/Controller.php @@ -281,7 +281,6 @@ abstract class Controller extends AbstractController } $sDecodedPassedToken = urldecode($sPassedToken); - var_dump([$sPassedToken, $sDecodedPassedToken]); if ($sDecodedPassedToken !== $sConfiguredAccessTokenValue){ $sMsg = "Invalid token passed under '$this->m_sAccessTokenConfigParamId' http param to reach '$sExecModule' page."; IssueLog::Error($sMsg, null, From 618d8e6468ba8c1cd948ef155e5c18c639d4d787 Mon Sep 17 00:00:00 2001 From: Dennis Lassiter Date: Wed, 24 Jan 2024 14:38:54 +0100 Subject: [PATCH 3/4] =?UTF-8?q?N=C2=B05775=20-=20Allow=20configuration=20o?= =?UTF-8?q?f=20OAuth=20client=20on=20MS=20Azure=20with=20single=20tenant?= =?UTF-8?q?=20(#553)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add Tenant-Support for Azure OAuthClient * Improvement: Make tenant required * Improvment: Removed check for null-value Since last commit, the "tenant"-field either set to a custom value or "common" by default. It is not allowed to be null * Add field description --------- Co-authored-by: Molkobain --- .../datamodel.itop-oauth-client.xml | 14 +++++++++++--- .../en.dict.itop-oauth-client.php | 2 ++ .../Client/OAuth/OAuthClientProviderAzure.php | 3 ++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/datamodels/2.x/itop-oauth-client/datamodel.itop-oauth-client.xml b/datamodels/2.x/itop-oauth-client/datamodel.itop-oauth-client.xml index c1efa7281..53893cba8 100644 --- a/datamodels/2.x/itop-oauth-client/datamodel.itop-oauth-client.xml +++ b/datamodels/2.x/itop-oauth-client/datamodel.itop-oauth-client.xml @@ -339,6 +339,11 @@ no true + + tenant + common + false +
@@ -364,15 +369,18 @@ 50 - + 60 - + 70 - + 80 + + 90 + diff --git a/datamodels/2.x/itop-oauth-client/en.dict.itop-oauth-client.php b/datamodels/2.x/itop-oauth-client/en.dict.itop-oauth-client.php index b64e1c838..f11877a21 100644 --- a/datamodels/2.x/itop-oauth-client/en.dict.itop-oauth-client.php +++ b/datamodels/2.x/itop-oauth-client/en.dict.itop-oauth-client.php @@ -93,6 +93,8 @@ Dict::Add('EN US', 'English', 'English', array( 'Class:OAuthClientAzure/Attribute:used_for_smtp+' => 'At least one OAuth client must have this flag to “Yes”, if you want iTop to use it for sending mails', 'Class:OAuthClientAzure/Attribute:used_for_smtp/Value:yes' => 'Yes', 'Class:OAuthClientAzure/Attribute:used_for_smtp/Value:no' => 'No', + 'Class:OAuthClientAzure/Attribute:tenant' => 'Tenant', + 'Class:OAuthClientAzure/Attribute:tenant+' => 'Tenant ID of the configured application. For multi-tenant application, use common.', )); // diff --git a/sources/Core/Authentication/Client/OAuth/OAuthClientProviderAzure.php b/sources/Core/Authentication/Client/OAuth/OAuthClientProviderAzure.php index 667d5875a..e77141d72 100644 --- a/sources/Core/Authentication/Client/OAuth/OAuthClientProviderAzure.php +++ b/sources/Core/Authentication/Client/OAuth/OAuthClientProviderAzure.php @@ -20,8 +20,9 @@ class OAuthClientProviderAzure extends OAuthClientProviderAbstract 'clientId' => $oOAuthClient->Get('client_id'), 'clientSecret' => $oOAuthClient->Get('client_secret'), 'redirectUri' => $oOAuthClient->Get('redirect_url'), + 'tenant' => $oOAuthClient->Get('tenant'), ]; $this->oVendorProvider = new Azure($aOptions, $collaborators); } -} \ No newline at end of file +} From 1b3a2c8470ac51790120df5cd1ea452d86853ae7 Mon Sep 17 00:00:00 2001 From: Molkobain Date: Wed, 24 Jan 2024 14:49:51 +0100 Subject: [PATCH 4/4] =?UTF-8?q?N=C2=B05775=20-=20Show=20error=20message=20?= =?UTF-8?q?to=20user=20in=20case=20of=20issue=20during=20token=20generatio?= =?UTF-8?q?n?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2.x/itop-oauth-client/assets/js/oauth_connect.js | 2 ++ .../src/Controller/AjaxOauthClientController.php | 10 ++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/datamodels/2.x/itop-oauth-client/assets/js/oauth_connect.js b/datamodels/2.x/itop-oauth-client/assets/js/oauth_connect.js index 4cc1c85d1..35dc4f391 100644 --- a/datamodels/2.x/itop-oauth-client/assets/js/oauth_connect.js +++ b/datamodels/2.x/itop-oauth-client/assets/js/oauth_connect.js @@ -92,6 +92,8 @@ const OAuthConnect = function(sClass, sId, sAjaxUri) { function (oData) { if (oData.status === 'success') { oOpenSignInWindow(oData.data.authorization_url, 'OAuth authorization') + } else { + alert(oData.error_description); } } ); diff --git a/datamodels/2.x/itop-oauth-client/src/Controller/AjaxOauthClientController.php b/datamodels/2.x/itop-oauth-client/src/Controller/AjaxOauthClientController.php index 4ec94b2f6..bfb47282b 100644 --- a/datamodels/2.x/itop-oauth-client/src/Controller/AjaxOauthClientController.php +++ b/datamodels/2.x/itop-oauth-client/src/Controller/AjaxOauthClientController.php @@ -10,6 +10,7 @@ use cmdbAbstractObject; use Combodo\iTop\Application\TwigBase\Controller\Controller; use Combodo\iTop\Core\Authentication\Client\OAuth\OAuthClientProviderFactory; use Dict; +use Exception; use IssueLog; use League\OAuth2\Client\Provider\Exception\IdentityProviderException; use MetaModel; @@ -31,8 +32,13 @@ class AjaxOauthClientController extends Controller $aResult = ['status' => 'success', 'data' => []]; - $sAuthorizationUrl = OAuthClientProviderFactory::GetAuthorizationUrl($oOAuthClient); - $aResult['data']['authorization_url'] = $sAuthorizationUrl; + try { + $sAuthorizationUrl = OAuthClientProviderFactory::GetAuthorizationUrl($oOAuthClient); + $aResult['data']['authorization_url'] = $sAuthorizationUrl; + } catch (Exception $oException) { + $aResult['status'] = 'error'; + $aResult['error_description'] = $oException->getMessage(); + } $this->DisplayJSONPage($aResult); }