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.

Sign up for GitHub

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

Jump to bottom

Create generic shares_base and s3_datasets_shares modules from current dataset_sharing #1283

Closed
dlpzx opened this issue May 17, 2024 · 2 comments
Closed

Create generic shares_base and s3_datasets_shares modules from current dataset_sharing #1283

dlpzx opened this issue May 17, 2024 · 2 comments
Assignees
@dlpzx
Labels
effort: large priority: high status: in-progress This issue has been picked and is being implemented type: newfeature New feature request
Projects
v2.6.0

Comments

@dlpzx
Copy link
Contributor

dlpzx commented May 17, 2024 •
edited

Is your feature request related to a problem? Please describe.
It is difficult to add new sharing item types with the current implementation of the datasets_sharing module.

Describe the solution you'd like
If we want to add more types of sharing(e.g. Redshift dataset sharing, notebook sharing) a first step will be the creation of a generic class that defines a data.all Shares abstraction that can be used by each of the particular sharing implementations.

Describe alternatives you've considered
This are rough design considerations that might change during implementation:

⚠️ WIP
This item is a continuation of #1123 that implements the top part of the diagram, the separation between datasets_base and s3_datasets. Now we need to do the same for shares.

Backend

image

1. shares_base module

With the generic code to create share objects and share items.

  • api - all APIs on ShareObject and the approval/reject/revoke workflow. List generic ShareItems
  • aws - nothing, it should be technology agnostic
  • db - generic ShareObject and ShareItem models. Operations on these models
    - in ShareObject: rename datasetUri as targetUri
    - in ShareItem: remove Glue and S3 details from the shareItem
    - repositories: all operations that do not involve details on S3/Glue
  • cdk - nothing, it does not involve any particular AWS IAM permissions or infra
  • handlers - ECS handler that handles shares tasks and executes sharing manager methods accordingly
  • services
    • sharing_service - interfaces to implement ShareProcessors
    • shares_enums - they are generic - ShareItemTypes will be used to know the items that can be shared
    • shares_permissions - they are generic
    • share_object_service
    • share_item_service
  • tasks
    • generic shareTask that loads all shareProcessors
    • generic shareVerifier that loads all shareProcessors
    • generic shareReapply that loads all shareProcessors

image

2. s3_datasets_shares module

With the specific code to share S3 dataset items.

  • api - TBD: if there is any API that is specific to s3 datasets it will remain here
  • aws: the same ones as before, lakeformation, s3, glue, kms...
  • db
    • Specific repositories to get share items with details of Tables, Folders, Buckets
  • cdk
    • pivot role and env role permissions
  • handlers: nothing
  • services: the specific services used to share S3 resources in the ECS task
    • processors
    • managers
    • other utilities
  • tasks: no sharing tasks! the actual sharing ECS task is generic, the specific logic is imported through services

Frontend

Most views related to shares are very generic. If anything, share item tables and share details (consumption details) might be s3-specific. Since the shares view is being modified in other workstreams this is paused at the moment.

Config.json

tbd
@dlpzx dlpzx added status: in-progress This issue has been picked and is being implemented type: newfeature New feature request priority: high effort: large labels May 17, 2024
This was referenced May 17, 2024
Closed
Merged
@dlpzx
Copy link
Contributor Author

dlpzx commented May 17, 2024 •
edited

Step-by-step implementation plan

  • Rename dataset_sharing to s3_datasets_shares and create shares_base module (PART1)
  • Move enums and permissions to shares_base(PART1)
  • Move ShareObject and ShareItem db models to shares_base (PART2)
  • Move Share state machines to shares_base into its own file (PART2)
  • Create Interface in shares_base to execute sharing tasks from s3_datasets_shares (PART3)
  • Same for Share-verify tasks (PART3)
  • Same for Share-reapply tasks (PART3)
  • Create base ShareItem model in shares_base. Use ShareItem to create S3ShareItems or GlueShareItems, S3BucketShareItem, S3AccessPointShareItem --> Instead of making multiple ShareItems with particular information about the shareItem (glutable...) we should be using the itemType and itemUri to get the table technical data. The task is now: remove the usage of shareItem.GlueTableName, GlueDatabaseName and AccessPointName (PART4)
  • Move generic support files to shares_base: exceptions and notifications (PART5)
  • Split APIs into generic APIs and s3-specific APIs (PART6)
  • Make share object service independent from s3 datasets and move to shares_base (PART7)
  • Make share item service independent from s3 datasets and move to shares_base, create a second share_item service in s3 if needed (PART8)
  • Split db-repositories (PART9)
  • Remove any other S3 reference in shares_base (PART10)
  • Rename files and clean-up s3_datasets_share (PART11)

The next items are enhancements

@dlpzx dlpzx mentioned this issue May 22, 2024
Merged
dlpzx added a commit that referenced this issue May 22, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
56fbd93 
…art 1 (renaming, enums and permissions) (#1284)

### Feature or Bugfix
- Feature
- 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.

In this PR:
- Rename `dataset_sharing` as `s3_dataset_shares`
- Create `shares_base` and introduce dependency (`s3_dataset_shares`
depends on `shares_base`)
- Move generic enums to shares_base
- Move generic permissions to shares_base


### 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.
This was referenced May 22, 2024
Merged
Merged
dlpzx added a commit that referenced this issue May 22, 2024
@dlpzx
Generic dataset module and specific s3_datasets module - part 6 (Fron…
372ac51 
…tend) (#1292)

### Feature or Bugfix
- Feature

### Detail
As explained in the design for #1123 we are trying to implement a
generic `datasets_base` module that can be used by any type of datasets
in a generic way.

In this PR we:
- Create DatasetsBase module in frontend. Depends on S3_Datasets module
- Move DatasetsList view and DatasetListItem component to DatasetsBase
- Add CreateDataset modal that allows multiple types of datasets
creation
- Fix routes and redirects to point at /datasets/ or any other
/X-dataset/
- Move dataset_base services. In backend/datasets_base/api we define the
following queries that are good candidates to become part of the
DatasetsBase module
- listDatasets - only used in DatasetsBase/DatasetList view - it should
be in DatasetsBase/services
- listOwnedDatasets - only used in Shares/SharesBoxList view - it should
be in Shares/services
- listDatasetsCreatedInEnvironment - only used in
Environments/EnvDataset tab - it should be in Environment/services

If we want to keep everything clean we could rename all "datasets" as
"s3_datasets" or equivalent in the S3_Datasets module. Because it is a
cosmetic change that would pollute the PR a lot I have decided not to
include it.

⚠️ UPDATE: Next steps
The Data Shared With You table in Environments>Datasets tab needs a
remake. It contains references to each type of item and it is very
coupled with s3-dataset-shares. For the moment I just made it work for
the changes of s3-datasets, but when completing the work in #1283 we
should fix this. Maybe in favor of DataGrid

### Relates
- #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.
dlpzx added a commit that referenced this issue May 23, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
a0fc4b0 
…art 2 (db objects to shares_base) (#1294)

### 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.

In this PR:
- Move shares state machines into its own file and move them to the
`shares_base` module.
- Create new ShareObjectRepository and copy some generic methods to it
- Move db share objects to shares_base:
- ShareObject has a field called datasetUri. For the scope of S3 and
Redshift datasets we can leave it as dataseturi, but if we want to
implement other kinds of sharing we should rethink it: we need to store
the "approvers" of the share in some way. For the moment I am not going
to go down that path until shares and s3 are uncoupled, then we can see
how we would implement a complete generic sharing.
- ShareObjectItem includes 3 fields related to the glue tables... we
need to get rid of them in the backend code and look up the info with
the itemType and itemUri. Left for part3 to keep the PR clean.


### 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.
This was referenced May 23, 2024
Merged
Open
@dlpzx dlpzx added this to To do in v2.6.0 via automation May 31, 2024
@dlpzx dlpzx moved this from To do to In progress in v2.6.0 May 31, 2024
@dlpzx dlpzx self-assigned this May 31, 2024
dlpzx added a commit that referenced this issue Jun 4, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
e20e750 
…art 3 (share processor and manager interfaces) (#1298)

### Feature or Bugfix
- Feature
- 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.

In this PR:
- Move ECS handler for sharing tasks to shares_base
- Move sharing tasks to shares_base
- Move DataSharingService to shares_base and rename it as
SharingService. Make SharingService generic and remove any reference to
specific items. In this process:
- attach and delete table and folder permissions are moved into the
specific share processor. delete permissions are processed item by item
and moved into the shareItemService
- Some methods are copied from ShareObjectRepository in
s3_datasets_shares to shares_base. They have not been removed from
s3_datasets_shares, instead there is a TODO marking those methods that
have been copied. The migration and clean-up of shareObjectRepository
will be done in a following PR
- Need to introduce `DatasetBaseRepository.get_dataset_by_uri(session,
share.datasetUri)` to avoid future circular dependencies: shares_base
depends only on datasets_base.
- Clean-up and consolidate methods: remove updates of the share items
outside of state machine transactions. Only re-used methods and dataset
lock handling in share_manager --> TODO: dataset_lock manager should be
its own service outside of sharing, but this is out of the scope of this
PR.
- Add updates of share_item statuses in except clauses for more robust
sharing
- Introduce ShareProcessor and ShareManager interfaces and use them in
the SharingService: instead of the processor inheriting the manager
class, the processor uses the ShareProcessor interface and constructs a
manager when needed.
- Introduce new load ImportMode `SHARES_TASK` and register
ShareProcessors in s3_datasets_share
(`backend/dataall/modules/s3_datasets_shares/__init__.py`)


![image](https://github.com/data-dot-all/dataall/assets/71252798/af4eafc3-c990-4532-ba25-62ef950791aa)

See full detail of SharingService design in #1283

### Next steps/Open questions
For failures I think we should rollback whatever actions where
performed. For example, if we are sharing a table and it failed in one
step, it should revert all the steps executed before. @petrkalos
@SofiaSazonova @noah-paige what do you think?



### 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.
This was referenced Jun 5, 2024
Merged
Merged
dlpzx added a commit that referenced this issue Jun 6, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
35708a9 
…art 4 (remove s3 info from shareItem db models) (#1311)

### 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.

In this PR:
- Remove the fields `GlueDatabaseName`, `GlueTableName` and
`S3AccessPointName` from the ShareObjectItem db model. The goal is to
have a generic ShareObjectItem and access the specific item information
through its itemUri and itemType.
- Migration script to drop those columns. 
- Disclaimer, there is still work to do on the gql types; but that is
out of the scope for this PR

### 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.
@dlpzx dlpzx mentioned this issue Jun 6, 2024
Open
dlpzx added a commit that referenced this issue Jun 6, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
2266126 
…art 5 (move exceptions and notifications to shares_base) (#1312)

### 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.

In this PR:
- Move share_exceptions to shares_base
- Move share_notification_service to shares_base


### 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.
@dlpzx dlpzx mentioned this issue Jun 7, 2024
Merged
dlpzx added a commit that referenced this issue Jun 17, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
52d44c7 
…art 6 (Split APIs and graphql types) (#1320)

### Feature or Bugfix
- Feature
- 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.

- First, this PR splits the query used in Worksheet and Environments to
list glue databases and list datasets. They are pretty different
queries, the one used in Worksheets is only relevant for S3 datasets,
while the one in Environment is focused on the share items in general:
- Introduce new API `listS3DatasetsSharedWithEnvGroup` to list shared
glue databases in Worksheets view. It is part of the s3_datasets_shares
module. This new API replaces the usage of `searchEnvironmentDataItems`
in Worksheets frontend.
- Remove Glue-parameters from `searchEnvironmentDataItems` API, this API
belongs to shares_base. It is only used in the Environment view >
Datasets tab, so I moved the API in frontend to modules/Environment.
- remove unused parameters (`tables`, `locations`) from statistics in
`api/types/ShareObjectStatistics`. Now the statistics are only generic.

- Introduce new API `getS3ConsumptionData` in s3_datasets_shares. This
new API call gets the details of gluedatabase/table, s3accesspoint that
were previously part of ShareObject. This way the graphql ShareObject
does not contain specific S3 info.

- The rest of the APIs have been split in `shares_base` and
`s3_datasets_shares`. In general, all the share lifecycle (create, add
items, approve...) is part of shares_base. listDatasetShareObjects,
verifyDatasetShareObjects used in the S3-Dataset UI are part of
s3_datasets_shares

- TODO: Review tests and create new tests for get_consumption_data.
Currently tests for shares and datasets are placed in the same folder. I
will open a separate PR to order the tests a bit before this

### Relates
- #1283 
- #1123 
- #955 
- #1277 ---> This PR needs
to be merged and then I will introduce some changes in the ShareView.

### 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.
@dlpzx dlpzx mentioned this issue Jun 18, 2024
Merged
dlpzx added a commit that referenced this issue Jun 19, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
3d2dfc7 
…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.
@dlpzx dlpzx mentioned this issue Jun 19, 2024
Merged
dlpzx added a commit that referenced this issue Jun 19, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
dda4153 
…art 8 (sharei_item_service) (#1350)

### 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 split the `share_item_service`logic into the a
generic service in `shares_base` and an specific service in
s3_datasets_shares.
- `ShareItemService` in shares_base only has shareItem logic without
references to S3 or Glue.
- `S3ShareItemService` in s3_datasets_shares has logic for share items
that are tables and folders.

The files' names are a bit messy but i don't want to pollute this PR
with more changes. I'll do a review of the file names in part 10.

### 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.
@dlpzx dlpzx mentioned this issue Jun 20, 2024
Merged
dlpzx added a commit that referenced this issue Jun 21, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
342798c 
…art 9 (share db repositories) (#1351)

### 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.

This PR includes:
- Split the ShareobjectRepository from s3_datasets_shares into:
- `ShareobjectRepository` (shares_base) - generic db operations on share
objects - no references to S3, Glue
- `ShareStatusRepository` (shares_base) - db operations related to the
sharing state machine states - a way to split the db operations into
smaller chunks
- `S3ShareobjectRepository` (s3_datasets_share) - db operations on s3
share objects - used only in s3_datasets_shares. They might contain
references to DatasetTables, S3Datasets... They are used in the clean-up
activities and to count resources in environment.

- Adapt `S3ShareobjectRepository` to S3 objects. For some queries it was
needed to add filters on the type of share items retrieved, so that if
in the future anyone adds a new share type the code still makes sense.
To add some more meaning, some functions are renamed to clearly point
out that they are s3 functions or what they do.

- Make `ShareobjectRepository` completely generic. The following queries
needed extra work:
- ShareObjectRepository.get_share_item - renamed as
`get_share_item_details`
- `list_shareable_items` - split in 2 parts
`list_shareable_items_of_type` + `paginated_list_shareable_items`: the
first function is invoked recursively over the list of share processors,
instead of querying the DatasetTable, DatasetStorageLocation and
DatasetBucket we query the shareable_type. The challenge is to get all
fields from the db Resource object that all of them are built upon. In
particular the field `itemName` does not match the BucketName (in
bucket) or the S3Prefix (in folders). For this reason I added a
migration script to backfill the DatasetBucket.name as
DatasetBucket.S3BucketName. and the DatasetStorageLocation.name with
DatasetStorageLocation.S3Prefix. `paginated_list_shareable_items` joins
the list of subqueries, filters and paginates.
- In verify_dataset_share_objects instead of using list_shareable_items,
I replaced it by `ShareObjectRepository.get_all_share_items_in_share`
which does not need tables, storage, avoiding the whole S3 logic and
avoiding unnecessary queries

- Remove S3 references from shares_base.api.resolvers. Use DatasetBase
and DatasetBaseRepository instead. Remove unused `ShareableObject`.

- I had some problems with circular dependencies so I created the
`ShareProcessorManager` in shares_base for the registration of
processors. The SharingService uses the manager to get all processors.

Missing items for Part10:
- Lake Formation cross region table references in
shares_base/services/share_item_service.py:add_shared_item
- remove table references in
shares_base/services/share_item_service.py:remove_shared_item
- remove s3_prefix references
shares_base/services/share_notification_service:notify_new_data_available_from_owners
- RENAMING! Right now the names are a bit misleading

### 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.
@dlpzx dlpzx mentioned this issue Jun 21, 2024
Merged
dlpzx added a commit that referenced this issue Jun 25, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
dc1d5de 
…art 10 (other s3 references in shares_base) (#1357)

### 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.

This PR:
- Remove the delete_resource_policy conditional for Tables in
`backend/dataall/modules/shares_base/services/share_item_service.py` -->
Permissions to the Table in data.all are granted once the share has
succeeded, the conditional that checks for share_failed tables should
not exist.
- Remove unnecessary check in share_item_service: in add_share_item we
check if it is a table whether it is a cross-region share. This check is
completely unnecessary because when we create a share request object we
are already checking if it is cross-region
- Use `get_share_item_details` in add_share_item - we want to check if
the table, folder, bucket exist so we need to query those tables.
- Move s3_prefix notifications to subscription task
- Fix error in query in
`backend/dataall/modules/shares_base/db/share_state_machines_repositories.py`


### 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.
@dlpzx dlpzx mentioned this issue Jun 25, 2024
Merged
@dlpzx dlpzx moved this from In progress to Review in progress in v2.6.0 Jun 26, 2024
dlpzx added a commit that referenced this issue Jun 27, 2024
@dlpzx
Generic shares_base module and specific s3_datasets_shares module - p…
2e80de4 
…art 11 (renaming and cleaning up s3_shares) (#1359)

### 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.

This is one of the last PRs focused on renaming files and cleaning-up
the s3_datasets_shares module. The first step is a consolidation of the
file and classes names in the services to clearly refer to s3_shares:
- `services.managed_share_policy_service.SharePolicyService` --->
`services.s3_share_managed_policy_service.S3SharePolicyService`
- `services.dataset_sharing_alarm_service.DatasetSharingAlarmService`
--> `services.s3_share_alarm_service.S3ShareAlarmService`
- `services.managed_share_policy_service.SharePolicyService` -->
`services.s3_share_managed_policy_service.S3SharePolicyService`

👀 The main refactoring happens in what is used to be
`services.dataset_sharing_service`.
- The part that implements the `DatasetServiceInterface` has been moved
to `services/s3_share_dataset_service.py` as the `S3ShareDatasetService`
- The part used in the resolvers and by other methods has been renamed
as `services.s3_share_service.py` and the methods for the folder/table
permissions are also added to the S3ShareService (from
share_item_service)

Lastly, there is one method previously in share_item_service that has
been moved to the GlueClient directly as
`get_glue_database_from_catalog`.


### 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.
@dlpzx dlpzx closed this as completed Jun 28, 2024
v2.6.0 automation moved this from Review in progress to Done Jun 28, 2024
@dlpzx
Copy link
Contributor Author

dlpzx commented Jun 28, 2024

The core functionality has been completed. There will be follow-up enhancements related to shares tackled in separate github issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dlpzx dlpzx

Labels
effort: large priority: high status: in-progress This issue has been picked and is being implemented type: newfeature New feature request
Projects
No open projects
v2.6.0
Done
Milestone
No milestone
Development

No branches or pull requests
1 participant
@dlpzx