Skip to content

Commit

Permalink
[Cases] RBAC: Rename class to scope (#95535)
Browse files Browse the repository at this point in the history
  • Loading branch information
cnasikas committed Mar 26, 2021
1 parent 2847861 commit a2e1da8
Show file tree
Hide file tree
Showing 21 changed files with 112 additions and 115 deletions.
6 changes: 3 additions & 3 deletions x-pack/plugins/cases/common/api/cases/case.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ const CaseBasicRt = rt.type({
[caseTypeField]: CaseTypeRt,
connector: CaseConnectorRt,
settings: SettingsRt,
// TODO: should a user be able to update the class?
class: rt.string,
// TODO: should a user be able to update the scope?
scope: rt.string,
});

const CaseExternalServiceBasicRt = rt.type({
Expand Down Expand Up @@ -80,7 +80,7 @@ const CasePostRequestNoTypeRt = rt.type({
title: rt.string,
connector: CaseConnectorRt,
settings: SettingsRt,
class: rt.string,
scope: rt.string,
});

/**
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/cases/common/api/cases/sub_case.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const SubCasesFindRequestRt = rt.partial({
searchFields: rt.array(rt.string),
sortField: rt.string,
sortOrder: rt.union([rt.literal('desc'), rt.literal('asc')]),
class: rt.string,
scope: rt.string,
});

export const SubCaseResponseRt = rt.intersection([
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/cases/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,4 @@ export const MAX_GENERATED_ALERTS_PER_SUB_CASE = MAX_ALERTS_PER_SUB_CASE / DEFAU
* This must be the same value that the security solution plugin uses to define the case kind when it registers the
* feature for the 7.13 migration only.
*/
export const SECURITY_SOLUTION_CONSUMER = 'securitySolution';
export const SECURITY_SOLUTION_SCOPE = 'securitySolution';
95 changes: 46 additions & 49 deletions x-pack/plugins/cases/server/authorization/authorization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { KueryNode } from '../../../../../src/plugins/data/server';
import { SecurityPluginStart } from '../../../security/server';
import { PluginStartContract as FeaturesPluginStart } from '../../../features/server';
import { GetSpaceFn, ReadOperations, WriteOperations } from './types';
import { getClassFilter } from './utils';
import { getScopesFilter } from './utils';

/**
* This class handles ensuring that the user making a request has the correct permissions
Expand All @@ -20,25 +20,25 @@ import { getClassFilter } from './utils';
export class Authorization {
private readonly request: KibanaRequest;
private readonly securityAuth: SecurityPluginStart['authz'] | undefined;
private readonly featureCaseClasses: Set<string>;
private readonly featureCaseScopes: Set<string>;
private readonly isAuthEnabled: boolean;
// TODO: create this
// private readonly auditLogger: AuthorizationAuditLogger;

private constructor({
request,
securityAuth,
caseClasses,
caseScopes,
isAuthEnabled,
}: {
request: KibanaRequest;
securityAuth?: SecurityPluginStart['authz'];
caseClasses: Set<string>;
caseScopes: Set<string>;
isAuthEnabled: boolean;
}) {
this.request = request;
this.securityAuth = securityAuth;
this.featureCaseClasses = caseClasses;
this.featureCaseScopes = caseScopes;
this.isAuthEnabled = isAuthEnabled;
}

Expand All @@ -59,58 +59,58 @@ export class Authorization {
isAuthEnabled: boolean;
}): Promise<Authorization> {
// Since we need to do async operations, this static method handles that before creating the Auth class
let caseClasses: Set<string>;
let caseScopes: Set<string>;
try {
const disabledFeatures = new Set((await getSpace(request))?.disabledFeatures ?? []);

caseClasses = new Set(
caseScopes = new Set(
features
.getKibanaFeatures()
// get all the features' cases classes that aren't disabled
// get all the features' cases scopes that aren't disabled
.filter(({ id }) => !disabledFeatures.has(id))
.flatMap((feature) => feature.cases ?? [])
);
} catch (error) {
caseClasses = new Set<string>();
caseScopes = new Set<string>();
}

return new Authorization({ request, securityAuth, caseClasses, isAuthEnabled });
return new Authorization({ request, securityAuth, caseScopes, isAuthEnabled });
}

private shouldCheckAuthorization(): boolean {
return this.securityAuth?.mode?.useRbacForRequest(this.request) ?? false;
}

public async ensureAuthorized(className: string, operation: ReadOperations | WriteOperations) {
public async ensureAuthorized(scope: string, operation: ReadOperations | WriteOperations) {
// TODO: remove
if (!this.isAuthEnabled) {
return;
}

const { securityAuth } = this;
const isAvailableClass = this.featureCaseClasses.has(className);
const isScopeAvailable = this.featureCaseScopes.has(scope);

// TODO: throw if the request is not authorized
if (securityAuth && this.shouldCheckAuthorization()) {
// TODO: implement ensure logic
const requiredPrivileges: string[] = [securityAuth.actions.cases.get(className, operation)];
const requiredPrivileges: string[] = [securityAuth.actions.cases.get(scope, operation)];

const checkPrivileges = securityAuth.checkPrivilegesDynamicallyWithRequest(this.request);
const { hasAllRequested, username, privileges } = await checkPrivileges({
kibana: requiredPrivileges,
});

if (!isAvailableClass) {
// TODO: throw if any of the class are not available
if (!isScopeAvailable) {
// TODO: throw if any of the scope are not available
/**
* Under most circumstances this would have been caught by `checkPrivileges` as
* a user can't have Privileges to an unknown class, but super users
* don't actually get "privilege checked" so the made up class *will* return
* a user can't have Privileges to an unknown scope, but super users
* don't actually get "privilege checked" so the made up scope *will* return
* as Privileged.
* This check will ensure we don't accidentally let these through
*/
// TODO: audit log using `username`
throw Boom.forbidden('User does not have permissions for this class');
throw Boom.forbidden('User does not have permissions for this scope');
}

if (hasAllRequested) {
Expand All @@ -129,11 +129,11 @@ export class Authorization {

// TODO: audit log
// TODO: User unauthorized. throw an error. authorizedPrivileges & unauthorizedPrivilages are needed for logging.
throw Boom.forbidden('Not authorized for this class');
throw Boom.forbidden('Not authorized for this scope');
}
} else if (!isAvailableClass) {
} else if (!isScopeAvailable) {
// TODO: throw an error
throw Boom.forbidden('Security is disabled but no class was found');
throw Boom.forbidden('Security is disabled but no scope was found');
}

// else security is disabled so let the operation proceed
Expand All @@ -143,46 +143,46 @@ export class Authorization {
savedObjectType: string
): Promise<{
filter?: KueryNode;
ensureSavedObjectIsAuthorized: (className: string) => void;
ensureSavedObjectIsAuthorized: (scope: string) => void;
}> {
const { securityAuth } = this;
if (securityAuth && this.shouldCheckAuthorization()) {
const { authorizedClassNames } = await this.getAuthorizedClassNames([ReadOperations.Find]);
const { authorizedScopes } = await this.getAuthorizedScopes([ReadOperations.Find]);

if (!authorizedClassNames.length) {
if (!authorizedScopes.length) {
// TODO: Better error message, log error
throw Boom.forbidden('Not authorized for this class');
throw Boom.forbidden('Not authorized for this scope');
}

return {
filter: getClassFilter(savedObjectType, authorizedClassNames),
ensureSavedObjectIsAuthorized: (className: string) => {
if (!authorizedClassNames.includes(className)) {
filter: getScopesFilter(savedObjectType, authorizedScopes),
ensureSavedObjectIsAuthorized: (scope: string) => {
if (!authorizedScopes.includes(scope)) {
// TODO: log error
throw Boom.forbidden('Not authorized for this class');
throw Boom.forbidden('Not authorized for this scope');
}
},
};
}

return { ensureSavedObjectIsAuthorized: (className: string) => {} };
return { ensureSavedObjectIsAuthorized: (scope: string) => {} };
}

private async getAuthorizedClassNames(
private async getAuthorizedScopes(
operations: Array<ReadOperations | WriteOperations>
): Promise<{
username?: string;
hasAllRequested: boolean;
authorizedClassNames: string[];
authorizedScopes: string[];
}> {
const { securityAuth, featureCaseClasses } = this;
const { securityAuth, featureCaseScopes } = this;
if (securityAuth && this.shouldCheckAuthorization()) {
const checkPrivileges = securityAuth.checkPrivilegesDynamicallyWithRequest(this.request);
const requiredPrivileges = new Map<string, [string]>();

for (const className of featureCaseClasses) {
for (const scope of featureCaseScopes) {
for (const operation of operations) {
requiredPrivileges.set(securityAuth.actions.cases.get(className, operation), [className]);
requiredPrivileges.set(securityAuth.actions.cases.get(scope, operation), [scope]);
}
}

Expand All @@ -193,24 +193,21 @@ export class Authorization {
return {
hasAllRequested,
username,
authorizedClassNames: hasAllRequested
? Array.from(featureCaseClasses)
: privileges.kibana.reduce<string[]>(
(authorizedClassNames, { authorized, privilege }) => {
if (authorized && requiredPrivileges.has(privilege)) {
const [className] = requiredPrivileges.get(privilege)!;
authorizedClassNames.push(className);
}

return authorizedClassNames;
},
[]
),
authorizedScopes: hasAllRequested
? Array.from(featureCaseScopes)
: privileges.kibana.reduce<string[]>((authorizedScopes, { authorized, privilege }) => {
if (authorized && requiredPrivileges.has(privilege)) {
const [scope] = requiredPrivileges.get(privilege)!;
authorizedScopes.push(scope);
}

return authorizedScopes;
}, []),
};
} else {
return {
hasAllRequested: true,
authorizedClassNames: Array.from(featureCaseClasses),
authorizedScopes: Array.from(featureCaseScopes),
};
}
}
Expand Down
10 changes: 5 additions & 5 deletions x-pack/plugins/cases/server/authorization/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ import { remove, uniq } from 'lodash';
import { nodeBuilder } from '../../../../../src/plugins/data/common';
import { KueryNode } from '../../../../../src/plugins/data/server';

export const getClassFilter = (savedObjectType: string, classNames: string[]): KueryNode => {
export const getScopesFilter = (savedObjectType: string, scopes: string[]): KueryNode => {
return nodeBuilder.or(
classNames.reduce<KueryNode[]>((query, className) => {
ensureFieldIsSafeForQuery('class', className);
query.push(nodeBuilder.is(`${savedObjectType}.attributes.class`, className));
scopes.reduce<KueryNode[]>((query, scope) => {
ensureFieldIsSafeForQuery('scope', scope);
query.push(nodeBuilder.is(`${savedObjectType}.attributes.scope`, scope));
return query;
}, [])
);
Expand Down Expand Up @@ -43,4 +43,4 @@ export const ensureFieldIsSafeForQuery = (field: string, value: string): boolean
};

export const includeFieldsRequiredForAuthentication = (fields: string[]): string[] =>
uniq([...fields, 'class']);
uniq([...fields, 'scope']);
18 changes: 9 additions & 9 deletions x-pack/plugins/cases/server/client/cases/create.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ describe('create', () => {
settings: {
syncAlerts: true,
},
class: 'awesome',
scope: 'awesome',
};

const savedObjectsClient = createMockSavedObjectsRepository({
Expand All @@ -57,7 +57,7 @@ describe('create', () => {

expect(res).toMatchInlineSnapshot(`
Object {
"class": "awesome",
"scope": "awesome",
"closed_at": null,
"closed_by": null,
"comments": Array [],
Expand Down Expand Up @@ -121,7 +121,7 @@ describe('create', () => {
"connector",
"settings",
],
"new_value": "{\\"type\\":\\"individual\\",\\"description\\":\\"This is a brand new case of a bad meanie defacing data\\",\\"title\\":\\"Super Bad Security Issue\\",\\"tags\\":[\\"defacement\\"],\\"connector\\":{\\"id\\":\\"123\\",\\"name\\":\\"Jira\\",\\"type\\":\\".jira\\",\\"fields\\":{\\"issueType\\":\\"Task\\",\\"priority\\":\\"High\\",\\"parent\\":null}},\\"settings\\":{\\"syncAlerts\\":true},\\"class\\":\\"awesome\\"}",
"new_value": "{\\"type\\":\\"individual\\",\\"description\\":\\"This is a brand new case of a bad meanie defacing data\\",\\"title\\":\\"Super Bad Security Issue\\",\\"tags\\":[\\"defacement\\"],\\"connector\\":{\\"id\\":\\"123\\",\\"name\\":\\"Jira\\",\\"type\\":\\".jira\\",\\"fields\\":{\\"issueType\\":\\"Task\\",\\"priority\\":\\"High\\",\\"parent\\":null}},\\"settings\\":{\\"syncAlerts\\":true},\\"scope\\":\\"awesome\\"}",
"old_value": null,
},
"references": Array [
Expand Down Expand Up @@ -151,7 +151,7 @@ describe('create', () => {
settings: {
syncAlerts: true,
},
class: 'awesome',
scope: 'awesome',
};

const savedObjectsClient = createMockSavedObjectsRepository({
Expand All @@ -162,7 +162,7 @@ describe('create', () => {

expect(res).toMatchInlineSnapshot(`
Object {
"class": "awesome",
"scope": "awesome",
"closed_at": null,
"closed_by": null,
"comments": Array [],
Expand Down Expand Up @@ -216,7 +216,7 @@ describe('create', () => {
settings: {
syncAlerts: true,
},
class: 'awesome',
scope: 'awesome',
};

const savedObjectsClient = createMockSavedObjectsRepository({
Expand All @@ -230,7 +230,7 @@ describe('create', () => {

expect(res).toMatchInlineSnapshot(`
Object {
"class": "awesome",
"scope": "awesome",
"closed_at": null,
"closed_by": null,
"comments": Array [],
Expand Down Expand Up @@ -429,7 +429,7 @@ describe('create', () => {
settings: {
syncAlerts: true,
},
class: 'awesome',
scope: 'awesome',
};

const savedObjectsClient = createMockSavedObjectsRepository({
Expand Down Expand Up @@ -458,7 +458,7 @@ describe('create', () => {
settings: {
syncAlerts: true,
},
class: 'awesome',
scope: 'awesome',
};
const savedObjectsClient = createMockSavedObjectsRepository({
caseSavedObject: mockCases,
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/cases/server/client/cases/create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export const create = async ({

try {
try {
await auth.ensureAuthorized(query.class, WriteOperations.Create);
await auth.ensureAuthorized(query.scope, WriteOperations.Create);
} catch (error) {
// TODO: log error using audit logger
throw error;
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/cases/server/client/cases/find.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export const find = async ({
});

for (const theCase of cases.casesMap.values()) {
ensureSavedObjectIsAuthorized(theCase.class);
ensureSavedObjectIsAuthorized(theCase.scope);
}

// TODO: Make sure we do not leak information when authorization is on
Expand Down
Loading

0 comments on commit a2e1da8

Please sign in to comment.