feat: hash api key in path and cleanup scoped account settings (#967)

* feat: store account settings without plain api key

INT-1811

- hashes api key in path used to store (imported) myparcel account settings
- cleans up orphaned account settings rows
- also fixes the automatic import of account settings after changing an api key

* fix: repair get time config

* refactor: have scope coordinates more available

Also trims inline documentation comments a bit

* fix: remove some unneeded checks

* fix: improve comments and add success log

* fix: never overwrite a fresh row with legacy during migrate

* feat: make the fingerprinting a migration

* doc: add docblocks to public methods

Author: joerivanveen

Controller/Adminhtml/Settings/CarrierConfigurationImport.php

@@ -9,13 +9,10 @@
 use Magento\Framework\App\Cache\Frontend\Pool;
 use Magento\Framework\App\Cache\TypeListInterface;
 use Magento\Framework\App\Config\ScopeConfigInterface;
-use Magento\Framework\App\Config\Storage\WriterInterface;
 use Magento\Framework\Controller\Result\JsonFactory;
+use MyParcelNL\Magento\Service\AccountSettings\Importer;
+use MyParcelNL\Magento\Service\AccountSettings\Maintenance as AccountSettingsMaintenance;
 use MyParcelNL\Magento\Service\Config;
-use MyParcelNL\Sdk\Model\Account\CarrierOptions;
-use MyParcelNL\Sdk\Services\Web\AccountWebService;
-use MyParcelNL\Sdk\Services\Web\CarrierOptionsWebService;
-use MyParcelNL\Sdk\Support\Collection;
 
 class CarrierConfigurationImport extends Action
 {
@@ -25,38 +22,41 @@ class CarrierConfigurationImport extends Action
     /**
      * @var mixed
      */
-    private                 $typeListInterface;
-    private WriterInterface $configWriter;
+    private                            $typeListInterface;
+    private Importer                   $importer;
+    private AccountSettingsMaintenance $accountSettingsMaintenance;
 
     /**
-     * @param \Magento\Framework\Controller\Result\JsonFactory   $resultFactory
-     * @param \Magento\Backend\App\Action\Context                $context
-     * @param \Magento\Framework\Model\ResourceModel\Db\Context  $dbContext
-     * @param \Magento\Framework\App\Cache\TypeListInterface     $typeListInterface
-     * @param \Magento\Framework\App\Config\ScopeConfigInterface $config
-     * @param \Magento\Framework\App\Cache\Frontend\Pool         $pool
+     * @param Context                    $context
+     * @param ScopeConfigInterface       $config
+     * @param JsonFactory                $resultFactory
+     * @param TypeListInterface          $typeListInterface
+     * @param Pool                       $pool
+     * @param Importer                   $importer
+     * @param AccountSettingsMaintenance $accountSettingsMaintenance
      */
     public function __construct(
-        Context              $context,
-        WriterInterface      $configWriter,
-        ScopeConfigInterface $config,
-        JsonFactory          $resultFactory,
-        TypeListInterface    $typeListInterface,
-        Pool                 $pool
+        Context                    $context,
+        ScopeConfigInterface       $config,
+        JsonFactory                $resultFactory,
+        TypeListInterface          $typeListInterface,
+        Pool                       $pool,
+        Importer                   $importer,
+        AccountSettingsMaintenance $accountSettingsMaintenance
     )
     {
         parent::__construct($context);
         $params  = $this->_request->getParams();
         $scope   = $params['scope'] ?? ScopeConfigInterface::SCOPE_TYPE_DEFAULT;
         $scopeId = $params['scopeId'] ?? 0;
 
-        // Let’s save the carrier configuration settings per api key, so it can be retrieved per api key as well.
-        $this->apiKey = $config->getValue(Config::XML_PATH_GENERAL . 'api/key', $scope, $scopeId);
+        $this->apiKey = $config->getValue(Config::XML_PATH_API_KEY, $scope, $scopeId);
 
-        $this->configWriter      = $configWriter;
-        $this->resultFactory     = $resultFactory;
-        $this->typeListInterface = $typeListInterface;
-        $this->pool              = $pool;
+        $this->resultFactory              = $resultFactory;
+        $this->typeListInterface          = $typeListInterface;
+        $this->pool                       = $pool;
+        $this->importer                   = $importer;
+        $this->accountSettingsMaintenance = $accountSettingsMaintenance;
     }
 
     /**
@@ -66,15 +66,11 @@ public function __construct(
      */
     public function execute()
     {
-        $configuration = $this->fetchConfigurations();
-        $this->configWriter->save(
-            Config::XML_PATH_GENERAL . "account_settings_$this->apiKey",
-            json_encode($this->createArray($configuration))
-        );
+        $this->importer->importFor($this->apiKey);
 
-        // Clear configuration cache right after saving the account settings, so the modal in the carrier specific
-        // configuration view will be showing the updated drop-off point.
         $this->clearCache();
+        // After the flush, because it reads config.
+        $this->accountSettingsMaintenance->reconcile();
 
         return $this->resultFactory->create()
                                    ->setData(
@@ -86,33 +82,6 @@ public function execute()
         ;
     }
 
-    /**
-     * @return \MyParcelNL\Sdk\Support\Collection
-     * @throws \MyParcelNL\Sdk\Exception\AccountNotActiveException
-     * @throws \MyParcelNL\Sdk\Exception\ApiException
-     * @throws \MyParcelNL\Sdk\Exception\MissingFieldException
-     */
-    public function fetchConfigurations(): Collection
-    {
-        $accountService = (new AccountWebService())->setApiKey($this->apiKey);
-
-        $account                     = $accountService->getAccount();
-        $shop                        = $account->getShops()
-                                               ->first()
-        ;
-        $shopId                      = $shop->getId();
-        $optionConfigurationService  = (new CarrierOptionsWebService())->setApiKey($this->apiKey);
-        $optionConfiguration         = $optionConfigurationService->getCarrierOptions($shopId);
-
-        return new Collection(
-            [
-                'shop'                   => $shop,
-                'account'                => $account,
-                'carrier_options'        => $optionConfiguration,
-            ]
-        );
-    }
-
     private function clearCache(): void
     {
         $cacheFrontendPool = $this->pool;
@@ -124,42 +93,4 @@ private function clearCache(): void
             ;
         }
     }
-
-    /**
-     * @param \MyParcelNL\Sdk\Support\Collection $settings
-     *
-     * @return array
-     * @TODO sdk#326 remove this entire function and replace with toArray
-     */
-    private function createArray(Collection $settings): array
-    {
-        /** @var \MyParcelNL\Sdk\Model\Account\Shop $shop */
-        $shop = $settings->get('shop');
-        /** @var \MyParcelNL\Sdk\Model\Account\Account $account */
-        $account = $settings->get('account');
-        /** @var \MyParcelNL\Sdk\Model\Account\CarrierOptions[]|Collection $carrierOptions */
-        $carrierOptions = $settings->get('carrier_options');
-
-        return [
-            'shop'            => [
-                'id'   => $shop->getId(),
-                'name' => $shop->getName(),
-            ],
-            'account'         => $account->toArray(),
-            'carrier_options' => array_map(static function (CarrierOptions $carrierOptions) {
-                $carrier = $carrierOptions->getCarrier();
-                return [
-                    'carrier'  => [
-                        'human' => $carrier->getHuman(),
-                        'id'    => $carrier->getId(),
-                        'name'  => $carrier->getName(),
-                    ],
-                    'enabled'  => $carrierOptions->isEnabled(),
-                    'label'    => $carrierOptions->getLabel(),
-                    'optional' => $carrierOptions->isOptional(),
-                    'type'     => $carrierOptions->getType(),
-                ];
-            }, $carrierOptions->all()),
-        ];
-    }
 }

Tests/Helpers/AccountSettingsHelpers.php

@@ -0,0 +1,24 @@
+<?php
+
+declare(strict_types=1);
+
+use MyParcelNL\Magento\Service\Config;
+use MyParcelNL\Magento\Service\Hash\Fingerprint;
+
+/**
+ * The config path an api key's account settings belong at. Uses the real Fingerprint, so a test
+ * expressing an expectation cannot drift from the algorithm production code uses.
+ */
+function settingsPathFor(string $apiKey): string
+{
+    return Config::XML_PATH_ACCOUNT_SETTINGS . (new Fingerprint())->of($apiKey);
+}
+
+/**
+ * The pre-fingerprint path, where the plaintext api key sat in the path itself. Only the rewrite
+ * patch may still encounter one.
+ */
+function legacySettingsPathFor(string $apiKey): string
+{
+    return Config::XML_PATH_ACCOUNT_SETTINGS . $apiKey;
+}

Tests/Helpers/MyParcelTokenLifecycleHarness.php

@@ -7,6 +7,7 @@
 use Magento\Config\Model\ResourceModel\Config\Data\Collection as ConfigDataCollection;
 use Magento\Config\Model\ResourceModel\Config\Data\CollectionFactory;
 use Magento\Framework\App\Cache\TypeListInterface;
+use Magento\Framework\App\Config\ScopeConfigInterface;
 use Magento\Framework\App\Config\Storage\WriterInterface;
 use Magento\Framework\App\RequestInterface;
 use Magento\Framework\DataObject;
@@ -61,12 +62,33 @@ public function valueAt(string $scope, int $scopeId): ?string
         return null;
     }
 
+    /**
+     * First row at this path, whatever scope it sits at. Account settings rows are unique by path,
+     * so a caller asserting on one does not have to name the scope it expects.
+     *
+     * @return array{path: string, value: string, scope: string, scope_id: int}|null
+     */
+    public function rowAt(string $path): ?array
+    {
+        foreach ($this->rows as $row) {
+            if ($row['path'] === $path) {
+                return $row;
+            }
+        }
+        return null;
+    }
+
     public function writer(): WriterInterface
     {
         $store  = $this;
         $writer = Mockery::mock(WriterInterface::class);
         $writer->shouldReceive('save')->andReturnUsing(
-            function (string $path, string $value, string $scope, int $scopeId) use ($store): void {
+            function (
+                string $path,
+                string $value,
+                string $scope = ScopeConfigInterface::SCOPE_TYPE_DEFAULT,
+                int $scopeId = 0
+            ) use ($store): void {
                 $store->save($path, $value, $scope, $scopeId);
             }
         );
@@ -100,6 +122,8 @@ public function collectionFactory(): CollectionFactory
                         $value = $row[$field] ?? null;
                         if (is_array($cond) && array_key_exists('in', $cond)) {
                             if (! in_array($value, $cond['in'], true)) { $ok = false; break; }
+                        } elseif (is_array($cond) && array_key_exists('like', $cond)) {
+                            if (! self::matchesSqlLike((string) $value, (string) $cond['like'])) { $ok = false; break; }
                         } elseif ($value !== $cond) {
                             $ok = false; break;
                         }
@@ -117,6 +141,17 @@ public function collectionFactory(): CollectionFactory
         return $factory;
     }
 
+    /**
+     * `_` matters as much as `%`: a prefix like `account_settings_` is full of underscores, so MySQL
+     * matches more loosely than the pattern looks, and a double honouring only `%` would hide that.
+     */
+    private static function matchesSqlLike(string $value, string $pattern): bool
+    {
+        $regex = str_replace(['%', '_'], ['.*', '.'], preg_quote($pattern, '/'));
+
+        return 1 === preg_match('/^' . $regex . '$/', $value);
+    }
+
     public function cacheTypeList(): TypeListInterface
     {
         return mockCacheTypeList();

Tests/Unit/Service/AccountSettings/ImporterTest.php

@@ -0,0 +1,58 @@
+<?php
+
+declare(strict_types=1);
+
+use Magento\Framework\App\Config\ScopeConfigInterface;
+use Magento\Framework\App\Config\Storage\WriterInterface;
+use MyParcelNL\Magento\Service\AccountSettings\Importer;
+use MyParcelNL\Magento\Service\Config;
+use MyParcelNL\Magento\Service\Hash\Fingerprint;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Only hasSettingsFor() is covered: the rest of Importer instantiates the SDK web services directly,
+ * so it has no seam to test through.
+ *
+ * @param array<string, string> $rowsByPath
+ */
+function importerFor(array $rowsByPath): Importer
+{
+    $scopeConfig = Mockery::mock(ScopeConfigInterface::class);
+    $scopeConfig->shouldReceive('getValue')->andReturnUsing(
+        static fn (string $path) => $rowsByPath[$path] ?? null
+    );
+
+    return new Importer(
+        Mockery::spy(WriterInterface::class),
+        $scopeConfig,
+        new Fingerprint(),
+        Mockery::spy(LoggerInterface::class)
+    );
+}
+
+function accountSettingsPath(string $apiKey): string
+{
+    return Config::XML_PATH_ACCOUNT_SETTINGS . (new Fingerprint())->of($apiKey);
+}
+
+it('reports settings present when a row exists for the key', function () {
+    $importer = importerFor([accountSettingsPath('live-key') => '{"shop":1}']);
+
+    expect($importer->hasSettingsFor('live-key'))->toBeTrue();
+});
+
+it('reports settings absent when no row exists at all', function () {
+    expect(importerFor([])->hasSettingsFor('live-key'))->toBeFalse();
+});
+
+it('does not mistake another key\'s row for its own', function () {
+    $importer = importerFor([accountSettingsPath('other-key') => '{"shop":9}']);
+
+    expect($importer->hasSettingsFor('live-key'))->toBeFalse();
+});
+
+it('treats an empty stored value as absent', function () {
+    $importer = importerFor([accountSettingsPath('live-key') => '']);
+
+    expect($importer->hasSettingsFor('live-key'))->toBeFalse();
+});