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

[ResponseOps][Rules] Validate timezone in rule routes #201508

Merged
merged 30 commits into from
Jan 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c3cc6cf
Validate timezone in alerting routes
cnasikas Nov 24, 2024
5fe0245
Validate snooze ID, rrule byweekday, bymonthday, and bymonth
cnasikas Nov 24, 2024
aeeb8b1
Merge branch 'main' into fix_snooze_bug
cnasikas Nov 25, 2024
58fd530
Merge branch 'main' into fix_snooze_bug
cnasikas Nov 27, 2024
802b5b1
Do not throw an error when getting active snoozes
cnasikas Nov 28, 2024
2d2f446
Fix cypress tests
cnasikas Nov 28, 2024
f6418aa
Merge branch 'main' into fix_snooze_bug
cnasikas Dec 2, 2024
b2c9f68
Merge branch 'main' into fix_snooze_bug
cnasikas Dec 5, 2024
fd2b7c3
Merge branch 'main' into fix_snooze_bug
cnasikas Dec 9, 2024
d63506b
So warning in the UI
cnasikas Dec 9, 2024
bebe419
Merge branch 'main' into fix_snooze_bug
cnasikas Dec 10, 2024
f05fe77
Improve text
cnasikas Dec 10, 2024
ce9e4da
Add unit tests
cnasikas Dec 12, 2024
cf5ff7c
Merge branch 'main' into fix_snooze_bug
cnasikas Dec 12, 2024
2c189ec
Merge branch 'main' into fix_snooze_bug
elasticmachine Dec 19, 2024
a3eb963
Merge branch 'main' into fix_snooze_bug
cnasikas Jan 8, 2025
36baf63
Merge branch 'fix_snooze_bug' of github.com:cnasikas/kibana into fix_…
cnasikas Jan 8, 2025
6fc61f5
Remove custom validation
cnasikas Jan 8, 2025
afd0bfa
Fix types
cnasikas Jan 8, 2025
398dca8
Merge branch 'main' into fix_snooze_bug
cnasikas Jan 13, 2025
4290ec1
Add more unit tests for schema validation
cnasikas Jan 13, 2025
7c722c4
Fix bug
cnasikas Jan 13, 2025
e12c485
Draft
cnasikas Jan 13, 2025
3639b49
Merge branch 'main' into fix_snooze_bug
cnasikas Jan 20, 2025
7ce30e1
Create validateOptions function
cnasikas Jan 20, 2025
bb55b17
[CI] Auto-commit changed files from 'node scripts/styled_components_m…
kibanamachine Jan 20, 2025
8ab2768
Validate rule schedule in snooze_rule
cnasikas Jan 20, 2025
8fc77c0
Merge branch 'fix_snooze_bug' of github.com:cnasikas/kibana into fix_…
cnasikas Jan 20, 2025
2a040fa
Fix integration tests
cnasikas Jan 21, 2025
551687c
Merge branch 'main' into fix_snooze_bug
cnasikas Jan 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/platform/packages/shared/kbn-rrule/rrule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@

import moment, { type Moment } from 'moment-timezone';

import { Frequency, Weekday, type WeekdayStr, type Options, type IterOptions } from './types';
import {
Frequency,
Weekday,
type WeekdayStr,
type Options,
type IterOptions,
ConstructorOptions,
} from './types';
import { sanitizeOptions } from './sanitize';

type ConstructorOptions = Omit<Options, 'byweekday' | 'wkst'> & {
byweekday?: Array<string | number> | null;
wkst?: Weekday | WeekdayStr | number | null;
};
import { validateOptions } from './validate';

const ISO_WEEKDAYS = [
Weekday.MO,
Expand All @@ -36,6 +39,7 @@ const TIMEOUT_LIMIT = 100000;

export class RRule {
private options: Options;

constructor(options: ConstructorOptions) {
this.options = sanitizeOptions(options as Options);
if (typeof options.wkst === 'string') {
Expand Down Expand Up @@ -134,6 +138,16 @@ export class RRule {
return dates;
}
}

static isValid(options: ConstructorOptions): boolean {
try {
validateOptions(options);

return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to find a way to surface the error message that validateOptions produces instead of just turning it into a boolean. Not a blocker for this PR because I see we're just using it to display a notification badge. If there's a way to surface the error message without a huge rewrite I'd say give it a shot, otherwise feel free to merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey Zacq! In my use case, I do not want to surface the message but to know which rules have an invalid snooze schedule (very rare after the guardrails we put) and show an "Invalid Snooze" notification badge which the user can click and remove. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds all right, we've got the bones in place for surfacing the error message in future use cases

} catch (e) {
return false;
}
}
}

const parseByWeekdayPos = function (byweekday: ConstructorOptions['byweekday']) {
Expand Down
11 changes: 9 additions & 2 deletions src/platform/packages/shared/kbn-rrule/sanitize.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('sanitizeOptions', () => {
interval: 1,
until: new Date('February 25, 2022 03:24:00'),
count: 3,
tzid: 'foobar',
tzid: 'UTC',
};

it('happy path', () => {
Expand All @@ -41,11 +41,18 @@ describe('sanitizeOptions', () => {
});

it('throws an error when tzid is missing', () => {
expect(() => sanitizeOptions({ ...options, tzid: '' })).toThrowError(
// @ts-expect-error
expect(() => sanitizeOptions({ ...options, tzid: null })).toThrowError(
'Cannot create RRule: tzid is required'
);
});

it('throws an error when tzid is invalid', () => {
expect(() => sanitizeOptions({ ...options, tzid: 'invalid' })).toThrowError(
'Cannot create RRule: tzid is invalid'
);
});

it('throws an error when until field is invalid', () => {
expect(() =>
sanitizeOptions({
Expand Down
6 changes: 5 additions & 1 deletion src/platform/packages/shared/kbn-rrule/sanitize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import moment from 'moment-timezone';
import type { Options } from './types';

export function sanitizeOptions(opts: Options) {
Expand All @@ -25,6 +26,10 @@ export function sanitizeOptions(opts: Options) {
throw new Error('Cannot create RRule: dtstart is an invalid date');
}

if (moment.tz.zone(options.tzid) == null) {
throw new Error('Cannot create RRule: tzid is invalid');
}

if (options.until && isNaN(options.until.getTime())) {
throw new Error('Cannot create RRule: until is an invalid date');
}
Expand All @@ -39,7 +44,6 @@ export function sanitizeOptions(opts: Options) {
}
}

// Omit invalid options
if (options.bymonth) {
// Only months between 1 and 12 are valid
options.bymonth = options.bymonth.filter(
Expand Down
5 changes: 5 additions & 0 deletions src/platform/packages/shared/kbn-rrule/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,8 @@ export type Options = Omit<IterOptions, 'refDT'> & {
count?: number;
tzid: string;
};

export type ConstructorOptions = Omit<Options, 'byweekday' | 'wkst'> & {
byweekday?: Array<string | number> | null;
wkst?: Weekday | WeekdayStr | number | null;
};
215 changes: 215 additions & 0 deletions src/platform/packages/shared/kbn-rrule/validate.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { validateOptions } from './validate';
import { Weekday, Frequency, type ConstructorOptions } from './types';

describe('validateOptions', () => {
const options: ConstructorOptions = {
wkst: Weekday.MO,
byyearday: [1, 2, 3],
bymonth: [1],
bysetpos: [1],
bymonthday: [1],
byweekday: [Weekday.MO],
byhour: [1],
byminute: [1],
bysecond: [1],
dtstart: new Date('September 3, 1998 03:24:00'),
freq: Frequency.YEARLY,
interval: 1,
until: new Date('February 25, 2022 03:24:00'),
count: 3,
tzid: 'UTC',
};

it('happy path', () => {
expect(() => validateOptions(options)).not.toThrow();
});

describe('dtstart', () => {
it('throws an error when dtstart is missing', () => {
expect(() =>
// @ts-expect-error
validateOptions({ ...options, dtstart: null })
).toThrowErrorMatchingInlineSnapshot(`"dtstart is required"`);
});

it('throws an error when dtstart is not a valid date', () => {
expect(() =>
validateOptions({ ...options, dtstart: new Date('invalid') })
).toThrowErrorMatchingInlineSnapshot(`"dtstart is an invalid date"`);
});
});

describe('tzid', () => {
it('throws an error when tzid is missing', () => {
// @ts-expect-error
expect(() => validateOptions({ ...options, tzid: null })).toThrowErrorMatchingInlineSnapshot(
`"tzid is required"`
);
});

it('throws an error when tzid is invalid', () => {
expect(() =>
validateOptions({ ...options, tzid: 'invalid' })
).toThrowErrorMatchingInlineSnapshot(`"tzid is an invalid timezone"`);
});
});

describe('interval', () => {
it('throws an error when count is not a number', () => {
expect(() =>
// @ts-expect-error
validateOptions({ ...options, interval: 'invalid' })
).toThrowErrorMatchingInlineSnapshot(`"interval must be an integer greater than 0"`);
});

it('throws an error when interval is not an integer', () => {
expect(() =>
validateOptions({ ...options, interval: 1.5 })
).toThrowErrorMatchingInlineSnapshot(`"interval must be an integer greater than 0"`);
});

it('throws an error when interval is <= 0', () => {
expect(() => validateOptions({ ...options, interval: 0 })).toThrowErrorMatchingInlineSnapshot(
`"interval must be an integer greater than 0"`
);
});
});

describe('until', () => {
it('throws an error when until field is an invalid date', () => {
expect(() =>
validateOptions({
...options,
until: new Date('invalid'),
})
).toThrowErrorMatchingInlineSnapshot(`"until is an invalid date"`);
});
});

describe('count', () => {
it('throws an error when count is not a number', () => {
expect(() =>
// @ts-expect-error
validateOptions({ ...options, count: 'invalid' })
).toThrowErrorMatchingInlineSnapshot(`"count must be an integer greater than 0"`);
});

it('throws an error when count is not an integer', () => {
expect(() => validateOptions({ ...options, count: 1.5 })).toThrowErrorMatchingInlineSnapshot(
`"count must be an integer greater than 0"`
);
});

it('throws an error when count is <= 0', () => {
expect(() => validateOptions({ ...options, count: 0 })).toThrowErrorMatchingInlineSnapshot(
`"count must be an integer greater than 0"`
);
});
});

describe('bymonth', () => {
it('throws an error with out of range values', () => {
expect(() =>
validateOptions({ ...options, bymonth: [0, 6, 13] })
).toThrowErrorMatchingInlineSnapshot(
`"bymonth must be an array of numbers between 1 and 12"`
);
});

it('throws an error with string values', () => {
expect(() =>
// @ts-expect-error
validateOptions({ ...options, bymonth: ['invalid'] })
).toThrowErrorMatchingInlineSnapshot(
`"bymonth must be an array of numbers between 1 and 12"`
);
});

it('throws an error when is empty', () => {
expect(() => validateOptions({ ...options, bymonth: [] })).toThrowErrorMatchingInlineSnapshot(
`"bymonth must be an array of numbers between 1 and 12"`
);
});
});

describe('bymonthday', () => {
it('throws an error with out of range values', () => {
expect(() =>
validateOptions({ ...options, bymonthday: [0, 15, 32] })
).toThrowErrorMatchingInlineSnapshot(
`"bymonthday must be an array of numbers between 1 and 31"`
);
});

it('throws an error with string values', () => {
expect(() =>
// @ts-expect-error
validateOptions({ ...options, bymonthday: ['invalid'] })
).toThrowErrorMatchingInlineSnapshot(
`"bymonthday must be an array of numbers between 1 and 31"`
);
});

it('throws an error when is empty', () => {
expect(() =>
validateOptions({ ...options, bymonthday: [] })
).toThrowErrorMatchingInlineSnapshot(
`"bymonthday must be an array of numbers between 1 and 31"`
);
});
});

describe('byweekday', () => {
it('throws an error with out of range values when it contains only numbers', () => {
expect(() =>
validateOptions({ ...options, byweekday: [0, 4, 8] })
).toThrowErrorMatchingInlineSnapshot(`"byweekday numbers must been between 1 and 7"`);
});

it('throws an error with invalid values when it contains only string', () => {
expect(() =>
validateOptions({ ...options, byweekday: ['+1MO', 'FOO', '+3WE', 'BAR', '-4FR'] })
).toThrowErrorMatchingInlineSnapshot(`"byweekday strings must be valid weekday strings"`);
});

it('throws an error when is empty', () => {
expect(() =>
validateOptions({ ...options, byweekday: [] })
).toThrowErrorMatchingInlineSnapshot(
`"byweekday must be an array of at least one string or number"`
);
});

it('throws an error with mixed values', () => {
expect(() =>
validateOptions({ ...options, byweekday: [2, 'MO'] })
).toThrowErrorMatchingInlineSnapshot(
`"byweekday values can be either numbers or strings, not both"`
);
});

it('does not throw with properly formed byweekday strings', () => {
expect(() =>
validateOptions({
...options,
byweekday: ['+1MO', '+2TU', '+3WE', '+4TH', '-4FR', '-3SA', '-2SU', '-1MO'],
})
).not.toThrow(`"byweekday numbers must been between 1 and 7"`);
});

it('does not throw with non recurrence values', () => {
expect(() =>
validateOptions({ ...options, byweekday: ['MO', 'TU', 'WE', 'TH', 'FR', 'SA', 'SU'] })
).not.toThrow(`"byweekday numbers must been between 1 and 7"`);
});
});
});
Loading
Loading