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.
It seems newer hoauth2 uses newer Cabal, which doesn't work in the
resolver for ghc-8.4. It may build, and you're welcome to try, but we're
dropping formal (e.g. CI-backed) support here.
- 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.
