From 193807b435074868efb6ad2f575abdcb1c8fe1f8 Mon Sep 17 00:00:00 2001 From: Daniel Seifert Date: Fri, 27 Jan 2023 08:49:09 +0100 Subject: [PATCH] assert that crendentialId and credential fits into database fields both values have no defined length, possibly the database fields are still too short --- composer.json | 3 +- .../Controller/Admin/d3user_webauthn.php | 3 +- .../Controller/d3_account_webauthn.php | 18 +++- .../Model/Credential/PublicKeyCredential.php | 26 +++++- .../Credential/PublicKeyCredentialList.php | 3 + .../translations/de/d3webauthn_lang.php | 1 + src/Setup/Actions.php | 7 +- .../Controller/d3_account_webauthnTest.php | 82 +++++++++++++------ .../Credential/PublicKeyCredentialTest.php | 4 +- 9 files changed, 114 insertions(+), 33 deletions(-) diff --git a/composer.json b/composer.json index 89cd32f..85c2a79 100644 --- a/composer.json +++ b/composer.json @@ -46,7 +46,8 @@ "nyholm/psr7-server": "^1.0.2", "ext-json": "*", "d3/testingtools": "^1.1", - "d3/oxid-dic-handler": "^1.0" + "d3/oxid-dic-handler": "^1.0", + "beberlei/assert": "^3.2" }, "require-dev": { "friendsofphp/php-cs-fixer": "^2.19", diff --git a/src/Application/Controller/Admin/d3user_webauthn.php b/src/Application/Controller/Admin/d3user_webauthn.php index 0d0605a..cfddf64 100755 --- a/src/Application/Controller/Admin/d3user_webauthn.php +++ b/src/Application/Controller/Admin/d3user_webauthn.php @@ -93,7 +93,6 @@ class d3user_webauthn extends AdminDetailsController /** * @return void - * @throws AssertionFailedException * @throws Throwable */ public function saveAuthn(): void @@ -116,7 +115,7 @@ class d3user_webauthn extends AdminDetailsController d3GetOxidDIC()->get('d3ox.webauthn.'.LoggerInterface::class)->error($e->getDetailedErrorMessage(), ['UserId' => $this->getEditObjectId()]); d3GetOxidDIC()->get('d3ox.webauthn.'.LoggerInterface::class)->debug($e->getTraceAsString()); d3GetOxidDIC()->get('d3ox.webauthn.'.UtilsView::class)->addErrorToDisplay($e); - } catch (Exception|NotFoundExceptionInterface|ContainerExceptionInterface|DoctrineDriverException $e) { + } catch (Exception|NotFoundExceptionInterface|ContainerExceptionInterface|DoctrineDriverException|AssertionFailedException $e) { d3GetOxidDIC()->get('d3ox.webauthn.'.LoggerInterface::class)->error($e->getMessage(), ['UserId' => $this->getEditObjectId()]); d3GetOxidDIC()->get('d3ox.webauthn.'.LoggerInterface::class)->debug($e->getTraceAsString()); d3GetOxidDIC()->get('d3ox.webauthn.'.UtilsView::class)->addErrorToDisplay($e->getMessage()); diff --git a/src/Application/Controller/d3_account_webauthn.php b/src/Application/Controller/d3_account_webauthn.php index f8d2d7d..5239669 100755 --- a/src/Application/Controller/d3_account_webauthn.php +++ b/src/Application/Controller/d3_account_webauthn.php @@ -26,6 +26,7 @@ use D3\Webauthn\Application\Model\Webauthn; use Doctrine\DBAL\Driver\Exception as DoctrineDriverException; use Doctrine\DBAL\Exception as DoctrineException; use OxidEsales\Eshop\Application\Controller\AccountController; +use OxidEsales\Eshop\Core\Language; use OxidEsales\Eshop\Core\Registry; use OxidEsales\Eshop\Core\Request; use OxidEsales\Eshop\Core\SeoEncoder; @@ -118,7 +119,6 @@ class d3_account_webauthn extends AccountController * @return void * @throws DoctrineDriverException * @throws DoctrineException - * @throws AssertionFailedException * @throws Throwable */ public function saveAuthn(): void @@ -140,7 +140,23 @@ class d3_account_webauthn extends AccountController $webauthn->saveAuthn($credential, d3GetOxidDIC()->get('d3ox.webauthn.'.Request::class)->getRequestEscapedParameter('keyname')); } } catch (WebauthnException $e) { + d3GetOxidDIC()->get('d3ox.webauthn.'.LoggerInterface::class)->error( + $e->getDetailedErrorMessage(), + ['UserId' => $this->getUser()->getId()] + ); + d3GetOxidDIC()->get('d3ox.webauthn.'.LoggerInterface::class)->debug($e->getTraceAsString()); d3GetOxidDIC()->get('d3ox.webauthn.'.UtilsView::class)->addErrorToDisplay($e); + } catch (AssertionFailedException $e) { + /** @var Language $language */ + $language = d3GetOxidDIC()->get('d3ox.webauthn.'.Language::class); + d3GetOxidDIC()->get('d3ox.webauthn.'.LoggerInterface::class)->error( + $e->getMessage(), + ['UserId' => $this->getUser()->getId()] + ); + d3GetOxidDIC()->get('d3ox.webauthn.'.LoggerInterface::class)->debug($e->getTraceAsString()); + d3GetOxidDIC()->get('d3ox.webauthn.'.UtilsView::class)->addErrorToDisplay( + $language->translateString('D3_WEBAUTHN_ERR_NOTCREDENTIALNOTSAVEABLE') + ); } } diff --git a/src/Application/Model/Credential/PublicKeyCredential.php b/src/Application/Model/Credential/PublicKeyCredential.php index 69eed8d..a3812d6 100755 --- a/src/Application/Model/Credential/PublicKeyCredential.php +++ b/src/Application/Model/Credential/PublicKeyCredential.php @@ -15,7 +15,10 @@ declare(strict_types=1); namespace D3\Webauthn\Application\Model\Credential; +use Assert\Assert; +use Assert\AssertionFailedException; use D3\TestingTools\Production\IsMockable; +use D3\Webauthn\Setup\Actions; use DateTime; use Doctrine\DBAL\Driver\Exception as DoctrineDriverException; use Doctrine\DBAL\Exception as DoctrineException; @@ -63,11 +66,20 @@ class PublicKeyCredential extends BaseModel /** * @param string $credentialId + * @throws AssertionFailedException */ public function setCredentialId(string $credentialId): void { + $encodedCID = base64_encode($credentialId); + + Assert::that($encodedCID) + ->maxLength( + Actions::FIELDLENGTH_CREDID, + 'the credentialId (%3$d) does not fit into the database field (%2$d)' + ); + $this->assign([ - 'credentialid' => base64_encode($credentialId), + 'credentialid' => $encodedCID, ]); } @@ -99,11 +111,20 @@ class PublicKeyCredential extends BaseModel /** * @param PublicKeyCredentialSource $credential + * @throws AssertionFailedException */ public function setCredential(PublicKeyCredentialSource $credential): void { + $encodedCredential = base64_encode(serialize($credential)); + + Assert::that($encodedCredential) + ->maxLength( + Actions::FIELDLENGTH_CREDENTIAL, + 'the credential source (%3$d) does not fit into the database field (%2$d)', + ); + $this->assign([ - 'credential' => base64_encode(serialize($credential)), + 'credential' => $encodedCredential, ]); } @@ -131,6 +152,7 @@ class PublicKeyCredential extends BaseModel * @throws DoctrineException * @throws NotFoundExceptionInterface * @throws Exception + * @throws AssertionFailedException */ public function saveCredentialSource(PublicKeyCredentialSource $publicKeyCredentialSource, string $keyName = null): void { diff --git a/src/Application/Model/Credential/PublicKeyCredentialList.php b/src/Application/Model/Credential/PublicKeyCredentialList.php index 02a996a..9d8d509 100755 --- a/src/Application/Model/Credential/PublicKeyCredentialList.php +++ b/src/Application/Model/Credential/PublicKeyCredentialList.php @@ -15,6 +15,7 @@ declare(strict_types=1); namespace D3\Webauthn\Application\Model\Credential; +use Assert\AssertionFailedException; use D3\TestingTools\Production\IsMockable; use Doctrine\DBAL\Driver\Exception as DoctrineDriverException; use Doctrine\DBAL\Exception as DoctrineException; @@ -158,8 +159,10 @@ class PublicKeyCredentialList extends ListModel implements PublicKeyCredentialSo } /** + * unused, but must fulfil the interface * @param PublicKeyCredentialSource $publicKeyCredentialSource * @return void + * @throws AssertionFailedException */ public function saveCredentialSource(PublicKeyCredentialSource $publicKeyCredentialSource): void { diff --git a/src/Application/translations/de/d3webauthn_lang.php b/src/Application/translations/de/d3webauthn_lang.php index 018e453..40831f8 100755 --- a/src/Application/translations/de/d3webauthn_lang.php +++ b/src/Application/translations/de/d3webauthn_lang.php @@ -47,4 +47,5 @@ $aLang = [ 'D3_WEBAUTHN_ERR_UNSECURECONNECTION' => 'Die Verwendung von Sicherheitsschlüsseln ist nur bei lokalen oder gesicherten Verbindungen (https) möglich.', 'D3_WEBAUTHN_ERR_LOGINPROHIBITED' => 'Die Anmeldung mit Sicherheitsschlüssel ist aus technischen Gründen derzeit leider nicht möglich. Bitte verwenden Sie statt dessen Ihr Passwort.', 'D3_WEBAUTHN_ERR_NOTLOADEDUSER' => "Kann keine Anmeldedaten von nicht geladenem Kundenkonto beziehen.", + 'D3_WEBAUTHN_ERR_NOTCREDENTIALNOTSAVEABLE' => "Der Schlüssel kann aus technischen Gründen nicht registriert werden. Bitte wenden Sie sich an den Shopbetreiber.", ]; diff --git a/src/Setup/Actions.php b/src/Setup/Actions.php index 0a1143b..fd6efd7 100644 --- a/src/Setup/Actions.php +++ b/src/Setup/Actions.php @@ -39,6 +39,9 @@ use Psr\Log\LoggerInterface; class Actions { + public const FIELDLENGTH_CREDID = 512; + public const FIELDLENGTH_CREDENTIAL = 2000; + use IsMockable; public $seo_de = 'sicherheitsschluessel'; @@ -56,8 +59,8 @@ class Actions `OXUSERID` char(32) NOT NULL, `OXSHOPID` int(11) NOT NULL, `NAME` varchar(100) NOT NULL, - `CREDENTIALID` varchar(512) NOT NULL, - `CREDENTIAL` varchar(2000) NOT NULL, + `CREDENTIALID` varchar(".self::FIELDLENGTH_CREDID.") NOT NULL, + `CREDENTIAL` varchar(".self::FIELDLENGTH_CREDENTIAL.") NOT NULL, `OXTIMESTAMP` timestamp NOT NULL DEFAULT current_timestamp() ON UPDATE current_timestamp(), PRIMARY KEY (`OXID`), KEY `CREDENTIALID_IDX` (`CREDENTIALID`), diff --git a/src/tests/unit/Application/Controller/d3_account_webauthnTest.php b/src/tests/unit/Application/Controller/d3_account_webauthnTest.php index 95a61bd..3720be8 100644 --- a/src/tests/unit/Application/Controller/d3_account_webauthnTest.php +++ b/src/tests/unit/Application/Controller/d3_account_webauthnTest.php @@ -15,6 +15,7 @@ declare(strict_types=1); namespace D3\Webauthn\tests\unit\Application\Controller; +use Assert\InvalidArgumentException; use D3\TestingTools\Development\CanAccessRestricted; use D3\Webauthn\Application\Controller\d3_account_webauthn; use D3\Webauthn\Application\Model\Credential\PublicKeyCredential; @@ -22,6 +23,7 @@ use D3\Webauthn\Application\Model\Credential\PublicKeyCredentialList; use D3\Webauthn\Application\Model\Exceptions\WebauthnException; use D3\Webauthn\Application\Model\Webauthn; use D3\Webauthn\tests\unit\WAUnitTestCase; +use Generator; use OxidEsales\Eshop\Application\Model\User; use OxidEsales\Eshop\Core\Registry; use OxidEsales\Eshop\Core\Request; @@ -287,14 +289,12 @@ class d3_account_webauthnTest extends WAUnitTestCase } /** - * @return array + * @return Generator */ - public function canSetAuthnRegisterDataProvider(): array + public function canSetAuthnRegisterDataProvider(): Generator { - return [ - 'dont throw exception' => [false], - 'throw exception' => [true], - ]; + yield 'dont throw exception' => [false]; + yield 'throw exception' => [true]; } /** @@ -333,6 +333,19 @@ class d3_account_webauthnTest extends WAUnitTestCase { $_POST['error'] = 'msg'; + /** @var LoggerInterface|MockObject $loggerMock */ + $loggerMock = $this->getMockForAbstractClass(LoggerInterface::class, [], '', true, true, true, ['error', 'debug']); + $loggerMock->expects($this->once())->method('error')->willReturn(true); + $loggerMock->expects($this->once())->method('debug')->willReturn(true); + d3GetOxidDIC()->set('d3ox.webauthn.'.LoggerInterface::class, $loggerMock); + + /** @var User|MockObject $userMock */ + $userMock = $this->getMockBuilder(User::class) + ->disableOriginalConstructor() + ->onlyMethods(['getId']) + ->getMock(); + $userMock->method('getId')->willReturn('userId'); + /** @var UtilsView|MockObject $utilsViewMock */ $utilsViewMock = $this->getMockBuilder(UtilsView::class) ->onlyMethods(['addErrorToDisplay']) @@ -340,12 +353,6 @@ class d3_account_webauthnTest extends WAUnitTestCase $utilsViewMock->expects($this->atLeastOnce())->method('addErrorToDisplay'); d3GetOxidDIC()->set('d3ox.webauthn.'.UtilsView::class, $utilsViewMock); - /** @var LoggerInterface|MockObject $loggerMock */ - $loggerMock = $this->getMockForAbstractClass(LoggerInterface::class, [], '', true, true, true, ['error', 'debug']); - $loggerMock->expects($this->never())->method('error')->willReturn(true); - $loggerMock->expects($this->never())->method('debug')->willReturn(true); - d3GetOxidDIC()->set('d3ox.webauthn.'.LoggerInterface::class, $loggerMock); - /** @var Request|MockObject $requestMock */ $requestMock = $this->getMockBuilder(Request::class) ->onlyMethods(['getRequestEscapedParameter']) @@ -355,8 +362,11 @@ class d3_account_webauthnTest extends WAUnitTestCase )->willReturn('errorMsg'); d3GetOxidDIC()->set('d3ox.webauthn.'.Request::class, $requestMock); - /** @var d3_account_webauthn $oControllerMock */ - $oControllerMock = oxNew(d3_account_webauthn::class); + /** @var d3_account_webauthn|MockObject $oControllerMock */ + $oControllerMock = $this->getMockBuilder(d3_account_webauthn::class) + ->onlyMethods(['getUser']) + ->getMock(); + $oControllerMock->method('getUser')->willReturn($userMock); $this->_oController = $oControllerMock; @@ -406,19 +416,33 @@ class d3_account_webauthnTest extends WAUnitTestCase * @test * @return void * @throws ReflectionException + * @dataProvider canSaveAuthnFailedDataProvider * @covers \D3\Webauthn\Application\Controller\d3_account_webauthn::saveAuthn */ - public function canSaveAuthnFailed() + public function canSaveAuthnFailed($exception) { $_POST['credential'] = 'msg'; $_POST['keyname'] = 'key_name'; + /** @var LoggerInterface|MockObject $loggerMock */ + $loggerMock = $this->getMockForAbstractClass(LoggerInterface::class, [], '', true, true, true, ['error', 'debug']); + $loggerMock->expects($this->once())->method('error')->willReturn(true); + $loggerMock->expects($this->once())->method('debug')->willReturn(true); + d3GetOxidDIC()->set('d3ox.webauthn.'.LoggerInterface::class, $loggerMock); + + /** @var User|MockObject $userMock */ + $userMock = $this->getMockBuilder(User::class) + ->disableOriginalConstructor() + ->onlyMethods(['getId']) + ->getMock(); + $userMock->method('getId')->willReturn('userId'); + /** @var Webauthn|MockObject $webauthnMock */ $webauthnMock = $this->getMockBuilder(Webauthn::class) ->onlyMethods(['saveAuthn']) ->getMock(); $webauthnMock->expects($this->once())->method('saveAuthn') - ->willThrowException(oxNew(WebauthnException::class)); + ->willThrowException($exception); d3GetOxidDIC()->set(Webauthn::class, $webauthnMock); /** @var UtilsView|MockObject $utilsViewMock */ @@ -428,8 +452,11 @@ class d3_account_webauthnTest extends WAUnitTestCase $utilsViewMock->expects($this->atLeastOnce())->method('addErrorToDisplay'); d3GetOxidDIC()->set('d3ox.webauthn.'.UtilsView::class, $utilsViewMock); - /** @var d3_account_webauthn $oControllerMock */ - $oControllerMock = oxNew(d3_account_webauthn::class); + /** @var d3_account_webauthn|MockObject $oControllerMock */ + $oControllerMock = $this->getMockBuilder(d3_account_webauthn::class) + ->onlyMethods(['getUser']) + ->getMock(); + $oControllerMock->method('getUser')->willReturn($userMock); $this->_oController = $oControllerMock; @@ -439,6 +466,15 @@ class d3_account_webauthnTest extends WAUnitTestCase ); } + /** + * @return Generator + */ + public function canSaveAuthnFailedDataProvider(): Generator + { + yield 'WebauthnException' => [oxNew(WebauthnException::class)]; + yield 'AssertionException' => [oxNew(InvalidArgumentException::class, 'msg', 200)]; + } + /** * @test * @throws ReflectionException @@ -467,14 +503,12 @@ class d3_account_webauthnTest extends WAUnitTestCase } /** - * @return array[] + * @return Generator */ - public function canDeleteDataProvider(): array + public function canDeleteDataProvider(): Generator { - return [ - 'has delete id' => ['deleteId', $this->once()], - 'has no delete id' => [null, $this->never()], - ]; + yield 'has delete id' => ['deleteId', $this->once()]; + yield 'has no delete id' => [null, $this->never()]; } /** diff --git a/src/tests/unit/Application/Model/Credential/PublicKeyCredentialTest.php b/src/tests/unit/Application/Model/Credential/PublicKeyCredentialTest.php index a7c4bcf..e7fdde4 100644 --- a/src/tests/unit/Application/Model/Credential/PublicKeyCredentialTest.php +++ b/src/tests/unit/Application/Model/Credential/PublicKeyCredentialTest.php @@ -240,12 +240,14 @@ class PublicKeyCredentialTest extends WAUnitTestCase /** @var PublicKeyCredential|MockObject $sut */ $sut = $this->getMockBuilder(PublicKeyCredential::class) - ->onlyMethods(['exists', 'getIdByCredentialId', 'load', 'save']) + ->onlyMethods(['exists', 'getIdByCredentialId', 'load', 'save', 'setCredential', 'setCredentialId']) ->getMock(); $sut->method('exists')->willReturn($credIdExist); $sut->expects($this->exactly((int) $doSave))->method('getIdByCredentialId'); $sut->expects($this->exactly((int) ($doSave && $credIdExist)))->method('load'); $sut->expects($this->exactly((int) $doSave))->method('save'); + $sut->expects($this->exactly((int) $doSave))->method('setCredential'); + $sut->expects($this->exactly((int) $doSave))->method('setCredentialId'); $this->callMethod( $sut,