Skip to content

Switched herokaiOnlyHandler to a general-purpose authCallback. #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jacobian wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Switched herokaiOnlyHandler to a general-purpose authCallback. #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ To log a user out, send them to `/auth/heroku/logout`.
| `sessionSyncNonce` | No | `null` | The name of a nonce cookie to validate sessions against |
| `ignoredRoutes` | No | `[]` | An array of regular expressions to match routes to be ignored when there is no session active |
| `oAuthServerURL` | No | `"https://id.heroku.com"` | The location of the Heroku OAuth server |
| `herokaiOnlyHandler` | No | `null` | A route handler that will be called on requests by non-Herokai |
| `authCallback` | No | `null` | An authorization callback that will be passed the [account object](https://devcenter.heroku.com/articles/platform-api-reference#account). Return `true` to allow access; `false` to not. |
| `authCallbackFailedHandler` | No | `null` | A route handler called if `authCallback` returns `false` |

## Test

Expand Down
20 changes: 20 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,33 @@ function setOptions(options) {
throw new Error('`herokaiOnlyHandler` must be a handler function');
}

if (options.authCallback && typeof(options.authCallback) !== 'function') {
throw new Error('`authCallback` must be a handler function');
}

if (options.authCallbackFailed && typeof(options.authCallbackFailed) !== 'function') {
throw new Error('`authCallbackFailed` must be a handler function');
}

if (options.authCallback && options.herokaiOnlyHandler) {
throw new Error('can\'t pass both `herokaiOnlyHandler` and `authCallback`');
}

// backwards-compat for herokaiOnlyHandler
if (options.herokaiOnlyHandler) {
options.authCallback = function(user) { return /@heroku\.com$/.test(user.email); };
options.authCallbackFailedHandler = options.herokaiOnlyHandler;
}

var defaults = {
herokaiOnlyHandler: null,
herokuAPIHost : null,
ignoredRoutes : [],
oAuthServerURL : 'https://id.heroku.com',
oAuthScope : 'identity',
sessionSyncNonce : null,
authCallback : null,
authCallbackFailedHandler: null,
};

for (var key in defaults) {
Expand Down
11 changes: 7 additions & 4 deletions lib/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,13 @@ module.exports = function(options) {
}

if (userSession) {
var isHerokai = /@heroku\.com$/.test(userSession.user.email);

if (options.herokaiOnlyHandler && !isHerokai) {
return options.herokaiOnlyHandler(req, res, next);
if (options.authCallback && !options.authCallback(userSession.user)) {
if (options.authCallbackFailedHandler) {
return options.authCallbackFailedHandler(req, res, next);
} else {
// FIXME: is this the right thing? Or a default 401?
reauthenticate(req, res);
Copy link
Contributor

@jclem jclem Feb 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems right to me. reauthenticate is a bit of a misnomer, as this actually 401s for a JSON request or redirects to /auth/heroku for a non-JSON request.

I believe you do need to return here, though. I haven't looked at this code in ages :(

Copy link
Contributor

@jclem jclem Feb 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you might be able to clean this stuff up by setting options.authCallback to a function() { return true; } by default and options.authCallbackFailedHandler = reauthenticate by default.

Then all you need is

options.authCallback(userSession.user) || return options.authCallbackFailedHandler(req, res, next);

}
}

ensureValidToken(userSession, function(err) {
Expand Down
18 changes: 18 additions & 0 deletions test/bouncer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,24 @@ describe('bouncer', function() {
});
});
});

context('and authCallback is set', function() {
var clientOptions;

beforeEach(function() {
clientOptions = {
authCallback: function(user) { return /@example\.com$/.test(user.email) },
authCallbackFailedHandler: function(req, res) { res.end('You are not allowed.'); }
};
});

context('and the user is authorized', function() {
it('performs the request like normal', function() {
herokuStubber.stubUser({ email: 'user@example.com' });
return itBehavesLikeANormalRequest(clientOptions);
});
});
});
});

describe('logging out', function() {
Expand Down