Insert Letsencrypt::Middleware dynamically

[olivierlacan]

Since all we have to do is check for force_ssl I don't see why we can't just do that inside an initialize hook and change the way the middleware is inserted accordingly.

Maybe I'm missing something obvious. This would reduce the amount of manual configuration required for this gem.

[olivierlacan]

I've tested this in a production application and it works perfectly but if you'd like me to add a test I wouldn't mind.

[jalada]

This is a nice idea! Thanks. I think it's definitely on the right lines.

However I'm not happy about the gem making the assumption that you're only going to use it in production. What about securing a staging environment that runs Rails under a different environment?

Is there another approach that would still keep this control but automate insertion position?

[olivierlacan]

We don't have to check for the environment, I was just mimicking your implementation since you're in suggesting inserting the middleware in production.rb

That actually would make sense for me since I am using this gem on an application with a staging environment.

On October 9, 2016 at 2:59:25 AM, David Somers (notifications@github.com(mailto:notifications@github.com)) wrote:

This is a nice idea! Thanks. I think it's definitely on the right lines.

However I'm not happy about the gem making the assumption that you're only going to use it in production. What about securing a staging environment that runs Rails under a different environment?

Is there another approach that would still keep this control but automate insertion position?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub(https://github.com/pixielabs/letsencrypt-rails-heroku/pull/10#issuecomment-252457144), or mute the thread(https://github.com/notifications/unsubscribe-auth/AAEBnkEdTFCBae9e7v0NC62i7md0KTyZks5qyDxtgaJpZM4KR3bK).

[olivierlacan]

@jalada Removed the conditional branch that was checking Rails.env.production? I feel like it's the minimum viable conditional at this point. Let me know if you have any other concerns.

[jalada]

Good point re: installing the gem in the :production group. Thanks for your continued work ☺️ A couple of things:

1. In its current state, will I need to change the major version of the gem? What happens if someone retains the middleware insertion in their config when they upgrade to this new version of the gem? If it's backwards compatible, it can just be a minor bump.

2. There's still something that doesn't feel right about inserting middleware 'on the sly'; I'm just concerned about making that kind of assumption, though it does also feel more Rails-y to have convention over configuration.

   If someone has other requirements about where this middleware needs to be are they going to become unstuck? For example if someone isn't forcing SSL, but is using some middleware that performs authentication. The user would want the LE middleware to be inserted before the authentication piece.

As an alternative, what about a helper function that could be called in an initialiser (perhaps including a Rails generator to create it) that adds the middleware in this manner, e.g.

rails g letsencrypt:initializer creates:

# config/initializers/letsencrypt.rb
Letsencrypt.add_middleware!

That way, installation is simpler, we take away the need to handle force_ssl, but the API is still retained and the user has the option to adjust how its inserted in the stack. I'm not sure this feels very 'standard' however.

Just a suggestion though; want to get the API right :)

[olivierlacan]

In its current state, will I need to change the major version of the gem? What happens if someone retains the middleware insertion in their config when they upgrade to this new version of the gem? If it's backwards compatible, it can just be a minor bump.

Not quite sure. I can test this fairly simply though.

There's still something that doesn't feel right about inserting middleware 'on the sly'; I'm just concerned about making that kind of assumption, though it does also feel more Rails-y to have convention over configuration.

If someone has other requirements about where this middleware needs to be are they going to become unstuck? For example if someone isn't forcing SSL, but is using some middleware that performs authentication. The user would want the LE middleware to be inserted before the authentication piece.

I don't know. I honestly value convention over configuration hence the PR. But your initializer alternative isn't bad at all.

How does the end-user adjust how the middleware is inserted? As an argument to Letsencrypt.add_middleware!? Seems like unnecessary indirection from the middleware manipulation methods. Why not just call those in the letsencrypt.rb middleware? It's true that it abstracts away the nastiness of the SSL checking.

[olivierlacan]

Chris Mar gave me a good example for automatic middleware insertion: https://github.com/plataformatec/devise/blob/88724e10adaf9ffd1d8dbfbaadda2b9d40de756a/lib/devise/rails.rb#L22-L27

Granted Devise is a hefty thing but I feel like requiring manual (or semi-manual) addition of an initializer (note than Devise already has an initializer but it's used for configuration, not boot-time setup) is a bit too much.

Your call regardless. :-)