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

Removes SslRequirement in favor of ForceSSL. #2410

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@JDutil
Member

JDutil commented Jan 9, 2013

I've added specs for the more common cases pointed out in #2308 but not for everything mentioned. I'd like to determine exactly which specs you guys would also like to add to this as well before I start on them though.

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Jan 9, 2013

Member

I think force_ssl might still be dangerous when it's added inside ApplicationController for example. The specs doesn't have that scenario.

Member

huoxito commented Jan 9, 2013

I think force_ssl might still be dangerous when it's added inside ApplicationController for example. The specs doesn't have that scenario.

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Jan 9, 2013

Member

I wasn't able to get a spec passing when I tried for that. The behavior should still work though I believe. I'd need more time to figure out how to properly spec it. It doesn't currently work so it's not a regression though. Using the config option is equivalent to wanting to put it into your ApplicationController also so we at least gain the ability to force SSL site wide with this.

The deployment tip guide could be updated to mention that you should set the config option rather than setting it in your ApplicationController to potentially help avoid confusion.

Member

JDutil commented Jan 9, 2013

I wasn't able to get a spec passing when I tried for that. The behavior should still work though I believe. I'd need more time to figure out how to properly spec it. It doesn't currently work so it's not a regression though. Using the config option is equivalent to wanting to put it into your ApplicationController also so we at least gain the ability to force SSL site wide with this.

The deployment tip guide could be updated to mention that you should set the config option rather than setting it in your ApplicationController to potentially help avoid confusion.

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ Jan 9, 2013

Member

Looks good, if we can get a nice solution to the ApplicationController use of force_ssl I think we've got a winner.

Member

BDQ commented Jan 9, 2013

Looks good, if we can get a nice solution to the ApplicationController use of force_ssl I think we've got a winner.

@JDutil JDutil referenced this pull request Jan 15, 2013

Closed

Remove SslRequirement. #45

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Jan 15, 2013

Member

@BDQ @huoxito I've tested this manually in an application and setting force_ssl in ApplicationController does work as intended now.

However I'm stumped on the best way to test it. I appear to need to do some funky modifications to the dummy apps ApplicationController then reload the entire dummy app before and after running an individual spec for seeing that it works. It seems a bit much to me to mess around with the dummy app by overwriting ApplicationController before/after just running this one spec. The force_ssl behavior is also well tested in Rails itself. So I think proving that setting the config.force_ssl works as intended should be sufficient in proving that Spree is not preventing the use of force_ssl

One approach I had tried to do in a less intrusive way, but am unable to get the application_controller changes to take effect could be seen here:
JDutil@9f72573

I'm open to any suggestions on how to test it, but it appears to be rather tricky to get the controller change to be picked up for just a single spec without interfering with other specs.

Member

JDutil commented Jan 15, 2013

@BDQ @huoxito I've tested this manually in an application and setting force_ssl in ApplicationController does work as intended now.

However I'm stumped on the best way to test it. I appear to need to do some funky modifications to the dummy apps ApplicationController then reload the entire dummy app before and after running an individual spec for seeing that it works. It seems a bit much to me to mess around with the dummy app by overwriting ApplicationController before/after just running this one spec. The force_ssl behavior is also well tested in Rails itself. So I think proving that setting the config.force_ssl works as intended should be sufficient in proving that Spree is not preventing the use of force_ssl

One approach I had tried to do in a less intrusive way, but am unable to get the application_controller changes to take effect could be seen here:
JDutil@9f72573

I'm open to any suggestions on how to test it, but it appears to be rather tricky to get the controller change to be picked up for just a single spec without interfering with other specs.

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Jan 19, 2013

Member

Hey @JDutil calling force_ssl in any app controller is dangerous.

The exact reason why I couldn't get this spec, #2308 (comment), passing is there is no way, none that I know of, to be sure that a controller called the class method force_ssl. Unless you change the implementation of FoceSsl. The ssl_required method, for example, sets a class attribute and therefore Spree is able to decide to force or not to force ssl.

I think this looks good except for the new config option. Allowing stores to have two urls for the same page doesn't sound as a good practice and imo adds unnecessary complexity.

Member

huoxito commented Jan 19, 2013

Hey @JDutil calling force_ssl in any app controller is dangerous.

The exact reason why I couldn't get this spec, #2308 (comment), passing is there is no way, none that I know of, to be sure that a controller called the class method force_ssl. Unless you change the implementation of FoceSsl. The ssl_required method, for example, sets a class attribute and therefore Spree is able to decide to force or not to force ssl.

I think this looks good except for the new config option. Allowing stores to have two urls for the same page doesn't sound as a good practice and imo adds unnecessary complexity.

@huoxito

View changes

Show outdated Hide outdated core/spec/controllers/spree/products_controller_spec.rb
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 19, 2013

Member

What's the status on this @huoxito and @JDutil?

Member

radar commented Mar 19, 2013

What's the status on this @huoxito and @JDutil?

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Mar 19, 2013

Member

It's ready to be merged if you guys want to accept it.

The issue I had was that I couldn't think of a good way to test setting force_ssl in the application controller, but it is tested by rails itself so I think it's fine.

Member

JDutil commented Mar 19, 2013

It's ready to be merged if you guys want to accept it.

The issue I had was that I couldn't think of a good way to test setting force_ssl in the application controller, but it is tested by rails itself so I think it's fine.

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Mar 19, 2013

Member

Hadn't the time to test it myself. It looks good. But I still think that Spree needs to say somewhere users should avoid using Rails force_ssl. Even with this merged in they would get an infinite redirect loop when force_ssl is added in ApplicationController or any other that interacts with Spree.

Member

huoxito commented Mar 19, 2013

Hadn't the time to test it myself. It looks good. But I still think that Spree needs to say somewhere users should avoid using Rails force_ssl. Even with this merged in they would get an infinite redirect loop when force_ssl is added in ApplicationController or any other that interacts with Spree.

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Mar 19, 2013

Member

@huoxito as I mentioned previously I've manually tested using force_ssl in the application controller, and it works as expected for me. I'm not sure where your concern about an infinite redirect loop is coming from. This PR was done in order to fix infinite redirect loops when trying to use force_ssl it isn't introducing that as an issue.

Member

JDutil commented Mar 19, 2013

@huoxito as I mentioned previously I've manually tested using force_ssl in the application controller, and it works as expected for me. I'm not sure where your concern about an infinite redirect loop is coming from. This PR was done in order to fix infinite redirect loops when trying to use force_ssl it isn't introducing that as an issue.

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Mar 19, 2013

Member

Ok then. It's just that I didn't see anywhere on the diff force_ssl being checked on controllers. Last time I looked into force_ssl it is also a class method and I'm not sure it's possible to check if a Class has a class method unless you tweak that method to allow you do to so. That was my main concern. e.g.

def self.ssl_allowed(*actions)
  class_attribute :ssl_allowed_actions # defined so we can tell later a class has ssl_allowed
  self.ssl_allowed_actions = actions
end

Good to know it's working though =). I just still don't think Rails force_ssl fits in Spree specs at all.

Member

huoxito commented Mar 19, 2013

Ok then. It's just that I didn't see anywhere on the diff force_ssl being checked on controllers. Last time I looked into force_ssl it is also a class method and I'm not sure it's possible to check if a Class has a class method unless you tweak that method to allow you do to so. That was my main concern. e.g.

def self.ssl_allowed(*actions)
  class_attribute :ssl_allowed_actions # defined so we can tell later a class has ssl_allowed
  self.ssl_allowed_actions = actions
end

Good to know it's working though =). I just still don't think Rails force_ssl fits in Spree specs at all.

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Apr 10, 2013

Member

@radar I've rebased this with master so that it will cleanly apply again with the latest 2.0 changes.

Member

JDutil commented Apr 10, 2013

@radar I've rebased this with master so that it will cleanly apply again with the latest 2.0 changes.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Apr 11, 2013

Member

@JDutil This branch seems to contain two unrelated commits. Could you rebase + remove those two commits and force-push the branch so it's just the one commit please?

Member

radar commented Apr 11, 2013

@JDutil This branch seems to contain two unrelated commits. Could you rebase + remove those two commits and force-push the branch so it's just the one commit please?

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Apr 11, 2013

Member

Done.

Member

JDutil commented Apr 11, 2013

Done.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Apr 12, 2013

Member

Added to master. Thanks for your persistance on this one @JDutil and @huoxito.

Member

radar commented Apr 12, 2013

Added to master. Thanks for your persistance on this one @JDutil and @huoxito.

@radar radar closed this in fa00f9b Apr 12, 2013

mickeyren added a commit to DynamoMTL/spree_social that referenced this pull request Apr 28, 2015

Merge remote-tracking branch 'spree/master'
* spree/master: (127 commits)
  Versionfile no longer used.
  Add missing locale :environment
  Update dev dependencies.
  Fixes spree spec support.
  Fixes Rails 5 timestamp deprecation warnings.
  Master branch now on Spree v3.1.0.beta
  Added flag to skip sign up for providers
  Fix missing ability to support 3d party devise-modules.
  No need to click through all steps for each spec.
  Namespace spree route.
  Mini tweaks to spec support.
  Enable cop EmptyLinesAroundBlockBody
  Remove ssl_allowed as of spree/spree/pull/2410
  Better colorized examples in docs.
  Remove Deface option "disabled".
  Update (c) year to 2015.
  Better contributors guidelines.
  Cache dependencies on Travis + test ruby 2.2.x
  Avoid contradictory quote style rules.
  Rework layout for master branch on bootstrap.
  ...

Conflicts:
	spree_social.gemspec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment