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

[Cases][RBAC] Rename scope to owner #96035

Merged
merged 1 commit into from
Apr 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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');
Copy link
Contributor

@XavierM XavierM Apr 1, 2021

Choose a reason for hiding this comment

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

I think the user will have a better understanding of the problem, isn't it?

Suggested change
throw Boom.forbidden('User does not have permissions for this owner');
throw Boom.forbidden(`User does not have permissions to access cases from ${this. featureCaseOwners}`);

Copy link
Contributor

Choose a reason for hiding this comment

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

If you agree, I will apply that to every msgs

Copy link
Member Author

@cnasikas cnasikas Apr 1, 2021

Choose a reason for hiding this comment

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

Hey! You are right! This will be taken care in this PR: #95477

}

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