search

not-kennethreitz / flask-sslify

Force SSL on your Flask app.
https://pypi.python.org/pypi/Flask-SSLify
BSD 2-Clause "Simplified" License
503 stars 85 forks source link

Allow certain paths from being redirected. #28

Closed tofias closed 9 years ago

tofias commented 9 years ago

My humble attempt to address Issue #25.

tofias commented 9 years ago

Might have gotten too ambitious, but once I started configing I could not be confining.

ryan-lane commented 9 years ago

I think the redirects stuff isn't in the scope of this PR. Can we remove that for now? Let's revisit it in another issue or PR.

tofias commented 9 years ago

Yup.

tofias commented 9 years ago

You'd be able to fake that now with SSL_SKIPS if you included just a '/' to be skipped.

tofias commented 9 years ago

Hey, what was the format() thing?

ryan-lane commented 9 years ago

See: https://github.com/kennethreitz/flask-sslify/commit/39223823d97eca91bf20eff9e18ce82b1973ec92

tofias commented 9 years ago

Okay, so is that what you meant?

ryan-lane commented 9 years ago

Sorry for the lengthy review :)

tofias commented 9 years ago

What if we change the naming convention to SSLIFY_XXX and use the set default method?

ryan-lane commented 9 years ago

Hm. I'm not too keen on a library mucking around with the app config as it may have unintended consequences and the end-user may not be aware that it's occurring. It still seems a lot more straightforward to continue using the arguments and to not introduce app.config settings. It's a lot more consistent with how the library currently works.

tofias commented 9 years ago

I looked at Flask-SQLAlchemy and copied the setdefault method. Was so proud, but you're the boss

Ehhh. I'd still be craving for those sexy/clean create_app functions. And really, how many people do you imagine could be using Flask-SSSLify and SSLIFY_XXX in config in someway that does not lineup (almost) exactly with what is going on on here? And this paves the way for something similar with app.testing as well when you get around to it.

ryan-lane commented 9 years ago

Let me take a look at some other libraries to see if this is a consistent pattern.

ryan-lane commented 9 years ago

Seems like a reasonable pattern, based on some other modules. Just need the updated docs and I think we're good to go.

tofias commented 9 years ago

👍

ryan-lane commented 9 years ago

I've tested locally and everything is working well. Merging in.

tofias commented 9 years ago

Cool. And thanks for making my first experience contributing to an Open Source project so positive.

sfriesel commented 9 years ago

I'm not too keen on a library mucking around with the app config as it may have unintended consequences

e.g. if the app argument is in fact a flask blueprint, which used to work in 0.1.4 and now gives

AttributeError: 'Blueprint' object has no attribute 'config'