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

Decoupling #49

Merged
merged 71 commits into from Feb 12, 2016

Conversation

Projects
None yet
3 participants
@ekryski
Copy link
Member

ekryski commented Feb 12, 2016

This brings in a pretty major overhaul to feathers authentication. With it comes:

  • Migrating existing code to use services

  • Standardizing on a hook spec:

    export default function(options = {}){
      const defaults = {passwordField: 'password'};
      options = Object.assign({}, defaults, options);
    
      return function(hook) {
        // do stuff
      };
    }
  • Adds support for authenticating with socketio and primus (#32)

  • Only signs the JWT with user id (#38)

  • Locks down socket authentication (#33)

  • Continues the work @marshallswain did on handling expired tokens (#25)

  • Adds a bunch more tests.

  • Adds support for OAuth2 (#43)

  • Adds a client side component for easy authentication with Feathers (#44)

  • Adds preliminary support for graceful fallback to cookies for JWT (#45)

  • Adds an example project showing all the different ways you can authenticate

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented on README.md in 35f051b Jan 28, 2016

Oops! Accidental drug reference or intentional subliminal message?

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented on 35f051b Jan 28, 2016

Looks so good! Super excited!

ekryski added some commits Jan 29, 2016

throw new Error(`Unsupported authentication 'type': ${options.type}`);
}

// return new Promise(function(resolve, reject) {

This comment has been minimized.

@daffl

daffl Feb 12, 2016

Member

Does this commented out code need to kept for reference?

This comment has been minimized.

@ekryski

ekryski Feb 12, 2016

Author Member

@daffl No we can get rid of it unless you want to take a crack at it.

This comment has been minimized.

@daffl

daffl Feb 12, 2016

Member

I actually didn't read all of it ;) What would it have to do?

This comment has been minimized.

@ekryski

ekryski Feb 12, 2016

Author Member

That's the promisfying we were trying to do the other night so that a developer doesn't have to call app.io.on('connect', cb) before they can authenticate. We would handle that for them. I couldn't get it to work. Too many promises and event listeners.


// Set up hook that adds adds token to data sent to server over sockets
app.mixins.push(function(service) {
service.before(hooks.populateParams());

This comment has been minimized.

@daffl

daffl Feb 12, 2016

Member

It looks like this needs hooks configured in the app for the authentication plugin to work at all. Should we throw an error if service.before and service.after isn't a function?

This comment has been minimized.

@ekryski

ekryski Feb 12, 2016

Author Member

Ya I just had that thought a couple minutes ago.

const token = hook.params.token;

if (!token) {
return Promise.resolve(hook);

This comment has been minimized.

@daffl

daffl Feb 12, 2016

Member

Technically you could just return hook;

This comment has been minimized.

@ekryski

ekryski Feb 12, 2016

Author Member

true. I still have tests to write for these hooks so I'll do that at the same time.

})
.then(user => {
// Check password
bcrypt.compare(password, user[this.options.passwordField], function(error, result) {

This comment has been minimized.

@daffl

daffl Feb 12, 2016

Member

I'm wondering if we should move promisify from https://github.com/feathersjs/feathers-socket-commons/blob/master/src/utils.js#L17 into feathers-commons and use this here. It would turn this section from 12 LOC into 2.

This comment has been minimized.

@ekryski

ekryski Feb 12, 2016

Author Member

Yup. We could do that. I'm going to iterate on this repo a bit more over the next few days so I don't think we should delay on merging but if we get that util in commons then I'll use it here. It would actually come in handy in a few spots.

This comment has been minimized.

@daffl

daffl Feb 12, 2016

Member

I was going to add to the comment that it's not super urgent but would be a nice refactoring just to make things a little more terse.

return app.service(options.userEndpoint).create(data, { internal: true }).then(user => {
return done(null, user);
}).catch(done);
}).catch(done);

This comment has been minimized.

@daffl

daffl Feb 12, 2016

Member

From 60 to here it can probably just be changed to

return app.service(options.userEndpoint).create(data, { internal: true })).then(user => done(user)).catch(done);

This comment has been minimized.

@ekryski

ekryski Feb 12, 2016

Author Member

I don't think so. Passport expects the first param to done to be an error or null.


return new Promise(function(resolve, reject){

let middleware = passport.authenticate(options.provider, authOptions, function(error, user) {

This comment has been minimized.

@daffl

daffl Feb 12, 2016

Member

This is where promisify would come in handy, too. Not super urgent but might make things easier to follow and less callback-y

This comment has been minimized.

@ekryski

ekryski Feb 12, 2016

Author Member

Ya totally.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Feb 12, 2016

Looks like it needs to be merge with master which is hopefully not too much of a disaster. Besides that, let's merge it, make a 0.2.0 release and iterate from there.

@ekryski

This comment has been minimized.

Copy link
Member Author

ekryski commented Feb 12, 2016

@daffl I had already did that and resolved conflicts. It's good there.

All the tests pass locally all the time. I dunno why that one test keeps timing out randomly. I can try setting the mocha timeout higher to see if that works.

@ekryski

This comment has been minimized.

Copy link
Member Author

ekryski commented Feb 12, 2016

never mind. The latest build did pass on all three platforms now that I added the timeout in the test. I think for some reason the tests were running fast enough that the expiredToken wasn't actually expired yet.

@ekryski

This comment has been minimized.

Copy link
Member Author

ekryski commented Feb 12, 2016

This PR. Closes #25, #32, #33, #38, #43, #44, #45.

ekryski added a commit that referenced this pull request Feb 12, 2016

@ekryski ekryski merged commit ebae315 into master Feb 12, 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 decoupling branch Feb 12, 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.