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

Fix socket auth #459

Merged
merged 9 commits into from Mar 23, 2017

Conversation

Projects
None yet
2 participants
@marshallswain
Copy link
Member

marshallswain commented Mar 22, 2017

Closes #455, #448, and #432

This change channels socket auth through the token service instead of calling app.authenticate manually before hitting the token service.

This also makes it so the client has to provide a strategy explicitly, and removes the strategy iteration from passport/authenticate.js.

Also updates tests.

marshallswain added some commits Mar 21, 2017

Fix socketio authentication
Channel socket auth through the token service instead of calling app.authenticate manually.

This also makes it so the client has to provide a strategy explicitly, and removes the strategy iteration from `passport/authenticate.js`.

Also updates tests.
The login event isn’t getting emitted.
I’ve added a couple of debugger statements here and have set up the tests so that the failing test is the only one running.  For some reason the debuggers never get hit.  I have yet to find the cause.
params: {},
body,
provider: 'socketio',
// body: data,

This comment has been minimized.

@marshallswain

marshallswain Mar 22, 2017

Author Member

Needs to be removed?

@@ -441,7 +441,7 @@ describe('REST authentication', function () {
});

describe('when authentication succeeds', () => {
it('emits login event', done => {
it.only('emits login event', done => {

This comment has been minimized.

@marshallswain

marshallswain Mar 22, 2017

Author Member

This is the failing test. Well, one of two that are failing for what looks like the same cause.

@@ -11,6 +11,7 @@ export default function authenticate (strategy, options = {}) {

return function (req, res, next) {
// If we are already authenticated skip
debugger;

This comment has been minimized.

@marshallswain

marshallswain Mar 22, 2017

Author Member

This debugger never gets hit.

marshallswain added some commits Mar 23, 2017

@marshallswain marshallswain requested review from daffl and ekryski Mar 23, 2017

@marshallswain marshallswain self-assigned this Mar 23, 2017

@marshallswain marshallswain requested review from ekryski and removed request for ekryski Mar 23, 2017

@ekryski
Copy link
Member

ekryski left a comment

Pretty minor stuff. Just removing some commented out code and updating the strategyName assignment.

if (!strategy) {
return Promise.reject(new errors.GeneralError(`You must provide an authentication 'strategy'`));
}

// NOTE (EK): Bring this in when we decide to make the strategy required by the client

This comment has been minimized.

@ekryski

ekryski Mar 23, 2017

Member

We can remove this note. strategy is required now.

@@ -21,10 +21,21 @@ export default function authenticate (options = {}) {
// Allow you to set a location for the success payload.
// Default is hook.params.user, req.user and socket.user.
const entity = strategyOptions.entity || strategyOptions.assignProperty || options.entity;
let failures = [];
let strategies;
let strategyName = request.body && request.body.strategy;

This comment has been minimized.

@ekryski

ekryski Mar 23, 2017

Member

This should probably be:

request.body = request.body || {};
let stategyName = request.body.strategy;
const error = new Error(`Your '${strategy}' authentication strategy is not registered with passport.`);
return callback(normalizeError(error));
}
// if (!strategy) {

This comment has been minimized.

@ekryski

ekryski Mar 23, 2017

Member

This can be removed.

return Promise.resolve(result.data);
}
// if (!app.passport._strategy(strategy)) {
// const error = new Error(`Your '${strategy}' authentication strategy is not registered with passport.`);

This comment has been minimized.

@ekryski

ekryski Mar 23, 2017

Member

This can also be removed

// Now that we are authenticated create our JWT access token
const params = Object.assign({ authenticated: true }, result);
return service.create(result, params).then(tokens => {
// const promise = app.authenticate(strategy, strategyOptions)(socket._feathers)

This comment has been minimized.

@ekryski

ekryski Mar 23, 2017

Member

This can be removed

@marshallswain marshallswain merged commit 8f8ca52 into master Mar 23, 2017

4 checks passed

codeclimate no new or fixed issues
Details
codeclimate/coverage 84.24% (+2.2%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@marshallswain marshallswain deleted the fix-socket-auth branch Mar 23, 2017

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.