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

Enable reading global profiler settings not only by admins and bots #17042

Closed
mgorsk1 opened this issue Jul 16, 2024 · 4 comments · Fixed by #18292
Closed

Enable reading global profiler settings not only by admins and bots #17042

mgorsk1 opened this issue Jul 16, 2024 · 4 comments · Fixed by #18292
Assignees
Labels
enhancement New feature or request profiler

Comments

@mgorsk1
Copy link
Contributor

mgorsk1 commented Jul 16, 2024

Is your feature request related to a problem? Please describe.

We have a feature on our platform where we enable users to execute profiling jobs themselves. We provide curated workflow template, users update table and schema name and execute ad-hoc jobs for their tables. This was working fine until we upgraded to OM 1.4.0.0, where #15889 was introduced. Now, regular users (not admins or bots) cannot execute profiling jobs as they get 403 error on fetching global profiler config.

Describe the solution you'd like
Requesting global profiler config (get /api/v1/system/settings/profilerConfiguration) is not restricted only to admins and bots. Proposed approaches:

  • move this resource out of system settings
  • create resource ProfilerConfig (or more generic SystemSettings) with operations create edit read like with other resources

What's particularly interesting about aforementioned implementation is that authorizeAdminOrBot method is used only once throughout whole OM service - in said endpoint.

Describe alternatives you've considered
Since we are grouping permissions using Teams/Groups (we have Teams A, B, C and we assign users to their respective teams, then we assign DatabaseSchema owner to a team. For example schema transactions from Trino service is owned by team A and all members of team A can edit metadata in transactions schema tables) we considered extending functionality of OpenMetadata with scoped bot users (so we could create bot X that would be a member of team A - this bot would inherit permissions of the team but would be treated as bot) #15891.

Additional context
We follow shift-left paradigm, so instead of running profiling jobs within OM Airflow, we instead allow users to do this with their personal credentials and on their own desired cadence. This is very important security-wise, as our OM instance service connections cannot use accounts (NPAs) that have access to actual data so moving this responsibility to end users is our only way to get profiling data into OM.

@mgorsk1 mgorsk1 added the enhancement New feature or request label Jul 16, 2024
@mgorsk1
Copy link
Contributor Author

mgorsk1 commented Jul 16, 2024

cc @TeddyCr as you've been working on global profiler config

@TeddyCr
Copy link
Contributor

TeddyCr commented Jul 23, 2024

Hello @mgorsk1 thanks for the detail, could you share a bit more about the below? I would love to understand this flow a bit better.

We have a feature on our platform where we enable users to execute profiling jobs themselves. We provide curated workflow template, users update table and schema name and execute ad-hoc jobs for their tables.

@mgorsk1
Copy link
Contributor Author

mgorsk1 commented Jul 23, 2024

Sure, we enable our users to use om profiling workflows themselves from within their personal jupiter notebooks. In such case they connect to appropriate systems using their personal credentials and authenticate against om api using their personal jwt tokens

@TeddyCr
Copy link
Contributor

TeddyCr commented Jul 25, 2024

Ok I see, what you mean. let's take a look. I'll mark it as 1.6, but we'll try to tackle it for a minor 1.5.x release.

@TeddyCr TeddyCr self-assigned this Jul 25, 2024
@TeddyCr TeddyCr added bug Something isn't working and removed bug Something isn't working labels Jul 25, 2024
TeddyCr added a commit that referenced this issue Oct 17, 2024
* fix: custom properties folder name in generation to match expected package name (i.e. customProperties -- uppercase P)

* fix: allow non admin/bot to read profiler global config with the right permission

* style: ran java linting

* fix: custom properties import casing
harshach pushed a commit that referenced this issue Oct 17, 2024
* fix: custom properties folder name in generation to match expected package name (i.e. customProperties -- uppercase P)

* fix: allow non admin/bot to read profiler global config with the right permission

* style: ran java linting

* fix: custom properties import casing
harshach pushed a commit that referenced this issue Oct 19, 2024
* fix: custom properties folder name in generation to match expected package name (i.e. customProperties -- uppercase P)

* fix: allow non admin/bot to read profiler global config with the right permission

* style: ran java linting

* fix: custom properties import casing
harshach pushed a commit that referenced this issue Nov 3, 2024
* fix: custom properties folder name in generation to match expected package name (i.e. customProperties -- uppercase P)

* fix: allow non admin/bot to read profiler global config with the right permission

* style: ran java linting

* fix: custom properties import casing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request profiler
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants