NilsG-S
commented
7 years ago @asclines This branch is done. I don't know what your schedule looks like, so just let me know if you're too busy to review it. If that is the case, I hope you don't mind if I merge the PR unilaterally. Also, I included silencing for tests, if you want to close #26.
P.S. 11,000 lines of code are from the package-lock.json
This PR is more about discussion than making changes. I'm not finished yet, but my code can serve as an example.
I should note that I originally intended this PR to be a simple implementation of password and email change functionality. However, over the course of researching our current system, I found several indications that it might not be optimal.
Definition of Terms:
Topics:
This PR is primarily about JWTs vs sessions. The current authentication system is a hybrid of JWTs and sessions. If I recall correctly, this was to support session revocation. My issue with the hybrid system is that it invalidates the reason to use JWTs in the first place. JWTs allow for a stateless authentication system, but come with serious drawbacks like an inability to revoke sessions natively. As far as I know, methods to add support for revocation fall under the following categories: blacklisting, refresh tokens, changing the signing key, short token expiration times, and user-targeted invalidation. The first two eventually require storing something in the database, which becomes a roll-your-own session system. Changing the signing key would invalidate everyone's sessions. Short token expiration times don't really help a lot, because an attacker will typically get all of their mischief done within minutes of breaking into a system. Needless to say, invalidating the token every minute would not make for a good user experience. The last typically involves storing a date in each user object that acts as a cutoff for valid tokens. In case of a compromised token, this date is set to the time of revocation, invalidating all previous tokens for that user. Staying with JWTs will require compromise, up to potentially forgoing session revocation. The least disruptive option would be user-targeted invalidation, but it feels very hackish.
As it is, we have something analogous to the first two options, i.e. sessions with the extra baggage of JWTs that are acting in the same capacity as cryptographically random strings stored in our database (what I've implemented in this PR). I'll probably switch over to
passportwith session cookies if we decide to make the change; my implementation is pretty much roll-your-own.A secondary topic is the use of
async/await. I chose to use this functionality because it cleaned up the code I was writing. It reduced the number of callbacks in use and looks cleaner than promises while being interoperable with them. The downsides are that it requires upgrading to a newer version of Node and a lack of style consistency across the codebase. The first issue should have minimal impact, as Node is almost always backwards compatible. The second issue I believe to be an acceptable price for having cleaner code. Also, much of the existing code will still have to be refined, so it should be possible to change over as we go.Notes:
Resources:
I looked at a lot of articles, but this one covers pretty much everything by itself.