config command fixes, remove deprecated annotation reflector (#22)

Resolves: #21 Changes: - [x] do not update sensitive flag each time config value updated - [x] remove deprecated and unused `IControllerMethodReflector` in `AppEcosystemAuthMiddleware`
nextcloud · Jul 25, 2023 · fef5e65 · fef5e65
1 parent 27f5187
commit fef5e65
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 81 deletions.
diff --git a/.github/workflows/docs-check.yml b/.github/workflows/docs-check.yml
@@ -1,9 +1,6 @@
 name: Docs check
 on:
   pull_request:
-    paths:
-      - '.github/workflows/docs-check.yml'
-      - 'docs/**'
 
 permissions:
   contents: read

diff --git a/lib/Command/ExAppConfig/SetConfig.php b/lib/Command/ExAppConfig/SetConfig.php
@@ -32,49 +32,53 @@ protected function configure() {
 		$this->addArgument('configkey', InputArgument::REQUIRED);
 
 		$this->addOption('value', null, InputOption::VALUE_REQUIRED);
-		$this->addOption('sensitive', null, InputOption::VALUE_NONE, 'Sensitive config value');
+		$this->addOption('sensitive', null, InputOption::VALUE_OPTIONAL, 'Sensitive config value', null);
 		$this->addOption('update-only', null, InputOption::VALUE_NONE, 'Only update config, if not exists - do not create');
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$appId = $input->getArgument('appid');
 		$exApp = $this->service->getExApp($appId);
 		if ($exApp === null) {
-			$output->writeln('ExApp ' . $appId . ' not found');
+			$output->writeln(sprintf('ExApp %s not found', $appId));
 			return 1;
 		}
 
-		if ($exApp->getEnabled()) {
-			$configKey = $input->getArgument('configkey');
-			$value = $input->getOption('value');
-			$sensitive = $input->getOption('sensitive');
-			$updateOnly = $input->getOption('update-only');
+		$configKey = $input->getArgument('configkey');
+		$value = $input->getOption('value');
+		$isSensitive = $input->getOption('sensitive');
+		$sensitive = (int) filter_var($isSensitive, FILTER_VALIDATE_BOOLEAN);
+		$updateOnly = $input->getOption('update-only');
 
-			if (!$updateOnly) {
-				$exAppConfig = $this->exAppConfigService->setAppConfigValue($appId, $configKey, $value, (int) $sensitive);
-				if ($exAppConfig === null) {
-					$output->writeln('ExApp ' . $appId . ' config ' . $configKey . ' not found');
-					return 1;
-				}
-			} else {
-				$exAppConfig = $this->exAppConfigService->getAppConfig($appId, $configKey);
-				if ($exAppConfig === null) {
-					$output->writeln('ExApp ' . $appId . ' config ' . $configKey . ' not found');
-					return 1;
-				}
-				$exAppConfig->setConfigvalue($value);
-				$exAppConfig->setSensitive((int) $sensitive);
-				if ($this->exAppConfigService->updateAppConfigValue($exAppConfig) !== 1) {
-					$output->writeln('ExApp ' . $appId . ' config ' . $configKey . ' not updated');
-					return 1;
-				}
+		$exAppConfig = $this->exAppConfigService->getAppConfig($appId, $configKey);
+		if (!$updateOnly) {
+			if ($exAppConfig !== null) {
+				$output->writeln(sprintf('ExApp %s config %s already exists. Use --update-only flag.', $appId, $configKey));
+				return 1;
+			}
+
+			$exAppConfig = $this->exAppConfigService->setAppConfigValue($appId, $configKey, $value, $sensitive);
+			if ($exAppConfig === null) {
+				$output->writeln(sprintf('ExApp %s config %s not found', $appId, $configKey));
+				return 1;
+			}
+		} else {
+			if ($exAppConfig === null) {
+				$output->writeln(sprintf('ExApp %s config %s not found', $appId, $configKey));
+				return 1;
+			}
+			$exAppConfig->setConfigvalue($value);
+			if ($isSensitive !== null) {
+				$exAppConfig->setSensitive($sensitive);
+			}
+			if ($this->exAppConfigService->updateAppConfigValue($exAppConfig) === null) {
+				$output->writeln(sprintf('ExApp %s config %s not updated', $appId, $configKey));
+				return 1;
 			}
-			$sensitiveMsg = $sensitive ? '[sensitive]' : '';
-			$output->writeln('ExApp ' . $appId . ' config ' . $configKey . ' set to ' . $value . ' ' . $sensitiveMsg);
-			return 0;
 		}
 
-		$output->writeln('ExApp ' . $appId . ' is disabled');
-		return 1;
+		$sensitiveMsg = $exAppConfig->getSensitive() === 1 ? '[sensitive]' : '';
+		$output->writeln(sprintf('ExApp %s config %s set to %s %s', $appId, $configKey, $value, $sensitiveMsg));
+		return 0;
 	}
 }
diff --git a/lib/Db/ExAppConfigMapper.php b/lib/Db/ExAppConfigMapper.php
@@ -71,21 +71,6 @@ public function findByAppConfigKeys(string $appId, array $configKeys): array {
 		return $this->findEntities($qb);
 	}
 
-	/**
-	 * @throws Exception
-	 */
-	public function updateAppConfigValue(ExAppConfig $appConfigEx): int {
-		$qb = $this->db->getQueryBuilder();
-		return $qb->update($this->tableName)
-			->set('configvalue', $qb->createNamedParameter($appConfigEx->getConfigvalue(), IQueryBuilder::PARAM_STR))
-			->set('sensitive', $qb->createNamedParameter($appConfigEx->getSensitive(), IQueryBuilder::PARAM_INT))
-			->where(
-				$qb->expr()->eq('appid', $qb->createNamedParameter($appConfigEx->getAppid(), IQueryBuilder::PARAM_STR)),
-				$qb->expr()->eq('configkey', $qb->createNamedParameter($appConfigEx->getConfigkey(), IQueryBuilder::PARAM_STR))
-			)
-		->executeStatement();
-	}
-
 	/**
 	 * @throws Exception
 	 */

diff --git a/lib/Middleware/AppEcosystemAuthMiddleware.php b/lib/Middleware/AppEcosystemAuthMiddleware.php
@@ -13,27 +13,23 @@
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\Middleware;
-use OCP\AppFramework\Utility\IControllerMethodReflector;
 use OCP\IL10N;
 use OCP\IRequest;
 use Psr\Log\LoggerInterface;
 use ReflectionMethod;
 
 class AppEcosystemAuthMiddleware extends Middleware {
-	private IControllerMethodReflector $reflector;
 	private AppEcosystemV2Service $service;
 	protected IRequest $request;
 	private IL10N $l;
 	private LoggerInterface $logger;
 
 	public function __construct(
-		IControllerMethodReflector $reflector,
 		AppEcosystemV2Service $service,
 		IRequest $request,
 		IL10N $l,
 		LoggerInterface $logger,
 	) {
-		$this->reflector = $reflector;
 		$this->service = $service;
 		$this->request = $request;
 		$this->l = $l;
@@ -43,7 +39,7 @@ public function __construct(
 	public function beforeController($controller, $methodName) {
 		$reflectionMethod = new ReflectionMethod($controller, $methodName);
 
-		$isAppEcosystemAuth = $this->hasAnnotationOrAttribute($reflectionMethod, 'AppEcosystemAuth', AppEcosystemAuth::class);
+		$isAppEcosystemAuth = !empty($reflectionMethod->getAttributes(AppEcosystemAuth::class));
 
 		if ($isAppEcosystemAuth) {
 			if (!$this->service->validateExAppRequestToNC($this->request)) {
@@ -52,26 +48,6 @@ public function beforeController($controller, $methodName) {
 		}
 	}
 
-	/**
-	 * @template T
-	 *
-	 * @param ReflectionMethod $reflectionMethod
-	 * @param string $annotationName
-	 * @param class-string<T> $attributeClass
-	 * @return boolean
-	 */
-	protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {
-		if (!empty($reflectionMethod->getAttributes($attributeClass))) {
-			return true;
-		}
-
-		if ($this->reflector->hasAnnotation($annotationName)) {
-			return true;
-		}
-
-		return false;
-	}
-
 	/**
 	 * If an AEAuthNotValidException is being caught
 	 *

diff --git a/lib/Service/ExAppConfigService.php b/lib/Service/ExAppConfigService.php
@@ -54,28 +54,30 @@ public function getAppConfigValues(string $appId, array $configKeys): ?array {
 	 * @param string $appId
 	 * @param string $configKey
 	 * @param mixed $configValue
-	 * @param int $sensitive
+	 * @param int|null $sensitive
 	 *
 	 * @return ExAppConfig|null
 	 */
-	public function setAppConfigValue(string $appId, string $configKey, mixed $configValue, int $sensitive = 0): ?ExAppConfig {
+	public function setAppConfigValue(string $appId, string $configKey, mixed $configValue, ?int $sensitive = null): ?ExAppConfig {
 		$appConfigEx = $this->getAppConfig($appId, $configKey);
 		if ($appConfigEx === null) {
 			try {
 				$appConfigEx = $this->mapper->insert(new ExAppConfig([
 					'appid' => $appId,
 					'configkey' => $configKey,
 					'configvalue' => $configValue ?? '',
-					'sensitive' => $sensitive,
+					'sensitive' => $sensitive ?? 0,
 				]));
 			} catch (Exception $e) {
 				$this->logger->error(sprintf('Failed to insert appconfig_ex value. Error: %s', $e->getMessage()), ['exception' => $e]);
 				return null;
 			}
 		} else {
 			$appConfigEx->setConfigvalue($configValue);
-			$appConfigEx->setSensitive($sensitive);
-			if ($this->updateAppConfigValue($appConfigEx) !== 1) {
+			if ($sensitive !== null) {
+				$appConfigEx->setSensitive($sensitive);
+			}
+			if ($this->updateAppConfigValue($appConfigEx) === null) {
 				$this->logger->error(sprintf('Error while updating appconfig_ex %s value.', $configKey));
 				return null;
 			}
@@ -129,11 +131,11 @@ public function getAppConfig(mixed $appId, mixed $configKey): ?ExAppConfig {
 	/**
 	 * @param ExAppConfig $exAppConfig
 	 *
-	 * @return int|null
+	 * @return ExAppConfig|null
 	 */
-	public function updateAppConfigValue(ExAppConfig $exAppConfig): ?int {
+	public function updateAppConfigValue(ExAppConfig $exAppConfig): ?ExAppConfig {
 		try {
-			return $this->mapper->updateAppConfigValue($exAppConfig);
+			return $this->mapper->update($exAppConfig);
 		} catch (Exception) {
 			return null;
 		}