Skip to content
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.

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

Monkey Patch _oauth2.getOAuthAccessToken to reformat Slack's OAuth2 v2 token response #14

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jonstorer
Copy link
Contributor

@jonstorer jonstorer commented May 25, 2023

A proposed change to add a hook that would allow this strategy to reformat the OAuth2 Token Response before sending it forward to the passport-oauth2 callback received some reasonable (though unfortunate) push back.

I took the advice in the push back, though probably the wrong part, and chose to not implement a solution that created a "maintenance burden". This new pr monkey patches node-oauth's OAuth2.getOAuthAccessToken method to reformat the OAuth2 Token Response before sending it forward to the passport-oauth2 callback.

This change introduces the method getOAuthAccessTokenAndHandleResponse which wraps this._oauth2.getOAuthAccessToken and reformats the response before sending it forward to the callback.


Original Summary for PR #13

Please see jaredhanson/passport-oauth2#174 AND #9

Slack's OAuth2 v2 implementation overloads the OAuthTokenResponse json payload to return multiple tokens. The root of the json object is for bot tokens & user tokens exist in a property called authed_user.

Passport is designed to authenticate Users. When passport attempts to use the root level accessToken, it either does not exist (no bot scopes provided in authorization request) or is a bot token.

This change leverages a new hook in the passport-oauth2 library to reformat the OAuthTokenResponse payload to set the correct accessToken, refreshToken, and params for the following scenarios:

  • user
  • user and bot
  • bot

The profileUrl is no longer defaulted during configuration. When the profile is not being skipped & a custom profileUrl was not provided, the profileUrl is set during handleOAuthAccessTokenResponse depending on which token is the root of the params object.


if this is merged, the README should be updated to emphasize the verify callback that accepts the params object from the OAuthTokenResponse request.

the params object will be reformatted to:

user_scope - user only

{
  id: 'user-id',
  scope: 'user-scope-1,user-scope-2',
  token_type: 'user',
  access_token: 'user-access-token',
  refresh_token: undefined,
  authed_user: {
    id: 'user-id',
    scope: 'user-scope-1,user-scope-2',
    access_token: 'user-access-token',
    token_type: 'user'
  },
  authed_bot: {}
}

scope - bot only

{
  id: 'bot-user-id',
  scope: 'bot-scope-1,bot-scope-2',
  token_type: 'bot',
  access_token: 'bot-token',
  refresh_token: undefined,
  authed_user: { id: 'user-id' },
  authed_bot: {
    id: 'bot-user-id',
    scope: 'bot-scope-1,bot-scope-2',
    token_type: 'bot',
    access_token: 'bot-token',
    refresh_token: undefined
  }
}

scope & user_scope - bot & user

{
  id: 'user-id',
  scope: 'user-scope-1,user-scope-2',
  token_type: 'user',
  access_token: 'user-access-token',
  refresh_token: undefined,
  authed_user: {
    id: 'user-id',
    scope: 'user-scope-1,user-scope-2',
    access_token: 'user-access-token',
    token_type: 'user'
  },
  authed_bot: {
    id: 'bot-user-id',
    scope: 'bot-scope-1,bot-scope-2',
    token_type: 'bot',
    access_token: 'bot-token',
    refresh_token: undefined
  }
}

@jonstorer
Copy link
Contributor Author

jonstorer commented May 25, 2023

@nmaves - @jeredhanson had some reasonable push back on the changes proposed in passport-oauth2. Read all about it here - jaredhanson/passport-oauth2#174 (comment)

I didn't like the idea of forking passport-oauth2 and more than Jered wanted to maintain an extra hook for a single strategy. So, I took the dirty approach and monkey pathed oauth2's getOAuthAccessToken method to achieve the same outcome. ¯\_(ツ)_/¯

@jonstorer
Copy link
Contributor Author

@nmaves what are the next steps to getting this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant