This PR changes the OpenAPI schema for the advanced playground to not
accept empty lists of environments and to not accept environment names
that don't match the env name pattern we use.
The pattern is the same as the one we use for controlling environment
names on creation.
However, there is a (small) chance that these may get out of sync later,
so we could do something to only define this pattern once (and import it
in the enterprise package), but that may be more work than is necessary,
and I'd suggest we do that later.
I've also added a minLength to the string items although it isn't
strictly necessary. It's primarily to give the users better feedback if
the name is empty.
This change adds an "edit" link to all playground strategies when they
are returned from the API, allowing the user to jump directly to the
evaluated strategy edit screen.
This change applies to both old and new strategies, so it should even
work in the old playground.
This does not use this information in the frontend yet.
## Discussion points:
Should "links" be an object or a singular string? I know the
notifications service uses just "link", but using an object will make
it easier to potentially add more actions in the future (such as
"enable"/"disable", maybe?)
Do we need to supply basePath? I noticed that the notifications links
only ever use an empty string for base path, so it might not be
necessary and can potentially be left out.
## Changes
I've implemented the link building in a new view model file. Inspecting
the output after the result is fully ready requires some gnarly
introspection and mapping, but it's tested.
Further, I've done a little bit of work to stop the playground service
using the schema types directly as the schema types now contain extra
information.
This PR also updates the `urlFriendlyString` arbitrary to not produce
strings that contain only periods. This causes issues when parsing URLs
(and is also something we struggle with in the UI).
https://github.com/Unleash/unleash/pull/4019 introduced a bug where API
token filtering was not taking into account ADMIN permissions, which
means the API tokens were not being displayed on the UI.
This PR fixes an issue where trying to use an admin token to import
features via the API resulted in a 500 (due to missing properties).
The solution is to catch when the user is using and admin token in the
controller, throw a 400, and tell them to use personal access tokens or
service accounts.
The PR includes a new test file for this specific use case. We don't
really test these cases many other places, so it seemed the logical
choice.
## What
As part of the move to enable custom-root-roles, our permissions model
was found to not be granular enough to allow service accounts to only be
allowed to create read-only tokens (client, frontend), but not be
allowed to create admin tokens to avoid opening up a path for privilege
escalation.
## How
This PR adds 12 new roles, a CRUD set for each of the three token types
(admin, client, frontend). To access the `/api/admin/api-tokens`
endpoints you will still need the existing permission (CREATE_API_TOKEN,
DELETE_API_TOKEN, READ_API_TOKEN, UPDATE_API_TOKEN). Once this PR has
been merged the token type you're modifying will also be checked, so if
you're trying to create a CLIENT api-token, you will need
`CREATE_API_TOKEN` and `CREATE_CLIENT_API_TOKEN` permissions. If the
user performing the create call does not have these two permissions or
the `ADMIN` permission, the creation will be rejected with a `403 -
FORBIDDEN` status.
### Discussion points
The test suite tests all operations using a token with
operation_CLIENT_API_TOKEN permission and verifies that it fails trying
to do any of the operations against FRONTEND and ADMIN tokens. During
development the operation_FRONTEND_API_TOKEN and
operation_ADMIN_API_TOKEN permission has also been tested in the same
way. I wonder if it's worth it to re-add these tests in order to verify
that the permission checker works for all operations, or if this is
enough. Since we're running them using e2e tests, I've removed them for
now, to avoid hogging too much processing time.
As requested in
[Linear](https://linear.app/unleash/issue/2-1147/unleash-cloud-make-keepalive-configurable)
this PR makes the serverKeepAliveTimeout configurable via the
SERVER_KEEPALIVE_TIMEOUT environment variable. This was already
configurable when starting Unleash programmatically, but it's nice to
have as an env variable as well
https://linear.app/unleash/issue/2-1137/roles-unification-on-the-ui
Root and project roles should be managed in a similar manner, which
means using the same roles route and tab for both.
Additionally, this includes a big revamp to the project roles to align
them more closely with the modern and standardized custom root roles
that were recently developed. They mostly use the same components.
There are still more things we want to improve and unify, but we've left
some of that out of this PR due to PR size concerns.
<!-- 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. -->
Adds an environment variable for switching off feature telemetry in
version check
<!-- 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! ❤️ -->
Implements the Advanced Playground Table
## 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 #
[1-1007](https://linear.app/unleash/issue/1-1007/env-aware-results-table)
<!-- (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? -->
---------
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
This change adds a little more detail to the client metrics schema. The
description for variants felt a
little unclear.
---------
Co-authored-by: Christopher Kolstad <chriswk@getunleash.ai>
## About the changes
Implements custom root roles, encompassing a lot of different areas of
the project, and slightly refactoring the current roles logic. It
includes quite a clean up.
This feature itself is behind a flag: `customRootRoles`
This feature covers root roles in:
- Users;
- Service Accounts;
- Groups;
Apologies in advance. I may have gotten a bit carried away 🙈
### Roles
We now have a new admin tab called "Roles" where we can see all root
roles and manage custom ones. We are not allowed to edit or remove
*predefined* roles.
This meant slightly pushing away the existing roles to `project-roles`
instead. One idea we want to explore in the future is to unify both
types of roles in the UI instead of having 2 separate tabs. This
includes modernizing project roles to fit more into our current design
and decisions.
Hovering the permissions cell expands detailed information about the
role:
### Create and edit role
Here's how the role form looks like (create / edit):
Here I categorized permissions so it's easier to visualize and manage
from a UX perspective.
I'm using the same endpoint as before. I tried to unify the logic and
get rid of the `projectRole` specific hooks. What distinguishes custom
root roles from custom project roles is the extra `root-custom` type we
see on the payload. By default we assume `custom` (custom project role)
instead, which should help in terms of backwards compatibility.
### Delete role
When we delete a custom role we try to help the end user make an
informed decision by listing all the entities which currently use this
custom root role:
~~As mentioned in the screenshot, when deleting a custom role, we demote
all entities associated with it to the predefined `Viewer` role.~~
**EDIT**: Apparently we currently block this from the API
(access-service deleteRole) with a message:
What should the correct behavior be?
### Role selector
I added a new easy-to-use role selector component that is present in:
- Users
- Service Accounts
- Groups
### Role description
I also added a new role description component that you can see below the
dropdown in the selector component, but it's also used to better
describe each role in the respective tables:
I'm not listing all the permissions of predefined roles. Those simply
show the description in the tooltip:
### Role badge
Groups is a bit different, since it uses a list of cards, so I added yet
another component - Role badge:
I'm using this same component on the profile tab:
## Discussion points
- Are we being defensive enough with the use of the flag? Should we
cover more?
- Are we breaking backwards compatibility in any way?
- What should we do when removing a role? Block or demote?
- Maybe some existing permission-related issues will surface with this
change: Are we being specific enough with our permissions? A lot of
places are simply checking for `ADMIN`;
- We may want to get rid of the API roles coupling we have with the
users and SAs and instead use the new hooks (e.g. `useRoles`)
explicitly;
- We should update the docs;
- Maybe we could allow the user to add a custom role directly from the
role selector component;
---------
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
## About the changes
Unleash version should be the one best representing its runtime. In this
regard, enterpriseVersion trumps unleash-server version.
This version is the one that's sent as part of our metrics.
<!-- 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. -->
Adds feature usage info and custom strategy counters to the version
check object.
<!-- 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? -->
## 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.
