## About the changes
When a feature is not found in a project we should fail with a NotFound
error. If the feature belongs to a different project, it should not be a
permission issue, because the user might not be aware (lack of
permissions/visibility) of that other project, so even in this case the
error should be NotFound (this also works if we ever allow the same
feature name in different projects)
Fixes#3726
---------
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
## About the changes
Edit application under
https://app.unleash-hosted.com/demo/applications/test-app is currently
not working as the appName is expected to come in the request body, but
it's actually part of the url. We have two options here:
1. We change the UI to adapt to the expectations of the request by
adding appName to the request body (and eventually removing appName from
the URL, which would be a breaking change)
2. We remove the restriction of only sending the appName in the body and
take the one that comes in the URL. We have a validation that verifies
that at least one of the two sets the appName
([here](e376088668/src/lib/services/client-metrics/instance-service.ts (L208))
we validate using [this
schema](e376088668/src/lib/services/client-metrics/schema.ts (L55-L70)))
In terms of REST API, we can assume that the appName will be present in
the resource `/api/admin/metrics/applications` (an endpoint we don't
have), but when we're updating an application we should refer to that
application by its URL: `/api/admin/metrics/applications/<appName>` and
the presence of an appName in the body might indicate that we're trying
to change the name of the application (something we currently not
support)
Based on the above, I believe going with the second option is best, as
it adheres to REST principles and does not require a breaking change.
Despite that, we only support updating applications as the creation is
done from metrics ingestion
Fixes: #3580
This PR aims to handle the increased log alarm volume seen by the SREs.
It appears that we get a large number of alarms because a client
disconnects early from the front-end API. These errors are then
converted into 500s because of missing handling. These errors appear to
be caused by the `http-errors` library in a dependency.
We also introduced a log line whenever we see errors now a while back,
and I don't think we need this logging (I was also the one who
introduced it).
The changes in this PR are specifically:
- When converting from arbitrary errors, give `BadRequestError` a 400
status code, not a 500.
- Add a dependency on `http-errors` (which is already a transitive
dependency because of the body parser) and use that to check whether an
error is an http-error.
- If an error is an http error, then propagate it to the user with the
status code and message.
- Remove warning logs when an error occurs. This was introduced to make
it easier to correlate an API error and the logs, but the system hasn't
been set up for that (yet?), so it's just noise now.
- When logging errors as errors, only do that if their status code would
be 500.
This change makes the logs that happen when we encounter an error a
little bit clearer. It logs the error message before the error ID and
also logs the full serialized message just in case.
This PR improves our handling of internal Joi errors, to make them more
sensible to the end user. It does that by providing a better description
of the errors and by telling the user what they value they provided was.
Previous conversion:
```json
{
"id": "705a8dc0-1198-4894-9015-f1e5b9992b48",
"name": "BadDataError",
"message": "\"value\" must contain at least 1 items",
"details": [
{
"message": "\"value\" must contain at least 1 items",
"description": "\"value\" must contain at least 1 items"
}
]
}
```
New conversion:
```json
{
"id": "87fb4715-cbdd-48bb-b4d7-d354e7d97380",
"name": "BadDataError",
"message": "Request validation failed: your request body contains invalid data. Refer to the `details` list for more information.",
"details": [
{
"description": "\"value\" must contain at least 1 items. You provided [].",
"message": "\"value\" must contain at least 1 items. You provided []."
}
]
}
```
## Restructuring
This PR moves some code out of `unleash-error.ts` and into a new file.
The purpose of this is twofold:
1. avoid circular dependencies (we need imports from both UnleashError
and BadDataError)
2. carve out a clearer separation of concerns, keeping `unleash-error` a
little more focused.
This change lowers the log level from warning to debug for when we see
unexpected error types.
Right now this triggers for Joi errors, which we still rely on pretty
heavily. Lowering this should clear up logs for most users.
If apiTokens are enabled breaks middleware chain with a 401 if no token
is found for requests to client and frontend apis. Previously the
middleware allowed the chain to process.
Removes the regex search for multiple slashes, and instead configures
the apiTokenMiddleware to reject unauthorized requests.
After a Team Retro, one of our squads felt like we needed more data on
our test suites. This is the first effort to make our test results
easier to grab. It uses the test-reporter action to add a github check
to our main build and PR builds with our test results.
This at least should make it easier to parse which tests are failing.
However, it does not give us trends. So it does not yet make it easier
to decide which tests are flaky just from a quick view.
---------
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
https://linear.app/unleash/issue/2-1071/prevent-users-from-disabling-password-authentication-when-there-are-no
Improves the behavior of disabling password based login by adding some
relevant information and a confirmation dialog with a warning. This felt
better than trying to disable the toggle, by still allowing the end
users to make the decision, except now it should be a properly informed
decision with confirmation.
- **Password based administrators**: Admin accounts that have a password
set;
- **Other administrators**: Other admin users that do not have a
password. May be SSO, but may also be users that did not set a password
yet;
- **Admin service accounts**: Service accounts that have the admin root
role. Depending on how you're using the SA this may not necessarily mean
locking yourself out of an admin account, especially if you secured its
token beforehand;
- **Admin API tokens**: Similar to the above. If you secured an admin
API token beforehand, you still have access to all features through the
API;
Each one of them link to the respective page inside Unleash (e.g. users
page, service accounts page, tokens page...);
If you try to disable and press "save", and only in that scenario, you
are presented with the following confirmation dialog:
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
Improves the openapi schema specifications for the schemas belonging to
the "Projects" tag.
Expected error codes/http statues, descriptions, and example data
---------
Co-authored-by: Christopher Kolstad <chriswk@getunleash.ai>
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
Variants were not being properly handled in the `flag-resolver`: The
fact that the default value of the variant is not falsy made it so we
never asked the external flag resolver for the value.
This also moves the logic from `Variant | undefined` to `Variant` where
we use the `getDefaultVariant()` helper method to return us a [default
variant](55274e4953/src/variant.ts (L37-L42)).
### What
In the demo when listing possible users to grant access to your project,
we inadvertently expose emails when listing users you can grant access
to. This PR anonymises the access list on the way out.
This PR adds the missing serialization of the AuthenticationRequired
response back in. It was mistakenly removed in #3633.
This PR also adds another test to verify that it the options property is
present.
This PR reuses the revision Id information from the "optimal 304 for
server SDKs" to improve the freshness of the frontend API config data.
In addition it allows us to reduce the polling (and eventually remove it
when we are confident).
---------
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
Adds a migration that renames `token_name` back to `username`, then adds
a new optional column named `token_name`
## Discussion points
<!-- Anything about the PR you'd like to discuss before it gets merged?
Got any questions or doubts? -->
I've added fallbacks for resolving username/tokenname on insert and on
making rows from results.
But this adds another column renaming, which is worth discussing
properly
This PR attempts to improve the error handling introduced in #3607.
## About the changes
## **tl;dr:**
- Make `UnleashError` constructor protected
- Make all custom errors inherit from `UnleashError`.
- Add tests to ensure that all special error cases include their
relevant data
- Remove `PasswordMismatchError` and `BadRequestError`. These don't
exist.
- Add a few new error types: `ContentTypeError`, `NotImplementedError`,
`UnauthorizedError`
- Remove the `...rest` parameter from error constructor
- Add an unexported `GenericUnleashError` class
- Move OpenAPI conversion function to `BadDataError` clas
- Remove explicit `Error.captureStackTrace`. This is done automatically.
- Extract `getPropFromString` function and add tests
### **In a more verbose fashion**
The main thing is that all our internal errors now inherit
from`UnleashError`. This allows us to simplify the `UnleashError`
constructor and error handling in general while still giving us the
extra benefits we added to that class. However, it _does_ also mean that
I've had to update **all** existing error classes.
The constructor for `UnleashError` is now protected and all places that
called that constructor directly have been updated. Because the base
error isn't available anymore, I've added three new errors to cover use
cases that we didn't already have covered: `NotImplementedError`,
`UnauthorizedError`, `ContentTypeError`. This is to stay consistent in
how we report errors to the user.
There is also an internal class, `GenericUnleashError` that inherits
from the base error. This class is only used in conversions for cases
where we don't know what the error is. It is not exported.
In making all the errors inherit, I've also removed the `...rest`
parameter from the `UnleashError` constructor. We don't need this
anymore.
Following on from the fixes with missing properties in #3638, I have
added tests for all errors that contain extra data.
Some of the error names that were originally used when creating the list
don't exist in the backend. `BadRequestError` and
`PasswordMismatchError` have been removed.
The `BadDataError` class now contains the conversion code for OpenAPI
validation errors. In doing so, I extracted and tested the
`getPropFromString` function.
### Main files
Due to the nature of the changes, there's a lot of files to look at. So
to make it easier to know where to turn your attention:
The changes in `api-error.ts` contain the main changes: protected
constructor, removal of OpenAPI conversion (moved into `BadDataError`.
`api-error.test.ts` contains tests to make sure that errors work as
expected.
Aside from `get-prop-from-string.ts` and the tests, everything else is
just the required updates to go through with the changes.
## Discussion points
I've gone for inheritance of the Error type over composition. This is in
large part because throwing actual Error instances instead of just
objects is preferable (because they collect stack traces, for instance).
However, it's quite possible that we could solve the same thing in a
more elegant fashion using composition.
## For later / suggestions for further improvements
The `api-error` files still contain a lot of code. I think it might be
beneficial to break each Error into a separate folder that includes the
error, its tests, and its schema (if required). It would help decouple
it a bit.
We don't currently expose the schema anywhere, so it's not available in
the openapi spec. We should look at exposing it too.
Finally, it would be good to go through each individual error message
and update each one to be as helpful as possible.
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
OpenAPI schema updates for Personal Access Tokens, http statuses,
property types and examples, return types
---------
Co-authored-by: Nuno Góis <github@nunogois.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
This PR removes the usage of crOnVariants flag, but keeps the behaviour,
so CR are now enabled on variants.
---------
Co-authored-by: Nuno Góis <github@nunogois.com>
When using PATs if the user that the PAT is for has been removed, we
currently log the missing user at ERROR level. Since this is not
something our SREs can fix, this PR downgrades the NotFoundError to
WARN, instead of ERROR.
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
<!-- Does it close an issue? Multiple? -->
Closes #
<!-- (For internal contributors): Does it relate to an issue on public
roadmap? -->
<!--
Relates to [roadmap](https://github.com/orgs/Unleash/projects/10) item:
#
-->
### Important files
<!-- PRs can contain a lot of changes, but not all changes are equally
important. Where should a reviewer start looking to get an overview of
the changes? Are any files particularly important? -->
## Discussion points
<!-- Anything about the PR you'd like to discuss before it gets merged?
Got any questions or doubts? -->
---------
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
