Skip to content
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

0.7 Release #139

Merged
merged 29 commits into from Mar 30, 2016

Conversation

Projects
None yet
3 participants
@ekryski
Copy link
Member

ekryski commented Mar 30, 2016

This PR has a bunch more bug fixes and some pretty critical security fixes

  • Lock down cookie (#132)
  • Add unit tests for cookie options
  • can now use default redirect routes with a custom handler (#121)
  • Add middleware tests for successfulLogin
  • Add middleware tests for failedLogin
  • Prevent emitting auth service events (#126)
  • Add tests to make sure auth service events are not fired
  • RestrictToOwner hook needs to throw an error (#128)
  • RestrictToRoles hook needs to throw an error (#127)
  • OAuth user profile should be updated (#124)
  • All hooks should support internal usage passthrough (#138)
  • Clear cookie on logout (#122)
  • Update docs, examples and generator
  • de-auth socket on logout (#136)
  • get logout integration tests working
  • Move to bcryptjs instead of native brcrypt
  • Removes ability to authenticate with the cookie that is used to transmit the JWT to the client

const defaults = {
cookie: 'fathers-jwt',

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

It's like your dad's JWT 😉

This comment has been minimized.

@marshallswain

marshallswain Mar 30, 2016

Member

Yep. JWT on punch cards.

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

LOL. Man I always make that typo. Cracks me up every time.

reject(new Error('Could not logout over socket'));
}, 10000);

socket.once('logged out', function() {

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

Couldn't this just be the logout event?

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

no, but we could use a callback instead of two events.

// 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;`;

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

Is this a standard default?

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

It just needs to be set in the past, so it can be whatever.

// }
// 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 => {

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

Do we need to add params here? this.get(hook.id, hook.params)?

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

Probably ya. Good catch! Going to have the same in restrictToRoles

successRedirect: '/auth/success',
failureRedirect: '/auth/failure',
tokenEndpoint: '/auth/token',
localEndpoint: '/auth/local',
userEndpoint: '/users',
header: 'authorization',
cookie: 'feathers-jwt'
cookie: {
enabled: true,

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

Can this just be considered disabled when someone set cookie: false?

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

I think we can probably do that.

@@ -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) {

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

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?

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

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 successRedirect is different than the default.

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

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)?

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

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 /auth/success, grab the JWT from the cookie, and close the pop-up window or the webview on mobile.

@@ -183,6 +186,20 @@ export let setupSocketIOAuthentication = function(app, options = {}) {
}).catch(errorHandler);
}
});

socket.on('logout', function() {

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

Ah ok I see why there is two events. But you can also send a callback from the client socket.on('logout', callback => callback()) so you don't need two different events (that's what we're doing for calling service methods.

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

Ha! I had started doing it that way but then I switched to be consistent with the authenticate and authenticated events. I think callbacks is probably a better idea.

@@ -111,6 +112,11 @@ export default function(options){
// Get our initialize service to that we can bind hooks
const localService = app.service(options.localEndpoint);

// prevent regular service events from being dispatched
if (app.io || app.primus) {

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

I'd probably go with if(typeof localService.filter === 'function'). We should probably also add warnings if someone tries to override those methods.

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

What were you thinking of for a warning?

if (typeof tokenService.filter === 'function') {
      tokenService.filter(() => false);
    } else if (app.io || app.primus) {
      throw new Error('You cannot override the `filter` function');
    }

How can we detect whether the filter event behaves the way it should other than testing to make sure that events don't get dispatched?

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

Nothing in here, it'll be added for socket-commons and feathers-hooks.

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

👍

@@ -179,6 +182,11 @@ export default function(options){
// Get our initialized service
const service = app.service(options.endPoint);

// prevent regular service events from being dispatched

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

Actually this can probably even go in the services setup

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

I had to think about this for a second but I think you are right. It should go in there because then someone can override setup if they really need that functionality for whatever reason. We might come back to this when we address scaling services.

@@ -119,10 +119,17 @@ const setupTests = initApp => {
expect(response.data).to.not.equal(undefined);

app.logout().then(() => {
expect(app.get('token')).to.equal(null);
expect(app.get('user')).to.equal(null);

app.service('messages').create({ text: 'auth test message' })
.then(msg => console.log('!!!', msg))

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

console.log

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

That looks like I left it in there ;)

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

Ya. It never gets there now but I'll remove it.

expect(response.statusCode).to.equal(401);
done();
});
});

it('returns error instead of data', (done) => {
request(options, function(err, response, body) {
it('returns erroror instead of data', (done) => {

This comment has been minimized.

@daffl

daffl Mar 30, 2016

Member

Global replace?

This comment has been minimized.

@ekryski

ekryski Mar 30, 2016

Author Member

yes thank you!

@ekryski

This comment has been minimized.

Copy link
Member Author

ekryski commented Mar 30, 2016

Ok this is good to go! Just need to merge some docs PRs and update the generator to use the restrictToAuthenticated hook.

@ekryski

This comment has been minimized.

Copy link
Member Author

ekryski commented Mar 30, 2016

This isn't actually broken it just depends on feathersjs-ecosystem/feathers-hooks#64.

@ekryski ekryski referenced this pull request Mar 30, 2016

Merged

V0.7 #99

@ekryski ekryski merged commit 62c3005 into master Mar 30, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ekryski ekryski deleted the 0.7 branch Mar 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.