HTTPS support

[stevenroose]

I added support for HTTPS serving with certificates configured. It is possible to serve both HTTP and HTTPS or to automatically redirect HTTP requests to HTTPS.

I also added the possibility to define the host and ports to listen on using the parameters of serve instead of only using the environment.

This change should not introduce breaking changes to existing users.

Perhaps it is a good idea to allow users to pass the database password using an environment variable as well. I believe in that case the database files can be safely exposed, f.e. in a GitHub repo.

[stevenroose]

I just realize that it is also possible to serve HTTPS without a certificate configured. However, not often practically useful (browsers block/alert users in this case, but can be useful for testing).

Perhaps a bool enableSSL can fix this so that the check of the other params is only performed before the SecureSockets.initialize() call.

[Scorpiion]

Hi and thanks for the pull request!

First there were some questions, I'll try to take them one by one first:

1. Q: Database password using an environment variable? A: This is already how it's done, via the MONGODB_URI variable. Passwords for mongodb is part of the url, please see here: http://docs.mongodb.org/manual/reference/connection-string/#standard-connection-string-format
2. (question added from HipChat conversation) Q: Could the DB layer be abstracted away, maybe it don't have to be so integrated? A: Yes, that would be a possibility, I have thoughts on this but not a concrete plan so far. Part of the reason why it is so integrated was to make mongodb easier to use and to minimize boilerplate connection setup code. In part Q1 is related to this as well, the password and connection handling is abstracted away. But yes, I'm not too foreign on the concept of defining a more clear DB layer.
3. (question added from HipChat conversation) Q: Is Vane still actively developed? A: Yes it is, it has just been put a little bit on hold lately. I have quite a bit of code that I need to cleanup before I can push it up, but I have planned to spend a week or two soon to only work on Vane fulltime.

Second, let talk about the code. I'll split my thoughts up in two parts, regarding syntax and semantics.

Syntax:

1. I think we want to call it "TLS" support and not "SSL". I know the term SSL is somewhat of a synonym but yet not totally correct, I think it is better to set a good example and use the term TLS. A simple "Ctrl + F" search here: https://api.dartlang.org/apidocs/channels/be/dartdoc-viewer/dart:io.SecureSocket gives 13 hits for TLS and 2 for SSL, I think that alone is big enough motivation.
2. I would appreciate if you could change "!sslOnly" to "sslOnly == false", simply to keep the code more consistent and readable. Same goes for "enableSSL" to "enableSSL == true" and "redirectHTTP == true".
3. On the creation of the new HTTPS url, I'm not 100% sure, but could you not use Uri.parse(), and then just flip the schema from "http" to "https"? I think it should be logically equivalent but less code, not 100% sure though, but maybe worth testing and/or consider. I tried this, it does not work like I was thinking, you can ignore this point.
4. Please use curly braces for all flow control structures, the "if(port == null)" etc are missing these. That is also recommended in the Dart style guide: https://www.dartlang.org/articles/style-guide/#do-use-curly-braces-for-all-flow-control-structures (I nor Vane does follow the Dart style guide to 100%, but still, this one I think is important)
5. Indentation after "new Uri(scheme: "https",", I would appreciate if you changed such that the indentation was aligned with the indentation of "scheme". I think it becomes easier to read the code that way.

Semantics:

1. "port == null", will pretty much never happen, you would have to manually pass (port: null) for that if statement to be true (this because it has a standard value set already, so it's not null if not set). Same goes for sslPort.
2. You have written the code such that "parameter overwrites environment", I think that needs to be changed. It is more likely that you on localhost for example set the port to 3333 if you feel like it. Later if you deploy that on your own somehow and don't set any environment variables, it will simply still use 3333. If you deploy to for example DartVoid, the system can override that port 3333 only when the code runs at DartVoid by setting the environment variable. That's why it was written like that from the first place. Do you understand how I'm thinking here? If you're unsure how to setup the logic for this to work I could fix that later if you want.

So, lots of comments, but overall I like your contribution, I hope you don't feel I'm offensive that is really not my intent here. I could clearly change some of the syntax things later but I thought it was better to just talk about it here instead of just merging and then change some of the code later. The semantic parts simply have to be fixed since they don't work as indented.

Ps. I have not had the time to really try and run the code just yet, these comments are based on me just reading the code. If I'm wrong on something please feel free to correct me. Ds.

[stevenroose]

That's a lot of text :)

• Question 1. I meant the certificate DB password, not the Mongo password. It has to be passed as a String to SecureSockets.initialize() and is currently a parameter of serve().
• Syntax 1. I first considered using HTTPS, I think that's what most users will understand the easiest. I changed to SSL because HTTPS seemed to long :D
• Syntax 2. I do not agree. What's a boolean good for if you are gonna compare it to true? But will change because of consistency.
• Syntax 3. Uri is a value class, all properties are read-only. I considered using Uri.parse() and serializing the URI myself, but that's just as ugly. I also considered using the original Uri's string and replacing the first occurrence of http:// with https:// but that's not as fool-proof as this method. (I know it sucks that it takes up so many lines.)
• Syntax 4. Regardless of your remark on Semantics 2, which do you prefer? The first line gets extremely long, but adding an extra third line for each param seemed me too much.

sslCertificateDatabasePassword = sslCertificateDatabasePassword != null ? sslCertificateDatabasePassword : Platform.environment['SSL_CERT_DB_PASS'];
// or
if(sslCertificateDatabasePassword == null)
    sslCertificateDatabasePassword = Platform.environment['SSL_CERT_DB_PASS'];

• Syntax 5. I only indent that way when the readability is key, like in public declarations (f.e. serve()), otherwise I use the minimum of 4 spaces for broken lines, as the style guide suggests

  DO indent continued lines with at least four spaces.

• Semantics 1 & 2. Environment over parameter makes more sense considering the default values of the environment. It's true that when the user does not pass a parameter with a default value defined, the environment will be ignored. However, I think that if a parameter is passed explicitly in serve(), it should overwrite environment variables to protect the user from environment variables it is not aware of that will otherwise tamper with his code. Perhaps removing all default values is a good solution?

[stevenroose]

Aha, found Uri.replace().