Lots of work here, mostly because I didn't want to turn off the
`noImplicitAnyLet` lint. This PR tries its best to type all the untyped
lets biome complained about (Don't ask me how many hours that took or
how many lints that was >200...), which in the future will force test
authors to actually type their global variables setup in `beforeAll`.
---------
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
https://linear.app/unleash/issue/SR-164/ticket-1106-user-with-createedit-project-segment-is-not-able-to-edit-a
Fixes a bug where the `UPDATE_PROJECT_SEGMENT` permission is not
respected, both on the UI and on the API. The original intention was
stated
[here](https://github.com/Unleash/unleash/pull/3346#discussion_r1140434517).
This was easy to fix on the UI, since we were simply missing the extra
permission on the button permission checks.
Unfortunately the API can be tricky. Our auth middleware tries to grab
the `project` information from either the params or body object, but our
`DELETE` method does not contain this information. There is no body and
the endpoint looks like `/admin/segments/:id`, only including the
segment id.
This means that, in the rbac middleware when we check the permissions,
we need to figure out if we're in such a scenario and fetch the project
information from the DB, which feels a bit hacky, but it's something
we're seemingly already doing for features, so at least it's somewhat
consistent.
Ideally what we could do is leave this API alone and create a separate
one for project segments, with endpoints where we would have project as
a param, like so:
`http://localhost:4242/api/admin/projects/:projectId/segments/1`.
This PR opts to go with the quick and hacky solution for now since this
is an issue we want to fix quickly, but this is something that we should
be aware of. I'm also unsure if we want to create a new API for project
segments. If we decide that we want a different solution I don't mind
either adapting this PR or creating a follow up.
Make each error class have to define its own status code. This makes
it easier for developers to see which code an error corresponds to and
means less jumping back and forth between files. In other words:
improved locality.
Unfortunately, the long switch needs to stay in case we get errors
thrown that aren't of the Unleash Error type, but we can move it to
the `fromLegacyError` file instead.
Tradeoff analysis by @kwasniew:
+ I like the locality of error to code reasoning
- now HTTP leaks to the non-HTTP code that throws those errors e.g. application services
If we had other delivery mechanisms other than HTTP then it wouldn't make sense to couple error codes to one protocol (HTTP). But since we're mostly doing web it may not be a problem.
@thomasheartman's response:
This is a good point and something I hadn't considered. The same data was always available on those errors (by using the same property), I've just made the declaration local to each error instead of something that the parent class handles. The idea was to make it easier to create new error classes with their corresponding error codes. Because the errors are intended to be API errors (or at least, I've always considered them to be that), I think that makes sense.
Taking your comment into consideration, I still think it's the right thing to do, but I'm not bullish about it. We could always walk it back later if we find that it's not appropriate. The old code is still available and we could easily enough roll back this change if we find that we want to decouple it later.
In some of the places we used `NoAccessError` for permissions, other
places we used it for a more generic 403 error with a different
message. This refactoring splits the error type into two distinct
types instead to make the error messages more consistent.
