Refactor project

[nunofgs]

This PR completely restructures the project into a more modern Promise-based approach. Among many many fixes, it also adds support for scopes, changes the license to MIT, validates user input according to the OAuth spec, removes express-specific middlewares, improves error handling and is fully tested.

I know this is a monumental PR and is quite a big shift in style and approach, it is my opinion that it greatly enhances this library and further establishes it as the de-facto OAuth server library for node.

I'd love to know what you think.

OAuth2-Server

Introduces a new OAuth server implementation with the following changes:

• Added a promise-based API which supports both promises and node-style callbacks.
• Added support for scopes for all grant types.
• Added syntax-checking to all user input as defined in the spec.
• Added OAuth client validation to authorization requests which fixes an edge-case where an auth code would be generated even if the OAuth client did not support authorization codes.
• Fixed authorization requests now defer to the authentication middleware in order to verify that the request is authenticated (in the process, multiple bugs were fixed since the original library was doing this in place and consequently skipping many important checks).
• Fixed incorrect (non-spec compliant) validation of the redirectUri in the authorization_code grant.
• Fixed incorrect handling of custom grant types (the spec requires that extended grants be specified in the form of a uri).
• Fixed non-spec compliant behavior when validating unsupported grant types.
• Fixed non-spec compliant errors being returned to the response.
• Fixed objects retrieved from the model are no longer injected into the current execution context and are instead returned in the original promise.
• Fixed parameters given to model calls are now objects previously fetched from the model which reduces the number of database queries per OAuth request.
• Fixed tokens are no longer allowed to be passed by GET parameter as recommended by the OAuth spec, for security reasons.
• Improved authorization_code grant by allowing the redirect_uri parameter to be omitted (as required by the spec).
• Improved authorization_code grant type by revoking the authorization code immediately upon issuing an access token (as required by the spec).
• Improved refresh_token grant type by revoking the previous refresh_token immediately upon issuing an access token (as required by the spec).
• Improved architecture by separating the grant-types, response-types and token-types into strongly typed classes.
• Improved error handling by throwing strongly-typed errors so they can be caught in our backend.
• Improved error handling by wrapping model errors in a server_error, as required by the spec.
• Improved validation of all objects retrieved from the model (no validation was being done on expiry dates, for example).
• Removed dependency on the express framework.
• Removed injection of response body and headers in favor of strongly-typed Request and Response classes.
• Renamed authorise, grant and authCodeGrant middlewares to more OAuth spec-friendly authenticate, token and authorize, respectively.

[jdjkelly]

This is an epic PR.

[thomseddon]

This is an epic PR :)

I plan to have a good look through this is in a week but it really does look good - I think I'm definitely open to taking this route...

[dmyers]

👍

[dmyers]

Isn't this fork being merged into core soon?

[nunofgs]

Yep, looks like the first version will show up in September. I'll update the .travis.yml as soon as a version is available.

[nunofgs]

Node 4.0.0 was released today. I've updated the PR to reflect this change in the .travis.yml file.

[nunofgs]

@thomseddon Thanks for taking a look and sorry about the size of the PR 😄

FYI, we've been using this fork in a few projects and iterating on it for a while. There haven't been any issues in months.

[RobertHerhold]

@nunofgs
Please re-add the license

[nunofgs]

@RobertHerhold I updated the license to MIT in the course of my refactor. This was done in part due to my suggestion in https://github.com/thomseddon/node-oauth2-server/issues/159.

I'll happily change it back to Apache if everyone prefers it, but I really dislike adding these 15-ish lines of license information at the top of each file.

@thomseddon: how do you feel about switching to MIT as part of this refactor?

[OllieJennings]

👍

[dmyers]

@thomseddon Do you think you'll have time soon? The scopes alone would do wonders for me on my projects.

[fb13]

Does this solve https://github.com/thomseddon/node-oauth2-server/issues/192?

[ruimarinho]

@fb13 it should, since no logging has been added on the refactor. @nunofgs, what about adding debugnyan?

[mjsalinger]

I'm 👎 on a promises-based approach personally - I tend to avoid using promises myself (I tend to use more of an async.js pattern, and am exploring ES6 Generators at the moment)... While promises work well for some people, they don't for everyone.

[ruimarinho]

@mjsalinger the API is fully callback-compliant. You can choose either method for your projects. Also, async and await will be based on Promises on ES2016, so basically you get compatibility with these new features for free (you can also yield a promise).

[frapontillo]

Is this going to be eventually merged in? I have read about the release of a new version, but I can't understand the timing nor the actual level of commitment. I really like this library and would love to see it improve!

[nunofgs]

@mjsalinger while I personally prefer Promises I did indeed consider that many users prefer the "node callback" style so I added support for both.

The way the support was implemented, as @ruimarinho stated, handles Promises, generators, async/await and node-style callbacks.

You can check out the tests supporting both styles in the server_test.js file.

[night]

I'd like to see this merged too. Is @thomseddon mia?

@nunofgs You do need to add examples + update README with the new methods and model requirements (scopes model methods).

[nunofgs]

@night: I moved the existing examples to a similar PR on koa-oauth-server#31. My reasoning was that the examples were all targeted for the express framework and this PR removes all dependencies on a specific framework.

However, I do agree that there should be a few framework-agnostic examples in this PR. I'll add them asap.

This is also a good time to mention that I have not created an express-specific module as part of this PR. In my opinion, that module should be named express-oauth-server and would be very similar in implementation to the Koa module.

[yankeeinlondon]

@nunofgs love the changes you're bringing to this and am about to jump into an an server implementation; @thomseddon are you still maintaining this? What are your feelings on this PR?

I'm not sure what proper etiquette is but as the last commit was in June should we set a timeframe by which this is forked? Don't want to do the wrong thing but just not sure the protocol. The PR definitely seems in-line with the way JS is moving and I think most people will see these changes as appropriate evolution from a very solid starting point.

[thomseddon]

@ksnyde You make a good point, I've had a bit of a catch 22 as I really like this pull request but I just don't have the time to give it the reviewing it needs (we started a national ISP in the last 12 months https://telcom.io/) - there's some good 👍 on this thread but I think it needs more review - is there really not a single line that could be improved? I'm not saying it needs to be changed - I just don't think anybody else has said that it doesn't!

This module is neglected, here's what I propose:

• If possible, please can somebody chip in to actually review this code, it deserves more than "looks good"
• I've just given @nunofgs write access to the repo, when you feel this has had adequate review please feel free to merge. I know you've worked with others on this ( @ruimarinho ) so if you both agree this has had enough review LGTM

Absolutely HATE being the bottle neck, so anyone else who want's to get involved just ping me and I will add you as a collaborator

[yankeeinlondon]

Awesome, thanks for the update @thomseddon, sounds like some sage advice. Oh and good luck with the telecom.

On the off-chance you know her ... do you know Dana Pressman (CEO/co-founder of Be Unlimited and now Hyperoptic)? She's a good friend but anyway your offers look pretty interesting. Do you focus on low-latency as well?

[thomseddon]

@ksnyde We certainly know of her :) They're the biggest in the UK for Gb residential...ours is slightly different as we do fibre to the home

Look forward to seeing some feedback on the PR

[yankeeinlondon]

Yeah i noticed that. I want 1gb to do the door for bragging rights alone. As for feedback, I will definitely do that once I'm able to get some QT on this area (pull in all directions at the moment).

[mjsalinger]

I should have spare cycles to review this PR this week or next (depending on some work stuff) - I can also put in some time reviewing some of the other PRs that could go into a 3.0.0 release.

Since 3.0.0 is a major refactoring and may also contain a bunch of features that have been requested for some time, I propose that a target date of 10/31 be set for release, with the possibility of release candidates for this release if needed. We can tag specific PRs/issues for resolution for 3.0 and collectively work towards that date. Thoughts? @thomseddon are you OK with this approach? I don't mind taking point on it while you're busy with your day job..

[mjsalinger]

Why is the license file being removed? Many companies requires the presence of a specific license file in order to use OSS software.

[thomseddon]

Agreed - happy to switch to MIT but lets include a copy for clarity

[nunofgs]

Got it. Will add the MIT license to the LICENSE file.

[thomseddon]

@mjsalinger suits me :)

[djMax]

fork fork fork... This is too long for a PR.

[arcreative]

Definitely enough for a major version at the very least, but I have some serious concerns about its stability. Wasn't able to get it working despite copious effort...

[djMax]

I started with getAccessToken() and just returned {}, and now I get "{"error":"invalid_argument","error_description":"Invalid argument: response must be an instance of Response"}" to an authorized request and nothing on the server console output. Ideas?

[djMax]

Line 60 of authenticate-handler.js - why is that forcing response to be Response when the express middleware isn't even passing a response to authenticate?

[lfk]

@arcreative and @djMax, just thought I'd chip in and say that I got every supported grant method working on this version, so it can be done. I believe @nunofgs' express middleware is out of date though, which might complicate matters for you. The koa middleware I wrote (link) works fine for me, with the following caveats (off the top of my head, might be missing something):

• Removed the expiration timestamp check on revoking tokens as discussed here (Edit: You could also simply update the timestamp in the returned object to any past date)
• expires_in is not included in token payloads, so I'm defaulting it in a hacky way
• Workaround for session-based login for auth code grant flow (ref) -- working as intended, but it was undocumented and took me quite a while to straighten out, see conversation

In addition to the above, some issues such as those pointed out and (even resolved) by @night (scopes not being inherited properly during renewal, etc.) might still be pending, I'm not up to speed on the latest there yet.

Imo the tweaks required to fix the above is fairly trivial, and I intend to provide my own PR to @nunofgs' branch next week, and then add some examples to my koa middleware. Really, this is painfully close to being complete, and it's a shame that the last 5% is taking 500% of the time (The Mythical Man Month strikes again).

[jeffijoe]

Is this PR ever coming to an npm near me? 😃

Currently in need of support for scopes, and it does not seem to be possible without this PR.

[djMax]

Is there a way to share context across model calls in the context of a given request? Should there be? Seems like it would be useful in cases where, for example, you want the app id while authenticating the user.

[mjsalinger]

I think this PR has gotten sufficiently long and sufficiently baked to be merged into master, and it's also the general consensus, so I'm going to do so - since @thomseddon gave the OK much earlier in the thread. Since 2.x is effectively dead, I don't see a problem with this at this point.

It still needs a little work and some testing before a 'release', so I'm going to submit issues tagged with 3.0 based on the remaining pull request comments. From there, anyone else who wants to help out by submitting issues/new PRs/etc. should do so.

@thomseddon If you don't agree with this approach, let me know and I'll back out the changes.

Let's let this bake another week or two with fixes from the community and from @nunofgs and myself, and then we'll release a release candidate.

[engelgabriel]

AWESOME MERGE!

[mjsalinger]

@lfk @night @djMax If you have any tweaks that you think would be worthwhile, please submit them as PRs against master so we can evaluate, review, etc.

[ruimarinho]

I think we should release this on npm with the next tag so the community can start experimenting while not breaking current npm install oauth2-server.

[nunofgs]

@mjsalinger: thank you. I've added 3 wiki pages and I'll submit a PR asap with the missing examples in this repo.

We do need to emphasize that most users should really be using https://github.com/seegno/express-oauth-server (the examples in this server are only relevant if you're writing an oauth implementation on a custom web framework).

[lfk]

@mjsalinger: At long last, thank you! Seonding what @ruimarinho said, would be nice to have this as next via npm.

[thomseddon]

So you know I'm all 👍 for this

I've added @mjsalinger @nunofgs and @ruimarinho as collaborators on the oauth2-server npm project.

I agree that this can be pushed as "next" as it stands, once the release is ready I will depreciate the https://www.npmjs.com/package/node-oauth2-server namespace too

@nunofgs Wiki docs look really good

[mjsalinger]

@thomseddon @nunofgs Do we want to put express-oauth-server and oauth-sever into a common GitHub org (#162)

[night]

I would be in favor of that at this point. I think this is more of a community ran project now, so it might be good to separate it.

Great work on the PR though!

[mjsalinger]

@thomseddon After a slight npm error (sorry) master has been tagged as next in npm.

[mjsalinger]

FYI I'll also be hanging out on the Freenode #node-oauth channel

[engelgabriel]

Now we can go ahead with RocketChat/rocketchat-oauth2-server#2