freckle / yesod-auth-oauth2

OAuth2 authentication for yesod
MIT License
71 stars 53 forks source link

Token endpoints expecting POST parameters #127

Closed nbloomf closed 4 years ago

nbloomf commented 4 years ago

Hi, and thank you for this project! I am new to working with oauth2, so apologies in advance if my question doesn't make sense. :)

I am running into a problem authenticating with WordPress.com; I'm getting the error response "The required 'client_id' parameter is missing" when attempting to fetch the token. Looking at the source for this library and hoauth2, this comment seems relevant:

-- OAuth2 spec allows `client_id` and `client_secret` to
-- either be sent in the header (as basic authentication)
-- OR as form/url params.
-- The OAuth server can choose to implement only one, or both.
-- Unfortunately, there is no way for the OAuth client (i.e. this library) to
-- know which method to use. Please take a look at the documentation of the
-- service that you are integrating with and either use `fetchAccessToken` or `fetchAccessToken2`

Sure enough, the documentation for WordPress.com's oauth2 integration uses form parameters in the body instead of headers, and it appears this library currently defaults to using fetchAccessToken, which does not add these form parameters. (code is here).

It looks like fetchAccessToken2only adds extra url-encoded parameters to the body of fetchAccessToken; I haven't checked whether that means it can be a drop-in replacement.

nbloomf commented 4 years ago

Here is my plugin by the way-

{-# LANGUAGE OverloadedStrings #-}

module Yesod.Auth.OAuth2.WordPressDotCom where

import qualified Data.Text as T
import Yesod.Auth.OAuth2.Prelude

pluginName :: Text
pluginName = "WordPress.com"

newtype WpUser = WpUser Int

instance FromJSON WpUser where
  parseJSON = withObject "WpUser" $ \o -> WpUser
    <$> o .: "ID"

oauth2WordPressDotCom
  :: ( YesodAuth m )
  => Text -- client ID
  -> Text -- client secret
  -> AuthPlugin m
oauth2WordPressDotCom clientId clientSecret =
  authOAuth2 pluginName oauth2 $ \manager token -> do
    (WpUser userId, userResponse) <-
      authGetProfile pluginName manager token
        "https://public-api.wordpress.com/rest/v1/me/"

    pure Creds
      { credsPlugin = pluginName
      , credsIdent = T.pack $ show userId
      , credsExtra = setExtra token userResponse
      }

  where
    oauth2 = OAuth2
      { oauthClientId = clientId
      , oauthClientSecret = clientSecret
      , oauthOAuthorizeEndpoint =
          "https://public-api.wordpress.com/oauth2/authorize"
            `withQuery` [ scopeParam "," [ "auth" ] ]
      , oauthAccessTokenEndpoint =
          "https://public-api.wordpress.com/oauth2/token"
      , oauthCallback = Nothing
      }
pbrisbin commented 4 years ago

Interesting! Without diving too deeply, I will say that if fetchAccessToken2 is in fact a drop-in, and all current plugins continue to work, I have no issue moving over to it. So the only blocker would be testing that fact.

The example script is meant to exercise every plugin we have. It just requires someone to sign up with all the services in question and add the appropriate local secrets to test. This isn't particularly complex, but it is time consuming. Would you be interested in doing that testing to unblock this?

The alternative would be to somehow inject that fetch function from the plugin definition site, so we could use the 2 version for only those that need it. There's no immediately obvious way to do that, so however we did it would be pretty high churn throughout the plugin interface.

nbloomf commented 4 years ago

Would you be interested in doing that testing to unblock this?

I can do that. I'll get a PR together.