This change fixes the OpenAPI schema to disallow non-string properties
on the top level of the context (except, of course, the `properties`
object).
This means that we'll no longer be seeing issues with rendering
invalid contexts, because we don't accept them in the first place.
This solution comes with some tradeoffs discussed in the [PR](https://github.com/Unleash/unleash/pull/6676). Following on from that, this solution isn't optimal, but it's a good stop gap. A better solution (proposed in the PR discussion) has been added as an idea for future projects.
The bulk of the discussion around the solution is included here for reference:
@kwasniew:
Was it possible to pass non string properties with our UI before?
Is there a chance that something will break after this change?
@thomasheartman:
Good question and good looking out 😄
You **could** pass non-string, top-level properties into the API before. In other words, this would be allowed:
```js
{
appName: "my-app",
nested: { object: "accepted" }
}
```
But notably, non-string values under `properties` would **not** be accepted:
```js
{
appName: "my-app",
properties: {
nested: { object: "not accepted" }
}
}
```
**However**, the values would not contribute to the evaluation of any constraints (because their type is invalid), so they would effectively be ignored.
Now, however, you'll instead get a 400 saying that the "nested" value must be a string.
I would consider this a bug fix because:
- if you sent a nested object before, it was most likely an oversight
- if you sent the nested object on purpose, expecting it to work, you would be perplexed as to why it didn't work, as the API accepted it happily
Furthermore, the UI will also tell you that the property must be a string now if you try to do it from the UI.
On the other hand, this does mean that while you could send absolute garbage in before and we would just ignore it, we don't do that anymore. This does go against how we allow you to send anything for pretty much all other objects in our API.
However, the SDK context is special. Arbitrary keys aren't ignored, they're actually part of the context itself and as such should have a valid value.
So if anything breaks, I think it breaks in a way that tells you why something wasn't working before. However, I'd love to hear your take on it and we can re-evaluate whether this is the right fix, if you think it isn't.
@kwasniew:
Coming from the https://en.wikipedia.org/wiki/Robustness_principle mindset I'm thinking if ignoring the fields that are incorrect wouldn't be a better option. So we'd accept incorrect value and drop it instead of:
* failing with client error (as this PR) or
* saving incorrect value (as previous code we had)
@thomasheartman:
Yeah, I considered that too. In fact, that was my initial idea (for the reason you stated). However, there's a couple tradeoffs here (as always):
1. If we just ignore those values, the end user doesn't know what's happened unless they go and dig through the responses. And even then, they don't necessarily know why the value is gone.
2. As mentioned, for the context, arbitrary keys can't be ignored, because we use them to build the context. In other words, they're actually invalid input.
Now, I agree that you should be liberal in what you accept and try to handle things gracefully, but that means you need to have a sensible default to fall back to. Or, to quote the Wikipedia article (selectively; with added emphasis):
> programs that receive messages should accept non-conformant input **as long as the meaning is clear**.
In this case, the meaning isn't clear when you send extra context values that aren't strings.
For instance, what's the meaning here:
```js
{
appName: "my-app",
nested: { object: "accepted", more: { further: "nesting" } }
}
```
If you were trying to use the `nested` value as an object, then that won't work. Ideally, you should be alerted.
Should we "unwind" the object and add all string keys as context values? That doesn't sound very feasible **or** necessarily like the right thing.
Did you just intend to use the `appName` and for the `nested` object to be ignored?
And it's because of this caveat that I'm not convinced just ignoring the keys are the right thing to do. Because if you do, the user never knows they were ignored or why.
----
**However**, I'd be in favor of ignoring they keys if we could **also** give the users warnings at the same time. (Something like what we do in the CR api, right? Success with warnings?)
If we can tell the user that "we ignored the `a`, `b`, and `c` keys in the context you sent because they are invalid values. Here is the result of the evaluation without taking those keys into account: [...]", then I think that's the ideal solution.
But of course, the tradeoff is that that increases the complexity of the API and the complexity of the task. It also requires UI adjustments etc. This means that it's not a simple fix anymore, but more of a mini-project.
But, in the spirit of the playground, I think it would be a worthwhile thing to do because it helps people learn and understand how Unleash works.
Via the API you can currently create gradualRollout strategies without
any parameters set, when visiting the UI afterwards, you can edit this,
because the UI reads the parameter list from the database and sees that
some parameters are required, and refuses to accept the data. This PR
adds defaults for gradualRollout strategies created from the API, making
sure gradual rollout strategies always have `rollout`, `groupId` and
`stickiness` set.
Provides store method for retrieving traffic usage data based on
period parameter, and UI + ui hook with the new chart for displaying
traffic usage data spread out over selectable month.
In this PR we copied and adapted a plugin written by DX for highlighting
a column in the chart:
There are some minor improvements planned which will come in a separate
PR, reversing the order in legend and tooltip so the colors go from
light to dark, and adding a month -sum below the legend
## Discussion points
- Should any of this be extracted as a separate reusable component?
---------
Co-authored-by: Nuno Góis <github@nunogois.com>
## About the changes
There seems to be a typo in the authorization header. We're keeping the
old typo as preferred just in case, but if not present we'll default to
the authorization header (not authorisation).
Not sure about the impact of this bug, as all registrations might be
using default project.
The changes to arbitraries here is to make typescript agree with our
schema types. Seems like somewhere between 4.8.4 and 5.4.2, typescript
got stricter.
## About the changes
We see some logs with: `Failed to store events: Error: The query is
empty` which suggests we're not sending events to batchStore. This will
help us confirm that and will give us better insights
An entrypoint for enterprise to register a hook which will be called
before the database and scheduler services are torn down. That way
enterprise can also perform graceful shutdown.
There was no need to join the entire metrics table, as it is a huge
table. We only needed all combinations of app_name, environment, and
feature_name. The new query retrieves all this data, which will then be
joined into the main query.
To check that users do indeed have permissions to update the roles from
project-service, we've been depending on req.user.id.
We had one error on Friday March 8th, where we managed to send
undefined/null to a method that requires a number. This PR assumes that
if we have an API token, and we have admin permissions and userId is not
set we're a legacy admin token.
It uses the util method for extractUserId(req: IAuthRequest | IApiRequest), so if we've passed through the apiTokenMiddleware first, we'll have userId -42, if we haven't, we'll get -1337.
Now frontend API requests will be counted separately under
getAllByfrontend. We are already tracking new FE db calls, so we can
build grafana dashboard.
https://linear.app/unleash/issue/2-2022/improve-actions-validation
Improves our current actions form validation.
Empty actions are now ignored on the payload and we get errors in
actions where any of the required fields are empty.
Also refactored our current actions into a constant map that can be
shared across frontend and backend.
## About the changes
This ports the CRUD store into OSS which is an abstraction to reduce the
amount of boilerplate code we have to write in stores.
By extending CRUDStore, the store becomes simply the type definition:
```typescript
type ActionModel = {
actionSetId: number;
action: string;
executionParams: Record<string, unknown>;
createdByUserId: number;
sortOrder: number;
};
export class ActionStore extends CRUDStore<
ActionModel & { id: number; createdAt: Date },
ActionModel
> {
}
```
And eventually specific mappings between those types can be provided (if
the mapping is more complex than camelCase -> snake_case):
```typescript
toRow: ({ project, name, actor, match, createdByUserId }) => ({
created_by_user_id: createdByUserId,
project,
name,
actor_id: actor,
source: match.source,
source_id: match.sourceId,
payload: match.payload,
}),
fromRow: ({
id,
created_at,
created_by_user_id,
project,
name,
actor_id,
source,
source_id,
payload,
}) => ({
id,
createdAt: created_at,
createdByUserId: created_by_user_id,
project,
name,
actor: actor_id,
match: {
source,
sourceId: source_id,
payload,
},
}),
```
Stores can also be extended to include additional functionality in case
you need to join with another table or do an aggregation, but it
significantly reduces the amount of boilerplate code needed to create a
basic store
We should not try to release the migration lock if where unable to
acquire it. By trying to close it when we have not successfully
connected to the database we end up hanging for a while before the
process is eventually killed.
I did not add a better error-message, as Unleash now gives a better
error stack and crashes immediate if you start without a database
password. We should still consider if you need to specify db credentials
or not. Technically it is possible to have a postgres without a password
(but it is likely not common).
Closes: #6408
## About the changes
We don't have a meaningful error for limits established by the
application. This could be a good starting point.
The error code is 400 cause I couldn't find anything better.
The name of the error was picked from UnleashApiErrorTypes:
2d8e9f87ff/src/lib/error/unleash-error.ts (L4-L34)
This column has not been used for 1.5 years and was replace by
**archived_at** column and people still get confused of why this is not
working as name suggests. Removing this column to remove technical debt.
## About the changes
Some of our metrics are not labeled correctly, one example is
`<base-path>/api/frontend/client/metrics` is labeled as
`/client/metrics`. We can see that in internal-backstage/prometheus:
This issue affects all endpoints that fail to validate the request body.
Also, endpoints that are rejected by the authorization-middleware or the
api-token-middleware are reported as `(hidden)`.
To gain more insights on our api usage but being protective of metrics
cardinality we're prefixing `(hidden)` with some well known base urls:
https://github.com/Unleash/unleash/pull/6400/files#diff-1ed998ca46ffc97c9c0d5d400bfd982dbffdb3004b78a230a8a38e7644eee9b6R17-R33
## How to reproduce:
Make an invalid call to metrics (e.g. stop set to null), then check
/internal-backstage/prometheus and find the 400 error. Expected to be at
`path="/api/client/metrics"` but will have `path=""`:
```shell
curl -H"Authorization: *:development.unleash-insecure-client-api-token" -H'Content-type: application/json' localhost:4242/api/client/metrics -d '{
"appName": "bash-test",
"instanceId": "application-name-dacb1234",
"environment": "development",
"bucket": {
"start": "2023-07-27T11:23:44Z",
"stop": null,
"toggles": {
"myCoolToggle": {
"yes": 25,
"no": 42,
"variants": {
"blue": 6,
"green": 15,
"red": 46
}
},
"myOtherToggle": {
"yes": 0,
"no": 100
}
}
}
}'
```
SCIM synchronizations requires a stable id no matter how many changes
are made to username and email (our other unique fields). In addition,
exposing internal incremented database ids to an external service (our
current id field) feels insecure. Our plan is to create either a uuidv7
or ulid when scim operations are performed against the user, so the
external scim provisioner has a stable globally unique id to use to
refer to the users they're modifying.
In order to stop privilege escalation via
`/api/admin/projects/:project/users/:userId/roles` and
`/api/admin/projects/:project/groups/:groupId/roles` this PR adds the
same check we added to setAccess methods to the methods updating access
for these two methods.
Also adds tests that verify that we throw an exception if you try to
assign roles you do not have.
Thank you @nunogois for spotting this during testing.
So, since our assumption about client instances ended up being wrong (or, less than stable).
This PR moves the EdgeUpgradeBanner to be displayed if the featureflag
displayEdgeBanner is enabled. That way, if customers comes back and says
they have upgraded but still get the banner, we can remove them from the
segment.
## About the changes
When edge is configured to automatically generate tokens, it requires
the token to be present in all unleash instances.
It's behind a flag which enables us to turn it on on a case by case
scenario.
The risk of this implementation is that we'd be adding load to the
database in the middleware that evaluates tokens (which are present in
mostly all our API calls. We only query when the token is missing but
because the /client and /frontend endpoints which will be the affected
ones are high throughput, we want to be extra careful to avoid DDoSing
ourselves
## Alternatives:
One alternative would be that we merge the two endpoints into one.
Currently, Edge does the following:
If the token is not valid, it tries to create a token using a service
account token and /api/admin/create-token endpoint. Then it uses the
token generated (which is returned from the prior endpoint) to query
/api/frontend. What if we could call /api/frontend with the same service
account we use to create the token? It may sound risky but if the same
application holding the service account token with permission to create
a token, can call /api/frontend via the generated token, shouldn't it be
able to call the endpoint directly?
The purpose of the token is authentication and authorization. With the
two tokens we are authenticating the same app with 2 different
authorization scopes, but because it's the same app we are
authenticating, can't we just use one token and assume that the app has
both scopes?
If the service account already has permissions to create a token and
then use that token for further actions, allowing it to directly call
/api/frontend does not necessarily introduce new security risks. The
only risk is allowing the app to generate new tokens. Which leads to the
third alternative: should we just remove this option from edge?
Since we're polling for updates to max revision id every second, and
listening for update events for revision id in the proxy repository then
running a refresh interval of 20secs in the proxy repo refresh seems
excessive.
This PR changes the frequency of the refresh to once per 45mins.
This PR adds a property issues to application schema, and also adds all
the missing features that have been reported by SDK, but do not exist in
Unleash.
Adds a migration to create and fill the `project_metrics_summary_trends`
This table is to be used in enterprise to update the metrics data daily
per project (after the aggregation of the hourly data)
Driving force for this was doing performance testing on the executive
dashboard.
This will allow to remove the expensive query to aggregate the data on
demand and will drop the total response time from 2.5sec to 125ms
(measurements done with 100 Projects, 10000 features and over 1M rows in
`client_metrics_env_daily`
Closes #
[1-2080](https://linear.app/unleash/issue/1-2080/create-the-project-metrics-summary-trends-table)
---------
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Follow up to: https://github.com/Unleash/unleash/pull/6298
We no longer need this table, since it was superseded by `action_events`
and is no longer used.
I believe it's safe to drop this table right away since the feature is
not being used yet.
https://linear.app/unleash/issue/2-1962/implement-new-action-events-logic
Adds a new `action_set_events` table for the new action events logic.
Even though observable events are technically immutable, we're storing
the observable event along with the action set event. This allows us to
avoid 1 join while allowing us to persist action set event information
after deleting observable events, if we wish to do so at a later stage.
In order to prevent users from being able to assign roles/permissions
they don't have, this PR adds a check that the user performing the
action either is Admin, Project owner or has the same role they are
trying to grant/add.
This addAccess method is only used from Enterprise, so there will be a
separate PR there, updating how we return the roles list for a user, so
that our frontend can only present the roles a user is actually allowed
to grant.
This adds the validation to the backend to ensure that even if the
frontend thinks we're allowed to add any role to any user here, the
backend can be smart enough to stop it.
We should still update frontend as well, so that it doesn't look like we
can add roles we won't be allowed to.
## About the changes
Our frontend API creates new instances of unleash-client-proxy. Because
this is by-design, we don't want to log a warning that was designed to
warn users about potential misconfiguration of Unleash Proxy.
As an extra, I'm renaming ProxyController to FrontendAPIController to
better reflect the intent of this controller.
## About the changes
This is a rough initial version as a PoC for a permission matrix.
This is only available after enabling the flag `userAccessUIEnabled`
that is set to true by default in local development.
The access was added to the users' admin page but could be embedded in
different contexts (e.g. when assigning a role to a user):
This is how the matrix looks like
---------
Co-authored-by: Nuno Góis <github@nunogois.com>
Fixes ##5799 and #5785
When you do not provide a token we should resolve to the "default"
environment to maintain backward compatibility. If you actually provide
a token we should prefer that and even block the request if it is not
valid.
An interesting fact is that "default" environment is not available on a
fresh installation of Unleash. This means that you need to provide a
token to actually get access to toggle configurations.
---------
Co-authored-by: Thomas Heartman <thomas@getunleash.io>
Created a build script that generates orval schemas with automatic
cleanup. Also generating new ones.
1. yarn gen:api **(generates schemas)**
2. rm -rf src/openapi/apis **(remove apis)**
3. sed -i '1q' src/openapi/index.ts **(remove all rows except first)**
This change takes the (now rather involved) type used to send CR
schedule suspension emails and extracts it into a proper exported type.
This will allow us to import it in enterprise as well instead of
redefining it.
