Skip to content

Commit

Permalink
Feed consistent permissions (#1722)
Browse files Browse the repository at this point in the history
- 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.
  • Loading branch information
dlpzx committed Dec 5, 2024
1 parent 5c2b167 commit ab1f6e5
Show file tree
Hide file tree
Showing 8 changed files with 79 additions and 37 deletions.
4 changes: 2 additions & 2 deletions backend/dataall/modules/dashboards/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

from dataall.base.loader import ImportMode, ModuleInterface


log = logging.getLogger(__name__)


Expand Down Expand Up @@ -33,8 +32,9 @@ def __init__(self):
from dataall.modules.catalog.indexers.registry import GlossaryRegistry, GlossaryDefinition
from dataall.modules.vote.services.vote_service import add_vote_type
from dataall.modules.dashboards.indexers.dashboard_indexer import DashboardIndexer
from dataall.modules.dashboards.services.dashboard_permissions import GET_DASHBOARD

FeedRegistry.register(FeedDefinition('Dashboard', Dashboard))
FeedRegistry.register(FeedDefinition('Dashboard', Dashboard, GET_DASHBOARD))

GlossaryRegistry.register(
GlossaryDefinition(
Expand Down
2 changes: 1 addition & 1 deletion backend/dataall/modules/datapipelines/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def __init__(self):
)
import dataall.modules.datapipelines.api

FeedRegistry.register(FeedDefinition('DataPipeline', DataPipeline))
FeedRegistry.register(FeedDefinition('DataPipeline', DataPipeline, GET_PIPELINE))

TargetType('pipeline', GET_PIPELINE, UPDATE_PIPELINE, MANAGE_PIPELINES)
TargetType('cdkpipeline', GET_PIPELINE, UPDATE_PIPELINE, MANAGE_PIPELINES)
Expand Down
5 changes: 5 additions & 0 deletions backend/dataall/modules/feed/api/registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
class FeedDefinition:
target_type: str
model: Type[Resource]
permission: str


class FeedRegistry(UnionTypeRegistry):
Expand All @@ -25,6 +26,10 @@ def register(cls, definition: FeedDefinition):
def find_model(cls, target_type: str):
return cls._DEFINITIONS[target_type].model

@classmethod
def find_permission(cls, target_type: str):
return cls._DEFINITIONS[target_type].permission

@classmethod
def find_target(cls, obj: Resource):
for target_type, definition in cls._DEFINITIONS.items():
Expand Down
2 changes: 1 addition & 1 deletion backend/dataall/modules/feed/api/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,4 @@ def resolve_feed_messages(context: Context, source: Feed, filter: dict = None):
_required_uri(source.targetUri)
if not filter:
filter = {}
return FeedService.list_feed_messages(targetUri=source.targetUri, filter=filter)
return FeedService.list_feed_messages(targetUri=source.targetUri, targetType=source.targetType, filter=filter)
39 changes: 31 additions & 8 deletions backend/dataall/modules/feed/services/feed_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
import logging

from dataall.base.context import get_context
from dataall.core.permissions.services.resource_policy_service import ResourcePolicyService
from dataall.modules.feed.db.feed_models import FeedMessage
from dataall.modules.feed.db.feed_repository import FeedRepository
from dataall.modules.feed.api.registry import FeedRegistry


logger = logging.getLogger(__name__)
Expand All @@ -27,10 +29,6 @@ def targetType(self):
return self._targetType


def _session():
return get_context().db_engine.scoped_session()


class FeedService:
"""
Encapsulate the logic of interactions with Feeds.
Expand All @@ -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(
session=session,
username=context.username,
groups=context.groups,
resource_uri=targetUri,
permission_name=FeedRegistry.find_permission(target_type=targetType),
)
return Feed(targetUri=targetUri, targetType=targetType)

@staticmethod
Expand All @@ -49,17 +56,33 @@ def post_feed_message(
targetType: str = None,
content: str = None,
):
with _session() as session:
context = get_context()
with context.db_engine.scoped_session() as session:
ResourcePolicyService.check_user_resource_permission(
session=session,
username=context.username,
groups=context.groups,
resource_uri=targetUri,
permission_name=FeedRegistry.find_permission(target_type=targetType),
)
m = FeedMessage(
targetUri=targetUri,
targetType=targetType,
creator=get_context().username,
creator=context.username,
content=content,
)
session.add(m)
return m

@staticmethod
def list_feed_messages(targetUri: str, filter: dict = None):
with _session() as session:
def list_feed_messages(targetUri: str, targetType: str, filter: dict = None):
context = get_context()
with context.db_engine.scoped_session() as session:
ResourcePolicyService.check_user_resource_permission(
session=session,
username=context.username,
groups=context.groups,
resource_uri=targetUri,
permission_name=FeedRegistry.find_permission(target_type=targetType),
)
return FeedRepository(session).paginated_feed_messages(uri=targetUri, filter=filter)
8 changes: 5 additions & 3 deletions backend/dataall/modules/s3_datasets/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,16 @@ def __init__(self):
from dataall.modules.s3_datasets.services.dataset_permissions import (
GET_DATASET,
UPDATE_DATASET,
GET_DATASET_TABLE,
GET_DATASET_FOLDER,
MANAGE_DATASETS,
)
from dataall.modules.s3_datasets.db.dataset_repositories import DatasetRepository
from dataall.modules.s3_datasets.db.dataset_models import DatasetStorageLocation, DatasetTable, S3Dataset

FeedRegistry.register(FeedDefinition('DatasetStorageLocation', DatasetStorageLocation))
FeedRegistry.register(FeedDefinition('DatasetTable', DatasetTable))
FeedRegistry.register(FeedDefinition('Dataset', S3Dataset))
FeedRegistry.register(FeedDefinition('DatasetStorageLocation', DatasetStorageLocation, GET_DATASET_FOLDER))
FeedRegistry.register(FeedDefinition('DatasetTable', DatasetTable, GET_DATASET_TABLE))
FeedRegistry.register(FeedDefinition('Dataset', S3Dataset, GET_DATASET))

GlossaryRegistry.register(
GlossaryDefinition(
Expand Down
22 changes: 12 additions & 10 deletions frontend/src/modules/Dashboards/views/DashboardView.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,16 +227,18 @@ const DashboardView = () => {
onClick={() => upVoteDashboard(dashboard.dashboardUri)}
upVotes={upVotes || 0}
/>
<Button
color="primary"
startIcon={<ForumOutlined fontSize="small" />}
sx={{ mt: 1, mr: 1 }}
onClick={() => setOpenFeed(true)}
type="button"
variant="outlined"
>
Chat
</Button>
{isAdmin && (
<Button
color="primary"
startIcon={<ForumOutlined fontSize="small" />}
sx={{ mt: 1, mr: 1 }}
onClick={() => setOpenFeed(true)}
type="button"
variant="outlined"
>
Chat
</Button>
)}
<Button
color="primary"
component={RouterLink}
Expand Down
34 changes: 22 additions & 12 deletions frontend/src/modules/Pipelines/views/PipelineView.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import { deleteDataPipeline, getDataPipeline } from '../services';
import { FeedComments, KeyValueTagList, Stack } from 'modules/Shared';
import { PipelineOverview } from '../components';

function PipelineViewPageHeader({ pipeline, deletePipeline }) {
function PipelineViewPageHeader({ pipeline, deletePipeline, isAdmin }) {
const [openFeed, setOpenFeed] = useState(false);
return (
<Grid container justifyContent="space-between" spacing={3}>
Expand Down Expand Up @@ -69,16 +69,18 @@ function PipelineViewPageHeader({ pipeline, deletePipeline }) {
</Grid>
<Grid item>
<Box sx={{ m: -1 }}>
<Button
color="primary"
startIcon={<ForumOutlined fontSize="small" />}
sx={{ mt: 1, mr: 1 }}
onClick={() => setOpenFeed(true)}
type="button"
variant="outlined"
>
Chat
</Button>
{isAdmin && (
<Button
color="primary"
startIcon={<ForumOutlined fontSize="small" />}
sx={{ mt: 1, mr: 1 }}
onClick={() => setOpenFeed(true)}
type="button"
variant="outlined"
>
Chat
</Button>
)}
<Button
color="primary"
component={RouterLink}
Expand Down Expand Up @@ -116,7 +118,8 @@ function PipelineViewPageHeader({ pipeline, deletePipeline }) {

PipelineViewPageHeader.propTypes = {
pipeline: PropTypes.object.isRequired,
deletePipeline: PropTypes.func.isRequired
deletePipeline: PropTypes.func.isRequired,
isAdmin: PropTypes.bool.isRequired
};
const PipelineView = () => {
const dispatch = useDispatch();
Expand All @@ -134,6 +137,7 @@ const PipelineView = () => {
{ label: 'Tags', value: 'tags', icon: <LocalOffer fontSize="small" /> },
{ label: 'Stack', value: 'stack', icon: <FaAws size={20} /> }
];
const [isAdmin, setIsAdmin] = useState(false);

const handleDeleteObjectModalOpen = () => {
setIsDeleteObjectModalOpen(true);
Expand All @@ -148,6 +152,11 @@ const PipelineView = () => {
const response = await client.query(getDataPipeline(params.uri));
if (!response.errors && response.data.getDataPipeline !== null) {
setPipeline(response.data.getDataPipeline);
setIsAdmin(
['Creator', 'Admin', 'Owner'].indexOf(
response.data.getDataPipeline.userRoleForPipeline
) !== -1
);
} else {
const error = response.errors
? response.errors[0].message
Expand Down Expand Up @@ -212,6 +221,7 @@ const PipelineView = () => {
<PipelineViewPageHeader
pipeline={pipeline}
deletePipeline={handleDeleteObjectModalOpen}
isAdmin={isAdmin}
/>
<Box sx={{ mt: 3 }}>
<Tabs
Expand Down

0 comments on commit ab1f6e5

Please sign in to comment.