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

[EDR Workflows] Fix Endpoint list RBAC problems #199803

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c7021b3
add acceptance test
gergoabraham Nov 12, 2024
a3311ad
do not fetch agent policies without privileges - fixes "Forbidden" issue
gergoabraham Nov 19, 2024
8e10847
fix and reenable policy list integration test
gergoabraham Nov 22, 2024
47f9d94
use `toBeInTheDocument()` in policy integration tests
gergoabraham Nov 22, 2024
63ca011
do not show Endpoint onboarding without integration policy write priv…
gergoabraham Nov 22, 2024
ffafa14
do not fetch package policies without needed privileges
gergoabraham Nov 22, 2024
8ab0f05
Merge branch 'main' into fix-endpoint-list-rbac-problems
elasticmachine Nov 22, 2024
a4c9984
Merge branch 'main' into fix-endpoint-list-rbac-problems
elasticmachine Nov 25, 2024
72ec5cf
Merge branch 'main' into fix-endpoint-list-rbac-problems
elasticmachine Nov 27, 2024
c050ba4
Merge branch 'main' into fix-endpoint-list-rbac-problems
elasticmachine Nov 27, 2024
0715246
Merge branch 'main' into fix-endpoint-list-rbac-problems
elasticmachine Dec 2, 2024
f81d682
Merge branch 'main' into fix-endpoint-list-rbac-problems
elasticmachine Dec 4, 2024
ee762ef
clarification for jsdoc
gergoabraham Dec 4, 2024
99a2c18
remove temporary testing timeouts
gergoabraham Dec 4, 2024
9810335
remove redundant test case
gergoabraham Dec 9, 2024
95eb39f
use Boolean() constructor
gergoabraham Dec 9, 2024
de27c61
improve logic readability
gergoabraham Dec 9, 2024
6fe4b3c
Merge branch 'main' into fix-endpoint-list-rbac-problems
gergoabraham Dec 9, 2024
d3e7737
Merge branch 'main' into fix-endpoint-list-rbac-problems
gergoabraham Dec 10, 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
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@
* 2.0.
*/

import { calculateEndpointAuthz, getEndpointAuthzInitialState } from './authz';
import {
calculateEndpointAuthz,
canFetchPackageAndAgentPolicies,
getEndpointAuthzInitialState,
} from './authz';
import type { FleetAuthz } from '@kbn/fleet-plugin/common';
import { createFleetAuthzMock } from '@kbn/fleet-plugin/common/mocks';
import { createLicenseServiceMock } from '../../../license/mocks';
Expand All @@ -15,6 +19,7 @@ import {
RESPONSE_CONSOLE_ACTION_COMMANDS_TO_RBAC_FEATURE_CONTROL,
type ResponseConsoleRbacControls,
} from '../response_actions/constants';
import type { Capabilities } from '@kbn/core-capabilities-common';

describe('Endpoint Authz service', () => {
let licenseService: ReturnType<typeof createLicenseServiceMock>;
Expand Down Expand Up @@ -91,48 +96,60 @@ describe('Endpoint Authz service', () => {
);
});

it('should not give canAccessFleet if `fleet.all` is false', () => {
fleetAuthz.fleet.all = false;
expect(calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canAccessFleet).toBe(
false
);
});
describe('Fleet', () => {
[true, false].forEach((value) => {
it(`should set canAccessFleet to ${value} if \`fleet.all\` is ${value}`, () => {
fleetAuthz.fleet.all = value;
expect(calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canAccessFleet).toBe(
value
);
});

it('should not give canReadFleetAgents if `fleet.readAgents` is false', () => {
fleetAuthz.fleet.readAgents = false;
expect(calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canReadFleetAgents).toBe(
false
);
});
it(`should set canReadFleetAgents to ${value} if \`fleet.readAgents\` is ${value}`, () => {
fleetAuthz.fleet.readAgents = value;
expect(
calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canReadFleetAgents
).toBe(value);
});

it('should not give canWriteFleetAgents if `fleet.allAgents` is false', () => {
fleetAuthz.fleet.allAgents = false;
expect(
calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canWriteFleetAgents
).toBe(false);
});
it(`should set canWriteFleetAgents to ${value} if \`fleet.allAgents\` is ${value}`, () => {
fleetAuthz.fleet.allAgents = value;
expect(
calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canWriteFleetAgents
).toBe(value);
});

it('should not give canReadFleetAgentPolicies if `fleet.readAgentPolicies` is false', () => {
fleetAuthz.fleet.readAgentPolicies = false;
expect(
calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canReadFleetAgentPolicies
).toBe(false);
it(`should set canReadFleetAgentPolicies to ${value} if \`fleet.readAgentPolicies\` is ${value}`, () => {
fleetAuthz.fleet.readAgentPolicies = value;
expect(
calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canReadFleetAgentPolicies
).toBe(value);
});

it(`should set canAccessFleet to ${value} if \`fleet.all\` is ${value}`, () => {
fleetAuthz.fleet.all = value;
expect(calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canAccessFleet).toBe(
value
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a duplication of the one on line 100?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed it is! 🦅 👁️ !
9810335


it(`should set canWriteIntegrationPolicies to ${value} if \`integrations.writeIntegrationPolicies\` is ${value}`, () => {
fleetAuthz.integrations.writeIntegrationPolicies = value;
expect(
calculateEndpointAuthz(licenseService, fleetAuthz, userRoles)
.canWriteIntegrationPolicies
).toBe(value);
});
});
});

it('should not give canAccessEndpointManagement if not superuser', () => {
it('should set canAccessEndpointManagement if not superuser', () => {
userRoles = [];
expect(
calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canAccessEndpointManagement
).toBe(false);
});

it('should give canAccessFleet if `fleet.all` is true', () => {
fleetAuthz.fleet.all = true;
expect(calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canAccessFleet).toBe(
true
);
});

it('should give canAccessEndpointManagement if superuser', () => {
userRoles = ['superuser'];
expect(
Expand Down Expand Up @@ -303,6 +320,7 @@ describe('Endpoint Authz service', () => {
canReadFleetAgentPolicies: false,
canReadFleetAgents: false,
canWriteFleetAgents: false,
canWriteIntegrationPolicies: false,
canAccessEndpointActionsLogManagement: false,
canAccessEndpointManagement: false,
canCreateArtifactsByPolicy: false,
Expand Down Expand Up @@ -336,4 +354,64 @@ describe('Endpoint Authz service', () => {
});
});
});

describe('canFetchPackageAndAgentPolicies()', () => {
describe('without granular Fleet permissions', () => {
it.each`
readFleet | readIntegrations | readPolicyManagement | result
${false} | ${false} | ${false} | ${false}
${true} | ${false} | ${false} | ${false}
${false} | ${true} | ${false} | ${false}
${true} | ${true} | ${false} | ${true}
${false} | ${false} | ${true} | ${true}
${true} | ${false} | ${true} | ${true}
${false} | ${true} | ${true} | ${true}
${true} | ${true} | ${true} | ${true}
`(
'should return $result when readFleet is $readFleet, readIntegrations is $readIntegrations and readPolicyManagement is $readPolicyManagement',
({ readFleet, readIntegrations, readPolicyManagement, result }) => {
const capabilities: Partial<Capabilities> = {
siem: { readPolicyManagement },
fleetv2: { read: readFleet },
fleet: { read: readIntegrations },
};

expect(canFetchPackageAndAgentPolicies(capabilities as Capabilities)).toBe(result);
}
);
});

describe('with granular Fleet permissions', () => {
it.each`
readFleet | readAgentPolicies | readIntegrations | readPolicyManagement | result
${false} | ${false} | ${false} | ${false} | ${false}
${false} | ${false} | ${true} | ${false} | ${false}
${false} | ${false} | ${false} | ${true} | ${true}
${false} | ${false} | ${true} | ${true} | ${true}
${false} | ${true} | ${false} | ${false} | ${false}
${false} | ${true} | ${true} | ${false} | ${false}
${false} | ${true} | ${false} | ${true} | ${true}
${false} | ${true} | ${true} | ${true} | ${true}
${true} | ${false} | ${false} | ${false} | ${false}
${true} | ${false} | ${true} | ${false} | ${false}
${true} | ${false} | ${false} | ${true} | ${true}
${true} | ${false} | ${true} | ${true} | ${true}
${true} | ${true} | ${false} | ${false} | ${false}
${true} | ${true} | ${true} | ${false} | ${true}
${true} | ${true} | ${false} | ${true} | ${true}
${true} | ${true} | ${true} | ${true} | ${true}
`(
'should return $result when readAgentPolicies is $readAgentPolicies, readFleet is $readFleet, readIntegrations is $readIntegrations and readPolicyManagement is $readPolicyManagement',
({ readAgentPolicies, readFleet, readIntegrations, readPolicyManagement, result }) => {
const capabilities: Partial<Capabilities> = {
siem: { readPolicyManagement },
fleetv2: { read: readFleet, agent_policies_read: readAgentPolicies },
fleet: { read: readIntegrations },
};

expect(canFetchPackageAndAgentPolicies(capabilities as Capabilities)).toBe(result);
}
);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import type { ENDPOINT_PRIVILEGES, FleetAuthz } from '@kbn/fleet-plugin/common';

import { omit } from 'lodash';
import type { Capabilities } from '@kbn/core-capabilities-common';
import type { ProductFeaturesService } from '../../../../server/lib/product_features_service';
import { RESPONSE_CONSOLE_ACTION_COMMANDS_TO_REQUIRED_AUTHZ } from '../response_actions/constants';
import type { LicenseService } from '../../../license';
Expand Down Expand Up @@ -99,10 +100,19 @@ export const calculateEndpointAuthz = (
const authz: EndpointAuthz = {
canWriteSecuritySolution,
canReadSecuritySolution,

// ---------------------------------------------------------
// Coming from Fleet authz
// ---------------------------------------------------------
canAccessFleet: fleetAuthz?.fleet.all ?? false,
canReadFleetAgentPolicies: fleetAuthz?.fleet.readAgentPolicies ?? false,
canWriteFleetAgents: fleetAuthz?.fleet.allAgents ?? false,
canReadFleetAgents: fleetAuthz?.fleet.readAgents ?? false,
canWriteIntegrationPolicies: fleetAuthz?.integrations.writeIntegrationPolicies ?? false,
paul-tavares marked this conversation as resolved.
Show resolved Hide resolved

// ---------------------------------------------------------
// Endpoint & policy management
// ---------------------------------------------------------
canAccessEndpointManagement: hasEndpointManagementAccess, // TODO: is this one deprecated? it is the only place we need to check for superuser.
canCreateArtifactsByPolicy: isPlatinumPlusLicense,
canWriteEndpointList,
Expand Down Expand Up @@ -166,6 +176,7 @@ export const getEndpointAuthzInitialState = (): EndpointAuthz => {
canReadFleetAgentPolicies: false,
canReadFleetAgents: false,
canWriteFleetAgents: false,
canWriteIntegrationPolicies: false,
canAccessEndpointActionsLogManagement: false,
canAccessEndpointManagement: false,
canCreateArtifactsByPolicy: false,
Expand Down Expand Up @@ -198,3 +209,24 @@ export const getEndpointAuthzInitialState = (): EndpointAuthz => {
canWriteEndpointExceptions: false,
};
};

/**
* Duplicate logic to calculate if user has privilege to fetch Agent Policies,
* working only with Capabilities, in order to be able to use it e.g. in middleware.
*
* The logic works with Fleet granular privileges (`subfeaturePrivileges`) both enabled and disabled.
*
* @param capabilities Capabilities from coreStart.application
*/
export const canFetchPackageAndAgentPolicies = (capabilities: Capabilities): boolean => {
paul-tavares marked this conversation as resolved.
Show resolved Hide resolved
const canReadPolicyManagement = (capabilities.siem?.readPolicyManagement ?? false) as boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: const canReadPolicyManagement = Boolean(capabilities.siem?.readPolicyManagement);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here I see why using Boolean() helps, as we can get rid off the nullish coalescing operator, but...

95eb39f


const fleetv2 = capabilities.fleetv2;
const canReadFleetAgentPolicies = (fleetv2?.read &&
(fleetv2?.agent_policies_read === true ||
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: A little bit hard to follow these conditions, wouldn't it come down to
(capabilities.fleetv2?.agent_policies_read ?? true) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it comes to that. the idea was to be able to read it as we accept either true or undefined, but yeah, maybe simply a fallback to the true value is more readable
de27c61

fleetv2?.agent_policies_read === undefined)) as boolean;

const canReadIntegrations = capabilities.fleet?.read as boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: const canReadIntegrations = Boolean(capabilities.fleet?.read);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here, i don't. i'm curious about your opinion: why do you prefer the Boolean() constructor to the as boolean type assertion here?

(changed anyway : ) 95eb39f)


return canReadPolicyManagement || (canReadFleetAgentPolicies && canReadIntegrations);
};
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ export interface EndpointAuthz {
canReadFleetAgents: boolean;
/** If the user has permissions to write Fleet Agents */
canWriteFleetAgents: boolean;
/** If the user has permissions to write Integration policies */
canWriteIntegrationPolicies: boolean;
paul-tavares marked this conversation as resolved.
Show resolved Hide resolved
/** If the user has permissions to access Endpoint management (includes check to ensure they also have access to fleet) */
canAccessEndpointManagement: boolean;
/** If the user has permissions to access Actions Log management and also has a platinum license (used for endpoint details flyout) */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ const PolicyEmptyState = React.memo<{
policyEntryPoint?: boolean;
}>(({ loading, onActionClick, actionDisabled, policyEntryPoint = false }) => {
const docLinks = useKibana().services.docLinks;
const { canAccessFleet, loading: authzLoading } = useUserPrivileges().endpointPrivileges;
const {
canAccessFleet,
canWriteIntegrationPolicies,
loading: authzLoading,
} = useUserPrivileges().endpointPrivileges;

return (
<div data-test-subj="emptyPolicyTable">
Expand Down Expand Up @@ -134,7 +138,7 @@ const PolicyEmptyState = React.memo<{

{authzLoading && <EuiSkeletonText lines={1} />}

{!authzLoading && canAccessFleet && (
{!authzLoading && canAccessFleet && canWriteIntegrationPolicies && (
<>
<EuiSpacer size="s" />
<EuiFlexGroup>
Expand All @@ -156,7 +160,9 @@ const PolicyEmptyState = React.memo<{
</>
)}

{!authzLoading && !canAccessFleet && <MissingFleetAccessInfo />}
{!authzLoading && !(canAccessFleet && canWriteIntegrationPolicies) && (
<MissingFleetAccessInfo />
)}
</EuiFlexItem>

<EuiFlexItem grow={2}>
Expand Down
Loading