Skip to content

Commit

Permalink
[Cases][RBAC] Rename scope to owner (#96035)
Browse files Browse the repository at this point in the history
  • Loading branch information
cnasikas authored Apr 1, 2021
1 parent afff0cc commit 7cf9172
Show file tree
Hide file tree
Showing 21 changed files with 109 additions and 109 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 scope?
scope: rt.string,
// TODO: should a user be able to update the owner?
owner: 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,
scope: rt.string,
owner: 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')]),
scope: rt.string,
owner: 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,7 +76,7 @@ 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_SCOPE = 'securitySolution';
export const SECURITY_SOLUTION_OWNER = 'securitySolution';

/**
* This flag governs enabling the case as a connector feature. It is disabled by default as the feature is not complete.
Expand Down
84 changes: 42 additions & 42 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 { getScopesFilter } from './utils';
import { getOwnersFilter } from './utils';

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

private constructor({
request,
securityAuth,
caseScopes,
caseOwners,
isAuthEnabled,
}: {
request: KibanaRequest;
securityAuth?: SecurityPluginStart['authz'];
caseScopes: Set<string>;
caseOwners: Set<string>;
isAuthEnabled: boolean;
}) {
this.request = request;
this.securityAuth = securityAuth;
this.featureCaseScopes = caseScopes;
this.featureCaseOwners = caseOwners;
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 caseScopes: Set<string>;
let caseOwners: Set<string>;
try {
const disabledFeatures = new Set((await getSpace(request))?.disabledFeatures ?? []);

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

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

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

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

const { securityAuth } = this;
const isScopeAvailable = this.featureCaseScopes.has(scope);
const isOwnerAvailable = this.featureCaseOwners.has(owner);

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

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

if (!isScopeAvailable) {
// TODO: throw if any of the scope are not available
if (!isOwnerAvailable) {
// TODO: throw if any of the owner are not available
/**
* Under most circumstances this would have been caught by `checkPrivileges` as
* 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
* a user can't have Privileges to an unknown owner, but super users
* don't actually get "privilege checked" so the made up owner *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 scope');
throw Boom.forbidden('User does not have permissions for this owner');
}

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 scope');
throw Boom.forbidden('Not authorized for this owner');
}
} else if (!isScopeAvailable) {
} else if (!isOwnerAvailable) {
// TODO: throw an error
throw Boom.forbidden('Security is disabled but no scope was found');
throw Boom.forbidden('Security is disabled but no owner 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: (scope: string) => void;
ensureSavedObjectIsAuthorized: (owner: string) => void;
}> {
const { securityAuth } = this;
if (securityAuth && this.shouldCheckAuthorization()) {
const { authorizedScopes } = await this.getAuthorizedScopes([ReadOperations.Find]);
const { authorizedOwners } = await this.getAuthorizedOwners([ReadOperations.Find]);

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

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

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

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

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

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

return authorizedScopes;
return authorizedOwners;
}, []),
};
} else {
return {
hasAllRequested: true,
authorizedScopes: Array.from(featureCaseScopes),
authorizedOwners: Array.from(featureCaseOwners),
};
}
}
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 getScopesFilter = (savedObjectType: string, scopes: string[]): KueryNode => {
export const getOwnersFilter = (savedObjectType: string, owners: string[]): KueryNode => {
return nodeBuilder.or(
scopes.reduce<KueryNode[]>((query, scope) => {
ensureFieldIsSafeForQuery('scope', scope);
query.push(nodeBuilder.is(`${savedObjectType}.attributes.scope`, scope));
owners.reduce<KueryNode[]>((query, owner) => {
ensureFieldIsSafeForQuery('owner', owner);
query.push(nodeBuilder.is(`${savedObjectType}.attributes.owner`, owner));
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, 'scope']);
uniq([...fields, 'owner']);
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,
},
scope: 'awesome',
owner: 'awesome',
};

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

expect(res).toMatchInlineSnapshot(`
Object {
"scope": "awesome",
"owner": "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},\\"scope\\":\\"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},\\"owner\\":\\"awesome\\"}",
"old_value": null,
},
"references": Array [
Expand Down Expand Up @@ -151,7 +151,7 @@ describe('create', () => {
settings: {
syncAlerts: true,
},
scope: 'awesome',
owner: 'awesome',
};

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

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

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

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

const savedObjectsClient = createMockSavedObjectsRepository({
Expand Down Expand Up @@ -458,7 +458,7 @@ describe('create', () => {
settings: {
syncAlerts: true,
},
scope: 'awesome',
owner: '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 @@ -83,7 +83,7 @@ export const create = async ({

try {
try {
await auth.ensureAuthorized(query.scope, WriteOperations.Create);
await auth.ensureAuthorized(query.owner, 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.scope);
ensureSavedObjectIsAuthorized(theCase.owner);
}

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

0 comments on commit 7cf9172

Please sign in to comment.