-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add cookie secret as part of config #1835
Add cookie secret as part of config #1835
Conversation
c4b2c43
to
2adf019
Compare
👍 once tests pass. |
BTW this was deployed in the test environment and it works fine. |
@antgonza I'm confused on the tests passing here - the encoded config file is not changed. I'm assuming that what is happening is that is just using 'None' as the COOKIE_SECRET. What should we do when the COOKIE_SECRET is not provided in the config file? Should we fail or generate one at random? |
Nop, here the COOKIE_SECRET = SECRET. The failure is gonna be when we merge to master. |
I guess I then misunderstood how it works - I though that the PR against master are the ones already using the encoded config file - otherwise the changes in the previous |
@@ -178,6 +178,7 @@ def _get_main(self, config): | |||
if not self.certificate_file: | |||
self.certificate_file = join(install_dir, 'qiita_core', | |||
'support_files', 'server.crt') | |||
self.cookie_secret = config.get('main', 'COOKIE_SECRET') |
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.
IMO, we should check if self.cookie_secret
is None
(as it is done for self.key_file
below). If it is None, I would go back to the current behavior, generate a random one, but also raising a Warning to the user to let him now that it was not set, in case that he actually needs to set it (e.g. load balancing).
I don't think it is critical to the point of raising an error, since the usage of that config value is specific to some functionality (we didn't realize that we needed until we wanted to do load balancing), but not signing at all I think it's bad.
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.
Functionality added
Comment addressed |
👍 |
does what it says on the tin.