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] move SAML auth to kbn-test #172678

Merged
merged 21 commits into from
Dec 8, 2023

Conversation

dmlemeshko
Copy link
Member

@dmlemeshko dmlemeshko commented Dec 6, 2023

Summary

This PR moves SAML session creation from FTR service to @kbn/test. It should simplify its adoption in non-FTR context, e.g. Cypress tests or jest integration tests:

import { SamlSessionManager } from '@kbn/test';

// create instance in your setup file
const sessionManager = new SamlSessionManager({
  hostOptions: {
    protocol,
    hostname,
    port,
    username,
    password,
  },
  log,
  isCloud
});

use it in your tests

sessionManager.getSessionCookieForRole('viewer');

const log = getService('log');
const isCloud = !!process.env.TEST_CLOUD;
// Sharing the instance within FTR config run means cookies are persistent for each role between tests.
const sessionManager = new SAMLSessionManager(config, log, isCloud);
Copy link
Member Author

@dmlemeshko dmlemeshko Dec 6, 2023

Choose a reason for hiding this comment

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

I'm not sure if Cypress test runner has access to config instance, so I imagine a possible alternative like:

new SAMLSessionManager(kbnHost, auth, log, isCloud)

where kbnHost and auth FTR and Cypress runner should provide on its own:

const kbnHost = {
    protocol: config.get('servers.kibana.protocol'),
    hostname: config.get('servers.kibana.hostname'),
    port: isCloud ? undefined : config.get('servers.kibana.port'),
};
const auth = { username: config.get('servers.kibana.username')}, password: config.get('servers.kibana.password') }

@MadameSheema @pheyos wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I believe the alternative is the best one for Cypress usage. Thanks :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in 82f750b

Comment on lines 59 to 65
// getLocalUsers = () => {
// const rolesDefinitionFilePath = resolve(
// REPO_ROOT,
// 'packages/kbn-es/src/serverless_resources/roles.yml'
// );
// const roles: string[] = Object.keys(loadYaml(fs.readFileSync(rolesDefinitionFilePath, 'utf8')));
// };
Copy link
Member

Choose a reason for hiding this comment

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

Is this commented out intentionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking to split packages/kbn-es/src/serverless_resources/roles.yml into 3 files and read as part of role validation when we run tests locally. But I will do it later to not keep PR open for long

Copy link
Member Author

@dmlemeshko dmlemeshko Dec 7, 2023

Choose a reason for hiding this comment

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

Note: Currently folks can specify non-existing role in the test and I believe tests will pass locally/Kibana CI. But it will fail on MKI pipeline since that role does not exist in real Cloud env.

@dmlemeshko dmlemeshko requested a review from azasypkin December 7, 2023 08:52
Comment on lines +22 to +23
username: config.get('servers.kibana.username'),
password: config.get('servers.kibana.password'),
Copy link
Member

Choose a reason for hiding this comment

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

question: just for my understanding: what are these credentials for?

Copy link
Member Author

Choose a reason for hiding this comment

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

@azasypkin it turns out we need to create instance of KbnClient to fetch Kibana version and for cloud we pass credentials to the client.

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

packages/kbn-test/src/auth/session_manager.ts Outdated Show resolved Hide resolved
import * as fs from 'fs';
import { User } from './session_manager';

export const getProjectType = (serverArgs: string[]) => {
Copy link
Member

Choose a reason for hiding this comment

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

question: I see this function isn't used anywhere yet, is it just a leftover or is it reserved for some future use?

Copy link
Member Author

Choose a reason for hiding this comment

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

I remove it in fac981e. Originally I planned to split svl roles yml into 3 files and validate roles per project, but decided to move it to the follow-up PR.

@dmlemeshko dmlemeshko added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.13.0 labels Dec 7, 2023
@dmlemeshko dmlemeshko marked this pull request as ready for review December 7, 2023 17:41
@dmlemeshko dmlemeshko requested review from a team as code owners December 7, 2023 17:41
@dmlemeshko dmlemeshko requested a review from pheyos December 7, 2023 17:41
@dmlemeshko dmlemeshko enabled auto-merge (squash) December 7, 2023 22:43
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #38 / Rule execution logic API Execution logic @ess @serverless Query type rules should query and get back expected alert structure when it is a alert on a alert

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/test 244 262 +18
Unknown metric groups

API count

id before after diff
@kbn/test 291 310 +19

History

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

@dmlemeshko dmlemeshko merged commit a4a0ad7 into elastic:main Dec 8, 2023
38 checks passed
@dmlemeshko dmlemeshko self-assigned this Dec 8, 2023
MadameSheema added a commit that referenced this pull request Dec 13, 2023
…ing (#172655)

Relates to:
* #166340
* #170852
* #170417
* #172678

## Summary

In this PR we are using the code implemented on
#170417 and
#172678 to allow SAML and role
testing inside Cypress.

* We are creating a Cypress task to use the above-developed code and be
able to retrieve a session cookie given a role.
* We updated the login task to know how we should perform the login
depending if we are in Serverless (MKI or serverless FTR) or ESS
* In the parallel serverless script:
* We are updating the `BASE_ENV_URL` variable to use the proper QA
environment (pending to be done in follow-up PRs, to extract this value
so it is not hardcoded cc @dkirchan )
* We are adding the `IS_SERVERLESS` environment variable needed for the
logic on the login task. This changed implied to update the
`es_archiver` file to continue work as expected.
* We have added the `TEST_CLOUD_HOST_NAME` environment variable needed
for the code we are reusing to retrieve the session cookie for MKI.
* We have updated the Security Solution quality gate script to set the
`role_users.json` file needed by the code we are reusing to get the
different session cookies on MKI
* We have adjusted the tests because the username now follows the
pattern `test <role>` (@dmlemeshko is it possible to have as username
just the role? Is this something that can impact other tests and teams?)
* We have [skipped](#173168) a
test that got unstable after the changes.

## How to test it in your machine

### Serverless FTR

1. Navigate to `x-pack/test/security_solution_cypress`
2. Execute `yarn cypress:open:qa:serverless`
3. Click on `E2E testing`
4. Click on any test to execute it


### Serverless MKI

Setup a valid Elastic Cloud API key for QA environment:

1. Navigate to QA environment.
2. Click on the `User menu button` located on the top right of the
header.
3. Click on `Organization`.
5. Click on the `API keys` tab.
6. Click on `Create API key` button.
7. Add a name, set an expiration date, assign an organization owner
role.
8. Click on `Create API key`
9. Save the value of the key

Store the saved key on `~/.elastic/cloud.json` using the following
format:

```json
{
  "api_key": {
    "qa": "<API_KEY>"
  }
}
```

Store the email and password of the account you used to login in the QA
Environment at the root directory of your Kibana project on
`.ftr/role_users.json`, using the following format:

```json
{
  "admin": {
    "email": "<email>",
    "password": "<password>"
  }
}
```

If you want to execute a test with a role different from the default
one, make sure you have created the user under your organization and is
added to the above json following the format:

```json
{
  "admin": {
    "email": "<email>",
    "password": "<password>"
  },
  "<roleName>": {
    "email": "<email>",
    "password": "<password>"
  }
}
```

1. Navigate to `x-pack/test/security_solution_cypress`
2. Execute `yarn cypress:open:qa:serverless`
3. Click on `E2E testing`
4. Click on any test to execute it

---------

Co-authored-by: kibanamachine <[email protected]>
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 v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants