Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task/use audit info in events #6872

Merged
merged 11 commits into from
Apr 18, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
} from '../../../../test/e2e/helpers/test-helper';
import getLogger from '../../../../test/fixtures/no-logger';
import { DEFAULT_ENV } from '../../../util/constants';
import type { IUserWithRootRole } from '../../../types';
import { type IUserWithRootRole, TEST_AUDIT_USER } from '../../../types';

let app: IUnleashTest;
let db: ITestDb;
Expand Down Expand Up @@ -73,11 +73,14 @@ beforeAll(async () => {
db.rawDatabase,
);

dummyAdmin = await app.services.userService.createUser({
name: 'Some Name',
email: '[email protected]',
rootRole: RoleName.ADMIN,
});
dummyAdmin = await app.services.userService.createUser(
{
name: 'Some Name',
email: '[email protected]',
rootRole: RoleName.ADMIN,
},
TEST_AUDIT_USER,
);
});

afterEach(async () => {
Expand Down Expand Up @@ -137,10 +140,12 @@ test('should support filtering on project', async () => {
await app.services.projectService.createProject(
{ name: 'projectA', id: 'projecta' },
dummyAdmin,
TEST_AUDIT_USER,
);
await app.services.projectService.createProject(
{ name: 'projectB', id: 'projectb' },
dummyAdmin,
TEST_AUDIT_USER,
);
await app.createFeature('ab_test1', 'projecta');
await app.createFeature('bd_test2', 'projectb');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ export default class DependentFeaturesController extends Controller {
feature,
},
req.user,
req.audit,
),
);

Expand All @@ -238,6 +239,7 @@ export default class DependentFeaturesController extends Controller {
},
projectId,
req.user,
req.audit,
),
);
res.status(200).end();
Expand All @@ -250,7 +252,12 @@ export default class DependentFeaturesController extends Controller {
const { child, projectId } = req.params;

await this.dependentFeaturesService.transactional((service) =>
service.deleteFeaturesDependencies([child], projectId, req.user),
service.deleteFeaturesDependencies(
[child],
projectId,
req.user,
req.audit,
),
);
res.status(200).end();
}
Expand Down
92 changes: 46 additions & 46 deletions src/lib/features/dependent-features/dependent-features-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ import type {
} from './dependent-features';
import type { IDependentFeaturesReadModel } from './dependent-features-read-model-type';
import type { EventService } from '../../services';
import type { IUser } from '../../server-impl';
import { SKIP_CHANGE_REQUEST } from '../../types';
import type { IAuditUser, IUser } from '../../server-impl';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No longer an issue: Primitive Obsession
The ratio of primivite types in function arguments is no longer above the threshold

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No longer an issue: String Heavy Function Arguments
The ratio of strings in function arguments is no longer above the threshold

import {
FeatureDependenciesRemovedEvent,
FeatureDependencyAddedEvent,
FeatureDependencyRemovedEvent,
SKIP_CHANGE_REQUEST,
} from '../../types';
import type { IChangeRequestAccessReadModel } from '../change-request-access-service/change-request-access-read-model';
import { extractUsernameFromUser } from '../../util';
import type { IFeaturesReadModel } from '../feature-toggle/types/features-read-model-type';

interface IDependentFeaturesServiceDeps {
Expand Down Expand Up @@ -52,8 +56,7 @@ export class DependentFeaturesService {
newFeatureName,
projectId,
}: { featureName: string; newFeatureName: string; projectId: string },
user: string,
userId: number,
auditUser: IAuditUser,
) {
const parents =
await this.dependentFeaturesReadModel.getParents(featureName);
Expand All @@ -66,8 +69,7 @@ export class DependentFeaturesService {
enabled: parent.enabled,
variants: parent.variants,
},
user,
userId,
auditUser,
),
),
);
Expand All @@ -77,22 +79,21 @@ export class DependentFeaturesService {
{ child, projectId }: { child: string; projectId: string },
dependentFeature: CreateDependentFeatureSchema,
user: IUser,
auditUser: IAuditUser,
): Promise<void> {
await this.stopWhenChangeRequestsEnabled(projectId, user);

return this.unprotectedUpsertFeatureDependency(
{ child, projectId },
dependentFeature,
extractUsernameFromUser(user),
user.id,
auditUser,
);
}

async unprotectedUpsertFeatureDependency(
{ child, projectId }: { child: string; projectId: string },
dependentFeature: CreateDependentFeatureSchema,
user: string,
userId: number,
auditUser: IAuditUser,
): Promise<void> {
const { enabled, feature: parent, variants } = dependentFeature;

Expand Down Expand Up @@ -148,82 +149,81 @@ export class DependentFeaturesService {
variants,
};
await this.dependentFeaturesStore.upsert(featureDependency);
await this.eventService.storeEvent({
type: 'feature-dependency-added',
project: projectId,
featureName: child,
createdBy: user,
createdByUserId: userId,
data: {
feature: parent,
enabled: featureDependency.enabled,
...(variants !== undefined && { variants }),
},
});
await this.eventService.storeEvent(
new FeatureDependencyAddedEvent({
project: projectId,
featureName: child,
auditUser,
data: {
feature: parent,
enabled: featureDependency.enabled,
...(variants !== undefined && { variants }),
},
}),
);
}

async deleteFeatureDependency(
dependency: FeatureDependencyId,
projectId: string,
user: IUser,
auditUser: IAuditUser,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the future, we can consider merging auditUser with user as they should always represent the same, but user has permissions and auditUser don't, but for now, let's do this as a gradual improvement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, agreed, the thing missing was having some way from audit user to UserWithPermission. so the one thing audituser has that most other user implementations do not have is the ip.

): Promise<void> {
await this.stopWhenChangeRequestsEnabled(projectId, user);

return this.unprotectedDeleteFeatureDependency(
dependency,
projectId,
extractUsernameFromUser(user),
user.id,
auditUser,
);
}

async unprotectedDeleteFeatureDependency(
dependency: FeatureDependencyId,
projectId: string,
user: string,
userId: number,
auditUser: IAuditUser,
): Promise<void> {
await this.dependentFeaturesStore.delete(dependency);
await this.eventService.storeEvent({
type: 'feature-dependency-removed',
project: projectId,
featureName: dependency.child,
createdBy: user,
createdByUserId: userId,
data: { feature: dependency.parent },
});
await this.eventService.storeEvent(
new FeatureDependencyRemovedEvent({
project: projectId,
featureName: dependency.child,
auditUser,
data: { feature: dependency.parent },
}),
);
}

async deleteFeaturesDependencies(
features: string[],
projectId: string,
user: IUser,
auditUser: IAuditUser,
): Promise<void> {
await this.stopWhenChangeRequestsEnabled(projectId, user);

return this.unprotectedDeleteFeaturesDependencies(
features,
projectId,
extractUsernameFromUser(user),
user.id,
auditUser,
);
}

async unprotectedDeleteFeaturesDependencies(
features: string[],
projectId: string,
user: string,
userId: number,
auditUser: IAuditUser,
): Promise<void> {
await this.dependentFeaturesStore.deleteAll(features);
await this.eventService.storeEvents(
features.map((feature) => ({
type: 'feature-dependencies-removed',
project: projectId,
featureName: feature,
createdBy: user,
createdByUserId: userId,
})),
features.map(
(feature) =>
new FeatureDependenciesRemovedEvent({
project: projectId,
featureName: feature,
auditUser,
}),
),
);
}

Expand Down
9 changes: 5 additions & 4 deletions src/lib/features/events/event-store.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import {
type IEvent,
type IBaseEvent,
SEGMENT_UPDATED,
FEATURE_IMPORT,
FEATURES_IMPORTED,
type IBaseEvent,
type IEvent,
type IEventType,
SEGMENT_UPDATED,
} from '../../types/events';
import type { LogProvider, Logger } from '../../logger';
import type { Logger, LogProvider } from '../../logger';
import type { IEventStore } from '../../types/stores/event-store';
import type { ITag } from '../../types/model';
import type { SearchEventsSchema } from '../../openapi/spec/search-events-schema';
Expand Down Expand Up @@ -394,6 +394,7 @@ class EventStore implements IEventStore {
feature_name: e.featureName,
project: e.project,
environment: e.environment,
ip: e.ip,
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,7 @@ class ExportImportController extends Controller {
const query = req.body;
const userName = extractUsername(req);

const data = await this.exportService.export(
query,
userName,
req.user.id,
);
const data = await this.exportService.export(query, req.audit);

this.openApiService.respondWithValidation(
200,
Expand Down Expand Up @@ -159,7 +155,7 @@ class ExportImportController extends Controller {
res: Response,
): Promise<void> {
this.verifyExportImportEnabled();
const { user } = req;
const { user, audit } = req;

if (user instanceof ApiUser && user.type === 'admin') {
throw new BadDataError(
Expand All @@ -170,7 +166,7 @@ class ExportImportController extends Controller {
const dto = req.body;

await this.importService.transactional((service) =>
service.import(dto, user),
service.import(dto, user, audit),
);

res.status(200).end();
Expand Down
Loading
Loading