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

Method + Path combination constraints #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wyattisimo
Copy link
Contributor

This update allows you to define constraints for discrete method and path combinations.

See the updated README for more details: https://github.com/Matchbook/rack-ssl-enforcer#method--path-combination-constraints

@wyattisimo
Copy link
Contributor Author

erg. Travis failed, but all tests passed when I ran them locally.

@wyattisimo
Copy link
Contributor Author

ooohhh, Travis is testing with 1.8.7, which doesn't support a #length method for Symbol

@wyattisimo
Copy link
Contributor Author

Amended commit e53a1df to be compatible with Ruby 1.8.7

@rymai
Copy link
Collaborator

rymai commented May 8, 2013

Thank you @wyattisimo!

It looks good to me, what do you think @tobmatth / @thibaudgg ?

@thibaudgg
Copy link
Collaborator

Looks good!

constraint_type = key.to_s[0, key.to_s.index('_') || key.to_s.length]

if key.to_s =~ /methods_with_paths/
# Match method and path constraints seperately, then combine the results.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"seperately" => "separately" ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha, I guess I need a spell-checker in my text editor 😁

@wyattisimo
Copy link
Contributor Author

amended commit 10594b6 to fix spelling: "seperately" => "separately"

@wyattisimo
Copy link
Contributor Author

Don't merge this yet. There's a bug when specifying a :methods_with_paths constraint along with other constraints because in the enforce_ssl_for? method, the if/else block creates two distinct paths of logic, but is contained within a loop using all?.

@tobmatth
Copy link
Owner

tobmatth commented May 8, 2013

@rymai I think there's a bug when specifying a :methods_with_paths constraint along with other constraints. Would not merge.

;)

@wyattisimo
Copy link
Contributor Author

@rymai @tobmatth Amended commit 00ddb0e with the fixes and a "complex" spec to test specifying :only, :only_methods, and :only_methods_with_paths constraints all at the same time.

This ended up requiring more significant changes than I originally expected. I tweaked enforce_ssl? so it's handling a little more logic and sending the complete list of provided constraints to enforce_ssl_for? (instead of only sending one class of constraints at a time). I think this made it easier to define the logic for combining the groups of constraints properly and actually makes it a little more readable.

Let me know what you think.

@rymai
Copy link
Collaborator

rymai commented May 14, 2013

Looks good to me now. @tobmatth is it ok for you?

@stevenleeg
Copy link

+1 for this pull request!

@rymai
Copy link
Collaborator

rymai commented Sep 6, 2013

@wyattisimo Could you please rebase on master? Thanks!

@tobmatth What do you think of this PR now?

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.

5 participants