-
Notifications
You must be signed in to change notification settings - Fork 217
feat: Introducing Dataset Permissions #4074
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
base: dev
Are you sure you want to change the base?
Conversation
tenthe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @JHoelli,
thanks a lot for the PR, this is a really cool new functionality.
I tested it locally and everything worked as expected, including the migration.
I only have a few minor code comments.
ui/src/app/dataset/components/datalake-configuration/datalake-configuration.component.ts
Outdated
Show resolved
Hide resolved
...ement/src/main/java/org/apache/streampipes/manager/permission/DataLakePermissionManager.java
Outdated
Show resolved
Hide resolved
...ement/src/main/java/org/apache/streampipes/manager/permission/DataLakePermissionManager.java
Outdated
Show resolved
Hide resolved
...peline-management/src/main/java/org/apache/streampipes/manager/pipeline/PipelineManager.java
Outdated
Show resolved
Hide resolved
...s-rest/src/main/java/org/apache/streampipes/rest/impl/datalake/AbstractDataLakeResource.java
Outdated
Show resolved
Hide resolved
...s-rest/src/main/java/org/apache/streampipes/rest/impl/datalake/AbstractDataLakeResource.java
Outdated
Show resolved
Hide resolved
...va/org/apache/streampipes/service/core/migrations/v099/CreateDatasetPermissionMigration.java
Outdated
Show resolved
Hide resolved
|
Hi @JHoelli, I just realized we need to change the logic for setting data lake permissions. Currently, this is handled in |
…mpipes/manager/pipeline/PipelineManager.java Co-authored-by: Philipp Zehnder <tenthe@users.noreply.github.com>
…mpipes/manager/pipeline/PipelineManager.java Co-authored-by: Philipp Zehnder <tenthe@users.noreply.github.com>
Current IssueWhen moving permission management for datasets from the Pipeline Manager to the DataLakeResource, we are facing an issue where the principal ID is lost if a data lake is created from a pipeline. This occurs because the getAuthenticatedUserSid() method only returns the user ID of the service principal (sp-service-client), which results in incorrect ownership assignments. On the other hand, when a data lake is created via API, the correct ownership is assigned, as the user token is properly used. Creating a data lake via API results in the correct ownership due to the provided user token. Possible SolutionsMove the Measurement Creation to the DataSinkInvocationIdea: Split the measurement creation and update process. Only create the measurement and pipeline permissions within the Pipeline Manager. Pros:
Cons:
Add an additional HeaderIdea: Introduce a new HTTP header to specify if the authenticated user is an admin. If the user is an admin, allow the system to use the user ID from the token (via TokenAuthenticationFilter) instead of the service principal ID or introduce an 'onbehalfof'. Reasoning: If a user already has admin rights, setting another user reduces those rights. Pros:
Cons:
Create Permissions in the Pipeline Manager and only check if Permissions are available in the DataMeasureResourceIdea: Revert to the old approach: create the permissions in the Pipeline Manager with the authenticated user. In the DataMeasureResource, simply check whether permissions already exist. If not, use the currently authenticated user to set the permission. Pros:
Cons:
|
Purpose
Intoducing first simple permissions for Dataset.
Assumptions
Concept
Example
A dashboard user or admin, the uses a chart which on the other hand is based on a dataset, also needs to be given explicit rights on that dataset to be able to see the chart with values.
Remarks
Backend
UI
Migration
Testing
PR introduces (a) breaking change(s): no
PR introduces (a) deprecation(s): no