Feed consistent permissions #1722

dlpzx · 2024-11-26T10:34:11Z

Feature or Bugfix

Feature
Bugfix

Detail

The Feeds module is used in the frontend in several modules. Some restrict access to admins only and some don't. In this PR we unify the behavior. ONLY ADMINS CAN SEE THE FEED in the frontend.

Dashboards: accessible to any user -----> add isAdmin
PIpelines: accessible to any user -----> add isAdmin
Redshift_Datasets: accessible to admin users only
Redshift_Tables : accessible to admin users only
S3_Datasets: accessible to admin users only
Folders: accessible to admin users only
Tables: accessible to admin users only

Alongside the frontend changes, the backend should follow the same logic and restrict the API calls with permissions checks. That is what this PR does, it introduces resource permission checks depending on the Feed targetType with GET_X permission checks.

Add security-focused tests for unauthorized cases

Testing

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)?
- 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.

petrkalos · 2024-11-26T13:56:28Z

backend/dataall/modules/feed/services/feed_service.py

@@ -41,6 +39,15 @@ def get_feed(
        targetUri: str = None,
        targetType: str = None,
    ) -> Feed:
+        context = get_context()
+        with context.db_engine.scoped_session() as session:
+            ResourcePolicyService.check_user_resource_permission(


nit: We used this logic in a couple of places already, does it worth exposing it as a decorator?

We use it for Stacks and for Feeds no? where else do we check that is using a function in the permission_name?

noah-paige

A call out that this is not restricted to only Admin/Creators on the backend for all target types:

For Pipelines GET_PIPELINE only given to pipeline creator so now on restricted both to only creators on FE and BE
For Dashboards GET_DASHBOARD is given to Dashboard creators and when dashboards have been shared to a group (but on FE only restricted to creator)
For S3 Dataset GET_DATASET is given to Dataset creator, steward and env Admin (but on FE only restricted to Creator or Steward)
For Redshift Dataset GET_REDSHIFT_DATASET is given to Dataset creator, steward and Env Admin (but on FE only restricted to Creator or Steward)
For S3 Tables GET_DATASET_TABLE is given to Dataset creator, steward and shared groups (but on FE only restricted to Creator or Steward)
For Redshift Tables GET_REDSHIFT_DATASET_TABLE is given to Dataset creator, steward (but on FE only restricted to Creator or Steward)
For S3 Folders GET_DATASET_FOLDER is given to Dataset creator, steward and shared groups (but on FE only restricted to Creator or Steward)

For the following should we make it consistent on server and client side:

Dashboards
S3 Dataset
Redshift Dataset
S3 Tables
S3 Folders

For Redshift Tables -- should GET_REDSHIFT_DATASET_TABLE be granted on share approval and similar changes to FE for feed?

dlpzx · 2024-11-29T09:57:09Z

A call out that this is not restricted to only Admin/Creators on the backend for all target types:

* For Pipelines `GET_PIPELINE` only given to pipeline creator so now on restricted both to only creators on FE and BE

* For Dashboards `GET_DASHBOARD` is given to Dashboard creators and when dashboards have been shared to a group (but on FE only restricted to creator)

* For S3 Dataset `GET_DATASET` is given to Dataset creator, steward and env Admin  (but on FE only restricted to Creator or Steward)

* For Redshift Dataset `GET_REDSHIFT_DATASET` is given to Dataset creator, steward and Env Admin  (but on FE only restricted to Creator or Steward)

* For S3 Tables `GET_DATASET_TABLE` is given to Dataset creator, steward and shared groups (but on FE only restricted to Creator or Steward)

* For Redshift Tables `GET_REDSHIFT_DATASET_TABLE` is given to Dataset creator, steward (but on FE only restricted to Creator or Steward)

* For S3 Folders `GET_DATASET_FOLDER` is given to Dataset creator, steward and shared groups (but on FE only restricted to Creator or Steward)

For the following should we make it consistent on server and client side:

* Dashboards

* S3 Dataset

* Redshift Dataset

* S3 Tables

* S3 Folders

For Redshift Tables -- should GET_REDSHIFT_DATASET_TABLE be granted on share approval and similar changes to FE for feed?

I have created a couple of reports to analyze the different behaviors. I think the full consistency analysis of the permissions is out of the scope of this PR

noah-paige · 2024-12-02T20:38:32Z

@dlpzx can we track an issue/story to expose feed to other non admin groups - then I will go ahead and approve this PR

dlpzx · 2024-12-03T08:19:35Z

#1726 I opened the issue, but I think i will work first on clarify the GET_DATASET, GET_TABLE behavior

- Feature - Bugfix The Feeds module is used in the frontend in several modules. Some restrict access to admins only and some don't. In this PR we unify the behavior. ONLY ADMINS CAN SEE THE FEED in the frontend. - Dashboards: accessible to any user -----> add isAdmin - PIpelines: accessible to any user -----> add isAdmin - Redshift_Datasets: accessible to admin users only - Redshift_Tables : accessible to admin users only - S3_Datasets: accessible to admin users only - Folders: accessible to admin users only - Tables: accessible to admin users only Alongside the frontend changes, the backend should follow the same logic and restrict the API calls with permissions checks. That is what this PR does, it introduces resource permission checks depending on the Feed targetType with GET_X permission checks. - [x] Add security-focused tests for unauthorized cases <img width="1183" alt="Screenshot 2024-11-26 at 14 49 56" src="https://github.com/user-attachments/assets/f71292f1-1c90-4e35-a040-17d246ce2b68"> - [X] UI shows chat button for admins (creators or admin team) - verified in Datasets and Dashboards - [X] UI does not show chat button for non-admins - verified in Datasets and Dashboards - [x] Deploy in AWS - Call getFeed, postFeedMessage with resource admin (with GET permissions) and get feed - [X] Dataset - [x] Table - [x] Folder - [X] Redshift Dataset - [X] Redshift Table - [x] Dashboard - Call getFeed, postFeedMessage with another team not the resource admin (with UPDATE permissions) and get unauthorized response: - [X] Dataset - [x] Table - [x] Folder - [x] Redshift Dataset - [x] Redshift Table - [x] Dashboard - <URL or Ticket> 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.

Check isAdmin for Feed in Dashboards and Pipelines

b2ad9b2

dlpzx force-pushed the feat/feed-consistent-permissions branch from 0940baf to c19810e Compare November 26, 2024 10:37

Check isAdmin for Feed in Backend service

5180c64

dlpzx force-pushed the feat/feed-consistent-permissions branch 2 times, most recently from 94741de to e4672cb Compare November 26, 2024 13:14

Added unauthorized test cases

239bcc8

dlpzx force-pushed the feat/feed-consistent-permissions branch from e4672cb to 239bcc8 Compare November 26, 2024 13:51

dlpzx marked this pull request as ready for review November 26, 2024 13:51

dlpzx requested review from petrkalos and noah-paige November 26, 2024 13:51

petrkalos reviewed Nov 26, 2024

View reviewed changes

petrkalos approved these changes Nov 26, 2024

View reviewed changes

noah-paige reviewed Nov 27, 2024

View reviewed changes

dlpzx mentioned this pull request Dec 3, 2024

Expose Feed to non-admin users with access to the data asset #1726

Open

noah-paige approved these changes Dec 3, 2024

View reviewed changes

dlpzx merged commit 5438bdb into main Dec 4, 2024
9 checks passed

dlpzx mentioned this pull request Dec 4, 2024

Security features v2.6.2 #1731

Closed

dlpzx mentioned this pull request Dec 9, 2024

2.6.2 Security features #1737

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feed consistent permissions #1722

Feed consistent permissions #1722

dlpzx commented Nov 26, 2024 •

edited

Loading

petrkalos Nov 26, 2024

dlpzx Nov 29, 2024

noah-paige left a comment

dlpzx commented Nov 29, 2024

noah-paige commented Dec 2, 2024

dlpzx commented Dec 3, 2024

Feed consistent permissions #1722

Feed consistent permissions #1722

Conversation

dlpzx commented Nov 26, 2024 • edited Loading

Feature or Bugfix

Detail

Testing

Relates

Security

petrkalos Nov 26, 2024

Choose a reason for hiding this comment

dlpzx Nov 29, 2024

Choose a reason for hiding this comment

noah-paige left a comment

Choose a reason for hiding this comment

dlpzx commented Nov 29, 2024

noah-paige commented Dec 2, 2024

dlpzx commented Dec 3, 2024

dlpzx commented Nov 26, 2024 •

edited

Loading