Stop returning extra information in GitHub result

[pbrisbin]

See #71.

New credsExtra keys:

• accessToken: so you can make your own follow-up requests
• userResponseJSON: so you can get more information out of the request
  we already made (you just have to parse it yourself)

Removed keys:

• access_token: renamed to accessToken
• avatar_url: can be re-parsed
• email: requires your own request to /emails
• login: can be re-parsed from userResponseJSON
• location: can be re-parsed, was not always present
• name: can be re-parse, was not not always present
• public_email: can be re-parsed, was not not always present

---

I'll start going provider by provider now.

[pbrisbin]

I decided to store the raw response at userResponse (not userResponseJSON), since it doesn't have to be JSON. We're providing our own lookup functions, so getUserResponse will return ByteString and getUserResponseJSON will return it parsed to FromJSON a, and may fail if misused.

Also, I'm starting to see a slightly better Provider type come together:

data Provider = Provider
  { pName :: ProviderName
  , pToOAuth2 :: ClientId -> ClientSecret -> OAuth2
  , pFetchUserResponse :: Manager -> OAuth2Token -> Either String ByteString
  , pParseCredsIdent :: ByteString -> Either String Text
  }

something :: YesodAuth m => Provider -> ClientId -> ClientSecret -> AuthPlugin m
something Provider{..} clientId clientSecret =
  authOAuth2 pName (pToOAuth2 clientId clientSecret) $ \manager token -> do
    -- Assumes we adjust authOAuth2 to use Either, not exceptions
    userResponse <- pFetchUserResponse manager token
    userId <- pParseCredsIdent userResponse

    pure Creds
      { credsPlugin = pName
      , credsIdent = userId
      , credsExtra = setExtra token userResponse
      }

I think I'll branch from this and try that out, as a replacement for #92

[jferris]

It's a bummer that we have to use fromJust here. Is this because Yesod.Auth.Creds doesn't have a type parameter, so we have to use stringly typed data for the "extra" bits? Would it be worth trying to change this in Yesod.Auth?

[jferris]

Also, this is not necessarily a bug, right? If somebody authenticates with username/password and then calls getAccessToken with those creds, that will compile and crash?

[pbrisbin]

Would it be worth trying to change this in Yesod.Auth?

I'm not really sure that would work. Even if it were Creds e m, for some extras type e, that e couldn't be plugin-specific because it must be the same across all plugins in use. Maybe I'm missing something clever though.

Also, this is not necessarily a bug, right? If somebody authenticates with username/password and then...

You're absolutely right; great catch. I think I lean towards making this Maybe now. I thought there was strong guarantees a value would be present, but non-oauth2-plugins is way too likely.

[jferris]

Nice to see some cleanup and simplification here.

[owickstrom]

@pbrisbin Great stuff, really looking forward to this change!

[jferris]

It may be more convenient for users to use Either String for all the get* functions. For example, that would let users combine them monadically without writing their own conversions between Either String and Maybe.

[pbrisbin]

I don't know that I follow your reasoning: I think we can't really predict what Monad the user will want to be in, so I don't know that Either would fit any better at call-sites than Maybe (they'll almost certainly be in Handler 99% of the time)... But I can get behind Either for this because it retains information users could choose to silence when in Maybe (e.g. hush), vs the current way where the user would have to invent information when in Either (e.g. note) -- so 👍

[pbrisbin]

Well, on second thought I'm not sure. Having these return Either feels like if Map.lookup returned either: that'd be silly because there's only one way it can fail.

I think I'm going to keep this for now.