The provider type was modelled as a newtype struct around a `String` but
there's a finite number of values, so really it should be an enum. This
wasn't too pressing until now but will shortly come in useful for the
configuration stuff, which needs a provider type too.
The error-handling in this repo grew pretty organically without us
giving much thought to an over-arching strategy. The end result of that
was a proliferation of different error types that weren't really adding
much value.
The API here is simple and the 4xx responses are really limited to bad
requests and bounce/complaint limit violations. Everything else is an
internal server error. As such, the `error` module could be simplified
by employing a blanket `Internal` error type to cover the multitude of
niche errors that should never occur during normal conditions. The
detail for those errors is still available in the `message` property and
Sentry will still log the backtrace of course.
I also noticed that the Rocket catcher stuff wasn't really being used,
so pulled that out too. We have a standard JSON error format like the
rest of the FxA ecosystem, and we always want the response payload to be
the serialisation of that structure.
One side-effect of all these changes is that the errno value has changed
in most cases. However, I took care to preserve the errno for the bounce
and complaint violations, because it's hard-coded in the auth server. It
wouldn't have been that onerous to open a PR for commensurate changes
over there, but the number of errors left here worked out in such a way
that made sense not to bother.
Fixes#201.
Email addresses that we receive from bounce, complaint and delivery
notifications may not be identical matches to what we set on messages.
We already did some normalization by lower-casing the address when
parsing. This change extends that normalization process by trimming
whitespace and unwrapping the address part if it has been wrapped in
angle bracket delimiters to separate it from the display name. This
matches the behaviour of the auth server.
We were sending errors to Sentry at the point where they are logged,
but there are a number of paths where an error is raised without being
logged. Instead, this change captures them at the point of creation,
which should mean we now capture every error.
Fixes#205.
SES provides a UTC-based timestamp for bounce and complaint events but
the auth server opted to ignore it. I'm not sure that was the optimal
decision because messages might linger on a queue (either ours or the
provider's) for a length of time before we process it. Given that we
already trust SES et al to deliver sane notifications, it doesn't seem
too great a stretch to also trust the timestamp on those notifications.
There was a minor violation of the law of Demeter in our `AppError`
struct, where it was directly exposing its inner `AppErrorKind` to
callers. Not a huge deal, but it meant that any changes made to the
inner structure would leak out and require corresponding changes to the
consuming code (and I am planning some of those inner changes as part of
issue #210). Better to expose just the parts that are necessary via its
own API.
At the same time, I opted to replace the home-baked `AppError::json()`
method with a more conventional `impl Serialize`, because it's less
astonishing and just as easy to use with `serde_json::to_string`.
We've grown a lot of modules in this repo and it's hard to see the wood
for the trees when you look in the `src` directory. This commit attempts
to neaten everything up a bit by scoping some of them under collective
parent modules:
* `auth_db`, `delivery_problems` and `message_data` have moved to `db`,
and the old `db` has moved to `db/core`.
* `healthcheck` and `send` have moved to `api`.
* `app_errors`, `duration`, `email_address` and `validate` have moved to
`types`, and `app_errors` is now just `error`.
* `serialize` has moved to `settings` as it was only referenced there.
Hopefully the end result makes it easier to zero in on modules of
interest when working in the repo.
Phil Booth
Vlad Filippov
Vijay Budhram