Safer auth settings

appinfo/info.xml

@@ -5,7 +5,7 @@
 	<summary>Integration of Zammad user support/ticketing solution</summary>
 	<description><![CDATA[Zammad integration provides a dashboard widget displaying your important notifications,
 	a search provider for tickets and notifications for new open tickets.]]></description>
-	<version>2.1.0</version>
+	<version>3.0.0</version>
 	<licence>agpl</licence>
 	<author>Julien Veyssier</author>
 	<namespace>Zammad</namespace>

appinfo/routes.php

@@ -10,12 +10,14 @@
  */
 
 return [
-    'routes' => [
-        ['name' => 'config#oauthRedirect', 'url' => '/oauth-redirect', 'verb' => 'GET'],
-        ['name' => 'config#setConfig', 'url' => '/config', 'verb' => 'PUT'],
-        ['name' => 'config#setAdminConfig', 'url' => '/admin-config', 'verb' => 'PUT'],
-        ['name' => 'zammadAPI#getNotifications', 'url' => '/notifications', 'verb' => 'GET'],
-        ['name' => 'zammadAPI#getZammadUrl', 'url' => '/url', 'verb' => 'GET'],
-        ['name' => 'zammadAPI#getZammadAvatar', 'url' => '/avatar/{imageId}', 'verb' => 'GET'],
-    ]
+	'routes' => [
+		['name' => 'config#oauthRedirect', 'url' => '/oauth-redirect', 'verb' => 'GET'],
+		['name' => 'config#setConfig', 'url' => '/config', 'verb' => 'PUT'],
+		['name' => 'config#setSensitiveConfig', 'url' => '/sensitive-config', 'verb' => 'PUT'],
+		['name' => 'config#setAdminConfig', 'url' => '/admin-config', 'verb' => 'PUT'],
+		['name' => 'config#setSensitiveAdminConfig', 'url' => '/sensitive-admin-config', 'verb' => 'PUT'],
+		['name' => 'zammadAPI#getNotifications', 'url' => '/notifications', 'verb' => 'GET'],
+		['name' => 'zammadAPI#getZammadUrl', 'url' => '/url', 'verb' => 'GET'],
+		['name' => 'zammadAPI#getZammadAvatar', 'url' => '/avatar/{imageId}', 'verb' => 'GET'],
+	]
 ];

lib/AppInfo/Application.php

@@ -29,11 +29,6 @@
 use OCP\IUserSession;
 use OCP\Notification\IManager as INotificationManager;
 
-/**
- * Class Application
- *
- * @package OCA\Zammad\AppInfo
- */
 class Application extends App implements IBootstrap {
 
 	public const APP_ID = 'integration_zammad';

lib/BackgroundJob/CheckOpenTickets.php

@@ -29,28 +29,16 @@
 
 use Psr\Log\LoggerInterface;
 
-/**
- * Class CheckOpenTickets
- *
- * @package OCA\Zammad\BackgroundJob
- */
 class CheckOpenTickets extends TimedJob {
 
-	/** @var ZammadAPIService */
-	protected $zammadAPIService;
-
-	/** @var LoggerInterface */
-	protected $logger;
-
-	public function __construct(ITimeFactory $time,
-		ZammadAPIService $zammadAPIService,
-		LoggerInterface $logger) {
+	public function __construct(
+		ITimeFactory $time,
+		private ZammadAPIService $zammadAPIService,
+		private LoggerInterface $logger,
+	) {
 		parent::__construct($time);
 		// Every 15 minutes
 		$this->setInterval(60 * 15);
-
-		$this->zammadAPIService = $zammadAPIService;
-		$this->logger = $logger;
 	}
 
 	protected function run($argument): void {

lib/Controller/ConfigController.php

@@ -16,49 +16,63 @@
 use OCA\Zammad\Reference\ZammadReferenceProvider;
 use OCA\Zammad\Service\ZammadAPIService;
 use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http;
+use OCP\AppFramework\Http\Attribute\NoAdminRequired;
+use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
+use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\IConfig;
 use OCP\IL10N;
-
 use OCP\IRequest;
+
 use OCP\IURLGenerator;
 use OCP\PreConditionNotMetException;
 
 class ConfigController extends Controller {
-	private IConfig $config;
-	private IURLGenerator $urlGenerator;
-	private IL10N $l;
-	private ZammadAPIService $zammadAPIService;
-	private ZammadReferenceProvider $zammadReferenceProvider;
-	private ?string $userId;
-
-	public function __construct(string $appName,
+
+	public function __construct(
+		string $appName,
 		IRequest $request,
-		IConfig $config,
-		IURLGenerator $urlGenerator,
-		IL10N $l,
-		ZammadAPIService $zammadAPIService,
-		ZammadReferenceProvider $zammadReferenceProvider,
-		?string $userId) {
+		private IConfig $config,
+		private IURLGenerator $urlGenerator,
+		private IL10N $l,
+		private ZammadAPIService $zammadAPIService,
+		private ZammadReferenceProvider $zammadReferenceProvider,
+		private ?string $userId,
+	) {
 		parent::__construct($appName, $request);
-		$this->config = $config;
-		$this->urlGenerator = $urlGenerator;
-		$this->l = $l;
-		$this->zammadAPIService = $zammadAPIService;
-		$this->zammadReferenceProvider = $zammadReferenceProvider;
-		$this->userId = $userId;
 	}
 
 	/**
-	 * set config values
-	 * @NoAdminRequired
+	 * Set config values
 	 *
 	 * @param array $values
 	 * @return DataResponse
 	 * @throws PreConditionNotMetException
 	 */
+	#[NoAdminRequired]
 	public function setConfig(array $values): DataResponse {
+		foreach ($values as $key => $value) {
+			if (in_array($key, ['token', 'token_type', 'url', 'oauth_state', 'redirect_uri'], true)) {
+				return new DataResponse([], Http::STATUS_BAD_REQUEST);
+			}
+			$this->config->setUserValue($this->userId, Application::APP_ID, $key, trim($value));
+		}
+
+		return new DataResponse([]);
+	}
+
+	/**
+	 * Set sensitive config values
+	 *
+	 * @param array $values
+	 * @return DataResponse
+	 * @throws PreConditionNotMetException
+	 */
+	#[NoAdminRequired]
+	#[PasswordConfirmationRequired]
+	public function setSensitiveConfig(array $values): DataResponse {
 		foreach ($values as $key => $value) {
 			$this->config->setUserValue($this->userId, Application::APP_ID, $key, trim($value));
 		}
@@ -81,35 +95,52 @@ public function setConfig(array $values): DataResponse {
 			$this->config->deleteUserValue($this->userId, Application::APP_ID, 'token_expires_at');
 		}
 		if (isset($result['error'])) {
-			return new DataResponse($result, 401);
+			return new DataResponse($result, Http::STATUS_UNAUTHORIZED);
 		} else {
 			return new DataResponse($result);
 		}
 	}
 
 	/**
-	 * set admin config values
+	 * Set admin config values
 	 *
 	 * @param array $values
 	 * @return DataResponse
 	 */
 	public function setAdminConfig(array $values): DataResponse {
+		foreach ($values as $key => $value) {
+			if (in_array($key, ['client_id', 'client_secret', 'oauth_instance_url'], true)) {
+				return new DataResponse([], Http::STATUS_BAD_REQUEST);
+			}
+			$this->config->setAppValue(Application::APP_ID, $key, $value);
+		}
+		return new DataResponse([]);
+	}
+
+	/**
+	 * Set sensitive admin config values
+	 *
+	 * @param array $values
+	 * @return DataResponse
+	 */
+	#[PasswordConfirmationRequired]
+	public function setSensitiveAdminConfig(array $values): DataResponse {
 		foreach ($values as $key => $value) {
 			$this->config->setAppValue(Application::APP_ID, $key, $value);
 		}
-		return new DataResponse(1);
+		return new DataResponse([]);
 	}
 
 	/**
-	 * receive oauth code and get oauth access token
-	 * @NoAdminRequired
-	 * @NoCSRFRequired
+	 * Receive oauth code and get oauth access token
 	 *
 	 * @param string $code
 	 * @param string $state
 	 * @return RedirectResponse
 	 * @throws PreConditionNotMetException
 	 */
+	#[NoAdminRequired]
+	#[NoCSRFRequired]
 	public function oauthRedirect(string $code = '', string $state = ''): RedirectResponse {
 		$configState = $this->config->getUserValue($this->userId, Application::APP_ID, 'oauth_state');
 		$clientID = $this->config->getAppValue(Application::APP_ID, 'client_id');
@@ -118,10 +149,11 @@ public function oauthRedirect(string $code = '', string $state = ''): RedirectRe
 		// anyway, reset state
 		$this->config->setUserValue($this->userId, Application::APP_ID, 'oauth_state', '');
 
+		$adminZammadOauthUrl = $this->config->getAppValue(Application::APP_ID, 'oauth_instance_url');
+
 		if ($clientID && $clientSecret && $configState !== '' && $configState === $state) {
 			$redirect_uri = $this->config->getUserValue($this->userId, Application::APP_ID, 'redirect_uri');
-			$zammadUrl = $this->config->getUserValue($this->userId, Application::APP_ID, 'url');
-			$result = $this->zammadAPIService->requestOAuthAccessToken($zammadUrl, [
+			$result = $this->zammadAPIService->requestOAuthAccessToken($adminZammadOauthUrl, [
 				'client_id' => $clientID,
 				'client_secret' => $clientSecret,
 				'code' => $code,
@@ -162,7 +194,8 @@ public function oauthRedirect(string $code = '', string $state = ''): RedirectRe
 	 * @throws PreConditionNotMetException
 	 */
 	private function storeUserInfo(): array {
-		$zammadUrl = $this->config->getUserValue($this->userId, Application::APP_ID, 'url');
+		$adminZammadOauthUrl = $this->config->getAppValue(Application::APP_ID, 'oauth_instance_url');
+		$zammadUrl = $this->config->getUserValue($this->userId, Application::APP_ID, 'url') ?: $adminZammadOauthUrl;
 
 		if (!$zammadUrl || !preg_match('/^(https?:\/\/)?[^.]+\.[^.].*/', $zammadUrl)) {
 			return ['error' => 'Zammad URL is invalid'];

lib/Controller/ZammadAPIController.php

@@ -15,6 +15,8 @@
 use OCA\Zammad\Service\ZammadAPIService;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
+use OCP\AppFramework\Http\Attribute\NoAdminRequired;
+use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
 use OCP\AppFramework\Http\DataDisplayResponse;
 use OCP\AppFramework\Http\DataResponse;
 
@@ -24,74 +26,36 @@
 
 class ZammadAPIController extends Controller {
 
-	/**
-	 * @var ZammadAPIService
-	 */
-	private $zammadAPIService;
-	/**
-	 * @var string|null
-	 */
-	private $userId;
-	/**
-	 * @var string
-	 */
-	private $accessToken;
-	/**
-	 * @var string
-	 */
-	private $tokenType;
-	/**
-	 * @var string
-	 */
-	private $refreshToken;
-	/**
-	 * @var string
-	 */
-	private $clientID;
-	/**
-	 * @var string
-	 */
-	private $clientSecret;
-	/**
-	 * @var string
-	 */
-	private $zammadUrl;
-
-	public function __construct(string $appName,
+	public function __construct(
+		string $appName,
 		IRequest $request,
-		IConfig $config,
-		ZammadAPIService $zammadAPIService,
-		?string $userId) {
+		private IConfig $config,
+		private ZammadAPIService $zammadAPIService,
+		private ?string $userId,
+	) {
 		parent::__construct($appName, $request);
-		$this->zammadAPIService = $zammadAPIService;
-		$this->userId = $userId;
-		$this->accessToken = $config->getUserValue($userId, Application::APP_ID, 'token');
-		$this->tokenType = $config->getUserValue($userId, Application::APP_ID, 'token_type');
-		$this->refreshToken = $config->getUserValue($userId, Application::APP_ID, 'refresh_token');
-		$this->clientID = $config->getAppValue(Application::APP_ID, 'client_id');
-		$this->clientSecret = $config->getAppValue(Application::APP_ID, 'client_secret');
-		$this->zammadUrl = $config->getUserValue($userId, Application::APP_ID, 'url');
 	}
 
 	/**
-	 * get zammad instance URL
-	 * @NoAdminRequired
+	 * Get zammad instance URL
 	 *
 	 * @return DataResponse
 	 */
+	#[NoAdminRequired]
 	public function getZammadUrl(): DataResponse {
-		return new DataResponse($this->zammadUrl);
+		$zammadUrl = $this->config->getUserValue($this->userId, Application::APP_ID, 'url');
+		return new DataResponse($zammadUrl);
 	}
 
 	/**
-	 * get zammad user avatar
-	 * @NoAdminRequired
-	 * @NoCSRFRequired
+	 * Get zammad user avatar
 	 *
 	 * @param string $imageId
 	 * @return DataDisplayResponse
 	 * @throws \Exception
 	 */
+	#[NoAdminRequired]
+	#[NoCSRFRequired]
 	public function getZammadAvatar(string $imageId = ''): DataDisplayResponse {
 		$avatarResponse = $this->zammadAPIService->getZammadAvatar($this->userId, $imageId);
 		if (isset($avatarResponse['error'])) {
@@ -107,15 +71,17 @@ public function getZammadAvatar(string $imageId = ''): DataDisplayResponse {
 	}
 
 	/**
-	 * get notifications list
-	 * @NoAdminRequired
+	 * Get notifications list
 	 *
 	 * @param ?string $since
 	 * @return DataResponse
 	 * @throws PreConditionNotMetException
 	 */
+	#[NoAdminRequired]
 	public function getNotifications(?string $since = null): DataResponse {
-		if ($this->accessToken === '' || !preg_match('/^(https?:\/\/)?[^.]+\.[^.].*/', $this->zammadUrl)) {
+		$accessToken = $this->config->getUserValue($this->userId, Application::APP_ID, 'token');
+		$zammadUrl = $this->config->getUserValue($this->userId, Application::APP_ID, 'url');
+		if ($accessToken === '' || !preg_match('/^(https?:\/\/)?[^.]+\.[^.].*/', $zammadUrl)) {
 			return new DataResponse('', 400);
 		}
 		$result = $this->zammadAPIService->getNotifications($this->userId, $since, 7);