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

Feat: improve share views - add filters #885

Merged
merged 16 commits into from
Nov 27, 2023
Merged

Feat: improve share views - add filters #885

merged 16 commits into from
Nov 27, 2023

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Nov 20, 2023

Feature or Bugfix

  • Feature

Detail

Relates

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

  • 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)? -- Yes, it introduces a new GraphQL Query: listAllGroups
    • Is the input sanitized? -- N/A as no input is needed
    • 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
Initial draft filtering in Share inbox
5294aa5
@dlpzx
Second draft
8f0dd8f
@dlpzx
Single view for ShareInbox and ShareOutbox and added filters
66e23bb
@dlpzx
Added filters in backend and extra resolver listAllGroups
963fd94
@dlpzx
Merge remote-tracking branch 'origin/feat/improve-share-views' into f…
1e8733b 
…eat/improve-share-views

# Conflicts:
#	backend/dataall/modules/dataset_sharing/db/share_object_repositories.py
#	frontend/src/modules/Shares/components/ShareBoxList.js
#	frontend/src/modules/Shares/components/ShareInboxList.js
#	frontend/src/modules/Shares/components/ShareOutboxList.js
@dlpzx dlpzx linked an issue Nov 20, 2023 that may be closed by this pull request
Enable "SHARES" tab under datasets for requestors #644
Closed
noah-paige
noah-paige reviewed Nov 22, 2023
View reviewed changes
frontend/src/modules/Shares/components/ShareBoxList.js
@@ -0,0 +1,489 @@
import {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For outbox) We use getShareRequestsFromMe to - setItems, setDatasets and setDatasetGroupOptions (and call the query 2 separate times for this)

  • Could we have it only called once and set all the subsequent states once we get a response
  • Either in 1 single outbox function or can set items and then when items change re-set the datasets and dataset group option

(For inbox) Similarly, we use getShareRequestsToMe to - setItems and setRequestGroupOptions

  • Can we also use this query to setDatasets rather than listDatasets
  • Could we have it only called once and set all the subsequent states once we get a response

The high level flow I am thinking

  • useEffect to setItems (as is already)
    • Depending on datasets shares view, inbox tab or outbox tab call the appropriate function to set items (the share objects)
  • Have a separate useEffect that re-renders on changes to items
    • Calls listAllGroups
    • Then can use the existing items already set to determine group options and datasets (Do not need to re query that same API)

Copy link
Contributor Author

@dlpzx dlpzx Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue is the pagination and number of items retrieved. When we are setting the options we do not want to set the options based on the first 10 shares only.

FILTER request owners roles datasets dataset owners
INBOX (received) ListRequests parse owners ListRequests parse roles ListDatasets ListGroups
OUTBOX (sent) ListGroups ListGroups+ListConsumptionRoles ListRequests parse datasets ListRequests parse datasets owners

ListRequest (getShareRequestsFromMe, getShareRequestsToMe, listDatasetShareObjects)

  • initial execution to set filterOptions in several functions - pageSize=Defaults.selectListFilter
  • initial execution fetchXItems - pageSize=Defaults.filter
  • filter change fetchXItems - pageSize=Defaults.filter

ListDataset, ListGroups, ListConsumptionRoles

  • initial execution to set filterOptions

Replacing the API call with the frontend function:

FILTER request owners roles datasets dataset owners
INBOX (received) fetchInboxRequestOptions fetchInboxRequestOptions fetchOwnedDatasetsOptions fetchMyGroupsAndRolesOptions
OUTBOX (sent) fetchMyGroupsAndRolesOptions fetchMyGroupsAndRolesOptions fetchOutboxRequestOptions fetchOutboxRequestOptions

filterOptions useEffect - ListRequest paginated with pageSize=Defaults.selectListFilter

  • on tab change

fetchItems useEffect - ListRequest paginated with pageSize=Defaults.filter

  • on tab change
  • on filter change

@noah-paige
Copy link
Contributor

noah-paige commented Nov 22, 2023 •
edited
Loading

Test:

  • Pre-existing shares showing in ShareBox View (both Received and Sent)
  • Open Share Request Button works

Share Received (Inbox)

  • Filter By Status
  • Filter by Dataset
    Only showing 5 datasets, when I own 9 Datasets. I think this may be due to some pagination issues because listDataset is not picking up the filter we are trying to pass in and setting the filter to filter = {'page': 1, 'pageSize': 5} in def list_datasets (when I look at the response in the UI I see the total count is 9 but it has 2 pages)

Also showing datasets that are shared with the user but not owned by the user in the received share requests (because of listDataset()) query logic

  • Filter By DatasetOwners (Group Showing 3 times)

Showing my same group 3 times since I am a part of 3 different environments and thus have 3 different EnvironmentGroup entries when calling listAllGroups() - think just need to add a distinct() filter here on the query or use a Set on the frontend

  • Filter By RequestOwners

This works but only shows the RequestPrincipals that are for shares with principalType Group. I think this is because we are pulling the principal.SamlGroupName which will all be the group name that owns the consumption role (when of type consumption role). We may want to add some logic to pull principalName rather than SamlGroupName for consumption role types

Share Sent (Outbox)

  • Filter By Status

  • Filter by Dataset
    Unlike above, it is working when using getShareRequestsFromMe to determine datasets

  • Filter By DatasetOwners (Group Showing 3 times)
    Unlike above, it is working when using getShareRequestsFromMe to determine datasetOwner Groups

  • Filter By RequestOwners
    Same issue as above with DatasetOwners (on listAllGroups)

Dataset Shares View

  • Pre-existing shares showing in Dataset Share Tab
  • Filter by Status or Request Owner
    Filters can be set but are not actually filtering the results in the Dataset Shares View. listDatasetShareObjects API needs similar filters in the backend the same way we have to getShareRequestsFromMe and getShareRequestsToMe

@noah-paige
Copy link
Contributor

Overall, I think the design and UI views are super and a great improvement! Left some comments on the issues I ran into, mostly minor fixes but some bigger ideas / thoughts as well

Let me know what you think @dlpzx

@dlpzx
Copy link
Contributor Author

dlpzx commented Nov 24, 2023 •
edited
Loading

Per customer request I added a filter for the IAM role name (That also solves the problem of duplicates in requester groups). To be able to support consumption roles I had to create a couple of resolvers.

The only thing left is reviewing the useEffect logic, which I addressed in your comment. I did some clean-up and renaming of functions. Now I think it is really ready for review.

I would merge this PR and then work on additional items of the issue if needed. I would add only those features that add value, avoiding a too messy to handle UI

@dlpzx dlpzx marked this pull request as ready for review November 24, 2023 09:55
@noah-paige
Copy link
Contributor

Updated Testing in AWS Deployment:

  • Pre-existing shares showing in ShareBox View (both Received and Sent)
  • Open Share Request Button works

Share Received (Inbox) - Testing Filters Show Correct Options and Work Filtering Shares

  • Filter By Status
  • Filter By RequestOwners - Showing Group Names of Requests
  • Filter By IAM Role - Showing IAM Roles of Requests
  • Filter by Dataset - Showing Datasets Owned by User’s Teams
  • Filter By DatasetOwners - Showing User’s Teams

Share Sent (Outbox)

  • Filter By Status
  • Filter By RequestOwners - Showing User’s Teams
  • Filter By IAM Role - Showing User’s Team IAM Roles / Consumption Roles
  • Filter by Dataset - Showing Datasets In User’s Shares Sent
  • Filter By DatasetOwners - Showing Dataset Owners of Datasets in User’s Shares Sent

Dataset Shares View

  • Pre-existing shares showing in Dataset Share Tab
  • Filter By Status, Request Owners, and Request IAM Role Names —> Showing same as inbox shares view

My one comment is that we are returning all request owners and all request IAM role names for all existing shares (not filtering for also the particular dataset we are looking into for the dataset-shares tab).

An enhancement could be to enforce filtering for only this dataset when calling getShareRequestsToMe in fetchInboxRequestOptions to limit the options shown for this particular tab (not sure how easy this is considering the way it is designed to re-use functions across the 3 different share views in UI)

  • Filters are correctly filtering Shares show on UI

The filters can be set but do not actual filter the outputted Share Objects in the dataset share tab. listDatasetShareObjects API needs similar filters in the backend the same way we have to getShareRequestsFromMe and getShareRequestsToMe

@dlpzx
Copy link
Contributor Author

dlpzx commented Nov 27, 2023

Hi @noah-paige, I changed a bit how we fetch share items in the dataset view. Instead of modifying the existing getDataset, I re-used the fetchInbox methods but setting the filter to the dataset_uri and dataset_owner. I also fixed some conflicts with the filters that were not set to the initial state on tab or window changes

@noah-paige
Copy link
Contributor

Code changes look good! Think this will fix the remaining issues I was seeing with the Dataset Shares View - to test once more and report back any findings

@noah-paige
Copy link
Contributor

Final Testing in AWS Deployment:

  • Pre-existing shares showing in ShareBox View (both Received and Sent)
  • Open Share Request Button works

Share Received (Inbox) - Testing Filters Show Correct Options and Work Filtering Shares

  • Filter By Status
  • Filter By RequestOwners - Showing Group Names of Requests
  • Filter By IAM Role - Showing IAM Roles of Requests
  • Filter by Dataset - Showing Datasets Owned by User’s Teams
  • Filter By DatasetOwners - Showing User’s Teams

Share Sent (Outbox)

  • Filter By Status
  • Filter By RequestOwners - Showing User’s Teams
  • Filter By IAM Role - Showing User’s Team IAM Roles / Consumption Roles
  • Filter by Dataset - Showing Datasets In User’s Shares Sent
  • Filter By DatasetOwners - Showing Dataset Owners of Datasets in User’s Shares Sent

Dataset Shares View

  • Pre-existing shares showing in Dataset Share Tab
  • Filter By Status, Request Owners, and Request IAM Role Names —> Showing Request Owners and IAM Roles for particular Dataset’s Shares
  • Filters are correctly filtering Shares shown on UI

noah-paige
noah-paige approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@noah-paige noah-paige left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is good to go! Looks great @dlpzx - approving now

@dlpzx dlpzx merged commit 8e9b7a1 into main Nov 27, 2023
8 checks passed
@dlpzx dlpzx deleted the feat/improve-share-views branch December 18, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noah-paige noah-paige noah-paige approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable "SHARES" tab under datasets for requestors
2 participants
@dlpzx @noah-paige