Add hook to handle OAuth Access Token Response

[jonstorer]

Add the method handleOAuthTokenResponse to allow subclasses to reformat the OAuth Token response before continuing throw the flow.

See: nmaves/passport-slack-oauth2#13

Slack's OAuth2 v2 implementation overrides the token response to return multiple tokens. A scenario may exist where a user will authenticate with "bot scope only" and a user accessToken is not returned. This hook allows the passport-slack-oauth2 provider to reformat the OAuthTokenResponse object such that passport can successfully authenticate for bot only access.

[jonstorer]

rm

[jonstorer]

@jaredhanson Hello! What are the next steps to get this PR merged?

[jaredhanson]

My main concern here is that, while Slack claims to be implementing OAuth 2.0, they don't follow the specification at all (thus, its not really OAuth 2.0).

While the hook that you've implemented seems reasonable as an extensibility point, I generally favor adding extensibility when its clear that multiple implementations need to be swapped in. In this case, the only "consumer" of this extensibility is Slack, and its only because its not implementing the spec. If it did implement the spec, the hook here wouldn't be needed.

One approach I would recommend would be to fork this package, publish it as passport-oauth2-slack-ext (or whatever name is appropriate), and then use that package as the dependency in passport-slack-oauth2. If there are other OAuth 2.0-based packages that could benefit from the hook here, then that would be rationale to merge this upstream (and I'd be in favor of it at that point). Until then, though, I'd prefer not to take on the maintenance burden of supporting a hook that's only used by a single provider.

[jonstorer]

@jaredhanson all good points. And I understand not wanting to create another maintenance burden. In the same vein, I don't have a desire to maintain a passport-oauth2-stack-ext fork (or whatever name is appropriate).

Would you be wiling to accept a PR for an extensibility point with an even smaller footprint? The current solution creates a path for IO work to be done, however it is not required. The solution would look something like:

OAuth2Strategy.prototype.handleOAuthAccessTokenResponse = function(accessToken, refreshToken, params) {
  return { accessToken, refreshToken, params };
};

which is similar to the authorizationParams extensibility hook

OAuth2Strategy.prototype.authorizationParams = function(options) {
  return {};
};

and tokenParams extensibility hook

OAuth2Strategy.prototype.tokenParams = function(options) {
  return {};
};

This hook would manifest in code as

self._oauth2.getOAuthAccessToken(code, params, function(err, accessToken, refreshToken, params) {
  if (err) { return self.error(self._createOAuthError('Failed to obtain access token', err)); 
  { accessToken, refreshToken, params } = self.handleOAuthAccessTokenResponse(accessToken, refreshToken, params);
  // ...
});

Please let me know, I look forward to your response.

Also, in order to unblock my slack integration needs, I've taken the least desirable approach and monkey patched this._oauth2.getOAuthAccessToken in order to reformat the token response before passing it forward to the framework's callback.