First cut for authentication middleware #305

daffl · 2016-10-09T06:58:41Z

All original tests are still passing. Update of the existing codebase will happen in another PR.

ekryski

Most of my comments are simply questions. The only one thing is I think a quick fix where it should be verifyJWT?

ekryski · 2016-10-09T22:11:02Z

src/options.js

+  token: {
+    name: 'token', // optional
+    service: '/auth/token', // optional string or Service
+    subject: 'auth', // optional


Good call moving this into a separate file and setting up jwt namespace. I guess we'll de-dupe these options later?

Yep, we'll clean it up later. De-duping and the local and token sections might change as well.

ekryski · 2016-10-09T22:11:56Z

src/services/authentication.js

+  remove(id, params) {
+    const token = id !== null ? id : params.token;
+
+    return this.authentication.verify({ token });


Doesn't this need to be verifyToken?

indeed, verifyJWT it is.

ekryski · 2016-10-09T22:13:37Z

src/token/from-request.js

+      debug('Parsing token from request');
+
+      // Normalize header capitalization the same way Node.js does
+      let token = req.headers && req.headers[header.toLowerCase()];


Do we want to support body and query string as well or just header? If it's not going to cause us problems maybe we should (at least body). I personally use querystring a lot in dev mode because I'm lazy but really we shouldn't support that officially.

How is body being used? I couldn't think of a case where it made much sense (even might cause issues with token being a legitimate property when calling create, update and patch).

Do you have use cases where query string is easier than setting the header? We can add it back in but I don't think it was documented so far with more than a brief mention. Removing it would be less to document and test and we don't risk promoting not a best practise.

I would vote to nix both and only potentially add support for body if it is requested after release.

I agree actually. You're totally right @daffl we shouldn't support body. Querystring might still be a thing actually if we are supporting short lived JWTs for things like pw reset however, let's just go with this for now. I'm going to tinker with something regarding the password reset tokens, possibly by only allowing querystring verification if the token is an authentication subject.

ekryski · 2016-10-09T22:16:20Z

src/token/populate-user.js

+  }
+
+  return function populateUser(data) {
+    const service = app.service(user.service);


Possibly we want to support passing an actual service as well, instead of just the path name.

ekryski · 2016-10-11T02:58:53Z

* First cut for authentication middleware * Fix service methods * Allow passing user service and service name

* upstream/0.8: (52 commits) Cookies will match jwt expiry by default. (feathersjs-ecosystem#308) First cut for authentication middleware (feathersjs-ecosystem#305) First cut for authentication middleware (feathersjs-ecosystem#305) OAuth require successRedirect with default successHandler DRY up the dynamic token and user service lookup. Always use service lookup. Normalize comparison URL & fix typo consistency: `callbackUrl` should be `callbackURL` Make sure the provider plugin name doesn't overwrite the OAuth provider name normalize the callbackURL Fix typo and simplify wording. fix typo missing 'd' adding more details to migration guide bumping version finished consolidating options updating all middleware to not have default and pull from global config getting all tests passing cleaning up client side tests. Still failing fixing a bunch of the tests and adding tests for all new middleware updating migration doc of things left to doc/complete ... # Conflicts: # package.json

* cleaning up dependencies * removing auth redirects and exposing middleware * expanding express middleware * moving what I can in services to setup method * updating dependencies * cleaning up middleware and adding debug logs * cleaning up services and adding debug logs * changing options for populate user hook to conform with other options * cleaning up main index file * fixing lint errors * getting example app working * fixing options for populate user middlware * fixing socket logout emitting * restructuring so you can set hooks to construct your token payload if you want to customize it * Default to a session cookie instead of 1 day * Switch to "user" instead of "data" for the response from auth * Make sure we clear the user out of locals so that you don't end up in a weird state. * Allow passing options when creating a JWT. Used to customize the type of token we want to generate (ie. confirmation, password reset, etc.) * setting version * don't throw an error in the decode token middleware * bump version * clearing cookie if use not found. Setting cookie age to same as JWT * bump version * Don’t mix options when signing tokens (#255) Fixes #254. * Attempt to get token right away. (#252) * Attempt to get token right away. This makes it so that we don’t have to wait for an async response in order to start making authenticated requests. * Also set up localStorage. * fix restrict to owner hook methods. Closes #228 * bump version * cookies should get set regardless of whether it was an xhr request if enabled * bumping version * adding migration guide * reorganizing middleware, hooks and services * updaing mocha * updating migration doc of things left to doc/complete * fixing a bunch of the tests and adding tests for all new middleware * cleaning up client side tests. Still failing * getting all tests passing * updating all middleware to not have default and pull from global config * finished consolidating options * bumping version * adding more details to migration guide * fix typo missing 'd' * Fix typo and simplify wording. * normalize the callbackURL If the callbackURL doesn’t begin with a slash, the Passport `authenticate()` call will mess up the URL depending on the referrer’s url. So if the page is `domain.com/auth/github`, the callback URL will become `domain.com/auth/auth/github`. This normalizes the URL so that relative URLs always begin with a leading slash and absolute URLs don’t get touched. * Make sure the provider plugin name doesn't overwrite the OAuth provider name * consistency: `callbackUrl` should be `callbackURL` * Normalize comparison URL & fix typo This performs the same normalization to the comparison URL for the callback (as is done for the callbackURL). Also fixes typo in callbackUrl, updates it to callbackURL. * Always use service lookup. Previously, we were setting up this._tokenService and this._userService in the `setup` function. This was a problem because not all services are registered by the time `setup` runs. In order to continue to allow the passing of either strings or an actual service object, this PR checks if we only have a string reference to the service stored. If so, it uses the `app.service` method of service lookup before attempting to use the service. This also fixes a problem that was occurring when trying to call this._userService or this._tokenService when this was undefined inside the middleware callbacks. * DRY up the dynamic token and user service lookup. This moves the dynamic service lookup to a function to be a bit more DRY. * OAuth require successRedirect with default successHandler If a successHandler hasn’t been passed, then the default `successHandler` will be used. The `successRedirect` will now be required in that scenario. * First cut for authentication middleware (#305) * First cut for authentication middleware * Fix service methods * Allow passing user service and service name * First cut for authentication middleware (#305) * First cut for authentication middleware * Fix service methods * Allow passing user service and service name * Cookies will match jwt expiry by default. (#308) * Cookies will match jwt expiry by default. * Add missing makeExpiry function. * Store config at `app.config` (#312) * Store config at `app.config` * Use app.set to store config. * Add test. * adding instanbul code coverage * Remove permissions hooks and middleware which will be put into feathers-permissions (#307) * Started implementation of more modularized module structure * Some reorganization * Implement Socket new authentication * More reorganization and start of integration tests * eslint fix * More integration tests and cleanup * reogranizing * Applying latest changes and merging with dev other branch * Socket.io authentication tests and login logout event * Improving socket tests and adding Primus * Some cleanup * Better error verification tests * Implement login and logout events for REST authentication (#325) * Fix tests * wip * first cut of auth working with passport. Clean up and tests to do * fixing event middleware resolution * Keep github together * Keep twitter together * getting tests passing. Still a couple more to do * removing unused hooks for now. May bring some back later * fixing lint errors * removing hashPassword hook. It now lives in feathers-authentication-local * adding a migration guide and new features docs * adding more detail to migration doc * cleaning up dependencies * getting tests passing again * adding some more tests. Implementing chained strategies * cleaning up dependencies * finishing integration tests and handling socket logout timeout * cleaning up example app * fixing up example * updating README * updating API docs

First cut for authentication middleware

612b8cb

ekryski suggested changes Oct 9, 2016

View reviewed changes

daffl added 2 commits October 10, 2016 10:43

Fix service methods

36c3200

Allow passing user service and service name

d3ad867

ekryski merged commit 6f0308f into 0.8 Oct 11, 2016

ekryski deleted the auth-middleware branch October 11, 2016 02:59

marshallswain pushed a commit that referenced this pull request Oct 11, 2016

First cut for authentication middleware (#305)

5d9cbc5

* First cut for authentication middleware * Fix service methods * Allow passing user service and service name

marshallswain pushed a commit that referenced this pull request Oct 11, 2016

First cut for authentication middleware (#305)

b4caf91

* First cut for authentication middleware * Fix service methods * Allow passing user service and service name

marshallswain pushed a commit that referenced this pull request Oct 11, 2016

First cut for authentication middleware (#305)

80120a2

* First cut for authentication middleware * Fix service methods * Allow passing user service and service name

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First cut for authentication middleware #305

First cut for authentication middleware #305

daffl commented Oct 9, 2016

ekryski left a comment

ekryski Oct 9, 2016

daffl Oct 9, 2016

ekryski Oct 9, 2016

daffl Oct 9, 2016

ekryski Oct 9, 2016 •

edited

Loading

daffl Oct 9, 2016

marshallswain Oct 10, 2016

ekryski Oct 11, 2016

ekryski Oct 9, 2016

ekryski commented Oct 11, 2016

First cut for authentication middleware #305

First cut for authentication middleware #305

Conversation

daffl commented Oct 9, 2016

ekryski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekryski Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekryski commented Oct 11, 2016

ekryski Oct 9, 2016 •

edited

Loading