This repository has been archived by the owner on Mar 22, 2022. It is now read-only.
Fix socket auth #459
Merged
Fix socket auth #459
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
marshallswain
commented
Mar 22, 2017
src/socket/handler.js
Outdated
| params: {}, | ||
| body, | ||
| provider: 'socketio', | ||
| // body: data, |
There was a problem hiding this comment.
Needs to be removed?
marshallswain
commented
Mar 22, 2017
test/integration/rest.test.js
Outdated
| @@ -441,7 +441,7 @@ describe('REST authentication', function () { | |||
| }); | |||
|
|
|||
| describe('when authentication succeeds', () => { | |||
| it('emits login event', done => { | |||
| it.only('emits login event', done => { | |||
There was a problem hiding this comment.
This is the failing test. Well, one of two that are failing for what looks like the same cause.
marshallswain
commented
Mar 22, 2017
src/express/authenticate.js
Outdated
| @@ -11,6 +11,7 @@ export default function authenticate (strategy, options = {}) { | |||
|
|
|||
| return function (req, res, next) { | |||
| // If we are already authenticated skip | |||
| debugger; | |||
There was a problem hiding this comment.
This debugger never gets hit.
ekryski
suggested changes
Mar 23, 2017
src/hooks/authenticate.js
Outdated
| 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 |
There was a problem hiding this comment.
We can remove this note. strategy is required now.
src/passport/authenticate.js
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
This should probably be:
request.body = request.body || {};
let stategyName = request.body.strategy;
src/socket/handler.js
Outdated
| const error = new Error(`Your '${strategy}' authentication strategy is not registered with passport.`); | ||
| return callback(normalizeError(error)); | ||
| } | ||
| // if (!strategy) { |
src/socket/handler.js
Outdated
| return Promise.resolve(result.data); | ||
| } | ||
| // if (!app.passport._strategy(strategy)) { | ||
| // const error = new Error(`Your '${strategy}' authentication strategy is not registered with passport.`); |
src/socket/handler.js
Outdated
| // 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) |
ekryski
approved these changes
Mar 23, 2017
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.