feat(server): add lusca #1166
feat(server): add lusca #1166
Conversation
|
Are there any notes that should be included for the users? Or is the more a drop-in/set-it-forget-it kind of feature? |
|
As is, it's kind of a drop-in, but there are more features that can be enabled, etc. I'll add a comment in the code about Lusca. |
|
This needs to be rebased on |
| var session = require('express-session');<% if (filters.mongoose) { %> | ||
| var mongoStore = require('connect-mongo')(session); | ||
| var mongoose = require('mongoose-bird')();<% } %><% } %> | ||
| var mongoose = require('mongoose-bird')();<% } %> |
|
rebased |
| var config = require('./environment');<% if (filters.auth) { %> | ||
| var passport = require('passport');<% } %><% if (filters.twitterAuth) { %> | ||
| var passport = require('passport');<% } %> | ||
| var session = require('express-session');<% if (filters.mongoose) { %> | ||
| var mongoStore = require('connect-mongo')(session); | ||
| var mongoose = require('mongoose');<% } %><% } %> |
There was a problem hiding this comment.
If you're removing <% if (filters.twitterAuth) { %> then you also need to remove the closing tag <% } %> here.
There was a problem hiding this comment.
oh damnit, yeah, that's from the rebase
|
I think the tests are failing because they're making a |
Awk34
force-pushed
the
lusca
branch
3 times, most recently
from
df57930 to
cbd0798
Compare
|
I rebased this on canary, but I'm going to guess you already figured that out. |
Oh yeah, nice catch! |
|
I'm going to hold off on merging this for now; I want to try to get supertest working with the |
What's the point? |
|
If we disable the csrf strategy for the tests then we're not really testing the same code that would run in production. Also it would be good to know if a future change breaks the implementation. |
|
I don't see there really being a difference if you were to get supertest to work with the CSRF tokens. It wouldn't affect the tests still. The tests aren't meant to test the Lusca stuff. |
|
They are integration(e2e) tests, which are supposed to test an app as a whole. IMO that includes security implementation. I'm not trying to test if lusca is using and decoding tokens properly, I'm going to assume it is. What I'd like to test is that our implementation is correct. |
|
If we can get it working with tests, that would be better; but if not, I don't thing it's a huge deal. |
|
I agree, I wouldn't want tests to put too much of a hold on implementing a security feature. I'll debug it more this weekend and see if I can get things straight between Travis and SauceLabs. Oddly enough the tests pass when I use Either way I'm setting Monday as my deadline. |
|
Well there is still something fishy about the way the e2e test fail when using |
No description provided.