Skip to content
This repository has been archived by the owner on Oct 11, 2022. It is now read-only.

Commit

Permalink
Refactor middleware
Browse files Browse the repository at this point in the history
This is a follow-up of PR #32.

General:
- Refactor tests:
  - Namespace all tests into their separate packages (for better
    organization of the output), including a subspace for middleware
  - Rename `expect` function to `waitFor` in all calls to
    testAsyncMulti(), to better reflect its behavior
- Copy .jshintrc into new packages

json-routes:
- Export RestMiddleware object for middleware packages to have a
  namespace to declare themselves within
- Add a `JsonRoutes.middleware` to eventually replace
  `JsonRoutes.middleWare` (since 'middleware' is one word)
- Update change log
- Remove 100 character line limit in README

rest-accounts-bearer-token:
- Split rest-accounts-bearer-token middleware package into a middleware
  package that parses a standard bearer token (rest-bearer-token-parser)
  and one that validates an auth token and sets the request.userId to
  the authorized Meteor.user's ID (authenticate-meteor-user-by-token)
- Attach middleware to RestMiddleware namespace, instead of adding it to
  every route. This allows middleware to be made available by simply
  adding its package. It can later be added to the middleware stack with
  JsonRoutes.middleware.use('path', RestMiddleware.<middlewareFunction>).
- Move tests to their appropriate package

simple-rest:
- Add token and auth middleware in tests to pass auth tests
  - This could be a sign that auth tests may be better suited in the
    corresponding middleware packages

rest-accounts-password:
- Use the latest version of json-routes (1.0.3)
- Add token parsing and auth middleware to the stack (currently added to
  all routes, which is bad - needs fix)
  • Loading branch information
kahmali committed May 31, 2015
1 parent 9ae0732 commit 7951114
Show file tree
Hide file tree
Showing 22 changed files with 721 additions and 245 deletions.
25 changes: 25 additions & 0 deletions packages/authenticate-meteor-user-by-token/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#simple:authenticate-meteor-user-by-token

SimpleRest middleware for validating a Meteor.user's login token

## Middleware Name

This middleware can be accessed as:

**`RestMiddleware.authenticateMeteorUserByToken`**

### Request Properties Required

- `request.authToken`
- _String_
- A valid login token for a `Meteor.user` account (requires `accounts-base`)

### Request Properties Modified

- `request.userId`
- _String_
- If the `request.authToken` is found in a user account, sets this to the ID of the authenticated user. Otherwise, `null`.

## Usage

Simply add this layer of middleware after any token parsing middleware, and voila!

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Should include usage example here.

69 changes: 69 additions & 0 deletions packages/authenticate-meteor-user-by-token/auth.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/* global JsonRoutes:false - from simple:json-routes package */

/**
* SimpleRest middleware for validating a Meteor.user's login token
*
* This middleware must be processed after the request.token has been set to a
* valid login token for a Meteor.user account (from a separate layer of
* middleware). If authentication is successful, the request.userId will be set
* to the ID of the authenticated user.
*
* @middleware
*/
RestMiddleware.authenticateMeteorUserByToken = function (req, res, next) {
var userId = getUserIdFromAuthToken(req);
if (userId) {
req.userId = userId;
}
next();
};

// Validate the req.authToken and set the req.userId of the authorized Meteor.user

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

This comment is inaccurate - it doesn't set the userId, only returns it. Also, since the function is called get user id from auth token, it would be better if it took the auth token as an argument instead of the whole request object.

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

Good point. I'll update it with an accurate doc comment. I'll also pass in the token directly, as you suggested.

function getUserIdFromAuthToken (req) {
if (! _.has(Package, "accounts-base")) {
return null;
}

var token = req.authToken;
if (! token) {
return null;
}

// TODO: Check token expiration?

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

I guess these TODOs will get done before merging?

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

I had a question about this here: #40 (comment)

This was the comment, in case you just want to respond here:

In the hashToken function on line 45, a check is made to verify that the token is a String (and the value of token is also being set in middleware as a String via req.authToken), so why are we relying on a value at token.when?

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Oh that's a good point, haha - we need to get the value from the user document a few lines down actually.

But my question is also more generally about the TODOs - there seem to be a lot of them, are they mostly for the future or will they be resolved in this PR?

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

I've probably been overusing TODOs. Most of them I just intend to be discussion points within this PR, but some, like the setup/teardown suggestion within the tests, are for future discussion. It's mostly just that these things pop into my mind when I'm coding and I want to remember to discuss them. What I will try to do is just create GitHub Issues for the bigger discussion points, and then maybe once I commit I can try to remember to go back and leave links to any specific lines of code they reference later. That way there aren't any lingering TODOs. I just find the TODOs handy because my IDE (Webstorm) has a pane where I can view the TODOs throughout the project, and easily navigate to them. I use them pretty liberally in other projects I work on, so it's just a habit I've carried over. If they're causing any confusion though I will try to find a better forum for them (like GitHub Issues).

This comment has been minimized.

Copy link
@aldeed

aldeed Jun 1, 2015

Collaborator

I usually use TODO for reminder to do prior to merging as opposed to XXX for something to deal with or think about in the future.

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Also, let's not associate the TODOs with large amounts of commented-out code. In general, I would prefer to have no commented code in the actual devel branch if possible (except for documentation, etc).

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

I think we may need to just remove this check for token expiration for now. I checked out the source for the Accounts package and token.when is set to the time the token was generated (verified in some manual tests). So we can't use the value to determine expiration without some notion from the user of the length of time for expiration. Handling this probably falls outside of the scope of this PR, so I'm not sure if I should leave the code (which amounts to a no-op), or just completely remove it.

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Weird. Yeah, let's remove the code - I obviously screwed up when I added it.

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

Also, I'll resolve/remove all the TODOs as you've both suggested.

//var tokenExpires =
// Package["accounts-base"].Accounts._tokenExpiration(token.when);
//if (new Date() >= tokenExpires) {
// throw new Meteor.Error("token-expired", "Your login token has expired. " +
// "Please log in again.");
//}

var user = Meteor.users.findOne({
"services.resume.loginTokens.hashedToken": hashToken(token)
});

if (user) {
return user._id;
}
else {
return null;

This comment has been minimized.

Copy link
@aldeed

aldeed Jun 1, 2015

Collaborator

Wrapping 'return null;' in else {} is unnecessary.

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Yes, I also prefer avoiding else after return.

}
}

function hashToken (unhashedToken) {
check(unhashedToken, String);

// The Accounts._hashStampedToken function has a questionable API where
// it actually takes an object of which it only uses one property, so don't
// give it any more properties than it needs.
var hashStampedTokenArg = { token: unhashedToken };
var hashStampedTokenReturn = Package["accounts-base"]
.Accounts
._hashStampedToken(hashStampedTokenArg);
check(hashStampedTokenReturn, {
hashedToken: String
});

// Accounts._hashStampedToken also returns an object, get rid of it and just
// get the one property we want.
return hashStampedTokenReturn.hashedToken;
}

This comment has been minimized.

Copy link
@aldeed

aldeed Jun 1, 2015

Collaborator

Take a look here: https://github.com/meteor/meteor/blob/d8944f1b4d5a393a1292359d9a6ff09f147e53a4/packages/accounts-base/accounts_server.js#L768-L780

We could actually remove this whole hashToken function and instead call

Package["accounts-base"].Accounts._hashLoginToken(unhashedToken);

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Oh, that's a great catch. Can't believe I missed that.

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

Nice find @aldeed. I'm just wondering why we access the accounts-base package in this way? Won't this just end up throwing an error for trying to access Accounts if Package['accounts-base'] is undefined? Why not just have the dependency on accounts-base? I'm sure there's a reason, just trying to better understand the motivation.

Also, and this is just my poor understanding of the Accounts login tokens, but why exactly do we need to hash them? When are they unhashed? Aren't they stored and sent to the user hashed? I think I may have been doing something wrong in Restivus because the default login and auth were just passing around the hashed token. I just want to tighten up any security issues there if possible.

This comment has been minimized.

Copy link
@aldeed

aldeed Jun 1, 2015

Collaborator

I don't remember exact details or reasons but tokens were previously not hashed and then there was a Meteor release awhile back when they switched to being hashed.

It's a good point regarding Package['accounts-base']. Accessing that way is useful when it's a weak dependency, but since we have a separate package for this now, it seems better to have a strong dependency and access Accounts directly assuming it will be defined.

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Yeah, a direct dependency on accounts-base would make sense here.

44 changes: 44 additions & 0 deletions packages/authenticate-meteor-user-by-token/auth_tests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
if (Meteor.isServer) {
JsonRoutes.add("get", "accounts-auth-user", function (req, res) {
JsonRoutes.sendResult(res, 200, req.userId);
});
}

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Put end bracket and else on the same line?

This comment has been minimized.

Copy link
@aldeed

aldeed Jun 1, 2015

Collaborator

Agree

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

Will do. I'll make sure I follow that style from here on out.

else { // Meteor.isClient
var token;
var userId;

// TODO: Do all of this work in some sort of setup/teardown (maybe use munit?)

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

If you can get it to work properly we could switch to that mocha unit test package.

testAsyncMulti ("Middleware - Authenticate User By Token - set req.userId", [
function (test, waitFor) {
Meteor.call ("clearUsers", waitFor (function () {
}));
},
function (test, waitFor) {
HTTP.post ("/users/register", {

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

This test relies on being run with the login package, we should add that to package.js inside onTest. Otherwise this test will fail if run by itself.

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

But I would be OK ignoring that and saying you just need to run them together for now.

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

This will be important for passing the tests in Travis CI (I've got it all configured on a separate branch, but this is one of the reasons that I can't get all the tests to pass yet), so I'll try to address this issue in all of the test suites. I'll make sure each package's tests pass when run alone.

data: {
username: "test",
email: "test@test.com",
password: "test"
}
}, waitFor (function (err, res) {
test.equal (err, null);
test.isTrue (Match.test (res.data, {
id: String,
token: String,
tokenExpires: String
}));

token = res.data.token;
userId = res.data.id;
}));
},
function (test, waitFor) {
HTTP.get ("/accounts-auth-user", {
headers: {Authorization: "Bearer " + token}
}, waitFor (function (err, res) {
test.equal (err, null);
test.equal (res.data, userId);
}));
}
]);
}
25 changes: 25 additions & 0 deletions packages/authenticate-meteor-user-by-token/package.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
Package.describe({
name: 'simple:authenticate-meteor-user-by-token',

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Is the word meteor in the name redundant? What other kind of user could we be talking about?

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

I'll do anything I can to shorten that name. :) You're right though. Definitely seems redundant. I'll get rid of it.

version: '0.0.1',
// Brief, one-line summary of the package.
summary: 'Authorize user via auth token',

This comment has been minimized.

Copy link
@aldeed

aldeed Jun 1, 2015

Collaborator

Authorize -> Authenticate

// URL to the Git repository containing the source code for this package.
git: '',
// By default, Meteor will default to using README.md for documentation.
// To avoid submitting documentation, set this field to null.
documentation: 'README.md'
});

Package.onUse(function(api) {
api.versionsFrom('1.0');
api.use('simple:json-routes@1.0.3');
api.addFiles('auth.js', 'server');
});

Package.onTest(function(api) {
api.use('tinytest');
api.use('test-helpers');
api.use('simple:authenticate-meteor-user-by-token');
api.use('simple:json-routes@1.0.3');
api.addFiles('auth_tests.js');
});
34 changes: 12 additions & 22 deletions packages/json-routes/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@

<https://atmospherejs.com/simple/json-routes>

The simplest bare-bones way to define server-side JSON API endpoints, without
any extra functionality. Based on [connect-route].
The simplest bare-bones way to define server-side JSON API endpoints, without any extra functionality. Based on [connect-route].

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Please avoid reformatting large amounts of markdown in a PR called "refactor middleware"?

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

Sorry about that. I thought it would be okay to sneak this in here because it's just complying with our coding guidelines, but this commit is certainly large enough without this, so I can move this change to another commit if you want. I agree with trying to keep the PRs as focused as possible.

EDIT: I also recommend using the rich diff for viewing these markdown changes. Much easier to find the edits.

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

I understand that, I'd just rather have smaller PRs in general. You can do markdown reformatting directly in a commit onto devel rather than putting it into this PR.

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

I agree. Sorry for stuffing this in here. I'll take care of reformatting the markdown in a separate commit.


## Example

Expand All @@ -21,25 +20,17 @@ JsonRoutes.add("get", "/posts/:id", function (req, res, next) {

Add a server-side route that returns JSON.

- `method` - The HTTP method that this route should accept: `"get"`, `"post"`,
etc. See the full list [here][connect-route L4]. The method name is
case-insensitive, so `'get'` and `'GET'` are both acceptable.
- `path` - The path, possibly with parameters prefixed with a `:`. See the
example.
- `handler(request, response, next)` - A handler function for this route.
`request` is a Node request object, `response` is a Node response object,
`next` is a callback to call to let the next middleware handle this route. You
don't need to use this normally.
- `method` - The HTTP method that this route should accept: `"get"`, `"post"`, etc. See the full list [here][connect-route L4]. The method name is case-insensitive, so `'get'` and `'GET'` are both acceptable.
- `path` - The path, possibly with parameters prefixed with a `:`. See the example.
- `handler(request, response, next)` - A handler function for this route. `request` is a Node request object, `response` is a Node response object, `next` is a callback to call to let the next middleware handle this route. You don't need to use this normally.

### JsonRoutes.sendResult(response, code, data)

Return data fom a route.

- `response` - the Node response object you got as an argument to your handler
function.
- `response` - the Node response object you got as an argument to your handler function.
- `code` - the status code to send. `200` for OK, `500` for internal error, etc.
- `data` - the data you want to send back, will be sent as serialized JSON with
content type `application/json`.
- `data` - the data you want to send back, will be sent as serialized JSON with content type `application/json`.

### JsonRoutes.setResponseHeaders(headerObj)

Expand All @@ -52,10 +43,9 @@ Set the headers returned by `JsonRoutes.sendResult`. Default value is:
}
```

## Adding middlewares
## Adding middleware

If you want to insert connect middleware and ensure that it runs before your
REST route is hit, use `JsonRoutes.middleWare`.
If you want to insert connect middleware and ensure that it runs before your REST route is hit, use `JsonRoutes.middleWare`.

```js
JsonRoutes.middleWare.use(function (req, res, next) {
Expand All @@ -68,13 +58,13 @@ JsonRoutes.middleWare.use(function (req, res, next) {

#### Unreleased

Allow case-insensitive method names to be passed as the first param to
`JsonRoutes.add()` (e.g., `JsonRoutes.add('get',...)` and
`JsonRoutes.add('GET',...)` are both acceptable)
- Add `JsonRoutes.middleware` to eventually replace `JsonRoutes.middleWare` (since 'middleware' is one word)
- Export `RestMiddleware` to provide a namespace for all middleware packages to add their middleware functions (only available on the server)
- Allow case-insensitive method names to be passed as the first param to `JsonRoutes.add()` (e.g., `JsonRoutes.add('get',...)` and `JsonRoutes.add('GET',...)` are both acceptable)

#### 1.0.3

Add `JsonRoutes.middleWare`
Add `JsonRoutes.middleWare` for adding middleware to the stack

[connect-route]: https://github.com/baryshev/connect-route
[connect-route L4]: https://github.com/baryshev/connect-route/blob/06f92e07dc8e4690f7f788df39b37b5db4b06f90/lib/connect-route.js#L4
5 changes: 3 additions & 2 deletions packages/json-routes/json-routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ JsonRoutes = {};
WebApp.rawConnectHandlers.use(connect.bodyParser());
WebApp.rawConnectHandlers.use(connect.query());

JsonRoutes.middleWare = connect();
WebApp.rawConnectHandlers.use(JsonRoutes.middleWare);
// TODO: Deprecate `middleWare` spelling in favor of `middleware`, since it's one word
JsonRoutes.middleware = JsonRoutes.middleWare = connect();

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

@aldeed @stubailo Just wanted to point this out since I forgot to mention it in the commit message (I can fix that). Are we okay with this change? Every spelling of middleware I've come across has been a single word. I'm not sure what the exact process should be for deprecating things like this.

This comment has been minimized.

Copy link
@aldeed

aldeed Jun 1, 2015

Collaborator

Yes, I agree it should be middleware.

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

+1

WebApp.rawConnectHandlers.use(JsonRoutes.middleware);

// List of all defined JSON API endpoints
JsonRoutes.routes = [];
Expand Down
15 changes: 15 additions & 0 deletions packages/json-routes/middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* The namespace for all REST middleware
*
* Each middleware package should define a single middleware function and add it
* to this namespace.
*
* Usage (in a separate package):
*
* RestMiddleware.coolMiddleware = function (req, res, next) {
* // Do interesting middleware stuff here
* };
*
* TODO: Consider having sub-namespaces for each lifecycle method (e.g., Middleware.pre, Middleware.auth, etc)
*/
RestMiddleware = {};
18 changes: 10 additions & 8 deletions packages/json-routes/package.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,21 @@ Package.describe({
});

Npm.depends({
connect: "2.11.0",
"connect-route": "0.1.5"
connect: '2.11.0',
'connect-route': '0.1.5'
});

Package.onUse(function(api) {
api.export("JsonRoutes");
api.versionsFrom('1.0');
api.addFiles('json-routes.js', "server");

api.use([
"webapp",
"underscore"
], "server");
'webapp',
'underscore'
], 'server');
api.addFiles('json-routes.js', 'server');
api.addFiles('middleware.js', 'server');

api.export('JsonRoutes', 'server');
api.export('RestMiddleware');

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

I think this should only be exported on the server.

This comment has been minimized.

Copy link
@kahmali

kahmali Jun 1, 2015

Author Collaborator

Definitely. I'll fix this for now, but @aldeed suggested moving the middleware to JsonRoutes.middleware as opposed to RestMiddleware, which I think is a nice idea, so this may go away entirely.

This comment has been minimized.

Copy link
@stubailo

stubailo Jun 1, 2015

Owner

Yeah, that sounds great as well.

});

Package.onTest(function(api) {
Expand Down
5 changes: 0 additions & 5 deletions packages/rest-accounts-bearer-token/README.md

This file was deleted.

70 changes: 0 additions & 70 deletions packages/rest-accounts-bearer-token/accounts_bearer_token.js

This file was deleted.

Loading

0 comments on commit 7951114

Please sign in to comment.