Skip to content
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

Openssl added for https #2745

Merged
merged 9 commits into from
Jun 3, 2018
Merged

Openssl added for https #2745

merged 9 commits into from
Jun 3, 2018

Conversation

SidharthBansal
Copy link
Member

@SidharthBansal SidharthBansal commented May 25, 2018

@jywarren new Facebook settings allow the login through the localhost only if we are using https. I found that we can provide the https in development by creating the certificates by openssl gem and running the
passenger start --ssl --ssl-certificate localhost.crt --ssl-certificate-key localhost.key --ssl-port 3000 command instead of passenger start.

This blog might be helpful. https://www.devmynd.com/blog/rails-local-development-https-using-self-signed-ssl-certificate/

Also, I have changed the installation guidelines a little. Check it out.

@jywarren
Copy link
Member

I think this is OK and if it does not interrupt the existing SSL config on production (@icarito can tell us it's ok or not?) then we should be fine and I"m OK merging this in. I think we're still seeing some issues with the travis containers so that may hold us up a bit first. Thanks!

@SidharthBansal
Copy link
Member Author

OK great to hear that. Please check this twice on production before merging @icarito. Also, we need to notify the existing developers about the command to launch the passenger server is changed.

@SidharthBansal
Copy link
Member Author

@jywarren I tried to understand the Travis issues but was not able to. Any help is greatly appreciated.

@jywarren
Copy link
Member

Retriggering! Now it's solved...

@PublicLabBot
Copy link

PublicLabBot commented May 25, 2018

2 Messages
📖 @SidharthBansal Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.

Generated by 🚫 Danger

Gemfile Outdated
@@ -130,3 +130,4 @@ gem 'omniauth-github', '~> 1.1', '>= 1.1.2'
gem 'activerecord-tableless'
gem 'figaro'
gem 'sanitize'
gem 'openssl', '~> 2.0.0.beta.1'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this into the development gems only, since we won't be using this in production? Thanks!!!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes for sure.

@jywarren
Copy link
Member

And if you could change the docs -- the README or corresponding file -- but just say that passenger start is fine for default, but /if you're working on Facebook integration or something that needs SSL/, then you should use the command passenger start --ssl --ssl-certificate localhost.crt --ssl-certificate-key localhost.key --ssl-port 3000 -- then we should be fine and won't complicate things for most developers. Thanks!!!

@SidharthBansal
Copy link
Member Author

SidharthBansal commented May 26, 2018

@jywarren for the non-secure connection (HTTP) we can use port number 3000 using the passenger start command as we currently use.
For the secure connections, we can use the port number 3001. We will use passenger start --ssl --ssl-certificate localhost.crt --ssl-certificate-key localhost.key --ssl-port 3001 command.
We can't use one port for both HTTPS and HTTP. Hence, I have defined two different ports.
I have made the required changes in the readme and the config/environments/development file.
Thanks

@SidharthBansal
Copy link
Member Author

SidharthBansal commented May 26, 2018

image
image
I hope it is ready to be merged.

@jywarren jywarren added the ready label May 31, 2018
@ghost ghost removed the ready label Jun 2, 2018
@SidharthBansal SidharthBansal reopened this Jun 2, 2018
@ghost ghost assigned SidharthBansal Jun 2, 2018
@SidharthBansal SidharthBansal force-pushed the openssl branch 3 times, most recently from a6495d0 to 598d22d Compare June 3, 2018 08:51
@jywarren
Copy link
Member

jywarren commented Jun 3, 2018

Ah, i think this was affected by the previous issue of Dockerfile config. If you rebase it over the latest master branch, it should pass!

@SidharthBansal
Copy link
Member Author

@jywarren can we merge this?

@jywarren jywarren merged commit 0bae192 into publiclab:master Jun 3, 2018
@ghost ghost removed the ready label Jun 3, 2018
@jywarren
Copy link
Member

jywarren commented Jun 3, 2018

Perfect, thank you!!!! 👍 👍 👍

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
* Openssl added for https

* Changed Installation guidelines

* localhost.key added

* Openssl moved to development and test group

* Updated readme

* changed config settings

* Updated readme

* patches

* CodeClimate Error Fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature explains that the issue is to add a new feature review-me summer-of-code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants