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 <lukas@statuscode.ch>

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('<p>'.implode(', ', $errors).'</p>');
+			foreach($errors as $error) {
+				$this->logger->error($error, ['app' => $this->appName]);
+			}
 		}
 
 		if (!$auth->isAuthenticated()) {
-			echo "<p>Not authenticated</p>";
+			return new Http\RedirectResponse($this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notProvisioned'));
-			exit();
 		}
 
 		// 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());

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;
 	}

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,

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
 		);
 	}
 

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());
+	}
 }