From 29c60c386966bdebc189ebaca7c7884af867e60f Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 3 Feb 2017 12:30:10 +0100 Subject: [PATCH] Add better error handling 1. Enable `debug` mode if debug mode is enabled in config.php 2. Log errors to the log file Also I fixed the unit tests that broke with https://github.com/nextcloud/user_saml/pull/81 Signed-off-by: Lukas Reschke --- lib/Controller/SAMLController.php | 19 +++++++++++++------ lib/Settings/Section.php | 7 ++++++- lib/samlsettings.php | 1 + tests/unit/Controller/SAMLControllerTest.php | 8 ++++++-- tests/unit/Settings/SectionTest.php | 20 ++++++++++++++++++-- 5 files changed, 44 insertions(+), 11 deletions(-) diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php index 21388dc..0645ce5 100644 --- a/lib/Controller/SAMLController.php +++ b/lib/Controller/SAMLController.php @@ -27,6 +27,7 @@ use OCA\User_SAML\UserBackend; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\IConfig; +use OCP\ILogger; use OCP\IRequest; use OCP\ISession; use OCP\IURLGenerator; @@ -48,6 +49,8 @@ class SAMLController extends Controller { private $urlGenerator; /** @var IUserManager */ private $userManager; + /** @var ILogger */ + private $logger; /** * @param string $appName @@ -59,6 +62,7 @@ class SAMLController extends Controller { * @param IConfig $config * @param IURLGenerator $urlGenerator * @param IUserManager $userManager + * @param ILogger $logger */ public function __construct($appName, IRequest $request, @@ -68,7 +72,8 @@ class SAMLController extends Controller { UserBackend $userBackend, IConfig $config, IURLGenerator $urlGenerator, - IUserManager $userManager) { + IUserManager $userManager, + ILogger $logger) { parent::__construct($appName, $request); $this->session = $session; $this->userSession = $userSession; @@ -77,6 +82,7 @@ class SAMLController extends Controller { $this->config = $config; $this->urlGenerator = $urlGenerator; $this->userManager = $userManager; + $this->logger = $logger; } /** @@ -169,6 +175,8 @@ class SAMLController extends Controller { * @NoCSRFRequired * @UseSession * @OnlyUnauthenticatedUsers + * + * @return Http\RedirectResponse|void */ public function assertionConsumerService() { $AuthNRequestID = $this->session->get('user_saml.AuthNRequestID'); @@ -181,14 +189,14 @@ class SAMLController extends Controller { $errors = $auth->getErrors(); - // FIXME: Appframworkize if (!empty($errors)) { - print_r('

'.implode(', ', $errors).'

'); + foreach($errors as $error) { + $this->logger->error($error, ['app' => $this->appName]); + } } if (!$auth->isAuthenticated()) { - echo "

Not authenticated

"; - exit(); + return new Http\RedirectResponse($this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notProvisioned')); } // Check whether the user actually exists, if not redirect to an error page @@ -197,7 +205,6 @@ class SAMLController extends Controller { $this->autoprovisionIfPossible($auth->getAttributes()); } catch (NoUserFoundException $e) { return new Http\RedirectResponse($this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notProvisioned')); - } $this->session->set('user_saml.samlUserData', $auth->getAttributes()); diff --git a/lib/Settings/Section.php b/lib/Settings/Section.php index 7f6c218..5c267d3 100644 --- a/lib/Settings/Section.php +++ b/lib/Settings/Section.php @@ -33,7 +33,12 @@ class Section implements IIconSection { /** @var IURLGenerator */ private $url; - public function __construct(IL10N $l, IURLGenerator $url) { + /** + * @param IL10N $l + * @param IURLGenerator $url + */ + public function __construct(IL10N $l, + IURLGenerator $url) { $this->l = $l; $this->url = $url; } diff --git a/lib/samlsettings.php b/lib/samlsettings.php index 5a0a906..79e0db1 100644 --- a/lib/samlsettings.php +++ b/lib/samlsettings.php @@ -44,6 +44,7 @@ class SAMLSettings { public function getOneLoginSettingsArray() { $settings = [ 'strict' => true, + 'debug' => $this->config->getSystemValue('debug', false), 'security' => [ 'nameIdEncrypted' => ($this->config->getAppValue('user_saml', 'security-nameIdEncrypted', '0') === '1') ? true : false, 'authnRequestsSigned' => ($this->config->getAppValue('user_saml', 'security-authnRequestsSigned', '0') === '1') ? true : false, diff --git a/tests/unit/Controller/SAMLControllerTest.php b/tests/unit/Controller/SAMLControllerTest.php index 6007f64..8bf9a2f 100644 --- a/tests/unit/Controller/SAMLControllerTest.php +++ b/tests/unit/Controller/SAMLControllerTest.php @@ -27,10 +27,10 @@ use OCA\User_SAML\UserBackend; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\IConfig; +use OCP\ILogger; use OCP\IRequest; use OCP\ISession; use OCP\IURLGenerator; -use OCP\IUserBackend; use OCP\IUserManager; use OCP\IUserSession; use Test\TestCase; @@ -52,6 +52,8 @@ class SAMLControllerTest extends TestCase { private $urlGenerator; /** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */ private $userManager; + /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */ + private $logger; /** @var SAMLController */ private $samlController; @@ -66,6 +68,7 @@ class SAMLControllerTest extends TestCase { $this->config = $this->createMock(IConfig::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->userManager = $this->createMock(IUserManager::class); + $this->logger = $this->createMock(ILogger::class); $this->samlController = new SAMLController( 'user_saml', @@ -76,7 +79,8 @@ class SAMLControllerTest extends TestCase { $this->userBackend, $this->config, $this->urlGenerator, - $this->userManager + $this->userManager, + $this->logger ); } diff --git a/tests/unit/Settings/SectionTest.php b/tests/unit/Settings/SectionTest.php index 43946f7..2bb84d8 100644 --- a/tests/unit/Settings/SectionTest.php +++ b/tests/unit/Settings/SectionTest.php @@ -21,16 +21,23 @@ namespace OCA\User_SAML\Tests\Settings; +use OCP\IL10N; +use OCP\IURLGenerator; + class SectionTest extends \Test\TestCase { /** @var \OCA\User_SAML\Settings\Section */ private $section; - /** @var \OCP\IL10N */ + /** @var IL10N|\PHPUnit_Framework_MockObject_MockObject */ private $l10n; + /** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ + private $urlGenerator; public function setUp() { $this->l10n = $this->createMock(\OCP\IL10N::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->section = new \OCA\User_SAML\Settings\Section( - $this->l10n + $this->l10n, + $this->urlGenerator ); return parent::setUp(); @@ -53,4 +60,13 @@ class SectionTest extends \Test\TestCase { public function testGetPriority() { $this->assertSame(75, $this->section->getPriority()); } + + public function testGetIcon() { + $this->urlGenerator + ->expects($this->once()) + ->method('imagePath') + ->with('user_saml', 'app-dark.svg') + ->willReturn('/apps/user_saml/myicon.svg'); + $this->assertSame('/apps/user_saml/myicon.svg', $this->section->getIcon()); + } }