Skip to content

Commit

Permalink
[RAM] Type safety for RegistryRuleType (#164516)
Browse files Browse the repository at this point in the history
## Summary

RegisterRuleType was not representing its definition correctly and the
type was not safe. So updated the code/type to avoid missing attribute
and functionality. Now summary alert is back
  

![image](https://github.com/elastic/kibana/assets/189600/95cb786a-7ab9-4d40-a65c-ecbe59a0fbd3)


### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
  • Loading branch information
XavierM authored Aug 23, 2023
1 parent dd0938b commit 0d85af2
Show file tree
Hide file tree
Showing 12 changed files with 438 additions and 325 deletions.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,6 @@ interface HasPrivileges {
type AuthorizedConsumers = Record<string, HasPrivileges>;
export interface RegistryAlertTypeWithAuth extends RegistryRuleType {
authorizedConsumers: AuthorizedConsumers;
hasAlertsMappings?: boolean;
hasFieldsForAAD?: boolean;
}

type IsAuthorizedAtProducerLevel = boolean;
Expand Down Expand Up @@ -371,7 +369,7 @@ export class AlertingAuthorization {
const ruleTypesWithAuthorization = Array.from(
this.augmentWithAuthorizedConsumers(ruleTypes, {})
);
const ruleTypesAuthorized: Map<string, RegistryRuleType> = new Map();
const ruleTypesAuthorized: Map<string, RegistryAlertTypeWithAuth> = new Map();
// map from privilege to ruleType which we can refer back to when analyzing the result
// of checkPrivileges
const privilegeToRuleType = new Map<
Expand All @@ -386,9 +384,7 @@ export class AlertingAuthorization {
const ruleTypeAuth = ruleTypesWithAuthorization.find((rtwa) => rtwa.id === ruleTypeId);
if (ruleTypeAuth) {
if (!ruleTypesAuthorized.has(ruleTypeId)) {
const { authorizedConsumers, hasAlertsMappings, hasFieldsForAAD, ...ruleType } =
ruleTypeAuth;
ruleTypesAuthorized.set(ruleTypeId, ruleType);
ruleTypesAuthorized.set(ruleTypeId, ruleTypeAuth);
}
for (const operation of operations) {
privilegeToRuleType.set(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
myApp: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]),
{
Expand Down Expand Up @@ -67,6 +69,8 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
myOtherApp: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]),
{
Expand Down Expand Up @@ -105,6 +109,8 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
myAppWithSubFeature: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
{
actionGroups: [],
Expand All @@ -122,6 +128,8 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
myAppWithSubFeature: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
{
actionGroups: [],
Expand All @@ -139,6 +147,8 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
myAppWithSubFeature: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]),
{
Expand Down Expand Up @@ -177,6 +187,8 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
myAppWithSubFeature: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
{
actionGroups: [],
Expand All @@ -194,6 +206,8 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
myAppWithSubFeature: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]),
{
Expand Down Expand Up @@ -233,6 +247,8 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
myAppWithSubFeature: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
{
actionGroups: [],
Expand All @@ -250,6 +266,8 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
myAppWithSubFeature: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]),
{
Expand Down Expand Up @@ -283,6 +301,8 @@ describe('asKqlFiltersByRuleTypeAndConsumer', () => {
isExportable: true,
authorizedConsumers: {},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]),
{
Expand Down Expand Up @@ -317,6 +337,8 @@ describe('asEsDslFiltersByRuleTypeAndConsumer', () => {
myApp: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]),
{
Expand Down Expand Up @@ -379,6 +401,8 @@ describe('asEsDslFiltersByRuleTypeAndConsumer', () => {
myOtherApp: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]),
{
Expand Down Expand Up @@ -449,6 +473,8 @@ describe('asEsDslFiltersByRuleTypeAndConsumer', () => {
myAppWithSubFeature: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
{
actionGroups: [],
Expand All @@ -466,6 +492,8 @@ describe('asEsDslFiltersByRuleTypeAndConsumer', () => {
myAppWithSubFeature: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
{
actionGroups: [],
Expand All @@ -483,6 +511,8 @@ describe('asEsDslFiltersByRuleTypeAndConsumer', () => {
myAppWithSubFeature: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]),
{
Expand Down Expand Up @@ -646,6 +676,8 @@ describe('asEsDslFiltersByRuleTypeAndConsumer', () => {
isExportable: true,
authorizedConsumers: {},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]),
{
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/server/routes/health.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ const ruleTypes = [
enabledInLicense: true,
minimumScheduleInterval: '1m',
defaultScheduleInterval: '10m',
hasAlertsMappings: false,
hasFieldsForAAD: false,
} as RegistryAlertTypeWithAuth,
];

Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/server/routes/legacy/health.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ const ruleTypes = [
producer: 'test',
enabledInLicense: true,
defaultScheduleInterval: '10m',
hasAlertsMappings: false,
hasFieldsForAAD: false,
} as RegistryAlertTypeWithAuth,
];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ describe('listAlertTypesRoute', () => {
},
producer: 'test',
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
} as RegistryAlertTypeWithAuth,
];
rulesClient.listRuleTypes.mockResolvedValueOnce(new Set(listTypes));
Expand All @@ -85,6 +87,8 @@ describe('listAlertTypesRoute', () => {
"authorizedConsumers": Object {},
"defaultActionGroupId": "default",
"enabledInLicense": true,
"hasAlertsMappings": false,
"hasFieldsForAAD": false,
"id": "1",
"isExportable": true,
"minimumLicenseRequired": "basic",
Expand Down Expand Up @@ -137,6 +141,8 @@ describe('listAlertTypesRoute', () => {
},
producer: 'alerts',
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
} as RegistryAlertTypeWithAuth,
];

Expand Down Expand Up @@ -190,6 +196,8 @@ describe('listAlertTypesRoute', () => {
},
producer: 'alerts',
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
} as RegistryAlertTypeWithAuth,
];

Expand Down
5 changes: 5 additions & 0 deletions x-pack/plugins/alerting/server/routes/rule_types.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ describe('ruleTypesRoute', () => {
defaultScheduleInterval: '10m',
doesSetRecoveryContext: false,
hasAlertsMappings: true,
hasFieldsForAAD: false,
} as RegistryAlertTypeWithAuth,
];
const expectedResult: Array<AsApiContract<RegistryAlertTypeWithAuth>> = [
Expand Down Expand Up @@ -169,6 +170,8 @@ describe('ruleTypesRoute', () => {
},
producer: 'alerts',
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
} as RegistryAlertTypeWithAuth,
];

Expand Down Expand Up @@ -222,6 +225,8 @@ describe('ruleTypesRoute', () => {
},
producer: 'alerts',
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
} as RegistryAlertTypeWithAuth,
];

Expand Down
50 changes: 28 additions & 22 deletions x-pack/plugins/alerting/server/rule_type_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ export interface RegistryRuleType
> {
id: string;
enabledInLicense: boolean;
hasFieldsForAAD: boolean;
hasAlertsMappings: boolean;
}

/**
Expand Down Expand Up @@ -362,26 +364,28 @@ export class RuleTypeRegistry {
}

public list(): Set<RegistryRuleType> {
return new Set(
Array.from(this.ruleTypes).map(
([
id,
{
name,
actionGroups,
recoveryActionGroup,
defaultActionGroupId,
actionVariables,
producer,
minimumLicenseRequired,
isExportable,
ruleTaskTimeout,
defaultScheduleInterval,
doesSetRecoveryContext,
alerts,
fieldsForAAD,
},
]: [string, UntypedNormalizedRuleType]) => ({
const mapRuleTypes: Array<[string, UntypedNormalizedRuleType]> = Array.from(this.ruleTypes);
const tempRegistryRuleType = mapRuleTypes.map<RegistryRuleType>(
([
id,
{
name,
actionGroups,
recoveryActionGroup,
defaultActionGroupId,
actionVariables,
producer,
minimumLicenseRequired,
isExportable,
ruleTaskTimeout,
defaultScheduleInterval,
doesSetRecoveryContext,
alerts,
fieldsForAAD,
},
]) => {
// KEEP the type here to be safe if not the map is ignoring it for some reason
const ruleType: RegistryRuleType = {
id,
name,
actionGroups,
Expand All @@ -402,9 +406,11 @@ export class RuleTypeRegistry {
hasFieldsForAAD: Boolean(fieldsForAAD),
hasAlertsMappings: !!alerts,
...(alerts ? { alerts } : {}),
})
)
};
return ruleType;
}
);
return new Set(tempRegistryRuleType);
}

public getAllTypes(): string[] {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ describe('aggregate()', () => {
name: 'myType',
producer: 'myApp',
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]);
beforeEach(() => {
Expand Down Expand Up @@ -156,6 +158,8 @@ describe('aggregate()', () => {
myApp: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
])
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ describe('find()', () => {
name: 'myType',
producer: 'myApp',
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]);
beforeEach(() => {
Expand Down Expand Up @@ -142,6 +144,8 @@ describe('find()', () => {
myApp: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
])
);
Expand Down Expand Up @@ -453,6 +457,8 @@ describe('find()', () => {
name: 'myType',
producer: 'myApp',
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
])
);
Expand Down Expand Up @@ -659,6 +665,8 @@ describe('find()', () => {
name: 'myType',
producer: 'myApp',
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
])
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ const listedTypes = new Set<RegistryRuleType>([
name: 'myType',
producer: 'myApp',
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
]);

Expand Down Expand Up @@ -112,6 +114,8 @@ describe('getTags()', () => {
myApp: { read: true, all: true },
},
enabledInLicense: true,
hasAlertsMappings: false,
hasFieldsForAAD: false,
},
])
);
Expand Down
Loading

0 comments on commit 0d85af2

Please sign in to comment.