-
Notifications
You must be signed in to change notification settings - Fork 118
0.7 Release #139
Changes from 17 commits
c8475e3
79f24db
d3655f0
f7fa8cc
b3bd9a2
72891cc
2e35da0
36cc00f
6736a07
a0b2ed4
8405986
0dc0ede
44aa4c3
72be2f8
2363314
c73fea5
1905e0d
1df214c
633775a
8148f8e
e80a1ba
944d8d2
d17adbf
24c67e0
b92eace
ab53e19
0c4f9cd
01a4a6b
96ab0d1
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 |
---|---|---|
|
@@ -36,9 +36,27 @@ export function authenticateSocket(options, socket, method) { | |
}); | ||
} | ||
|
||
// Returns a promise that de-authenticates a socket | ||
export function logoutSocket(socket, method) { | ||
return new Promise((resolve, reject) => { | ||
// If we don't get a logged out message within 10 seconds | ||
// consider it a failure. | ||
const timeout = setTimeout(function() { | ||
reject(new Error('Could not logout over socket')); | ||
}, 10000); | ||
|
||
socket.once('logged out', function() { | ||
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. Couldn't this just be the 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, but we could use a callback instead of two events. |
||
clearTimeout(timeout); | ||
resolve(); | ||
}); | ||
|
||
socket[method]('logout'); | ||
}); | ||
} | ||
|
||
// Returns the value for a cookie | ||
export function getCookie(name) { | ||
if(typeof document !== 'undefined') { | ||
if (typeof document !== 'undefined') { | ||
const value = `; ${document.cookie}`; | ||
const parts = value.split(`; ${name}=`); | ||
|
||
|
@@ -50,13 +68,22 @@ export function getCookie(name) { | |
return null; | ||
} | ||
|
||
// Returns the value for a cookie | ||
export function clearCookie(name) { | ||
if (typeof document !== 'undefined') { | ||
document.cookie = `${name}=;expires=Thu, 01 Jan 1970 00:00:01 GMT;`; | ||
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. Is this a standard default? 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. It just needs to be set in the past, so it can be whatever. |
||
} | ||
|
||
return null; | ||
} | ||
|
||
// Tries the JWT from the given key either from a storage or the cookie | ||
export function getJWT(key, storage) { | ||
return Promise.resolve(storage.getItem(key)).then(jwt => { | ||
const cookieKey = getCookie(key); | ||
export function getJWT(tokenKey, cookieKey, storage) { | ||
return Promise.resolve(storage.getItem(tokenKey)).then(jwt => { | ||
const cookieToken = getCookie(cookieKey); | ||
|
||
if(cookieKey) { | ||
return cookieKey; | ||
if (cookieToken) { | ||
return cookieToken; | ||
} | ||
|
||
return jwt; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,10 @@ export default function(options = {}){ | |
throw new Error(`The 'restrictToOwner' hook should only be used as a 'before' hook.`); | ||
} | ||
|
||
if (!hook.id) { | ||
throw new Error(`The 'restrictToOwner' hook should only be used on the 'get', 'update', 'patch' and 'remove' service methods.`); | ||
} | ||
|
||
// If it was an internal call then skip this hook | ||
if (!hook.params.provider) { | ||
return hook; | ||
|
@@ -30,13 +34,17 @@ export default function(options = {}){ | |
throw new Error(`'${options.idField} is missing from current user.'`); | ||
} | ||
|
||
// NOTE (EK): This just scopes the query for the resource requested to the | ||
// current user, which will result in a 404 if they are not the owner. | ||
hook.params.query[options.ownerField] = id; | ||
|
||
// TODO (EK): Maybe look up the actual document in this hook and throw a Forbidden error | ||
// if (field && id && field.toString() !== id.toString()) { | ||
// throw new errors.Forbidden('You do not have valid permissions to access this.'); | ||
// } | ||
// look up the document and throw a Forbidden error if the user is not an owner | ||
return new Promise((resolve, reject) => { | ||
this.get(hook.id).then(data => { | ||
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. Do we need to add 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 ya. Good catch! Going to have the same in restrictToRoles |
||
const field = data[options.ownerField]; | ||
|
||
if ( field === undefined || field.toString() !== id.toString() ) { | ||
reject(new errors.Forbidden('You do not have the permissions to access this.')); | ||
} | ||
|
||
resolve(hook); | ||
}).catch(reject); | ||
}); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,13 +21,21 @@ const PROVIDERS = { | |
// Options that apply to any provider | ||
const defaults = { | ||
idField: '_id', | ||
shouldSetupSuccessRoute: true, | ||
shouldSetupFailureRoute: true, | ||
successRedirect: '/auth/success', | ||
failureRedirect: '/auth/failure', | ||
tokenEndpoint: '/auth/token', | ||
localEndpoint: '/auth/local', | ||
userEndpoint: '/users', | ||
header: 'authorization', | ||
cookie: 'feathers-jwt' | ||
cookie: { | ||
enabled: true, | ||
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. Can this just be considered disabled when someone set 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 think we can probably do that. |
||
name: 'feathers-jwt', | ||
httpOnly: false, | ||
secure: process.env.NODE_ENV === 'production' ? true : false, | ||
expires: new Date() | ||
} | ||
}; | ||
|
||
export default function auth(config = {}) { | ||
|
@@ -52,27 +60,17 @@ export default function auth(config = {}) { | |
|
||
// Merge and flatten options | ||
const authOptions = Object.assign({}, defaults, app.get('auth'), config); | ||
|
||
// If we should redirect on success and the redirect route is the same as the | ||
// default then we'll set up a route handler. Otherwise we'll leave it to the developer | ||
// to set up their own custom route handler. | ||
if (authOptions.successRedirect === defaults.successRedirect) { | ||
debug(`Setting up successRedirect route: ${authOptions.successRedirect}`); | ||
|
||
app.get(authOptions.successRedirect, function(req, res){ | ||
res.sendFile(path.resolve(__dirname, 'public', 'auth-success.html')); | ||
}); | ||
|
||
// If a custom success redirect is passed in or it is disabled then we | ||
// won't setup the default route handler. | ||
if (authOptions.successRedirect !== defaults.successRedirect) { | ||
authOptions.shouldSetupSuccessRoute = false; | ||
} | ||
|
||
// If we should redirect on failure and the redirect route is the same as the | ||
// default then we'll set up a route handler. Otherwise we'll leave it to the developer | ||
// to set up their own custom route handler. | ||
if (authOptions.failureRedirect === defaults.failureRedirect) { | ||
debug(`Setting up failureRedirect route: ${authOptions.failureRedirect}`); | ||
|
||
app.get(authOptions.failureRedirect, function(req, res){ | ||
res.sendFile(path.resolve(__dirname, 'public', 'auth-fail.html')); | ||
}); | ||
// If a custom failure redirect is passed in or it is disabled then we | ||
// won't setup the default route handler. | ||
if (authOptions.failureRedirect !== defaults.failureRedirect) { | ||
authOptions.shouldSetupFailureRoute = false; | ||
} | ||
|
||
// Set the options on the app | ||
|
@@ -115,7 +113,7 @@ export default function auth(config = {}) { | |
// be dealing with a config param and not a provider config | ||
// If that's the case we don't need to merge params and we | ||
// shouldn't try to set up a service for this key. | ||
if (!isObject(config[key])) { | ||
if (!isObject(config[key]) || key === 'cookie') { | ||
return; | ||
} | ||
|
||
|
@@ -145,6 +143,24 @@ export default function auth(config = {}) { | |
// Register error handling middleware for redirecting to support | ||
// redirecting on authentication failure. | ||
app.use(middleware.failedLogin(authOptions)); | ||
|
||
// Setup route handler for default success redirect | ||
if (authOptions.shouldSetupSuccessRoute) { | ||
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. What was the reason again for adding those? I mean if you do a redirect it will almost certainly be to your own success page no? 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. The problem is if someone wants to use the same route name but a different handler. Which, I think is actually probably the most common use case. You can't rely on just inspecting whether the 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. What I'm saying is why are there default auth redirect HTML pages at all? I might be missing something but they don't seem particularly useful. If you do a browser redirect it'll pretty much always be to your app right? Couldn't we make it required and throw an error if it isn't set (or disabled)? 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. For web apps you are correct, you would pretty much always override them. However, if you use the pop-up method or are on mobile, with the defaults you don't need to do anything to the server. You check that you navigated to |
||
debug(`Setting up successRedirect route: ${authOptions.successRedirect}`); | ||
|
||
app.get(authOptions.successRedirect, function(req, res){ | ||
res.sendFile(path.resolve(__dirname, 'public', 'auth-success.html')); | ||
}); | ||
} | ||
|
||
// Setup route handler for default failure redirect | ||
if (authOptions.shouldSetupFailureRoute) { | ||
debug(`Setting up failureRedirect route: ${authOptions.failureRedirect}`); | ||
|
||
app.get(authOptions.failureRedirect, function(req, res){ | ||
res.sendFile(path.resolve(__dirname, 'public', 'auth-fail.html')); | ||
}); | ||
} | ||
}; | ||
} | ||
|
||
|
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.
It's like your dad's JWT 😉
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.
Yep. JWT on punch cards.
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.
LOL. Man I always make that typo. Cracks me up every time.