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

[FTR] allow to call roleScopedSupertest service with Cookie header #192727

Merged
merged 39 commits into from
Sep 19, 2024

Conversation

dmlemeshko
Copy link
Member

@dmlemeshko dmlemeshko commented Sep 12, 2024

Summary

During the sync with kibana-security team we agreed on how Kibana APIs should be tested:

API Authentication in Kibana: Public vs. Internal APIs

Kibana provides both public and internal APIs, each requiring authentication with the correct privileges. However, the method of testing these APIs varies, depending on how they are untilized by end users.

  • Public APIs: When testing HTTP requests to public APIs, API key-based authentication should be used. It reflect how end user call these APIs. Due to existing restrictions, we utilize Admin user credentials to generate API keys for various roles. While the API key permissions are correctly scoped according to the assigned role, the user will internally be recognized as Admin during authentication.

  • Internal APIs: Direct HTTP requests to internal APIs are generally not expected. However, for testing purposes, authentication should be performed using the Cookie header. This approach simulates client-side behavior during browser interactions, mirroring how internal APIs are indirectly invoked.

To simplify the process of creating/updating the tests, this PR makes few changes to roleScopedSupertest service

  1. testing public APIs (nothing changed)
      const supertestAdminWithApiKey = await roleScopedSupertest.getSupertestWithRoleScope('admin', {
        withCustomHeaders: { 'accept-encoding': 'gzip' },
      });
      const response = await supertestAdminWithApiKey.get('/app/kibana');
  1. testing internal APIs
      const  supertestAdminWithCookieCredentials = await roleScopedSupertest.getSupertestWithRoleScope(
        'admin',
        {
          useCookieHeader: true, // will use Cookie header instead of API key
          withInternalHeaders: true,
        }
      );
      await supertestAdminWithCookieCredentials
        .post(`/internal/kibana/settings/${TEST_SETTING}`)
        .send({ value: 100 })
        .expect(200);

I updated some of the existing tests according to the new approach.

Docs for serverless and deployment-agnostic api integration tests were updated accordingly

@dmlemeshko dmlemeshko added v9.0.0 release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Sep 13, 2024
@dmlemeshko dmlemeshko changed the title [FTR] allow to call supertestWithRoleScope service with Cookie header [FTR] allow to call roleScopedSupertest service with Cookie header Sep 13, 2024
Comment on lines +26 to +32
supertestAdminWithCookieCredentials = await roleScopedSupertest.getSupertestWithRoleScope(
'admin',
{
useCookieHeader: true,
withInternalHeaders: true,
}
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Since all the APIs in this suite are internal, we use supertest scoped to Admin role and cookie header for auth

Comment on lines +27 to +33
supertestAdminWithCookieCredentials = await roleScopedSupertest.getSupertestWithRoleScope(
'admin',
{
useCookieHeader: true,
withInternalHeaders: true,
}
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Since all the APIs in this suite are internal, we use supertest scoped to Admin role and cookie header for auth

Comment on lines +24 to +34
supertestAdminWithCookieCredentials = await roleScopedSupertest.getSupertestWithRoleScope(
'admin',
{
useCookieHeader: true,
withInternalHeaders: true,
withCustomHeaders: {
[ELASTIC_HTTP_VERSION_HEADER]: KQL_TELEMETRY_ROUTE_LATEST_VERSION,
'content-type': 'application/json',
},
}
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Since all the APIs in this suite are internal, we use supertest scoped to Admin role and cookie header for auth. Repeated headers moved directly to scoped supertest creation.

Copy link
Member

@wayneseymour wayneseymour left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

packages/kbn-ftr-common-functional-services

@dmlemeshko dmlemeshko self-assigned this Sep 17, 2024
@dmlemeshko dmlemeshko enabled auto-merge (squash) September 18, 2024 08:49
Copy link
Contributor

@jeramysoucy jeramysoucy left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Mostly nits and some typos. Thank you for putting this together!

x-pack/test_serverless/README.md Outdated Show resolved Hide resolved
x-pack/test_serverless/README.md Outdated Show resolved Hide resolved
x-pack/test_serverless/README.md Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note - we'll want to revisit these when we make Spaces CRUD APIs public and enable multiple spaces in serverless.

@dmlemeshko dmlemeshko requested a review from a team as a code owner September 18, 2024 12:02
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM !!

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jeramysoucy jeramysoucy left a comment

Choose a reason for hiding this comment

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

LGTM! Just found one missed change. Thanks for making these updates, Dima!

@@ -68,7 +68,7 @@ export default function ({ getService }: FtrProviderContext) {

({ body, status } = await supertestWithoutAuth
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this one was missed

Suggested change
({ body, status } = await supertestWithoutAuth
({ body, status } = await supertestAdminWithCookieCredentials

Copy link
Member Author

Choose a reason for hiding this comment

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

But this scenario is testing the case without auth: we suppose to get 401.

On main we don't set API key header https://github.com/elastic/kibana/blob/main/x-pack/test_serverless/api_integration/test_suites/common/platform_security/api_keys.ts#L69-L79

Should I keep it as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. I think that was maybe unintentional, looking at the comment

// expect a rejection because we're not using the internal header

I think the test was intended to fail for not utilizing the internal header, like with the other test cases.
But, this is on our team, not yours. I don't want to hold up your PR. I can fix this when I open a PR to adjust the access level for some of our other endopints.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/ftr-common-functional-services 81 83 +2
Unknown metric groups

API count

id before after diff
@kbn/ftr-common-functional-services 106 108 +2

History

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

cc @dmlemeshko

@dmlemeshko dmlemeshko merged commit d9148f1 into elastic:main Sep 19, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.