pkp/pkp-lib#9895 app key and encryption service provider attached to container #9918

touhidurabir · 2024-04-28T07:47:24Z

touhidurabir · 2024-05-06T06:00:51Z

classes/core/PKPEncryptionServiceProvider.php

+        if (!PKPAppKey::hasKey()) {
+            return;
+        }


This is to allow the application to boot and run if app_key not set and we don't want to make it a must requirement for app to run . We can remove it if we decide to it make a must .

Because we're adding the app key automatically both on install and upgrade, I see no downsides to making the app key a requirement right now. It'll become necessary later as we adopt more of Laravel, so we may as well add the requirement now, since it's low burden to the admin. It'll keep our code simpler.

touhidurabir · 2024-05-06T06:07:28Z

classes/install/Installer.php

+        if (!PKPAppKey::hasKeyVariable()) {
+            error_log("No key variable named `app_key` defined in the `general` section of config file. Please update the config file's general section and add line `app_key = `");
+            return true;
+        }


If app_key variable not defined, we are not halting the process as not making the app key a must but if we decide to make it a must, we can set error and return false to halt the process .

touhidurabir · 2024-05-06T06:07:56Z

classes/install/Installer.php

+        try {
+            PKPAppKey::writeToConfig(PKPAppKey::generate());
+        } catch (Throwable $exception) {
+            error_log($exception->getMessage());
+        } finally {
+            return true;
+        }
+    }


If app_key writing to config file we are not halting the process as not making the app key a must but if we decide to make it a must, we can set error and return false to halt the process .

touhidurabir · 2024-05-06T06:11:37Z

classes/cliTool/UpgradeTool.php

+    public function checkForAppKey(): void
+    {
+        // if the app key variable `app_key` set in the config, nothing to do
+        if (PKPAppKey::hasKeyVariable()) {
+            return;
+        }
+
+        printf("\n\e[;43mWARNING: It is noticed that there is not `app_key` variable defined in the `general` section of the config file which is necessary to cookie and other encryption purpose.\nWe suggest add the following line in the `general` section of config file.\e[0m\n\n");
+
+        printf("\e[;44mapp_key = \e[0m\n");
+    }


on upgrade check if

app_key variable not defined in the config file, just returning but we can add another warning here .

if app key not set, showing a warning .

if we decided to make the app key a must, we can turn it into error/exception .

asmecher

I've suggested a change in the OJS part of this that'll affect this PR too; otherwise looks good!

asmecher

Some suggestions for change! The biggest shift from the last review is that I think we should pick a side on adding the app key vs. keeping it optional. We're already adding it on upgrade and new install, so I think we should make it mandatory and reduce the redundant checks and messages.

asmecher · 2024-05-30T17:53:07Z

classes/cliTool/UpgradeTool.php

@@ -75,6 +76,7 @@ public function execute()
    public function check()
    {
        $this->checkVersion(VersionCheck::getLatestVersion());
+        $this->checkForAppKey();


If someone runs php tools/upgrade.php check before running the upgrade process (it's a normal practice), it'll seem to them that they need to manually generate and add the app key, when the upgrade process (https://github.com/pkp/pkp-lib/pull/9918/files#diff-8ae64e2e6552e29dc508801b292812f26856a2d5e80147614e8b39d48e29d906) should take care of it automatically during upgrade. If we're going to add it during upgrade and installation, I think we should trust that process (and make it robust) rather than adding the extra checks here.

asmecher · 2024-05-30T17:55:24Z

classes/cliTool/CommandInterface.php

+ *
+ * @trait CommandInterface
+ *
+ * @ingroup tools


Now that we're using namespaces, we shouldn't need @ingroup / @group anymore -- and as we're moving from Doxygen to phpDocumentor, where @ingroup / @group` aren't supported, we'll need to remove them anyhow. (Here and elsewhere)

asmecher · 2024-05-30T17:57:52Z

classes/core/PKPAppKey.php

+/**
+ * @file classes/core/PKPAppKey.php
+ *
+ * Copyright (c) 2014-2024 Simon Fraser University


asmecher · 2024-05-30T18:04:17Z

classes/core/PKPAppKey.php

+    /**
+     * Get the list of supported ciphers
+     */
+    public static function getSupportedCiphers(): array


I don't think the getter function adds value; in general I'd like to decrease our use of getters/setters (and eventually property hooks will remove more justification for these). I don't have a strong opinion about static variables vs. constants, but we do have an existing pattern for public const MY_CONST_NAME = abc in classes, and the UPPER_SNAKE_CASE convention does make the invariability clear.

asmecher · 2024-05-30T18:04:41Z

classes/core/PKPAppKey.php

+     */
+    public static function hasKey(): bool
+    {
+        return !empty(Config::getVar('general', 'app_key', ''));


You can leave the 3rd parameter blank if you don't want to specify a default; also below

(minor, but still needs attention)

asmecher · 2024-05-30T18:15:34Z

classes/core/PKPEncryptionServiceProvider.php

+        if (!PKPAppKey::hasKey()) {
+            return;
+        }


Because we're adding the app key automatically both on install and upgrade, I see no downsides to making the app key a requirement right now. It'll become necessary later as we adopt more of Laravel, so we may as well add the requirement now, since it's low burden to the admin. It'll keep our code simpler.

asmecher · 2024-05-30T18:17:19Z

classes/migration/upgrade/v3_5_0/I9895_AddAppKeyToConfigFile.php

+
+        // will set an error if app key variable not set
+        // but will not halt the process
+        if (!PKPAppKey::hasKeyVariable()) {


I think this should add the variable if it's not present; there's no benefit to making the admin go through it manually.

asmecher · 2024-05-30T18:18:01Z

classes/migration/upgrade/v3_5_0/I9895_AddAppKeyToConfigFile.php

+     */
+    public function down(): void
+    {
+        throw new DowngradeNotSupportedException();


I suggest we leave the down() function empty rather than throwing an exception. There's no harm in leaving the variable in the config file, and if the upgrade is run again, it will preserve the existing value.

asmecher · 2024-05-30T18:18:48Z

classes/migration/upgrade/v3_5_0/I9895_AddAppKeyToConfigFile.php

+    }
+
+    /**
+     * Reverse the downgrades


Reverses the migration

asmecher · 2024-05-30T18:21:00Z

classes/core/PKPAppKey.php

+
+        if (!static::hasKeyVariable()) {
+            // Error if the config key `app_key` not defined under `general` section
+            throw new Exception('Config variable named `app_key` not defined in the `general` section');


I think this is redundant with https://github.com/pkp/pkp-lib/pull/9918/files#diff-8ae64e2e6552e29dc508801b292812f26856a2d5e80147614e8b39d48e29d906R36 (I have a suggestion on that line already).

I Think we can keep this one as these are 2 different method (one to set/write the key variable and one to set/write the key itself) which can work independent of each other .

touhidurabir · 2024-06-14T09:27:42Z

@asmecher all tests are passing . Ready for a final look and if all ok, good to merge .

asmecher · 2024-06-29T01:07:19Z

classes/config/ConfigParser.php

@@ -3,30 +3,23 @@
 /**
 * @file classes/config/ConfigParser.php
 *
- * Copyright (c) 2014-2021 Simon Fraser University
- * Copyright (c) 2000-2021 John Willinsky
+ * Copyright (c) 2024 Simon Fraser University


Not a big deal, but don't remove old dates when they're present -- just update the latest date if it's not current.

asmecher · 2024-06-29T01:10:30Z

classes/core/PKPAppKey.php

+     *
+     * @throws \Exception
+     */
+    public static function writeAppKeyVaribaleToConfig(): bool


Varibale is a typo: should be Variable. (Here and elsewhere.)

…command line or upgrade

touhidurabir mentioned this pull request Apr 28, 2024

Introduce APP KEY feature of Laravel #9895

Closed

touhidurabir force-pushed the i9895_main branch from 1a6a116 to 635822e Compare May 3, 2024 13:38

touhidurabir marked this pull request as ready for review May 3, 2024 13:40

touhidurabir commented May 6, 2024

View reviewed changes

asmecher requested changes May 23, 2024

View reviewed changes

touhidurabir force-pushed the i9895_main branch from 646455a to eaebaec Compare May 28, 2024 19:43

asmecher requested changes May 30, 2024

View reviewed changes

touhidurabir force-pushed the i9895_main branch 2 times, most recently from 3cbf7f3 to af71ccd Compare June 13, 2024 11:43

asmecher requested changes Jun 29, 2024

View reviewed changes

touhidurabir added 10 commits July 1, 2024 00:02

pkp#9895 app key and entryption service provider attached to container

8b7c296

pkp#9895 app key manager and command added

1d48302

pkp#9895 added app key check and add in upgrade process

ffdfbe4

pkp#9895 added app key as sensitive config data

35b20a7

pkp#9895 moved app key setup on upgrade as part of migration

4b623ba

pkp#9895 made App KEY mandatory and added feature to add variable at …

f1d8be7

…command line or upgrade

pkp#9895 fixed app key migration

fe9b9aa

pkp#9895 updated app key config retrieval

23c1b05

pkp#9895 fixed app key config retrieval issue

eb612b6

pkp#9895 typo fixed

4d85c0f

touhidurabir force-pushed the i9895_main branch from 9258892 to 4d85c0f Compare June 30, 2024 18:02

touhidurabir merged commit 203067f into pkp:main Jul 1, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkp/pkp-lib#9895 app key and encryption service provider attached to container #9918

pkp/pkp-lib#9895 app key and encryption service provider attached to container #9918

touhidurabir commented Apr 28, 2024

touhidurabir May 6, 2024

asmecher May 30, 2024

touhidurabir May 6, 2024

touhidurabir May 6, 2024

touhidurabir May 6, 2024

asmecher left a comment

asmecher left a comment

asmecher May 30, 2024

asmecher May 30, 2024

asmecher May 30, 2024

asmecher May 30, 2024

asmecher May 30, 2024

asmecher Jun 12, 2024

asmecher May 30, 2024

asmecher May 30, 2024

asmecher May 30, 2024

asmecher May 30, 2024

asmecher May 30, 2024

touhidurabir Jun 7, 2024

touhidurabir commented Jun 14, 2024

asmecher Jun 29, 2024

asmecher Jun 29, 2024

pkp/pkp-lib#9895 app key and encryption service provider attached to container #9918

pkp/pkp-lib#9895 app key and encryption service provider attached to container #9918

Conversation

touhidurabir commented Apr 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmecher left a comment

Choose a reason for hiding this comment

asmecher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

touhidurabir commented Jun 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment