#7366 user api key generation process update

pkp · Aug 25, 2022 · f674691 · f674691
1 parent 57aafa8
commit f674691
Show file tree

Hide file tree

Showing 5 changed files with 158 additions and 44 deletions.
diff --git a/classes/migration/upgrade/v3_4_0/I7366_UpdateUserAPIKeySettings.php b/classes/migration/upgrade/v3_4_0/I7366_UpdateUserAPIKeySettings.php
@@ -0,0 +1,52 @@
+<?php
+
+/**
+ * @file classes/migration/upgrade/v3_4_0/I7366_UpdateUserAPIKeySettings.php
+ *
+ * Copyright (c) 2014-2022 Simon Fraser University
+ * Copyright (c) 2000-2022 John Willinsky
+ * Distributed under the GNU GPL v3. For full terms see the file docs/COPYING.
+ *
+ * @class I7366_UpdateUserAPIKeySettings
+ * @brief Describe upgrade/downgrade for updating user API related settings
+ */
+
+namespace PKP\migration\upgrade\v3_4_0;
+
+use Illuminate\Support\Facades\DB;
+
+class I7366_UpdateUserAPIKeySettings extends \PKP\migration\Migration
+{
+    /**
+     * Run the migrations.
+     */
+    public function up(): void
+    {
+        $users = DB::select(
+            DB::raw(
+                "SELECT u.user_id FROM users u 
+                JOIN user_settings enabled_setting ON (enabled_setting.user_id = u.user_id AND enabled_setting.setting_name = 'apiKeyEnabled')
+                LEFT JOIN user_settings key_setting ON (key_setting.user_id = u.user_id AND key_setting.setting_name = 'apiKey') 
+                WHERE key_setting.user_id IS NULL"
+            )
+        );
+
+        collect($users)
+            ->pluck('user_id')
+            ->chunk(1000)
+            ->each(
+                fn ($ids) => DB::table('user_settings')
+                    ->where('setting_name', 'apiKeyEnabled')
+                    ->whereIn('user_id', $ids->toArray())
+                    ->delete()
+            );
+    }
+
+    /**
+     * Reverse the migration.
+     */
+    public function down(): void
+    {
+
+    }
+}
diff --git a/classes/user/form/APIProfileForm.php b/classes/user/form/APIProfileForm.php
@@ -17,15 +17,17 @@
 
 use APP\core\Application;
 use APP\notification\NotificationManager;
-
 use APP\template\TemplateManager;
 use Firebase\JWT\JWT;
 use PKP\config\Config;
-
 use PKP\notification\PKPNotification;
+use PKP\user\User;
 
 class APIProfileForm extends BaseProfileForm
 {
+    public const API_KEY_NEW = 1;
+    public const API_KEY_DELETE = 0;
+
     /**
      * Constructor.
      *
@@ -53,7 +55,9 @@ public function readInputData()
         parent::readInputData();
 
         $this->readUserVars([
-            'apiKeyEnabled', 'generateApiKey',
+            'apiKeyEnabled', 
+            'generateApiKey',
+            'apiKeyAction',
         ]);
     }
 
@@ -70,21 +74,23 @@ public function fetch($request, $template = null, $display = false)
     {
         $user = $request->getUser();
         $secret = Config::getVar('security', 'api_key_secret', '');
+        $templateMgr = TemplateManager::getManager($request);
+
         if ($secret === '') {
-            $notificationManager = new NotificationManager();
-            $notificationManager->createTrivialNotification(
-                $user->getId(),
-                PKPNotification::NOTIFICATION_TYPE_WARNING,
-                [
-                    'contents' => __('user.apiKey.secretRequired'),
-                ]
-            );
-        } elseif ($user->getData('apiKey')) {
-            $templateMgr = TemplateManager::getManager($request);
-            $templateMgr->assign([
-                'apiKey' => JWT::encode($user->getData('apiKey'), $secret, 'HS256'),
-            ]);
+            $this->handleOnMissingAPISecret($templateMgr, $user);
+            return parent::fetch($request, $template, $display);
         }
+
+        $templateMgr->assign($user->getData('apiKey') ? [
+                'apiKey' => JWT::encode($user->getData('apiKey'), $secret, 'HS256'),
+                'apiKeyAction' => self::API_KEY_DELETE,
+                'apiKeyActionTextKey' => 'user.apiKey.remove',
+            ] : [
+                'apiKeyAction' => self::API_KEY_NEW,
+                'apiKeyActionTextKey' => 'user.apiKey.generate',
+            ]
+        );
+
         return parent::fetch($request, $template, $display);
     }
 
@@ -95,25 +101,45 @@ public function execute(...$functionArgs)
     {
         $request = Application::get()->getRequest();
         $user = $request->getUser();
+        $templateMgr = TemplateManager::getManager($request);
 
-        $apiKeyEnabled = (bool) $this->getData('apiKeyEnabled');
-        $user->setData('apiKeyEnabled', $apiKeyEnabled);
-
-        // remove api key if exists
-        if (!$apiKeyEnabled) {
-            $user->setData('apiKeyEnabled', null);
+        if (Config::getVar('security', 'api_key_secret', '') === '') {
+            $this->handleOnMissingAPISecret($templateMgr, $user);
+            parent::execute(...$functionArgs);
         }
 
-        // generate api key
-        if ($apiKeyEnabled && !is_null($this->getData('generateApiKey'))) {
-            $secret = Config::getVar('security', 'api_key_secret', '');
-            if ($secret) {
-                $user->setData('apiKey', sha1(time()));
-            }
-        }
+        $apiKeyAction = (int)$this->getData('apiKeyAction');
+
+        $user->setData('apiKeyEnabled', $apiKeyAction === self::API_KEY_NEW ? 1 : null);
+        $user->setData('apiKey', $apiKeyAction === self::API_KEY_NEW ? sha1(time()) : null);
+
+        $this->setData('apiKeyAction', (int)!$apiKeyAction);
 
         parent::execute(...$functionArgs);
     }
+
+    /**
+     * Handle on missing API secret
+     * 
+     * @param TemplateManager $templateMgr
+     * @param User $user
+     *
+     * @return void
+     */
+    protected function handleOnMissingAPISecret(TemplateManager $templateMgr, User $user): void
+    {
+        $notificationManager = new NotificationManager();
+        $notificationManager->createTrivialNotification(
+            $user->getId(),
+            PKPNotification::NOTIFICATION_TYPE_WARNING,
+            [
+                'contents' => __('user.apiKey.secretRequired'),
+            ]
+        );
+        $templateMgr->assign([
+            'apiSecretMissing' => true,
+        ]);
+    }
 }
 
 if (!PKP_STRICT_MODE) {

diff --git a/cypress/tests/integration/API.spec.js b/cypress/tests/integration/API.spec.js
@@ -17,6 +17,7 @@ describe('API tests', function() {
 			);
 		});
 	});
+
 	it("Tries an API request without authorization", function() {
 		cy.request({
 			url: 'index.php/publicknowledge/api/v1/users',
@@ -31,18 +32,33 @@ describe('API tests', function() {
 		cy.get('.app__userNav button').click();
 		cy.get('a:contains("Edit Profile")').click();
 		cy.get('a[name="apiSettings"]').click();
-		cy.get('input[id="apiKeyEnabled"]').check();
-		cy.get('input[id="generateApiKey"]').check();
-		cy.get('form[id="apiProfileForm"] button:contains("Save")').click();
+		cy.get('form[id="apiProfileForm"] button:contains("Create API Key")').click();
 		cy.waitJQuery();
 		cy.get('span:contains("Your changes have been saved.")');
 		cy.get('input[id^="apiKey-"]').invoke('val').as('apiKey').then(function() {
 			cy.log(this.apiKey);
 		});
 		cy.logout();
 	});
+
 	it('Tries an API request with an API key', function() {
 		// Ensure that an API request returns a 200 OK code.
 		cy.request('index.php/publicknowledge/api/v1/users?apiToken=' + this.apiKey);
 	});
+
+	it("Deletes a manager's API key", function() {
+		cy.login('dbarnes', null, 'publicknowledge');
+		cy.get('.app__userNav button').click();
+		cy.get('a:contains("Edit Profile")').click();
+		cy.get('a[name="apiSettings"]').click();
+		cy.get('form[id="apiProfileForm"] button:contains("Delete")').click();
+		cy.waitJQuery();
+		cy.on('window:confirm', (text) => {
+			return true;
+		});
+		cy.waitJQuery();
+		cy.get('span:contains("Your changes have been saved.")');
+		cy.get('input[id^="apiKey-"]').invoke('val').should('eq', 'None');
+		cy.logout();
+	});
 })
diff --git a/locale/en_US/user.po b/locale/en_US/user.po
@@ -426,11 +426,20 @@ msgid "user.apiKeyEnabled"
 msgstr "Enable external applications with the API key to access this account"
 
 msgid "user.apiKey.generate"
-msgstr "Generate new API key"
+msgstr "Create API Key"
+
+msgid "user.apiKey.remove"
+msgstr "Delete"
 
 msgid "user.apiKey.generateWarning"
 msgstr "Generating a new API key will invalidate any existing key for this user."
 
+msgid "user.apiKey.removeWarning"
+msgstr "Deleting a key will revoke access to any application that uses it."
+
+msgid "user.apiKey.remove.confirmation.message"
+msgstr "Are you sure you want to delete this API key?"
+
 msgid "user.apiKey.secretRequired"
 msgstr "Before generating an API key, your site administrator must set a secret in the config file (\"api_key_secret\")."
 

diff --git a/templates/user/apiProfileForm.tpl b/templates/user/apiProfileForm.tpl
@@ -23,21 +23,32 @@
 
 	{include file="controllers/notification/inPlaceNotification.tpl" notificationId="apiProfileNotification"}
 
-	{fbvFormSection list=true}
-		{fbvElement id=apiKeyEnabled type="checkbox" label="user.apiKeyEnabled" checked=$apiKeyEnabled value=1}
-		{fbvElement id=generateApiKey type="checkbox" label="user.apiKey.generate" value=1}
-	{/fbvFormSection}
-	<p>{translate key="user.apiKey.generateWarning"}</p>
-
-	{fbvFormSection}
+	{fbvFormSection title="user.apiKey"}
 		{if !$apiKey}{assign var=apiKey value="common.none"|translate}{/if}
-		{fbvElement id=apiKey type="text" label="user.apiKey" readonly="true" value=$apiKey size=$fbvStyles.size.MEDIUM}
+		{fbvElement id=apiKey type="text" readonly="true" inline=true value=$apiKey size=$fbvStyles.size.MEDIUM}
+		{if !$apiSecretMissing}
+			{fbvElement id=apiKeyAction type="hidden" readonly="true" value=$apiKeyAction}
+			<button
+				type="submit"
+				{if $apiKeyAction === \PKP\user\form\APIProfileForm::API_KEY_DELETE}
+					onClick="return confirm({translate|json_encode|escape key='user.apiKey.remove.confirmation.message'})"
+					class="pkpButton pkpButton--isWarnable"
+				{else}
+					class="pkp_button pkp_button_primary"
+				{/if}
+			>
+				{translate key=$apiKeyActionTextKey}
+			</button>
+		{/if}
+		<p>
+			{translate key=($apiKeyAction === \PKP\user\form\APIProfileForm::API_KEY_NEW) ? "user.apiKey.generateWarning" : "user.apiKey.removeWarning"}
+		</p>
 	{/fbvFormSection}
 
 	<p>
-		{capture assign="privacyUrl"}{url router=\PKP\core\PKPApplication::ROUTE_PAGE page="about" op="privacy"}{/capture}
+		{capture assign="privacyUrl"}
+			{url router=\PKP\core\PKPApplication::ROUTE_PAGE page="about" op="privacy"}
+		{/capture}
 		{translate key="user.privacyLink" privacyUrl=$privacyUrl}
 	</p>
-
-	{fbvFormButtons hideCancel=true submitText="common.save"}
 </form>