-
Notifications
You must be signed in to change notification settings - Fork 6
Implement authentication through a Facebook flow #47
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
Changes from all commits
10a9c7d
1b4a928
05d2eb3
002a80c
631688a
92085bf
0598b42
f365472
e8520e7
4e48c20
7eeb53d
590ead3
cb528f3
cf40a02
cfdb882
21daea9
101b0d0
84eef09
79265de
ee83cb7
cfcc0ca
57bf038
cb52e46
5f8f9fe
706ab7b
93db8e2
aa8fb3b
d732f30
a1a8b26
7d31a1b
5400675
d4c32d5
89e8dfc
07ba7f7
df6d2a9
00649fa
2b93393
c5a5f6b
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 |
|---|---|---|
|
|
@@ -26,6 +26,15 @@ convict.addFormat({ | |
| coerce: (val) => (val === null ? null : parseInt(val)) | ||
| }); | ||
|
|
||
| convict.addFormat({ | ||
| name: 'hex', | ||
| validate: function(val) { | ||
| if (/[^a-fA-F0-9]/.test(val)) { | ||
| throw new Error('must be a hex key'); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| const conf = convict({ | ||
| adminPass: { | ||
| doc: 'The password for the admin user. Follow OWASP guidelines for passwords', | ||
|
|
@@ -72,6 +81,9 @@ const conf = convict({ | |
| default: '', | ||
| } | ||
| }, | ||
| publicHost: { | ||
| doc: 'Public host for this server, especially for auth callback' | ||
| }, | ||
| sensorthings: { | ||
| server: { | ||
| doc: 'SensorThings remote API server', | ||
|
|
@@ -93,6 +105,39 @@ const conf = convict({ | |
| } | ||
| } | ||
| }, | ||
| behindProxy: { | ||
| doc: `Set this to true if the server runs behind a reverse proxy. This is | ||
| especially important if the proxy implements HTTPS with | ||
| userAuth.cookieSecure. Also with this Express will trust the | ||
| X-Forwarded-For header. Set to 1 or auto if you're behind a proxy.`, | ||
| default: false, | ||
| // Format is "*" because otherwise convict infers it's a boolean from the | ||
| // default value and will refuse 1 or "auto". | ||
| format: '*', | ||
|
Collaborator
Author
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. format is "*" because otherwise convict infers it's a boolean from the default and will refuse 1 or "auto".
Member
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. Could you add all these comments as in code comments, please? |
||
| }, | ||
| userAuth: { | ||
| cookieSecure: { | ||
| doc: `This configures whether the cookie should be set and sent for | ||
| HTTPS only. This is important to set to 'true' if the application | ||
| runs on HTTPS.`, | ||
| default: false, | ||
| }, | ||
| sessionSecret: { | ||
| doc: 'This secret is used to sign session cookie', | ||
| default: defaultValue, | ||
| format: avoidDefault, | ||
| }, | ||
| facebook: { | ||
| clientId: { | ||
| doc: 'Facebook clientId', | ||
| format: 'nat' | ||
| }, | ||
| clientSecret: { | ||
| doc: 'Facebook clientSecret', | ||
| format: 'hex' | ||
| }, | ||
| }, | ||
| }, | ||
| version: { | ||
| doc: 'API version. We follow SensorThing\'s versioning format as described at http://docs.opengeospatial.org/is/15-078r6/15-078r6.html#34', | ||
| format: value => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| import jwt from 'jsonwebtoken'; | ||
| import config from '../config'; | ||
| import db from '../models/db'; | ||
|
|
||
| import { | ||
| ApiError, | ||
|
|
@@ -16,13 +17,25 @@ function unauthorized(res) { | |
| } | ||
|
|
||
| export default (scopes) => { | ||
| // For now we only allow 'admin' scope. | ||
| const validScopes = ['admin', 'client', 'user'].filter( | ||
| scope => scopes.includes(scope) | ||
| ); | ||
|
|
||
| if (!validScopes.length) { | ||
| throw new Error(`No valid scope found in "${scopes}"`); | ||
| } | ||
|
|
||
| return (req, res, next) => { | ||
| const authHeader = req.headers['authorization']; | ||
| if (!authHeader) { | ||
| const authHeader = req.headers.authorization; | ||
| // Accepting a query parameter as well allows GET requests. | ||
| const authQuery = req.query.authorizationToken; | ||
|
Collaborator
Author
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. To be able to use a simple browser request, we can pass the JWT token in this query parameter. I haven't looked if there is anything kinda standardized for this yet. |
||
|
|
||
| if (!authHeader && !authQuery) { | ||
| return unauthorized(res); | ||
| } | ||
|
|
||
| const token = authHeader.split('Bearer ')[1]; | ||
| const token = authHeader ? authHeader.split('Bearer ')[1] : authQuery; | ||
| if (!token) { | ||
| return unauthorized(res); | ||
| } | ||
|
Member
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. Maybe: const token = (authHeader && authHeader.split('Bearer ')[1]) || authQuery;
if (!token) {
return unauthorized(res);
}
Collaborator
Author
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. yeah why not; I can use a ternary which can be a tiny bit more readable. |
||
|
|
@@ -31,36 +44,43 @@ export default (scopes) => { | |
| // need to get the owner of the token so we can get the appropriate secret. | ||
| const decoded = jwt.decode(token); | ||
|
|
||
| // For now we only allow authenticated requests from the admin user. | ||
| // When this changes we will have a different secret per sensor and per | ||
| // user. | ||
| if (!decoded || !decoded.id || decoded.id !== 'admin') { | ||
| if (!decoded || !decoded.id || !decoded.scope) { | ||
| return unauthorized(res); | ||
| }; | ||
| } | ||
|
|
||
|
Collaborator
Author
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. note: directly calling |
||
| const secret = config.get('adminSessionSecret'); | ||
| if (!validScopes.includes(decoded.scope)) { | ||
| console.log('Error while authenticating, invalid scope', decoded); | ||
| return unauthorized(res); | ||
| } | ||
|
|
||
| // Verify JWT signature. | ||
| jwt.verify(token, secret, (error, decoded) => { | ||
| if (error) { | ||
| return unauthorized(res); | ||
| } | ||
| let secretPromise; | ||
| switch(decoded.scope) { | ||
| case 'client': | ||
| secretPromise = db().then(({ Clients }) => | ||
| Clients.findById(decoded.id, { attributes: ['secret'] }) | ||
| ).then(client => client.secret); | ||
| break; | ||
| case 'user': | ||
| case 'admin': | ||
| secretPromise = Promise.resolve(config.get('adminSessionSecret')); | ||
| break; | ||
| default: | ||
| // should not happen because we check this earlier | ||
| next(new Error(`Unknown scope ${decoded.scope}`)); | ||
| } | ||
|
|
||
| // XXX Get allowed scopes from sensor/user. | ||
| // Verify JWT signature. | ||
| secretPromise.then(secret => { | ||
| jwt.verify(token, secret, (error) => { | ||
| if (error) { | ||
| console.log('Error while verifying the token', error); | ||
| return unauthorized(res); | ||
| } | ||
|
|
||
| // For now we only allow 'admin' scope. | ||
| const validScopes = ['admin'].filter(scopeIndex => { | ||
| return scopes.indexOf(scopeIndex) != -1; | ||
| req[decoded.scope] = decoded.id; | ||
| req.authScope = decoded.scope; | ||
|
Collaborator
Author
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. these 2 lines are repeated a few times, it's likely a good idea to factorize this, but maybe in a separate patch ?
Member
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. Probably, but where are the repetitions? :)
Collaborator
Author
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. there are 3 locations where we set That said, maybe we don't want this in I'll think of this and maybe we can rework it when implementing google and fx accounts.
Member
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. File another issue and reference it in the code, please :)
Collaborator
Author
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. after all I just did what I suggested in a separate commit. Easier than creating an issue :) Tell me what you think. |
||
| return next(); | ||
| }); | ||
|
|
||
| if (!validScopes.length) { | ||
| return unauthorized(res); | ||
| } | ||
|
|
||
| // If everything is good, save the decoded payload for use in other | ||
| // routes. | ||
| req.decoded = decoded; | ||
| next(); | ||
| }); | ||
| }).catch(err => next(err || new Error('Unexpected error'))); | ||
| }; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,16 @@ let deferreds = []; | |
| let state = IDLE; | ||
| let db = null; | ||
|
|
||
| module.exports = () => { | ||
| const { name, user, password, host, port } = config.get('db'); | ||
|
|
||
| const sequelize = new Sequelize(name, user, password, { | ||
| host, | ||
| port, | ||
| dialect: 'postgres', | ||
| logging: false | ||
| }); | ||
|
Collaborator
Author
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. extracting this out of the exported function makes it possible to export it so that we can use it elsewhere. (in my case: for the session store)
Collaborator
Author
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 don't think this makes a big perf difference, as I believe what makes the big perf hit is loading all the model and calling |
||
|
|
||
| export default function() { | ||
| if (state === READY) { | ||
| return Promise.resolve(db); | ||
| } | ||
|
|
@@ -41,24 +50,13 @@ module.exports = () => { | |
|
|
||
| state = INITIALIZING; | ||
|
|
||
| const dbConfig = config.get('db'); | ||
| const { name, user, password, host, port } = dbConfig; | ||
|
|
||
| const sequelize = new Sequelize(name, user, password, { | ||
| host, | ||
| port, | ||
| dialect: 'postgres', | ||
| logging: false | ||
| }); | ||
|
|
||
| db = {}; | ||
|
|
||
| fs.readdirSync(__dirname) | ||
| .filter(file => { | ||
| return ((file.indexOf('.js') !== 0) && | ||
| (file !== 'db.js') && | ||
| (file !== 'users.js') && | ||
| (file.indexOf('.swp') < 0)); | ||
| !file.startsWith('.') && | ||
| (file !== 'db.js')); | ||
| }) | ||
| .forEach(file => { | ||
| const model = sequelize.import(path.join(__dirname, file)); | ||
|
|
@@ -74,7 +72,7 @@ module.exports = () => { | |
| db.sequelize = sequelize; | ||
| db.Sequelize = Sequelize; | ||
|
|
||
| return db.sequelize.sync().then(() => { | ||
| return sequelize.sync().then(() => { | ||
| while (deferreds.length) { | ||
| deferreds.pop().resolve(db); | ||
| } | ||
|
|
@@ -86,4 +84,6 @@ module.exports = () => { | |
| deferreds.pop().reject(e); | ||
| } | ||
| }); | ||
| }; | ||
| } | ||
|
|
||
| export { sequelize, Sequelize }; | ||
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.
this is nice as we can work with promises instead of using end + done callback.
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.
Nice!