-
Notifications
You must be signed in to change notification settings - Fork 116
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
Added option to specify host address to bind servers to #37
Changes from 4 commits
3e3636b
465d49f
4e84818
3bd2476
76fec61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,6 +84,7 @@ var appServer = function(config) { | |
if (config.verify) { | ||
self.express.use(endpoint, alexaVerifierMiddleware()); | ||
} | ||
|
||
self.express.post(endpoint, function(req, res) { | ||
var json = req.body, | ||
response_json; | ||
|
@@ -126,6 +127,7 @@ var appServer = function(config) { | |
self.error("Error loading app [" + main + "]: " + e); | ||
} | ||
}); | ||
|
||
return self.apps; | ||
}; | ||
|
||
|
@@ -149,11 +151,13 @@ var appServer = function(config) { | |
|
||
self.start = function() { | ||
// Instantiate up the server | ||
//TODO: add i18n support (i18n-node might be a good look) | ||
self.express = express(); | ||
//TODO: change this to make sure it doesn't affect other non-Alexa services/apps | ||
self.express.use(bodyParser.urlencoded({ extended: true })); | ||
|
||
//We need the rawBody for request verification | ||
self.express.use(function(req, res, next) { | ||
self.express.use(function(req, res, next) { //TODO: change code to possibly use bodyParser.json() | ||
// mark the request body as already having been parsed so it's ignored by | ||
// other body parser middlewares | ||
req._body = true; | ||
|
@@ -220,31 +224,41 @@ var appServer = function(config) { | |
if (privateKey != undefined && certificate != undefined) { | ||
var credentials = { key: privateKey, cert: certificate }; | ||
|
||
try { //The line below can fail it the certs were generated incorrectly. But we can continue startup without HTTPS | ||
self.httpsInstance = https.createServer(credentials, self.express).listen(config.httpsPort); //create the HTTPS server | ||
self.log("Listening on HTTPS port " + config.httpsPort); | ||
try { // These two lines below can fail it the certs were generated incorrectly. But we can continue startup without HTTPS | ||
var httpsServer = https.createServer(credentials, self.express); // create the HTTPS server | ||
|
||
//TODO: add separate option to specify specific host address for HTTPS server to bind to ??? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tiny annoying nitpick. Can we add a space after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No problem! |
||
if (typeof config.host === 'string') { | ||
self.httpsInstance = httpsServer.listen(config.httpsPort, config.host); | ||
self.log("Listening on https://" + config.host + ":" + config.httpsPort); | ||
} else { | ||
self.httpsInstance = httpsServer.listen(config.httpsPort); | ||
self.log("Listening on HTTPS port " + config.httpsPort); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would say "Listening on https://127.0.0.1:port or localhost ... |
||
} | ||
} catch (error) { | ||
self.error("Failed to listen via HTTPS Error: " + error); | ||
} | ||
} else { | ||
self.error("Failed to load privateKey or certificate from /sslcert. HTTPS will not be enabled"); | ||
|
||
} | ||
|
||
} else { | ||
self.error("privateKey: '" + config.privateKey + "' or certificate: '" + config.certificate + "' do not exist in /sslcert. HTTPS will not be enabled"); | ||
|
||
} | ||
} else { | ||
self.error("privatekey, httpsPort, or certificate paramater not set in config. HTTPS will not be enabled"); | ||
|
||
} | ||
|
||
} | ||
|
||
// Start the server listening | ||
config.port = config.port || process.env.port || 80; | ||
self.httpInstance = self.express.listen(config.port); | ||
self.log("Listening on HTTP port " + config.port); | ||
|
||
if (typeof config.host === 'string') { | ||
self.httpInstance = self.express.listen(config.port, config.host); | ||
self.log("Listening on http://" + config.host + ":" + config.port); | ||
} else { | ||
self.httpInstance = self.express.listen(config.port); | ||
self.log("Listening on HTTP port " + config.port); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reading this, are we creating both an http and https endpoint? Shouldn't be just https if that's enabled? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, since the initial check already checks if https is enabled, and if so, will then proceed to initialize |
||
|
||
// Run the post() method if defined | ||
if (typeof config.post == "function") { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/*jshint expr: true*/ | ||
"use strict"; | ||
var chai = require("chai"); | ||
var expect = chai.expect; | ||
chai.config.includeStack = true; | ||
var request = require("supertest-as-promised"); | ||
var alexaAppServer = require("../index"); | ||
|
||
describe("Alexa App Server with Examples & Custon Server Bindings", function() { | ||
var testServer; | ||
|
||
afterEach(function() { | ||
testServer.stop(); | ||
}); | ||
|
||
it("mounts hello world app (HTTP only)", function() { | ||
testServer = alexaAppServer.start({ | ||
port: 3000, | ||
host_address: "127.0.0.1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was going to test it via referring to the actual address (like |
||
server_root: 'examples' | ||
}); | ||
return request(testServer.express) | ||
.get('/alexa/helloworld') | ||
.expect(200); | ||
}); | ||
|
||
it("mounts hello world app (HTTP & HTTPS)", function() { | ||
testServer = alexaAppServer.start({ | ||
port: 3000, | ||
host: "127.0.0.1", | ||
server_root: 'examples', | ||
httpsEnabled: true, | ||
httpsPort: 6000, | ||
privateKey: 'private-key.pem', | ||
certificate: 'cert.cer' | ||
}); | ||
|
||
return request(testServer.express) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just like above. |
||
.get('/alexa/helloworld') | ||
.expect(200); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: personally I don't find these TODOs useful, because we have Github issues. If you want them there, maybe add a link to the github issue opened for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially put the TODO's so that if anyone else is trying to fix any current issues, they know where in the code to look for. I like the idea of adding the links to the issues, so thanks!