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

Remove unnecessary SSL code #10339

Merged
merged 3 commits into from
Feb 1, 2023
Merged

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Jan 30, 2023

What? Why?

Once upon a time, SSL over HTTP (HTTPS) was new and not supported by default. We still got some leftover code from those times which can be removed now.

What should we test?

  • Open a private window to not use any HSTS settings.
  • Visit http://staging.../ and make sure that you are redirected to https://staging.../.
  • Then log in as super admin and go to the general settings.
  • The two "security" options should be gone and the layout should not be compromised.

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

Rails comes with ActionDispatch::SSL which is enabled in staging and
production. We don't need this ancient gem last updated in 2014.
It's an outdated Spree setting. We always enforce SSL in production and
staging while development and test environments are running without SSL.
This setting didn't have any effect.
@mkllnk mkllnk self-assigned this Jan 30, 2023
@mkllnk mkllnk marked this pull request as ready for review January 30, 2023 04:04
Copy link
Contributor

@jibees jibees left a comment

Choose a reason for hiding this comment

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

👌
Thanks for cleaning the code! 🧼

@filipefurtad0 filipefurtad0 self-assigned this Feb 1, 2023
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Feb 1, 2023
@filipefurtad0
Copy link
Contributor

Hey @mkllnk ,

Thanks for the detailed steps on how to check this:

  • Opened a private window to not use any HSTS settings.
  • Visited http://staging.../ -> redirection to https://staging.../. occurred 👍
  • Logged in as super admin and visited the general settings
  • The two "security" options are gone now 👍
  • The layout looks good 👍

All good here! Merging.

@filipefurtad0 filipefurtad0 merged commit 095e520 into openfoodfoundation:master Feb 1, 2023
@filipefurtad0 filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Feb 1, 2023
@filipefurtad0
Copy link
Contributor

Hey @mkllnk,

I've noticed this error, on #devops-notifications:
https://openfoodnetwork.slack.com/archives/CEF14NU3V/p1675275022293559


Unhandled error
NoMethodError: undefined method `disable_starttls_auto' for #<Net::SMTP smtp.sendgrid.net:587 started=false>
Did you mean?  disable_starttls
              enable_starttls_auto
Location
gems/mail-2.8.0.1/lib/mail/network/delivery_methods/smtp.rb:138 - block in build_smtp_session

Emails seem not to be arriving for staging-UK - strangely, they seem to be arriving for staging-AU

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Feb 2, 2023

Emails are broken on staging-FR too; this was probably introduced on this PR.

EDIT: nope, that should not be this PR - we still have the SSL setting on staging-FR

image

@mkllnk mkllnk deleted the ssl branch February 27, 2023 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants