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

Fix: worksheet UI improvements - fix Team and list Environments of Team #1111

Merged
merged 7 commits into from
Apr 4, 2024

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Mar 15, 2024

Feature or Bugfix

  • Feature
  • Bugfix

Detail

  • Make Team static when opening a Worksheet. It is the same Team that the one that owns the Worksheet
  • Modify ListEnvironments to show only the environments for this user AND TEAM
  • Fix one bug on shared databases being visible for all teams of an environment
  • todo: open 2 issues for the issues found to be fixed on a separate PR

Testing

Testing scenario:

  • Environment 1: TeamA and TeamB added to environment
  • Environment 2: TeamA and TeamB added to environment. TeamA creates consumption role CRA
  • Dataset created 1: Env1-TeamA
  • Dataset imported 1: Env1-TeamA
  • Dataset imported with central catalog: Env1-TeamA ---> shared with Env1-TeamB
  • Dataset created2: Env2-TeamA ---> shared with Env1-TeamA and Env1-CRA

Worksheet owned by TeamA

  • UI can Select Env1 or Env2
  • When selecting env1: UI shows OWNED CREATED database without duplicates
  • When selecting env1: UI shows OWNED IMPORTED database without duplicates
  • When selecting env1: UI shows OWNED IMPORTED CENTRAL CATALOG database without duplicates --> 🔴 The listing of tables is correct, but the query fails Insufficient permissions to execute the query. Insufficient Lake Formation permission(s) on rl_imported_central_sse_c3
  • When selecting env1: UI shows SHARED database without duplicates 🔴 it shows 1 single database for the 2 shares (TeamA and CR_A). Each share requested a different table. In the drop-down it shows both tables which is incorrect, but, querying the table shared with CR_A fails TABLE_NOT_FOUND: line 1:15: Table 'awsdatacatalog.dataall_lcreated_a2_l7pm61ii_shared.books_raw' does not exist ---> FIXED!
  • When selecting env2: UI shows OWNED database in env2
  • UI can change from environment, database... without glitches - databases and tables are cleaned up

Worksheet owned by TeamB

  • When selecting env1: UI shows SHARED IMPORTED CENTRAL CATALOG database without duplicates 🔴 Error: it shows the SHARED database with TeamA!! --> FIXED!
  • When selecting env2: UI shows no databases
  • UI can change from environment, database... without glitches

Issues found:

  • Insufficient Lake Formation permissions when running Query for OWNED IMPORTED CENTRAL CATALOG database
  • When there are consumption roles the shared database shows all tables shared (the ones with team IAM roles and the ones with CR) when it should only show the ones for the team IAM roles. The issue is that now we have a single Glue shared database and we are getting the tables from Glue, which means we get all the shared tables. The issue is in getSharedDatasetTables --> Fixed
  • Showing Shared databases with other teams --> Fixed

Relates

Security

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

  • 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 changed the title Fix/worksheet UI improvements Fix: worksheet UI improvements - fix Team and list Environments of Team Mar 15, 2024
@dlpzx dlpzx marked this pull request as ready for review March 27, 2024 12:05
@dlpzx dlpzx requested a review from SofiaSazonova March 27, 2024 12:05
@@ -517,6 +517,8 @@ def update_consumption_role(session, uri, env_uri, input):

@staticmethod
def query_user_environments(session, username, groups, filter) -> Query:
if filter and filter.get('SamlGroupName') and filter.get('SamlGroupName') in groups:
groups = [filter.get('SamlGroupName')]
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this new groups variable mean? Why we need to change the initial groups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

groups includes all the groups that the user belongs to, SamlGroupName includes a single group. We verify that the selected group is part of the groups of the user

Copy link
Contributor

@SofiaSazonova SofiaSazonova Apr 3, 2024

Choose a reason for hiding this comment

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

But why afterwards you want to reset groups as the array consisting only of filter.get('SamlGroupName')?
It seems to be not safe, since we eliminate all other groups. I know, it won't be saved to database, but still in further code we cant refer to groups as all the groups that the user belongs to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep the bar high :) I have changed the way of doing this in this commit: 44262c7

I still need to test, but that will wait until tomorrow

@noah-paige noah-paige linked an issue Apr 3, 2024 that may be closed by this pull request
@dlpzx
Copy link
Contributor Author

dlpzx commented Apr 3, 2024

I want to do some investigation on the issue for consumption roles. Moving the PR to draft

@dlpzx dlpzx marked this pull request as draft April 3, 2024 16:06
@dlpzx dlpzx marked this pull request as ready for review April 4, 2024 09:54
@dlpzx dlpzx merged commit 28c291e into main Apr 4, 2024
8 checks passed
@dlpzx dlpzx deleted the fix/worksheet-ui-improvements branch April 11, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Worksheet drilldown improvements
2 participants