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

Feed consistent permissions #1722

Merged
merged 3 commits into from
Dec 4, 2024
Merged

Feed consistent permissions #1722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2ad9b2
Check isAdmin for Feed in Dashboards and Pipelines
dlpzx Nov 26, 2024
5180c64
Check isAdmin for Feed in Backend service
dlpzx Nov 26, 2024
239bcc8
Added unauthorized test cases
dlpzx Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions backend/dataall/modules/dashboards/__init__.py
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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)
11 changes: 8 additions & 3 deletions backend/dataall/modules/redshift_datasets/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,16 @@ def __init__(self):
FEED_REDSHIFT_DATASET_TABLE_NAME,
VOTE_REDSHIFT_DATASET_NAME,
)

from dataall.modules.redshift_datasets.services.redshift_dataset_permissions import (
GET_REDSHIFT_DATASET,
GET_REDSHIFT_DATASET_TABLE,
)
import dataall.modules.redshift_datasets.api

FeedRegistry.register(FeedDefinition(FEED_REDSHIFT_DATASET_TABLE_NAME, RedshiftTable))
FeedRegistry.register(FeedDefinition(FEED_REDSHIFT_DATASET_NAME, RedshiftDataset))
FeedRegistry.register(
FeedDefinition(FEED_REDSHIFT_DATASET_TABLE_NAME, RedshiftTable, GET_REDSHIFT_DATASET_TABLE)
)
FeedRegistry.register(FeedDefinition(FEED_REDSHIFT_DATASET_NAME, RedshiftDataset, GET_REDSHIFT_DATASET))

GlossaryRegistry.register(
GlossaryDefinition(
Expand Down
8 changes: 5 additions & 3 deletions backend/dataall/modules/s3_datasets/__init__.py
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
12 changes: 12 additions & 0 deletions tests_new/integration_tests/modules/feed/test_feed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,15 @@ def test_get_feed_invalid(client1, session_s3_dataset1):
assert_that(get_feed).raises(GqlError).when_called_with(client1, None, S3_DATASET_TARGET_TYPE).contains(
'targetUri', 'must not be null'
)


def test_post_feed_message_unauthorized(client2, session_s3_dataset1):
assert_that(post_feed_message).raises(GqlError).when_called_with(
client2, session_s3_dataset1.datasetUri, S3_DATASET_TARGET_TYPE, 'message'
).contains('UnauthorizedOperation', 'GET_DATASET', session_s3_dataset1.datasetUri)


def test_get_feed_unauthorized(client2, session_s3_dataset1):
assert_that(get_feed).raises(GqlError).when_called_with(
client2, session_s3_dataset1.datasetUri, S3_DATASET_TARGET_TYPE
).contains('UnauthorizedOperation', 'GET_DATASET', session_s3_dataset1.datasetUri)
Loading