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.

Sign up for GitHub

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][Alerting] Fix stackAlerts plugin missing rac API auth scope #193948

Merged
merged 4 commits into from
Oct 7, 2024
Merged

[ResponseOps][Alerting] Fix stackAlerts plugin missing rac API auth scope #193948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
869f01c
Fix stackAlerts plugin missing rac API auth scope
umbopepato Sep 25, 2024
37fdc74
Add get_alerts_index test with only stackAlerts:read privilege
umbopepato Oct 1, 2024
996d3c7
Merge branch 'main' into fix-stack-alerts-endpoints-authentication
umbopepato Oct 4, 2024
452c99a
Merge branch 'main' into fix-stack-alerts-endpoints-authentication
umbopepato Oct 7, 2024
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
4 changes: 2 additions & 2 deletions x-pack/plugins/stack_alerts/server/feature.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export const BUILT_IN_ALERTS_FEATURE: KibanaFeatureConfig = {
all: [],
read: [],
},
api: [],
api: ['rac'],
ui: [],
},
read: {
Expand Down Expand Up @@ -108,7 +108,7 @@ export const BUILT_IN_ALERTS_FEATURE: KibanaFeatureConfig = {
all: [],
read: [],
},
api: [],
api: ['rac'],
ui: [],
},
},
Expand Down
8 changes: 8 additions & 0 deletions x-pack/test/functional_with_es_ssl/config.base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,14 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
spaces: ['*'],
},
],
elasticsearch: {
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for adding this?

Copy link
Member Author

Choose a reason for hiding this comment

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

See #193948 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

But before it was working without it, why does this role (alerts_and_actions_role) need extra permission? Did some tests start failing because we added the rac API privilege?

Copy link
Member

Choose a reason for hiding this comment

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

But before it was working without it, why does this role (alerts_and_actions_role) need extra permission? Did some tests start failing because we added the rac API privilege?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just added it to simulate a correct scenario (currently the user needs access to the index to be able to see the controls bar) but if we don't mind seeing the error toast in the functional test I can remove it, the other tests work just fine

Copy link
Member

Choose a reason for hiding this comment

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

I think is fine what I do not get is why the alerts_and_actions_role role needs it and not the stackAlertsOnlyReadSpacesAll which is used by the tests. I am sure I am missing something 🙂.

indices: [
{
names: ['.alerts-*'],
privileges: ['read'],
},
],
},
},
only_actions_role: {
kibana: [
Expand Down
18 changes: 18 additions & 0 deletions x-pack/test/rule_registry/common/lib/authentication/roles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,23 @@ export const logsOnlyAllSpacesAll: Role = {
},
};

export const stackAlertsOnlyReadSpacesAll: Role = {
name: 'stack_alerts_only_read_spaces_all',
privileges: {
elasticsearch: {
indices: [],
},
kibana: [
{
feature: {
stackAlerts: ['read'],
},
spaces: ['*'],
},
],
},
};

export const stackAlertsOnlyAllSpacesAll: Role = {
name: 'stack_alerts_only_all_spaces_all',
privileges: {
Expand Down Expand Up @@ -511,6 +528,7 @@ export const allRoles = [
securitySolutionOnlyReadSpacesAll,
observabilityOnlyAllSpacesAll,
logsOnlyAllSpacesAll,
stackAlertsOnlyReadSpacesAll,
stackAlertsOnlyAllSpacesAll,
observabilityOnlyReadSpacesAll,
observabilityOnlyAllSpacesAllWithReadESIndices,
Expand Down
16 changes: 12 additions & 4 deletions x-pack/test/rule_registry/common/lib/authentication/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ import {
observabilityMinReadAlertsAllSpacesAll,
observabilityOnlyAllSpacesAllWithReadESIndices,
securitySolutionOnlyAllSpacesAllWithReadESIndices,
stackAlertsOnlyAllSpacesAll,
stackAlertsOnlyReadSpacesAll as stackAlertsOnlyReadSpacesAllRole,
stackAlertsOnlyAllSpacesAll as stackAlertsOnlyAllSpacesAllRole,
} from './roles';
import { User } from './types';

Expand Down Expand Up @@ -130,6 +131,12 @@ export const obsOnlyReadSpacesAll: User = {
roles: [observabilityOnlyReadSpacesAll.name],
};

export const stackAlertsOnlyReadSpacesAll: User = {
username: 'stack_alerts_only_read_spaces_all',
password: 'stack_alerts_only_read_spaces_all',
roles: [stackAlertsOnlyReadSpacesAllRole.name],
};

export const users = [
superUser,
secOnly,
Expand Down Expand Up @@ -177,10 +184,10 @@ export const logsOnlySpacesAll: User = {
roles: [logsOnlyAllSpacesAll.name],
};

export const stackAlertsOnlySpacesAll: User = {
export const stackAlertsOnlyAllSpacesAll: User = {
username: 'stack_alerts_only_all_spaces_all',
password: 'stack_alerts_only_all_spaces_all',
roles: [stackAlertsOnlyAllSpacesAll.name],
roles: [stackAlertsOnlyAllSpacesAllRole.name],
};

export const obsOnlySpacesAllEsRead: User = {
Expand Down Expand Up @@ -297,7 +304,8 @@ export const allUsers = [
secOnlyReadSpacesAll,
obsOnlySpacesAll,
logsOnlySpacesAll,
stackAlertsOnlySpacesAll,
stackAlertsOnlyReadSpacesAll,
stackAlertsOnlyAllSpacesAll,
obsSecSpacesAll,
obsSecReadSpacesAll,
obsMinReadAlertsRead,
Expand Down
40 changes: 23 additions & 17 deletions x-pack/test/rule_registry/security_and_spaces/tests/basic/get_alerts_index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@

import expect from '@kbn/expect';

import { superUser, obsOnlySpacesAll, secOnlyRead } from '../../../common/lib/authentication/users';
import {
superUser,
obsOnlySpacesAll,
secOnlyRead,
stackAlertsOnlyReadSpacesAll,
} from '../../../common/lib/authentication/users';
import type { User } from '../../../common/lib/authentication/types';
import { FtrProviderContext } from '../../../common/ftr_provider_context';
import { getSpaceUrlPrefix } from '../../../common/lib/authentication/spaces';
Expand All @@ -22,27 +27,19 @@ export default ({ getService }: FtrProviderContext) => {
const SPACE1 = 'space1';
const APM_ALERT_INDEX = '.alerts-observability.apm.alerts-default';
const SECURITY_SOLUTION_ALERT_INDEX = '.alerts-security.alerts';
const STACK_ALERT_INDEX = '.alerts-stack.alerts-default';

const getAPMIndexName = async (user: User, space: string, expectedStatusCode: number = 200) => {
const resp = await supertestWithoutAuth
.get(`${getSpaceUrlPrefix(space)}${ALERTS_INDEX_URL}?features=apm`)
.auth(user.username, user.password)
.set('kbn-xsrf', 'true')
.expect(expectedStatusCode);
return resp.body.index_name as string[];
};

const getSecuritySolutionIndexName = async (
const getIndexName = async (
Copy link
Member Author

Choose a reason for hiding this comment

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

Reduced similar functions to a single one parametrized based on featureIds

featureIds: string[],
user: User,
space: string,
expectedStatusCode: number = 200
) => {
const resp = await supertestWithoutAuth
.get(`${getSpaceUrlPrefix(space)}${ALERTS_INDEX_URL}?features=siem`)
.get(`${getSpaceUrlPrefix(space)}${ALERTS_INDEX_URL}?features=${featureIds.join(',')}`)
.auth(user.username, user.password)
.set('kbn-xsrf', 'true')
.expect(expectedStatusCode);

return resp.body.index_name as string[];
};

Expand All @@ -52,24 +49,33 @@ export default ({ getService }: FtrProviderContext) => {
});
describe('Users:', () => {
it(`${obsOnlySpacesAll.username} should be able to access the APM alert in ${SPACE1}`, async () => {
const indexNames = await getAPMIndexName(obsOnlySpacesAll, SPACE1);
const indexNames = await getIndexName(['apm'], obsOnlySpacesAll, SPACE1);
expect(indexNames.includes(APM_ALERT_INDEX)).to.eql(true); // assert this here so we can use constants in the dynamically-defined test cases below
});

it(`${superUser.username} should be able to access the APM alert in ${SPACE1}`, async () => {
const indexNames = await getAPMIndexName(superUser, SPACE1);
const indexNames = await getIndexName(['apm'], superUser, SPACE1);
expect(indexNames.includes(APM_ALERT_INDEX)).to.eql(true); // assert this here so we can use constants in the dynamically-defined test cases below
});

it(`${secOnlyRead.username} should NOT be able to access the APM alert in ${SPACE1}`, async () => {
const indexNames = await getAPMIndexName(secOnlyRead, SPACE1);
const indexNames = await getIndexName(['apm'], secOnlyRead, SPACE1);
expect(indexNames?.length).to.eql(0);
});

it(`${secOnlyRead.username} should be able to access the security solution alert in ${SPACE1}`, async () => {
const indexNames = await getSecuritySolutionIndexName(secOnlyRead, SPACE1);
const indexNames = await getIndexName(['siem'], secOnlyRead, SPACE1);
expect(indexNames.includes(`${SECURITY_SOLUTION_ALERT_INDEX}-${SPACE1}`)).to.eql(true); // assert this here so we can use constants in the dynamically-defined test cases below
});

it(`${stackAlertsOnlyReadSpacesAll.username} should be able to access the stack alert in ${SPACE1}`, async () => {
const indexNames = await getIndexName(
['stackAlerts'],
stackAlertsOnlyReadSpacesAll,
SPACE1
);
expect(indexNames.includes(STACK_ALERT_INDEX)).to.eql(true);
});
});
});
};
6 changes: 3 additions & 3 deletions x-pack/test/rule_registry/security_and_spaces/tests/basic/search_strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
obsOnlySpacesAll,
logsOnlySpacesAll,
secOnlySpacesAllEsReadAll,
stackAlertsOnlySpacesAll,
stackAlertsOnlyAllSpacesAll,
superUser,
} from '../../../common/lib/authentication/users';

Expand Down Expand Up @@ -360,8 +360,8 @@ export default ({ getService }: FtrProviderContext) => {
const result = await secureBsearch.send<RuleRegistrySearchResponse>({
supertestWithoutAuth,
auth: {
username: stackAlertsOnlySpacesAll.username,
password: stackAlertsOnlySpacesAll.password,
username: stackAlertsOnlyAllSpacesAll.username,
password: stackAlertsOnlyAllSpacesAll.password,
},
referer: 'test',
kibanaVersion,
Expand Down