[Security solution][Endpoint] API error when edit/create TA by policy and license under platinum #113363

dasansol92 · 2021-09-29T08:19:08Z

Summary

Throws new error when trying to edit TA by policy when changing policies state. Only allows user don't modify it or change it to global.
Throws new error when trying to create new TA by policy.
Adds unit tests and new error type.

For maintainers

This was checked for breaking API changes and was labeled appropriately

…ght license

…ps by policy

academo · 2021-09-29T08:42:08Z

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/service.test.ts

@@ -408,10 +454,27 @@ describe('service', () => {
          savedObjectClient,
          packagePolicyClient,
          trustedAppByPolicy.id,
-          toUpdateTrustedApp(trustedAppByPolicy as MaybeImmutable<TrustedApp>)
+          toUpdateTrustedApp(trustedAppByPolicy as MaybeImmutable<TrustedApp>),


unrelated code?

academo

I'm not sure if we do unit testing for the services and utilities, I'd say that isUserTryingToModifyEffectScopeWithoutPermissions should have a unit test as well as the current tests are only testing integration but I leave it to your discretion. The code looks good as it is 🐑🐑🐑

dasansol92 · 2021-09-29T08:51:36Z

I'm not sure if we do unit testing for the services and utilities, I'd say that isUserTryingToModifyEffectScopeWithoutPermissions should have a unit test as well as the current tests are only testing integration but I leave it to your discretion. The code looks good as it is 🐑🐑🐑

Yeah, I didn't add a specific unit test to that function because it's not exported. It's just a private one to keep the code organised.
It's tested by the handlers and services unit tests (when testing TA by policy under license test cases). Agree this could have his own unit test, but as it's not exported and it is tested by the services/handlers tests I think we are good.

paul-tavares

This looks 🤩 awesome

Left a few comments, but you can push them to another PR if needed. I'm 👍

🚢

paul-tavares · 2021-09-29T13:21:35Z

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/errors.ts

@@ -25,6 +25,15 @@ export class TrustedAppPolicyNotExistsError extends Error {
    );
  }
 }
+export class TrustedAppPolicyPermissionsError extends Error {


Suggestion: extend EndpointError instead as the base class. When you do that, you should not need to set the type below, as the base class already takes care of it (but instead of type it uses name)

paul-tavares · 2021-09-29T13:28:12Z

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/errors.ts

+
+  constructor() {
+    super(
+      'Your Kibana license has been downgraded. As such, individual policy configuration is no longer supported.'


Suggestion: use the same text as the one that Isolation is using: 'Your license level does not allow for this action' (here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/server/endpoint/routes/actions/isolation.ts#L79)

Also,

Maybe we should have a generic Endpoint error for license related errors - like a EndpointLicenseError in server/endpoint/errors.ts?

If you decide to stay with this one - since this seems to be very specific for only license violation, maybe rename the Error to TrustedAppPolicyLicenseError?

Either way - this comment can be done in a subsequent PR.

paul-tavares · 2021-09-29T13:31:46Z

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/handlers.ts

@@ -87,6 +88,11 @@ const errorHandler = <E extends Error>(
    return res.badRequest({ body: { message: error.message, attributes: { type: error.type } } });
  }

+  if (error instanceof TrustedAppPolicyPermissionsError) {
+    logger.error(error);
+    return res.badRequest({ body: { message: error.message, attributes: { type: error.type } } });


Suggestion: just send the error:

Suggested change

return res.badRequest({ body: { message: error.message, attributes: { type: error.type } } });

return res.badRequest({ body: error });

paul-tavares · 2021-09-29T13:32:48Z

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/service.test.ts

      ).rejects.toBeInstanceOf(TrustedAppPolicyNotExistsError);
    });
+
+    it('should throw wwhen license under platinum and by policy', async () => {


extra w in wwhen 😁

paul-tavares · 2021-09-29T13:38:10Z

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/service.ts

+    return false;
+  } else if (
+    isEqual(
+      currentTrustedApp.tags.map((policy) => policy.replace('policy:', '')).sort(),


one way to avoid this is to use the exceptionListItemToTrustedApp mapping function in x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/mapping.ts and change this function's signature so that currentTrustedApp is of type TrustedApp

…se helper mapping function to avoid use string replace method

paul-tavares

Some comments, but 👍

paul-tavares · 2021-09-29T15:23:17Z

x-pack/plugins/lists/common/schemas/response/exception_list_item_schema.mock.ts

@@ -40,7 +40,7 @@ export const getExceptionListItemSchemaMock = (
  name: NAME,
  namespace_type: NAMESPACE_TYPE,
  os_types: [],
-  tags: ['user added string for a tag', 'malware'],
+  tags: ['sdasdasd', 'malware'],


😄 was this intentional?

..._solution/public/management/pages/trusted_apps/view/components/create_trusted_app_flyout.tsx

paul-tavares · 2021-09-29T15:26:25Z

x-pack/plugins/security_solution/server/endpoint/errors.ts

@@ -22,3 +22,10 @@ export class EndpointAppContentServicesNotStartedError extends EndpointError {
    super('EndpointAppContextService has not been started (EndpointAppContextService.start())');
  }
 }
+export class EndpointLicenseError extends EndpointError {
+  public readonly name = 'EndpointLicenseError';


you don't need this. EndpointError#constructor will set it auto-magically 😄

see:

kibana/x-pack/plugins/security_solution/server/endpoint/errors.ts

Line 14 in 9fbb612

this.name = this.constructor.name;

x-pack/plugins/security_solution/server/endpoint/errors.ts

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/handlers.ts

paul-tavares · 2021-09-29T15:32:11Z

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/mocks.ts

@@ -36,6 +37,32 @@ export const getTrustedAppByPolicy = function (): TrustedApp {
  };
 };

+export const getPutTrustedAppByPolicy = function (): ExceptionListItemSchema {


can you include the word mock in the function name?

dasansol92 · 2021-09-30T12:44:30Z

@elasticmachine merge upstream

…hanges_to_the_TA_effect_scope_is_to_set_it_to_global-1688

kibanamachine · 2021-09-30T14:51:05Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	4.3MB	4.3MB	+288.0B

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`securitySolution`	874	878	+4

History

💚 Build #156494 succeeded 36f13fd
💔 Build #156471 failed f39e832
💚 Build #156319 succeeded 128421d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

… and license under platinum (elastic#113363) * New function to check if the effect scope has been changed without right license * Checks if license is under platinum when creating/updating trusted apps by policy * Change error type. Use translations in frontend for api error. Also use helper mapping function to avoid use string replace method * Remove name from constructor and changed mock function name Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-09-30T14:54:23Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

… and license under platinum (#113363) (#113545) * New function to check if the effect scope has been changed without right license * Checks if license is under platinum when creating/updating trusted apps by policy * Change error type. Use translations in frontend for api error. Also use helper mapping function to avoid use string replace method * Remove name from constructor and changed mock function name Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: David Sánchez <davidsansol92@gmail.com>

dasansol92 added 2 commits September 28, 2021 13:07

New function to check if the effect scope has been changed without ri…

3d74da9

…ght license

Checks if license is under platinum when creating/updating trusted ap…

128421d

…ps by policy

dasansol92 added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 29, 2021

dasansol92 requested a review from a team as a code owner September 29, 2021 08:19

dasansol92 requested review from academo and parkiino September 29, 2021 08:19

academo reviewed Sep 29, 2021

View reviewed changes

academo approved these changes Sep 29, 2021

View reviewed changes

dasansol92 requested a review from paul-tavares September 29, 2021 12:40

paul-tavares approved these changes Sep 29, 2021

View reviewed changes

Change error type. Use translations in frontend for api error. Also u…

f39e832

…se helper mapping function to avoid use string replace method

dasansol92 requested review from a team as code owners September 29, 2021 15:21

dasansol92 requested a review from paul-tavares September 29, 2021 15:21

paul-tavares approved these changes Sep 29, 2021

View reviewed changes

Remove name from constructor and changed mock function name

36f13fd

dasansol92 requested a review from paul-tavares September 30, 2021 09:10

Merge branch 'master' into feat/olm-edit_API_should_ensure_that_any_c…

777f3fc

…hanges_to_the_TA_effect_scope_is_to_set_it_to_global-1688

paul-tavares approved these changes Sep 30, 2021

View reviewed changes

dasansol92 enabled auto-merge (squash) September 30, 2021 13:03

dasansol92 merged commit 4f95060 into elastic:master Sep 30, 2021

kibanamachine mentioned this pull request Sep 30, 2021

[7.x] [Security solution][Endpoint] API error when edit/create TA by policy and license under platinum (#113363) #113545

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security solution][Endpoint] API error when edit/create TA by policy and license under platinum #113363

[Security solution][Endpoint] API error when edit/create TA by policy and license under platinum #113363

dasansol92 commented Sep 29, 2021 •

edited

Loading

academo Sep 29, 2021

academo left a comment •

edited

Loading

dasansol92 commented Sep 29, 2021

paul-tavares left a comment

paul-tavares Sep 29, 2021

paul-tavares Sep 29, 2021

paul-tavares Sep 29, 2021

paul-tavares Sep 29, 2021

paul-tavares Sep 29, 2021

paul-tavares left a comment

paul-tavares Sep 29, 2021

dasansol92 Sep 29, 2021

paul-tavares Sep 29, 2021

paul-tavares Sep 29, 2021

dasansol92 commented Sep 30, 2021

kibanamachine commented Sep 30, 2021

References to deprecated APIs

kibanamachine commented Sep 30, 2021

	return res.badRequest({ body: { message: error.message, attributes: { type: error.type } } });
	return res.badRequest({ body: error });

[Security solution][Endpoint] API error when edit/create TA by policy and license under platinum #113363

[Security solution][Endpoint] API error when edit/create TA by policy and license under platinum #113363

Conversation

dasansol92 commented Sep 29, 2021 • edited Loading

Summary

For maintainers

Choose a reason for hiding this comment

academo left a comment • edited Loading

Choose a reason for hiding this comment

dasansol92 commented Sep 29, 2021

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasansol92 commented Sep 30, 2021

kibanamachine commented Sep 30, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

References to deprecated APIs

History

kibanamachine commented Sep 30, 2021

💚 Backport successful

dasansol92 commented Sep 29, 2021 •

edited

Loading

academo left a comment •

edited

Loading