Prior to this commit, some errors would be thrown (missing parameter,
invalid state, incorrect approot) while others would be handled via the
set-message-redirect approach (handshake failure, fetch-token failure,
etc).
This commit consolidates all of these cases into a single DispatchError
type, and then uses MonadError (concretely ExceptT) to capture them all
and handle them in one place ourselves.
It then updates that handling to:
- Use onErrorHtml
onErrorHtml will, by default, set-message-redirect. That make this
behavior neutral for users running defaults. For users that have
customized this, it will be an improvement that all our error cases
now respect it.
- Provided a JSON representation of errors
- Attach a random correlation identifier
The last two were just nice-to-haves that were cheap to add once the
code was in this state.
Note that the use of MonadError requires a potentially "bad" orphan
MonadUnliftIO instance for ExceptT, but I'd like to see that instance
become a reality and think it needs some real-world experimentation to
get there, so here I am.
When the state token is sent to an OAuth2 provider, it undergoes
%-encoding as a URL parameter. Presumably, the OAuth2 provider decodes
it as part of handling things (because it would take work to prevent
their own web frameworks from doing so), and then re-%-encodes it coming
back to us again as a callback parameter.
For us, and all existing providers, + is not a %-encoded character, so
it's sent as-is and sent back as-is. So far so good.
ClassLink, though, chooses to decode + to space. I'm not aware of the
actual spec or if this is a reasonable thing to do, but they do. This
results in them sending %20 back to us, which doesn't match and we fail.
We can't predict or prescribe what providers do in this area, so our
options are:
- Look for a match in our Session as-is OR with spaces replaced by +
This is harder than it sounds: a token could contain +'s or spaces,
and we'd be getting back only spaces. To succeed, we'd actually have
to check every permutation of space/+ substitution.
- Filter + from our tokens
The only downside is we may generate slightly fewer than 30
characters, and so produce slightly less secure tokens.
I chose this option.
- Generate tokens without + to begin with
This would be ideal, but I'm just not familiar enough with
Crypto.Random. I would happily accept a PR to use this option.
hoauth2's fetchAccessToken provides credentials in the Authorization
header, while fetchAccessToken2 provides them in that header but also
the POST body.
It was discovered that some providers only support one or the other, so
using fetchAccessToken2 would be preferred since it should work with
either. This happened in #129.
However, we discovered at least one provider (Okta) that actively
rejects requests unless they're supplying credentials in exactly one
place:
Cannot supply multiple client credentials. Use one of the following:
credentials in the Authorization header, credentials in the post
body, or a client_assertion in the post body."
This patch reverts back to fetchAccessToken, but makes it possible to
for client to use fetchAccessToken2 if necessary via alternative
functions.
- Update to ghc-8.8 / lts-16.0
- Update to hoauth2 >= 1.11.0
- authGetBS has pre-encoded errors a v1.9
- oauthClientSecret is Maybe at v1.11
- Tweak non-default Resolvers as required
Previously:
- System.Random, which seeds from system time (possible attack)
- 30 characters, a-z (low entropy)
Now:
- Crypto.Random, accepted as "cryptographically secure"
- 64 random bytes, Base64-encoded
cryptonite was already a transitive dependency, so there is really no
downside to this.
Fixes#132.
This comment comes from hoauth2:
-- 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`
`fetchAccessToken2` is a drop-in replacement for `fetchAccessToken` that just adds `client_id` and `client_secret` to the body as form parameters, as permitted by [RFC 6749](https://tools.ietf.org/html/rfc6749#section-2.3.1). Some authorization server implementations only accept client credentials in this form.
This was lazy and resulted in a confusing error experience where a
JSONDecodingError fetching credentials appeared as an Unknown OAuth2
ErrorResponse, making it appear like the OAuth2 provider was indicating
this error to us, instead of it being a simple incorrect parser in our
own code.
ErrorResponse is specifically meant to parse error parameters sent to us
by the OAuth2 provider. They may be user-actionable and can be safely
displayed. This is a very narrow use-case. The Unknown constructor is
required for us to be exhaustive on our string error names, but it
should not be hijacked to store our own errors.
This commit separates and documents the two error scenarios.
I had hoped to get away from this entirely, to an Either-based
interface, but that seems to be stalling as an initiative. So in the
meantime, let's at least make our exceptions more meaningful.
For some reason, I thought tryIO would catch our own exception is we
threw them via throwIO, but that's incorrect. Our own exceptions are not
IOExceptions, so they squeak by. This fixes that.
- Latest LTS-11.5
- Allow hoauth2-1.7, needs to be extra-dep though
- Support *and require* yesod-1.6
This required:
- Less lifts
- HandlerFor, WidgetFor, etc
- Lost MonadThrow, but can use MonadIO instead
- Incorrect indentation
- We should always accept Id/Secret last
- The function is oauth... not oAuth...
Because of the mis-naming, at least we could fix the argument-order in a
backwards-compatible way, deprecating the old function/interface.
Even though it's "guaranteed" that values will be present because we set
them, nothing stops end-users from using these functions on Creds values
created by other plugins! Since that seems common, it would be
irresponsible of us to remain so unsafe.
