chore(streams): returns 403 when user has no read access by kdelemme · Pull Request #217742 · elastic/kibana

kdelemme · 2025-04-09T18:57:16Z

Summary

Resolves https://github.com/elastic/streams-program/issues/247

This PR changes the error thrown when attempting to access a stream without the read privileges, from 404 to 403.
I had to use the functional tests feature flags setup to be able to use custom role on serverless: More details

kdelemme · 2025-04-09T19:24:51Z

x-pack/test/common/utils/server_route_repository/create_admin_service_from_repository.ts

+export async function getCustomRoleApiClient<TServerRouteRepository extends ServerRouteRepository>(
+  st: ReturnType<typeof RoleScopedSupertestProvider>
+): Promise<RepositorySupertestClient<TServerRouteRepository>> {
+  return await getApiClient(st, 'customRole');
+}
+
+export async function getAdminApiClient<TServerRouteRepository extends ServerRouteRepository>(
+  st: ReturnType<typeof RoleScopedSupertestProvider>
+): Promise<RepositorySupertestClient<TServerRouteRepository>> {
+  return await getApiClient(st, 'admin');
+}
+


I didn't want to change all the tests so I kept getAdminApiClient but one could argue we could have only one getApiClient(st, "role") instead... let me know if I should consolidate this or if it's fine like this

I'm in favor of keeping the function split by role, it makes it much easier to audit later where we are using which role by a simple reference check

kdelemme · 2025-04-09T21:16:29Z

I thought I had a good idea

miltonhultgren · 2025-04-10T07:25:38Z

x-pack/test/api_integration/deployment_agnostic/apis/observability/streams/crud.ts

+
+    describe('read streams', () => {
+      it('fails when users has not read access', async () => {
+        await getStream(customRoleApiClient, STREAM_NAME, 403);


It's minor but perhaps we should actually test that is succeeds for the admin role?

it won't hurt adding it, but I have a problem with my setup since customRole does not seem to be available on serverless env

…fix'

dmlemeshko

left a comment, should be an easy fix

dmlemeshko · 2025-04-10T17:06:16Z

x-pack/test/common/utils/server_route_repository/create_admin_service_from_repository.ts

+export async function getCustomRoleApiClient<TServerRouteRepository extends ServerRouteRepository>(
+  st: ReturnType<typeof RoleScopedSupertestProvider>
+): Promise<RepositorySupertestClient<TServerRouteRepository>> {
+  return await getApiClient(st, 'customRole');


In #217882 I'm changing the custom role naming and you will have to get the value via samlAuth.getCustomRole()

Could you make a change so that it won't be broken in the follow-up PR?

I should be able, let's see. I'll wait for your change to happen before updating

jasonrhodes

Approving on behalf of "Observability UI" without having looked at this much ... FWIW looks like this was triggered by the test_serverless/functional/.../streams/ directory, the index.ts and read_privilege.ts files. @miltonhultgren is in the "Observability UI" GH team so his review should have satisfied that CODEOWNERS requirement. Who knows.

miltonhultgren · 2025-04-11T07:36:54Z

Could be those changes happened after my approval?

kdelemme · 2025-04-11T13:26:50Z

blocked until #217882 is merged

flash1293 · 2025-04-15T15:20:11Z

@kdelemme seems like this is ready to go?

kdelemme · 2025-04-15T15:24:04Z

@flash1293 saml customRoles api should have changed, let me see if I don't have to fix something

kdelemme · 2025-04-15T15:39:57Z

@flash1293 tests should be fixed now

dmlemeshko

Code LGTM, thanks for adding CustomRoleScopedSupertestProvider

elasticmachine · 2025-04-16T19:56:57Z

💚 Build Succeeded

Buildkite Build
Commit: 21d5f93

Metrics [docs]

✅ unchanged

History

💔 Build #293792 failed 98eed22
💛 Build #293552 was flaky 815b156
💔 Build #293328 failed 8cee401
💚 Build #292000 succeeded e57e9bb
💔 Build #291725 failed 36eca18

kibanamachine · 2025-04-16T20:03:35Z

Starting backport for target branches: 8.x

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

kibanamachine · 2025-04-16T20:09:32Z

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 217742

Questions ?

Please refer to the Backport tool documentation

)

kibanamachine · 2025-04-18T20:48:43Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 217742 locally

kibanamachine · 2025-04-21T21:48:32Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 217742 locally

kibanamachine · 2025-04-22T21:48:42Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 217742 locally

kdelemme · 2025-04-23T12:31:36Z

💚 All backports created successfully

Status	Branch	Result
✅	8.19

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

Questions ?

Please refer to the Backport tool documentation

) (cherry picked from commit e57a825) # Conflicts: # x-pack/test_serverless/functional/test_suites/observability/index.feature_flags.ts

kibanamachine · 2025-04-23T22:49:01Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

) (#218954) # Backport This will backport the following commits from `main` to `8.19`: - [chore(streams): returns 403 when user has no read access (#217742)](#217742)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

)

Returns 403 when users has no read access to stream

ed61a69

kdelemme added v9.1.0 v8.19.0 backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes labels Apr 9, 2025

kdelemme commented Apr 9, 2025

View reviewed changes

kdelemme marked this pull request as ready for review April 9, 2025 19:25

kdelemme requested a review from a team as a code owner April 9, 2025 19:25

kdelemme requested a review from a team April 9, 2025 19:25

Add test

36eca18

kdelemme force-pushed the streams/read-authorization-error branch from 7159443 to 36eca18 Compare April 9, 2025 19:32

miltonhultgren approved these changes Apr 10, 2025

View reviewed changes

Use functional tests feature flag setup

87a549a

kdelemme requested review from a team as code owners April 10, 2025 13:57

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

e57e9bb

…fix'

elastic-vault-github-plugin-prod bot requested a review from a team as a code owner April 10, 2025 14:07

dmlemeshko reviewed Apr 10, 2025

View reviewed changes

jasonrhodes approved these changes Apr 10, 2025

View reviewed changes

tonyghiani approved these changes Apr 11, 2025

View reviewed changes

Merge branch 'main' into streams/read-authorization-error

708f6c1

kdelemme changed the title ~~chore(streams): returs 403 when user has no read access~~ chore(streams): returns 403 when user has no read access Apr 15, 2025

Use new samlAuth custom role api provided

8cee401

kdelemme requested a review from dmlemeshko April 15, 2025 15:40

flash1293 and others added 3 commits April 16, 2025 09:24

Merge branch 'main' into streams/read-authorization-error

815b156

Merge branch 'main' into streams/read-authorization-error

da95850

Add custom role scoped supertest service

98eed22

dmlemeshko approved these changes Apr 16, 2025

View reviewed changes

Add missing service

21d5f93

kdelemme merged commit e57a825 into elastic:main Apr 16, 2025
9 checks passed

akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request Apr 17, 2025

chore(streams): returns 403 when user has no read access (elastic#217742

bac1aaa

)

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 18, 2025

kdelemme mentioned this pull request Apr 23, 2025

[8.19] chore(streams): returns 403 when user has no read access (#217742) #218954

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 24, 2025

akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request May 29, 2025

chore(streams): returns 403 when user has no read access (elastic#217742

315adbc

)

Conversation

kdelemme commented Apr 9, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

kdelemme Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

kdelemme commented Apr 9, 2025

Uh oh!

miltonhultgren Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

kdelemme Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

dmlemeshko left a comment

Choose a reason for hiding this comment

Uh oh!

dmlemeshko Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

kdelemme Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

jasonrhodes left a comment

Choose a reason for hiding this comment

Uh oh!

miltonhultgren commented Apr 11, 2025

Uh oh!

kdelemme commented Apr 11, 2025

Uh oh!

flash1293 commented Apr 15, 2025

Uh oh!

kdelemme commented Apr 15, 2025

Uh oh!

kdelemme commented Apr 15, 2025

Uh oh!

dmlemeshko left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 16, 2025

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

Uh oh!

kibanamachine commented Apr 16, 2025

Uh oh!

kibanamachine commented Apr 16, 2025

💔 All backports failed

Manual backport

Questions ?

Uh oh!

kibanamachine commented Apr 18, 2025

Uh oh!

kibanamachine commented Apr 21, 2025

Uh oh!

kibanamachine commented Apr 22, 2025

Uh oh!

kdelemme commented Apr 23, 2025

💚 All backports created successfully

Questions ?

Uh oh!

kibanamachine commented Apr 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kdelemme commented Apr 9, 2025 •

edited by kibanamachine

Loading