-
Notifications
You must be signed in to change notification settings - Fork 916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pkp/pkp-lib#9895 app key and encryption service integration #4257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small rearrangement, thanks!
config.TEMPLATE.inc.php
Outdated
; cipher = 'aes-256-cbc' | ||
|
||
; Define should the cookie at user's end need to be encrypted | ||
; Enabling/Disbaling will force all user to re-login |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*Disabling
dbscripts/xml/upgrade.xml
Outdated
@@ -13,6 +13,7 @@ | |||
|
|||
<install version="3.5.0.0"> | |||
<code function="checkPhpVersion" /> | |||
<code function="addAppKey" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be added as a migration, not a <code ...>
addition in the Upgrade
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asmecher I understand it should part of upgrade and need to moved to upgrade class but I am confused about the migration part . I does not modify/update any DB tables but update the config.inc.php to add a unique app key if the app_key
variable defined/set in the config.inc.php
file . thats why it's added as <code ...>
to run. or I am misunderstanding something ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The <code ...>
pattern leads to accumulating cruft in the Upgrade
class, so we're using Migration classes instead for new code. Migrations are good for whatever, not just database schema adaptations!
docs/release-notes/README-3.5.0
Outdated
@@ -11,9 +11,15 @@ See config.TEMPLATE.inc.php for a description and examples of all supported | |||
configuration parameters. | |||
|
|||
New config.inc.php parameters added for general: | |||
- session_cookie_enctyption_key (default value: ''), allow cookie encryption when set | |||
- app_key (default value: ''), application specific key will used internally for encryption/decryption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a brief instruction for how to create this for those who are upgrading -- on second thought, since the upgrade script does this automatically, it would be better to note here that it'll be added on upgrade (since we normally don't do that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion for a change (please port to the other apps)!
2a3c298
to
d2926f6
Compare
for pkp/pkp-lib#9895