The new major version improves the naming of the fields of the OAuth2
record type. This type is central to this library and we leak it freely.
Users who make their own plugins are expected to construct values of
this type to pass into our functions, this makes the new version
disruptive to our code and our users'.
We have two options:
1. Update and release our own new major version
The major downside is that the current LTS resolver will then not
update beyond our currently-released version. We have no immediate
plans for new features in this library, but if we have bugs reported
to be fixed we would either have to manage a complex backporting or
ask our Stack users to wait for the next major LTS, which has
historically been many months.
Users who wish to use our new version would need to also bring in
hoauth2, and who knows what else.
2. Release a fully-compatible update
As mentioned, we leak OAuth2(..) through this library's interface. In
order to be truly backwards-compatible, we would have to use CCP to
define an "old style" OAuth2 and use that throughout, such that
in-the-wild OAuth2 values continue to work as-is.
This would not be a good long-term solution as it introduces a fair
amount of naming confusion and will lead to import conflicts for any
users who also import hoauth2-2.0 modules in the same project.
3. Release a mostly-compatible update
This is the path this commit explores. We can update our own code to
be hoauth2-2.0 compatible and use CPP to define the hoauth2-2.0-like
OAuth2 if we're still on hoauth2-1.x.
This gets us compiling in either case and "forward functional", with
the exception of users who define their own plugins (which is rare).
Because of that use-case, this should technically be a major version
bump for ourselves (though I'm open to the argument we could treat
the local-provider use-case differently), however it is still better
than Option 1 in a few ways:
- We still compile with hoauth2-1.x, so can be brought in easily as
an isolated extra-dep
- If there is a reported bug that we decide to only fix in the newer
versions, the path for the user is better: they can pull us as an
extra-dep and likely need no changes. Even if they're doing a
custom plugin, the required changes are minor
Workflows that use the default GITHUB_TOKEN cannot trigger other
Workflows. This is a security thing (thanks crypto-bros) that prevents
us from pushing a tag in an attempt to trigger a Release.
Instead, we move that tagging to the Release Workflow itself and allow
that to run on pushes to main in addition to pushes of tags. This way,
pushes of tags continue to upload as before, but also pushes of changed
versions will now create a tag and upload, as desired.
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.
