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] enable roles management testing for Observability project #196514

Merged

Conversation

dmlemeshko
Copy link
Member

@dmlemeshko dmlemeshko commented Oct 16, 2024

Summary

This PR makes changes in FTR saml_auth service to allow creating custom role for Oblt serverless project, when roles management is explicitly enabled with --xpack.security.roleManagementEnabled=true in Kibana server arguments.

I also added role_management/custom_role_access.ts as a test example. Currently roles management is enabled in x-pack/test_serverless/functional/test_suites/observability/config.feature_flags.ts and after this PR is merged, more tests with custom roles can be added for Oblt project.

How to run tests:

node scripts/functional_tests --config x-pack/test_serverless/functional/test_suites/observability/config.feature_flags.ts

@@ -53,6 +53,8 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) {
if (roleAuthc) {
await samlAuth.invalidateM2mApiKeyWithRoleScope(roleAuthc);
}
// delete custom role
await samlAuth.deleteCustomRole();
Copy link
Member Author

Choose a reason for hiding this comment

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

we need to delete role, because platform security tests validate security roles UI and roles list.

Comment on lines +76 to +78
// TODO: 'Add data' and 'Project Settings' should be hidden
// await testSubjects.missingOrFail('~nav-item-id-observabilityOnboarding');
// await testSubjects.missingOrFail('~nav-item-id-project_settings_project_nav');
Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot 2024-10-16 at 12 01 26

I believe both Add Data and Project Settings should be hidden when role has no access to it.

@dmlemeshko dmlemeshko self-assigned this Oct 16, 2024
@dmlemeshko dmlemeshko added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 v8.16.0 backport:version Backport to applied version labels labels Oct 16, 2024
@dmlemeshko dmlemeshko marked this pull request as ready for review October 16, 2024 10:36
@dmlemeshko dmlemeshko requested review from a team as code owners October 16, 2024 10:36
);

// Return true if the value is explicitly set to 'true', otherwise false
return roleManagementArg?.split('=')[1] === 'true' || false;
Copy link
Member

Choose a reason for hiding this comment

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

Why is true a string literal, but false is a boolean?

Copy link
Member

Choose a reason for hiding this comment

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

I get that coercion will make true become a boolean, but that's if coercion is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

we are looking for the exact --xpack.security.roleManagementEnabled=true match in Kibana server arguments to understand if roles management was explicitly enabled. I believe Kibana won't start if you try to pass this flag with something other than true or false

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 mean only 'true' is expected to enable the functionality, so I set flag to false for any other case (including argument is completely missing)

Copy link
Member

Choose a reason for hiding this comment

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

So the return type of boolean, is only actually a boolean when it's false.
So if it's the string literal 'true' and not a boolean true, it seems like the return type should be false || 'true', and not boolean right?

I guess I'm used to seeing a boolean return type with all return calls being an actual boolean, not a string literal depending on type coercion.

Perhaps it's just a nit at this point.

Copy link
Member Author

@dmlemeshko dmlemeshko Oct 16, 2024

Choose a reason for hiding this comment

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

I don't think you can start Kibana with --xpack.security.roleManagementEnabled='true', schema validation won't allow it. I don't try validate the cases for Kibana server, I believe FTR won't even start if literal 'true' or 'false' will be in arguments

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 right, there is no way to face literal 'true' in place:

 proc [kibana] [2024-10-16T14:13:30.865+02:00][INFO ][root] Kibana is shutting down {"service":{"node":{"roles":["background_tasks","ui"]}}}
 proc [kibana] [2024-10-16T14:13:30.884+02:00][FATAL][root] Reason: [config validation of [xpack.security].roleManagementEnabled]: expected value of type [boolean] but got [string]
 proc [kibana] Error: [config validation of [xpack.security].roleManagementEnabled]: expected value of type [boolean] but got [string]
 proc [kibana]     at ObjectType.validate (type.ts:171:13)
 proc [kibana]     at ConfigService.validateAtPath (config_service.ts:334:19)
 proc [kibana]     at config_service.ts:354:28
 proc [kibana]     at /Users/dmle/github/kibana/node_modules/rxjs/src/internal/operators/map.ts:58:33
 proc [kibana]     at OperatorSubscriber._this._next (/Users/dmle/github/kibana/node_modules/rxjs/src/internal/operators/OperatorSubscriber.ts:70:13)

await samlAuth.invalidateM2mApiKeyWithRoleScope(roleAuthc);
await samlAuth.deleteCustomRole();
Copy link
Member

Choose a reason for hiding this comment

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

So now the devs have to make two extra calls to finish?
Possible to make this a single call?

Copy link
Member Author

@dmlemeshko dmlemeshko Oct 16, 2024

Choose a reason for hiding this comment

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

I don't think we need to combine it.

There is nothing new in the workflow: API key and Kibana role are 2 different entities and we already handle "cleaning" in stateful separately. In theory API key is still valid after role deletion and can be used on its own.

But now I realised it makes sense to delete cached cookie for custom role whenever setCustomRole or deleteCustomRole is called. Cookies are cached per role on the first call to speedup test execution, but as custom role is re-created we need to regenerate cookie as well. I will test it (maybe cookie will remains valid with new privileges in place) and address in the new PR

Copy link
Member Author

@dmlemeshko dmlemeshko Oct 16, 2024

Choose a reason for hiding this comment

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

small fix dbdcd59

tl; dr; generated cookie remains valid after role is re-created after deletion or its privileges were updated. I don't think we need a logic to drop it from cache at the moment

The change I did: we need to make sure descriptors is not null aka contains some valid privileges.

.set(INTERNAL_REQUEST_HEADERS)
.set(adminCookieHeader);

expect(status).to.be(204);
Copy link
Member

Choose a reason for hiding this comment

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

If the status is not a 204, is it still possible that it was indeed deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting point.
I guess both create and delete operations are tested within security plugin testing scope. 204 status is successful request by design, so I don't think we need an extra ES query to validate Kibana APIs did their job.

Copy link
Member Author

Choose a reason for hiding this comment

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

btw I checked that it is deleted via UI, but I think we are good to rely on status code only

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, I was just wondering if that null assignment within the map symbol below could be a problem if the 204 check fails, but the item was actually deleted, then the map would be out of date and could be a difficult bug to track down.
But if you think it's a non-issue, I'm good with it.

Copy link
Member Author

@dmlemeshko dmlemeshko Oct 16, 2024

Choose a reason for hiding this comment

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

Aha, I didn't relate it to the supportedRoleDescriptors not being updated. You are right, we should re-set it for the following test file that might use custom roles as well. Let me have a look

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved before actual delete call bcfc49f

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.

CR only, LGTM

@@ -34,8 +34,18 @@ const getDefaultServerlessRole = (projectType: string) => {
}
};

const isRoleManagementExplicitlyEnabled = (args: string[]): boolean => {
const roleManagementArg = args.find((arg) =>
arg.startsWith('--xpack.security.roleManagementEnabled=')
Copy link
Member

Choose a reason for hiding this comment

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

It may simplify things to pull in the getopts package for parsing these. I see --serverless is parsed similarly elsewhere, so don't see a need to change it now, but making a note in case we refactor later.

Copy link
Contributor

@mohamedhamed-ahmed mohamedhamed-ahmed 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 for doing the effort here.
Tried it in a couple of tests and works as expected.

@dmlemeshko dmlemeshko enabled auto-merge (squash) October 17, 2024 15:24
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

cc @dmlemeshko

@delanni delanni disabled auto-merge October 18, 2024 12:17
@delanni delanni merged commit 16c965f into elastic:main Oct 18, 2024
26 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16

https://github.com/elastic/kibana/actions/runs/11403529950

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 18, 2024
…tic#196514)

## Summary

This PR makes changes in FTR `saml_auth` service to allow creating
custom role for Oblt serverless project, when roles management is
explicitly enabled with `--xpack.security.roleManagementEnabled=true` in
Kibana server arguments.

I also added [role_management/custom_role_access.ts
](x-pack/test_serverless/functional/test_suites/observability/role_management/custom_role_access.ts)
as a test example. Currently roles management is enabled in
`x-pack/test_serverless/functional/test_suites/observability/config.feature_flags.ts`
and after this PR is merged, more tests with custom roles can be added
for Oblt project.

How to run tests:

```
node scripts/functional_tests --config x-pack/test_serverless/functional/test_suites/observability/config.feature_flags.ts
```

(cherry picked from commit 16c965f)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.16

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 18, 2024
#196514) (#196862)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[FTR] enable roles management testing for Observability project
(#196514)](#196514)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Dzmitry
Lemechko","email":"dzmitry.lemechko@elastic.co"},"sourceCommit":{"committedDate":"2024-10-18T12:17:13Z","message":"[FTR]
enable roles management testing for Observability project
(#196514)\n\n## Summary\r\n\r\nThis PR makes changes in FTR `saml_auth`
service to allow creating\r\ncustom role for Oblt serverless project,
when roles management is\r\nexplicitly enabled with
`--xpack.security.roleManagementEnabled=true` in\r\nKibana server
arguments.\r\n\r\nI also added
[role_management/custom_role_access.ts\r\n](x-pack/test_serverless/functional/test_suites/observability/role_management/custom_role_access.ts)\r\nas
a test example. Currently roles management is enabled
in\r\n`x-pack/test_serverless/functional/test_suites/observability/config.feature_flags.ts`\r\nand
after this PR is merged, more tests with custom roles can be
added\r\nfor Oblt project.\r\n\r\nHow to run tests:\r\n\r\n```\r\nnode
scripts/functional_tests --config
x-pack/test_serverless/functional/test_suites/observability/config.feature_flags.ts\r\n```","sha":"16c965f853f17565e2da996b1f2ab21e9e33a003","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","v8.16.0","backport:version"],"title":"[FTR]
enable roles management testing for Observability
project","number":196514,"url":"https://github.com/elastic/kibana/pull/196514","mergeCommit":{"message":"[FTR]
enable roles management testing for Observability project
(#196514)\n\n## Summary\r\n\r\nThis PR makes changes in FTR `saml_auth`
service to allow creating\r\ncustom role for Oblt serverless project,
when roles management is\r\nexplicitly enabled with
`--xpack.security.roleManagementEnabled=true` in\r\nKibana server
arguments.\r\n\r\nI also added
[role_management/custom_role_access.ts\r\n](x-pack/test_serverless/functional/test_suites/observability/role_management/custom_role_access.ts)\r\nas
a test example. Currently roles management is enabled
in\r\n`x-pack/test_serverless/functional/test_suites/observability/config.feature_flags.ts`\r\nand
after this PR is merged, more tests with custom roles can be
added\r\nfor Oblt project.\r\n\r\nHow to run tests:\r\n\r\n```\r\nnode
scripts/functional_tests --config
x-pack/test_serverless/functional/test_suites/observability/config.feature_flags.ts\r\n```","sha":"16c965f853f17565e2da996b1f2ab21e9e33a003"}},"sourceBranch":"main","suggestedTargetBranches":["8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196514","number":196514,"mergeCommit":{"message":"[FTR]
enable roles management testing for Observability project
(#196514)\n\n## Summary\r\n\r\nThis PR makes changes in FTR `saml_auth`
service to allow creating\r\ncustom role for Oblt serverless project,
when roles management is\r\nexplicitly enabled with
`--xpack.security.roleManagementEnabled=true` in\r\nKibana server
arguments.\r\n\r\nI also added
[role_management/custom_role_access.ts\r\n](x-pack/test_serverless/functional/test_suites/observability/role_management/custom_role_access.ts)\r\nas
a test example. Currently roles management is enabled
in\r\n`x-pack/test_serverless/functional/test_suites/observability/config.feature_flags.ts`\r\nand
after this PR is merged, more tests with custom roles can be
added\r\nfor Oblt project.\r\n\r\nHow to run tests:\r\n\r\n```\r\nnode
scripts/functional_tests --config
x-pack/test_serverless/functional/test_suites/observability/config.feature_flags.ts\r\n```","sha":"16c965f853f17565e2da996b1f2ab21e9e33a003"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Dzmitry Lemechko <dzmitry.lemechko@elastic.co>
@jbudz
Copy link
Member

jbudz commented Oct 18, 2024

@dmlemeshko there's a version gap with this backport. Can you check if this should also be backported to 8.x/8.17?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants