Show Errors if Share Fails #1168
Labels
Comments
|
Suggested places |
dlpzx
added
type: enhancement
dlpzx
added a commit
that referenced
this issue
Jun 19, 2024
…art 7 (share_object_service) (#1340) ### Feature or Bugfix - Refactoring ### Detail As explained in the design for #1123 and #1283 we are trying to implement generic `datasets_base` and `shares_base` modules that can be used by any type of datasets and by any type of shareable object in a generic way. The goal of this PR is to move the `share_object_service` to `shares_base` and refactor any dependency to S3 in the service. - Move file and fix imports of ShareObjectService - Use DatasetsBase and DatasetsBaseRepository instead of the S3 equivalents -⚠️ Avoid Dashboard check logic in `ShareObjectService.submit_share_object` see below -⚠️ Avoid SharePolicyService logic in `ShareObjectService.create_share_object` see below - Create ShareLogsService for logs - Remove unused methods - I also copied share_item_service to shares_base (it will be used in next PR) #### Avoid Dashboard check logic in `ShareObjectService.submit_share_object` Currently, whenever a share request is submitted, we check if the REQUESTER environment has dashboards enabled and if there are shared tables we verify that the Quicksight subscription is active. Alternative: perform this check in the share processor of tables. It solves the issue, but it gives a poorer user experience as it is difficult to figure out for the requester why the share failed. This can be solved holistically as requested in #1168. Decision: move the logic to the processor and make the table share fail. #### Avoid SharePolicyService logic in `ShareObjectService.create_share_object` When a share request is first created, we perform a series of operations to ensure that an IAM policy for the share requester principal IAM role is created. Alternative 1: move this logic inside the share processor. Not sure if it is possible. It would be the ideal solution, but the SharePolicyService throws errors in the create share object API if the policy is not attached. Alternative 2: implement interface to define share_policies (similar to the dataset-actions that use share logic in `backend/dataall/modules/s3_datasets_shares/services/dataset_sharing_service.py`). Decision: we want to preserve the user experience of having the IAM policy created before the share request is processed. Plus, it is not an uncommon pattern that could get extended by other dataset types, for example redshift sharing might need additional share policies. For this reason this PR implements alternative 2 ### Relates - #1283 - #1123 - #955 ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
As a user I want to see the reason why share request is processed with errors.
Right now we have only health status, but it doesn't show the errors occurred during share processing.
The text was updated successfully, but these errors were encountered: