[Security Solutions][Detection Engine] Unskips tests after ES promotion and adds deleteUserRole utility #90533

FrankHassanabad · 2021-02-05T23:01:44Z

Summary

Unskips tests after a ES promotion and adds a delete user role utility.

Removes one any from the utils by switching to using ProvidedType

Before:

After:

Turns out that return types on overloaded functions aren't easy fwiw and will fall on the bottom one which in this case looked to be any which we don't want:
microsoft/TypeScript#24275 (comment)

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-02-05T23:02:54Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

rylnd

This looks great, thank you for getting these tests back online, adding the missing symmetry, and fixing type errors while you're at it! It's all very much appreciated. 🙏

rylnd · 2021-02-06T00:15:45Z

x-pack/test/detection_engine_api_integration/security_and_spaces/roles_users_utils/index.ts


 export const createUserAndRole = async (
-  securityService: ReturnType<FtrProviderContext['getService']>,
+  securityService: ProvidedType<typeof services['security']>,


Nit: I don't see ProvidedType being used much externally, if at all; it seems more common to pass getService: FtrProviderContext['getService'] to helper functions like this.

The trick for this and why I can't use that is because we want the return type from it and it's function overloaded which does not work well for type script.

Yeah I see what you're saying though. Most people are passing the service directly and then calling the getService('security') against that and then using it.

I can change this to use that and remove the ProvidedType. Might be better for us to stick with the existing patterns here and not look odd ball for maintainers.

rylnd · 2021-02-06T00:29:24Z

x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_index.ts

      beforeEach(async () => {
        await createUserAndRole(security, role);
      });

+      afterEach(async () => {


These have got me thinking generally about how we can guarantee these kinds of hooks to be symmetric. My first thought is helper functions that generate these hooks, e.g.

const withUserRole = (role, security) => { beforeEach(); afterEach(); }

but maybe that's too much abstraction? Just a thought.

I think this is a neat idea 👍

Not tracking all the way on this. How do you use those functions like you have above. I can't call beforeEach and then afterEach like that. I might be dense here though.

dhurley14 · 2021-02-08T16:27:24Z

x-pack/test/detection_engine_api_integration/security_and_spaces/roles_users_utils/index.ts

+) => {
+  await securityService.role.delete(roleName);
+  await securityService.user.delete(roleName);
+};


Nit: I would ask we switch the ordering of these delete calls such that we delete the user first and then the role. This would better reflect the setup of posting the role first and then the user.

Yeah, it does seem wrong if I deleted a role first and the user had a dangling role. I am curious how it allowed me to do that in first place but you are correct this would be better for future proofing things and is the way it should operate.

dhurley14

LGTM! Thanks for taking the time to improve the types and get these tests unskipped :)

marshallmain · 2021-02-09T04:39:47Z

x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_index.ts

@@ -88,7 +91,7 @@ export default ({ getService }: FtrProviderContext) => {
          .expect(403);
        expect(body).to.eql({
          message:
-            'security_exception: action [cluster:admin/ilm/get] is unauthorized for user [t1_analyst], this action is granted by the privileges [read_ilm,manage_ilm,manage,all]',
+            'security_exception: action [cluster:admin/ilm/get] is unauthorized for user [t1_analyst], this action is granted by the cluster privileges [read_ilm,manage_ilm,manage,all]',


are messages a defined part of the ES API spec? we've been burned a few times by wording changes in these messages causing tests to fail and get skipped. I could imagine a new privilege being added that grants [cluster:admin/ilm/get] and the array [read_ilm,manage_ilm,manage,all] would then include the new privilege. I don't think that would be considered a breaking change in ES but it would break this test.

Perhaps we can verify part of the message, such as that it begins with security_exception instead of asserting the whole string?

It shouldn't change often from talking to others about this one when ES was promoted. If it were to change again it could be at either the start, end, or middle of the string. For example they could change the wording of security_exception: to something else at the start of it.

From my understanding, during the promotions of elastic search and when people skip tests they don't view the string failures like this one as something to necessarily minimize but rather as a way for Kibana core members and us to double check the wording and the elastic tickets to ensure these changes in the strings are intentful from the elastic team and make sense when it bubbles up to the UI.

Kibana core skips the tests as we're the best to fix them but if we see something such as a misspellings or worse something that looks wrong such as the ending string array of [read_ilm,manage_ilm,manage,all] missing in one of the properties, then the diff in the failure is useful for everyone.

We have previously rejected the promotion and asked downstream elastic to issue a fix before the promotion is allowed to move forward in some failure cases rather than do a skip.

kibanamachine · 2021-02-09T05:07:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: df8bc09

Metrics [docs]

✅ unchanged

History

💚 Build #104463 succeeded c56f629

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

…romotion and adds deleteUserRole utility (#90533) (#90847) * [Security Solutions][Detection Engine] Unskips tests after ES promotion and adds deleteUserRole utility (#90533) ## Summary Unskips tests after a ES promotion and adds a delete user role utility. Ref: #90229 #88302 Removes one `any` from the utils by switching to using `ProvidedType` Before: <img width="558" alt="Screen Shot 2021-02-05 at 2 45 37 PM" src="https://user-images.githubusercontent.com/1151048/107098890-8dce5b80-67cd-11eb-8f6e-51f83eef4647.png"> After: <img width="513" alt="Screen Shot 2021-02-05 at 4 13 23 PM" src="https://user-images.githubusercontent.com/1151048/107098898-9161e280-67cd-11eb-8085-a5220938834e.png"> Turns out that return types on overloaded functions aren't easy fwiw and will fall on the bottom one which in this case looked to be `any` which we don't want: microsoft/TypeScript#24275 (comment) ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios # Conflicts: # x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_index.ts # x-pack/test/detection_engine_api_integration/security_and_spaces/tests/delete_signals_migrations.ts * Fixes test failure as the permissions are true for 7.x -> 7.12 compared to false for 7.11

Unskip tests after ES promotion

c3fdd53

FrankHassanabad requested review from a team as code owners February 5, 2021 23:01

FrankHassanabad added the release_note:skip Skip the PR/issue when compiling release notes label Feb 5, 2021

FrankHassanabad self-assigned this Feb 5, 2021

FrankHassanabad added v8.0.0 v7.12.0 Team:Detections and Resp Security Detection Response Team labels Feb 5, 2021

Changes one type to remove the any

c56f629

FrankHassanabad requested review from rylnd and dhurley14 February 5, 2021 23:20

rylnd approved these changes Feb 6, 2021

View reviewed changes

dhurley14 reviewed Feb 8, 2021

View reviewed changes

dhurley14 approved these changes Feb 8, 2021

View reviewed changes

FrankHassanabad added 2 commits February 8, 2021 19:40

Merge branch 'master' into unskip-tests

1526d66

Updated from PR review

df8bc09

FrankHassanabad requested a review from a team as a code owner February 9, 2021 03:03

marshallmain reviewed Feb 9, 2021

View reviewed changes

FrankHassanabad merged commit 5a27e69 into elastic:master Feb 9, 2021

FrankHassanabad deleted the unskip-tests branch February 9, 2021 20:35

FrankHassanabad mentioned this pull request Feb 9, 2021

[7.x] [Security Solutions][Detection Engine] Unskips tests after ES promotion and adds deleteUserRole utility (#90533) #90847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solutions][Detection Engine] Unskips tests after ES promotion and adds deleteUserRole utility #90533

[Security Solutions][Detection Engine] Unskips tests after ES promotion and adds deleteUserRole utility #90533

FrankHassanabad commented Feb 5, 2021 •

edited

elasticmachine commented Feb 5, 2021

rylnd left a comment

rylnd Feb 6, 2021

FrankHassanabad Feb 9, 2021

FrankHassanabad Feb 9, 2021 •

edited

rylnd Feb 6, 2021

dhurley14 Feb 8, 2021

FrankHassanabad Feb 9, 2021 •

edited

dhurley14 Feb 8, 2021

FrankHassanabad Feb 9, 2021 •

edited

dhurley14 left a comment

marshallmain Feb 9, 2021

FrankHassanabad Feb 9, 2021

kibanamachine commented Feb 9, 2021

[Security Solutions][Detection Engine] Unskips tests after ES promotion and adds deleteUserRole utility #90533

[Security Solutions][Detection Engine] Unskips tests after ES promotion and adds deleteUserRole utility #90533

Conversation

FrankHassanabad commented Feb 5, 2021 • edited

Summary

Checklist

elasticmachine commented Feb 5, 2021

rylnd left a comment

Choose a reason for hiding this comment

rylnd Feb 6, 2021

Choose a reason for hiding this comment

FrankHassanabad Feb 9, 2021

Choose a reason for hiding this comment

FrankHassanabad Feb 9, 2021 • edited

Choose a reason for hiding this comment

rylnd Feb 6, 2021

Choose a reason for hiding this comment

dhurley14 Feb 8, 2021

Choose a reason for hiding this comment

FrankHassanabad Feb 9, 2021 • edited

Choose a reason for hiding this comment

dhurley14 Feb 8, 2021

Choose a reason for hiding this comment

FrankHassanabad Feb 9, 2021 • edited

Choose a reason for hiding this comment

dhurley14 left a comment

Choose a reason for hiding this comment

marshallmain Feb 9, 2021

Choose a reason for hiding this comment

FrankHassanabad Feb 9, 2021

Choose a reason for hiding this comment

kibanamachine commented Feb 9, 2021

💚 Build Succeeded

Metrics [docs]

History

FrankHassanabad commented Feb 5, 2021 •

edited

FrankHassanabad Feb 9, 2021 •

edited

FrankHassanabad Feb 9, 2021 •

edited

FrankHassanabad Feb 9, 2021 •

edited