Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2025-03-18 00:19:49 +01:00
Code Issues Projects Releases Wiki Activity

Fix: validate that the project is correct when getting feature by project (#2344)

Browse Source 
## What

This PR fixes a bug where fetching a feature toggle via the
`/api/admin/projects/:projectId/features/:featureName` endpoint doesn't
validate that the feature belongs to the provided project. The same
thing applies to the archive functionality. This has also been fixed.

In doing so, it also adds corresponding tests to check for edge cases,
updates the 403 error response we use to provide clearer steps for the
user, and adds more error responses to the OpenAPI documentation.

## Why

As mentioned in #2337, it's unexpected that the provided project
shouldn't matter at all, and after discussions internally, it was also
discovered that this was never intended to be the case.

## Discussion points

It might be worth rethinking this for Unleash v5. Why does the features
API need the projects part at all when features are unique across the
entire instance? Would it be worth reverting to a simpler feature API
later or would that introduce issues with regards to how different
projects can have different active environments and so on?

### Further improvements

I have _not_ provided schemas for the error responses for the endpoints
at this time. I considered it, but because it would introduce new schema
code, more tests, etc, I decided to leave it for later. There's a
thorough OpenAPI walkthrough coming up, so I think it makes sense to do
it as part of that work instead. I am happy to be challenged on this,
however, and will implement it if you think it's better.

### Why 403 when the project is wrong?

We could also have used the 404 status code for when the feature exists
but doesn't belong to this project, but this would require more (and
more complex) code. We also already use 403 for cases like this for
post, patch, and put. Finally, the [HTTP spec's section on the 403
status code](https://httpwg.org/specs/rfc9110.html#status.403) says the
following (emphasis mine):

> The 403 (Forbidden) status code indicates that the server
**_understood the request but refuses to fulfill it_**. A server that
wishes to make public why the request has been forbidden can describe
that reason in the response content (if any).
>
> If authentication credentials were provided in the request, the server
considers them insufficient to grant access. The client SHOULD NOT
automatically repeat the request with the same credentials. The client
MAY repeat the request with new or different credentials. However, **_a
request might be forbidden for reasons unrelated to the credentials_**.

As such, I think using 403 makes sense in this case.

---

Closes #2337.
This commit is contained in:
Thomas Heartman 2022-11-08 13:34:01 +01:00 committed by GitHub
parent 88a9e0cb9b
commit f5fb7b66d1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 166 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
src/lib/routes/admin-api/project/features.ts
View File

@ -38,7 +38,10 @@ import { createResponseSchema } from '../../../openapi/util/create-response-sche
import { FeatureEnvironmentSchema } from '../../../openapi/spec/feature-environment-schema';
import { SetStrategySortOrderSchema } from '../../../openapi/spec/set-strategy-sort-order-schema';
import { emptyResponse } from '../../../openapi/util/standard-responses';
import {
emptyResponse,
getStandardResponses,
} from '../../../openapi/util/standard-responses';
import { SegmentService } from '../../../services/segment-service';
interface FeatureStrategyParams {
@ -319,7 +322,17 @@ export default class ProjectFeaturesController extends Controller {
openApiService.validPath({
operationId: 'getFeature',
tags: ['Features'],
responses: { 200: createResponseSchema('featureSchema') },
description:
'This endpoint returns the information about the requested feature if the feature belongs to the specified project.',
summary: 'Get a feature.',
responses: {
200: createResponseSchema('featureSchema'),
403: {
description:
'You either do not have the required permissions or used an invalid URL.',
},
...getStandardResponses(401, 404),
},
}),
],
});
@ -364,7 +377,17 @@ export default class ProjectFeaturesController extends Controller {
openApiService.validPath({
tags: ['Features'],
operationId: 'archiveFeature',
responses: { 200: emptyResponse },
description:
'This endpoint archives the specified feature if the feature belongs to the specified project.',
summary: 'Archive a feature.',
responses: {
200: emptyResponse,
403: {
description:
'You either do not have the required permissions or used an invalid URL.',
},
...getStandardResponses(401, 404),
},
}),
],
});
@ -438,8 +461,12 @@ export default class ProjectFeaturesController extends Controller {
req: Request<FeatureParams, any, any, any>,
res: Response,
): Promise<void> {
const { featureName } = req.params;
const feature = await this.featureService.getFeature(featureName);
const { featureName, projectId } = req.params;
const feature = await this.featureService.getFeature(
featureName,
false,
projectId,
);
res.status(200).json(feature);
}
@ -493,7 +520,6 @@ export default class ProjectFeaturesController extends Controller {
);
}
// TODO: validate projectId
async archiveFeature(
req: IAuthRequest<
{ projectId: string; featureName: string },
@ -503,9 +529,13 @@ export default class ProjectFeaturesController extends Controller {
>,
res: Response<void>,
): Promise<void> {
const { featureName } = req.params;
const { featureName, projectId } = req.params;
const userName = extractUsername(req);
await this.featureService.archiveToggle(featureName, userName);
await this.featureService.archiveToggle(
featureName,
userName,
projectId,
);
res.status(202).send();
}

29
src/lib/services/feature-toggle-service.ts
View File

@ -159,7 +159,7 @@ class FeatureToggleService {
const id = await this.featureToggleStore.getProjectId(featureName);
if (id !== projectId) {
throw new InvalidOperationError(
'Project id does not match the project that the feature belongs to',
`The operation could not be completed. The feature exists, but the provided project id ("${projectId}") does not match the project that the feature belongs to ("${id}"). Try using "${id}" in the request URL instead of "${projectId}".`,
);
}
}
@ -548,15 +548,22 @@ class FeatureToggleService {
* GET /api/admin/projects/:project/features/:featureName
* @param featureName
* @param archived - return archived or non archived toggles
* @param projectId - provide if you're requesting the feature in the context of a specific project.
*/
async getFeature(
featureName: string,
archived: boolean = false,
projectId?: string,
): Promise<FeatureToggleWithEnvironment> {
return this.featureStrategiesStore.getFeatureToggleWithEnvs(
featureName,
archived,
);
const feature =
await this.featureStrategiesStore.getFeatureToggleWithEnvs(
featureName,
archived,
);
if (projectId) {
await this.validateFeatureContext({ featureName, projectId });
}
return feature;
}
/**
@ -865,9 +872,17 @@ class FeatureToggleService {
return feature;
}
// todo: add projectId
async archiveToggle(featureName: string, createdBy: string): Promise<void> {
async archiveToggle(
featureName: string,
createdBy: string,
projectId?: string,
): Promise<void> {
const feature = await this.featureToggleStore.get(featureName);
if (projectId) {
await this.validateFeatureContext({ featureName, projectId });
}
await this.featureToggleStore.archive(featureName);
const tags = await this.tagStore.getAllTagsForFeature(featureName);
await this.eventStore.store(

85
src/test/e2e/api/admin/project/features.e2e.test.ts
View File

@ -13,7 +13,7 @@ import {
import ApiUser from '../../../../../lib/types/api-user';
import { ApiTokenType } from '../../../../../lib/types/models/api-token';
import IncompatibleProjectError from '../../../../../lib/error/incompatible-project-error';
import { IVariant, WeightType } from '../../../../../lib/types/model';
import { IVariant, RoleName, WeightType } from '../../../../../lib/types/model';
import { v4 as uuidv4 } from 'uuid';
import supertest from 'supertest';
import { randomId } from '../../../../../lib/util/random-id';
@ -459,6 +459,89 @@ test('Getting feature that does not exist should yield 404', async () => {
.expect(404);
});
describe('Interacting with features using project IDs that belong to other projects', () => {
const otherProject = 'project2';
const featureName = 'new-toggle';
const nonExistingProject = 'this-is-not-a-project';
beforeAll(async () => {
const dummyAdmin = await app.services.userService.createUser({
name: 'Some Name',
email: 'test@getunleash.io',
rootRole: RoleName.ADMIN,
});
await app.services.projectService.createProject(
{ name: otherProject, id: otherProject },
dummyAdmin,
);
// ensure the new project has been created
await app.request
.get(`/api/admin/projects/${otherProject}`)
.expect(200);
// create toggle in default project
await app.request
.post('/api/admin/projects/default/features')
.send({ name: featureName })
.expect(201);
});
afterAll(async () => {
await db.stores.projectStore.delete(otherProject);
await db.stores.featureToggleStore.delete(featureName);
await db.stores.userStore.deleteAll();
});
test("Getting a feature yields 403 if the provided project id doesn't match the feature's project", async () => {
await app.request
.get(`/api/admin/projects/${otherProject}/features/${featureName}`)
.expect(403);
});
test("Getting a feature yields 403 if the provided project doesn't exist", async () => {
await app.request
.get(
`/api/admin/projects/${nonExistingProject}/features/${featureName}`,
)
.expect(403);
});
test("Archiving a feature yields 403 if the provided project id doesn't match the feature's project", async () => {
await app.request
.delete(
`/api/admin/projects/${otherProject}/features/${featureName}`,
)
.expect(403);
});
test("Archiving a feature yields 403 if the provided project doesn't exist", async () => {
await app.request
.delete(
`/api/admin/projects/${nonExistingProject}/features/${featureName}`,
)
.expect(403);
});
test("Trying to archive a feature that doesn't exist should yield a 404, regardless of whether the project exists or not.", async () => {
await app.request
.delete(
`/api/admin/projects/${nonExistingProject}/features/${
featureName + featureName
}`,
)
.expect(404);
await app.request
.delete(
`/api/admin/projects/${otherProject}/features/${
featureName + featureName
}`,
)
.expect(404);
});
});
test('Should update feature toggle', async () => {
const url = '/api/admin/projects/default/features';
const name = 'new.toggle.update';

22
src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap
View File

@ -4991,6 +4991,7 @@ If the provided project does not exist, the list of events will be empty.",
},
"/api/admin/projects/{projectId}/features/{featureName}": {
"delete": {
"description": "This endpoint archives the specified feature if the feature belongs to the specified project.",
"operationId": "archiveFeature",
"parameters": [
{
@ -5014,12 +5015,23 @@ If the provided project does not exist, the list of events will be empty.",
"200": {
"description": "This response has no body.",
},
"401": {
"description": "Authorization information is missing or invalid. Provide a valid API token as the \`authorization\` header, e.g. \`authorization:*.*.my-admin-token\`.",
},
"403": {
"description": "You either do not have the required permissions or used an invalid URL.",
},
"404": {
"description": "The requested resource was not found.",
},
},
"summary": "Archive a feature.",
"tags": [
"Features",
],
},
"get": {
"description": "This endpoint returns the information about the requested feature if the feature belongs to the specified project.",
"operationId": "getFeature",
"parameters": [
{
@ -5050,7 +5062,17 @@ If the provided project does not exist, the list of events will be empty.",
},
"description": "featureSchema",
},
"401": {
"description": "Authorization information is missing or invalid. Provide a valid API token as the \`authorization\` header, e.g. \`authorization:*.*.my-admin-token\`.",
},
"403": {
"description": "You either do not have the required permissions or used an invalid URL.",
},
"404": {
"description": "The requested resource was not found.",
},
},
"summary": "Get a feature.",
"tags": [
"Features",
],