diff --git a/addons/workflow/services.py b/addons/workflow/services.py index 4752d1b3c4d..860fe8680c9 100644 --- a/addons/workflow/services.py +++ b/addons/workflow/services.py @@ -216,7 +216,7 @@ def get_user_accessible_templates(user: OSFUser, **filters) -> List[WorkflowTemp Returns: List of WorkflowTemplate objects the user can access """ - accessible_nodes = AbstractNode.objects.filter(_contributors=user, is_deleted=False) + accessible_nodes = AbstractNode.objects.get_nodes_for_user(user, include_mapcore_groups=True) visibility_filter = Q(pk__in=[]) visibility_filter |= Q(visibility=WorkflowTemplate.VISIBILITY_PUBLIC) diff --git a/addons/workflow/tests/test_template_access.py b/addons/workflow/tests/test_template_access.py new file mode 100644 index 00000000000..5582b13adbb --- /dev/null +++ b/addons/workflow/tests/test_template_access.py @@ -0,0 +1,165 @@ +# -*- coding: utf-8 -*- +"""Property tests for the template access predicate. + +The node-membership branch of template/engine access must admit exactly the +users who hold an explicit role on a related node: contributors and members +of mAP core groups bound to the node. Implicit parent-admin READ and public +visibility of the node itself grant nothing through this branch. +""" + +import uuid + +import pytest + +from framework.auth.core import Auth +from framework.exceptions import HTTPError + +from osf.models.mapcore_group import MapCoreGroup +from osf.models.mapcore_node_group import MapCoreNodeGroup +from osf.models.mapcore_user_group import MapCoreUserGroup +from osf.utils import permissions +from osf_tests.factories import ( + AuthUserFactory, + InstitutionFactory, + NodeFactory, + ProjectFactory, +) +from tests.base import OsfTestCase + +from addons.workflow.models import ( + WorkflowActivation, + WorkflowDefinitionSnapshot, + WorkflowEngine, + WorkflowTemplate, +) +from addons.workflow.services import get_user_accessible_templates +from addons.workflow.views import _get_engine_or_404, _get_template_or_404 + +pytestmark = pytest.mark.django_db + + +class TemplateAccessPredicateTests(OsfTestCase): + def setUp(self): + super().setUp() + self.owner = AuthUserFactory() + self.node = ProjectFactory(creator=self.owner) + institution = InstitutionFactory() + self.engine = WorkflowEngine.objects.create( + engine_id=str(uuid.uuid4()), + gateway_base_url='https://workflow.example/api/', + signing_kid='kid-test', + institution=institution, + ) + snapshot = WorkflowDefinitionSnapshot.objects.create( + engine=self.engine, + definition_id='definition-access', + definition_key='definition-access', + name='Access Process', + version=1, + ) + self.template = WorkflowTemplate.objects.create( + node=self.node, + definition=snapshot, + registered_by=self.owner, + ) + + def _bind_mapcore_group(self, node, user, permission=permissions.WRITE, enable_addon=True): + if enable_addon: + node.add_addon('groups', auth=Auth(node.creator)) + mapcore_group = MapCoreGroup.objects.create(_id='group-{}'.format(uuid.uuid4())) + MapCoreNodeGroup.objects.create( + node=node, + group=node.get_group(permission), + mapcore_group=mapcore_group, + creator=node.creator, + ) + MapCoreUserGroup.objects.create(user=user, mapcore_group=mapcore_group) + + def _assert_has_access(self, user, template=None, engine=None): + template = template or self.template + engine = engine or self.engine + assert _get_template_or_404(template.pk, user) == template + assert _get_engine_or_404(engine.engine_id, user) == engine + assert template in get_user_accessible_templates(user) + + def _assert_denied(self, user, template=None, engine=None): + template = template or self.template + engine = engine or self.engine + with pytest.raises(HTTPError) as excinfo: + _get_template_or_404(template.pk, user) + assert excinfo.value.code == 404 + with pytest.raises(HTTPError) as excinfo: + _get_engine_or_404(engine.engine_id, user) + assert excinfo.value.code == 404 + assert template not in get_user_accessible_templates(user) + + def test_contributor_has_access(self): + contributor = AuthUserFactory() + self.node.add_contributor(contributor, permissions=permissions.READ, auth=Auth(self.owner), save=True) + self._assert_has_access(contributor) + + def test_mapcore_group_member_has_access(self): + member = AuthUserFactory() + self._bind_mapcore_group(self.node, member) + self._assert_has_access(member) + + def _assert_activation_access(self, user): + assert _get_template_or_404(self.template.pk, user) == self.template + assert _get_engine_or_404(self.engine.engine_id, user) == self.engine + # get_user_accessible_templates only lists templates hosted on the + # user's own nodes; activation-node members are out of its scope + assert self.template not in get_user_accessible_templates(user) + + def test_contributor_has_access_via_activation(self): + contributor = AuthUserFactory() + activation_node = ProjectFactory() + activation_node.add_contributor(contributor, permissions=permissions.READ, auth=Auth(activation_node.creator), save=True) + WorkflowActivation.objects.create( + node=activation_node, + template=self.template, + activated_by=self.owner, + ) + self._assert_activation_access(contributor) + + def test_mapcore_group_member_has_access_via_activation(self): + member = AuthUserFactory() + activation_node = ProjectFactory() + self._bind_mapcore_group(activation_node, member) + WorkflowActivation.objects.create( + node=activation_node, + template=self.template, + activated_by=self.owner, + ) + self._assert_activation_access(member) + + def test_parent_admin_denied(self): + parent_admin = AuthUserFactory() + parent = ProjectFactory(creator=parent_admin) + component = NodeFactory(parent=parent, creator=self.owner) + snapshot = WorkflowDefinitionSnapshot.objects.create( + engine=self.engine, + definition_id='definition-component', + definition_key='definition-component', + name='Component Process', + version=1, + ) + template = WorkflowTemplate.objects.create( + node=component, + definition=snapshot, + registered_by=self.owner, + ) + assert component.has_permission(parent_admin, permissions.READ) + self._assert_denied(parent_admin, template=template) + + def test_stranger_denied_on_public_node(self): + self.node.is_public = True + self.node.save() + self._assert_denied(AuthUserFactory()) + + def test_unrelated_user_denied(self): + self._assert_denied(AuthUserFactory()) + + def test_group_member_denied_when_groups_addon_disabled(self): + member = AuthUserFactory() + self._bind_mapcore_group(self.node, member, enable_addon=False) + self._assert_denied(member) diff --git a/addons/workflow/tests/test_token.py b/addons/workflow/tests/test_token.py new file mode 100644 index 00000000000..9c3710e592b --- /dev/null +++ b/addons/workflow/tests/test_token.py @@ -0,0 +1,48 @@ +# -*- coding: utf-8 -*- + +import pytest + +from addons.workflow.token import MAX_TOKEN_NAME_LENGTH, build_token_name + + +class TestBuildTokenName: + def test_no_label(self): + name = build_token_name('executor') + assert name == 'Workflow delegation: executor' + + def test_short_label(self): + name = build_token_name('executor', 'test on proj') + assert name == 'Workflow delegation: executor (test on proj)' + assert len(name) <= MAX_TOKEN_NAME_LENGTH + + def test_exact_boundary(self): + # 'Workflow delegation: executor' = 29 chars + # suffix = ' (' + label + ')' = len(label) + 3 + # total = 32 + len(label) = 100 => len(label) = 68 + label = 'x' * 68 + name = build_token_name('executor', label) + assert len(name) == MAX_TOKEN_NAME_LENGTH + assert '...' not in name + + def test_one_over_truncates(self): + label = 'x' * 69 + name = build_token_name('executor', label) + assert len(name) == MAX_TOKEN_NAME_LENGTH + assert name.endswith('...)') + + def test_very_long_label(self): + label = 'x' * 300 + name = build_token_name('executor', label) + assert len(name) == MAX_TOKEN_NAME_LENGTH + assert name.endswith('...)') + + def test_japanese_label(self): + label = '論文・根拠データ公開申請(ファイル選択) on ' + 'あ' * 100 + name = build_token_name('executor', label) + assert len(name) <= MAX_TOKEN_NAME_LENGTH + assert name.startswith('Workflow delegation: executor (') + + def test_all_roles(self): + for role in ('creator', 'manager', 'executor'): + name = build_token_name(role, 'a' * 200) + assert len(name) <= MAX_TOKEN_NAME_LENGTH diff --git a/addons/workflow/token.py b/addons/workflow/token.py index 978c24c0f57..7c053c92e57 100644 --- a/addons/workflow/token.py +++ b/addons/workflow/token.py @@ -16,6 +16,20 @@ ALLOWED_TOKEN_ROLES = frozenset({'creator', 'manager', 'executor'}) REQUIRED_DELEGATION_FIELDS = frozenset({'token_id', 'token_value', 'scope', 'token_owner'}) +MAX_TOKEN_NAME_LENGTH = 100 + + +def build_token_name(role: str, label: str = '') -> str: + token_name = f'Workflow delegation: {role}' + if label: + suffix = f' ({label})' + max_suffix = MAX_TOKEN_NAME_LENGTH - len(token_name) + if len(suffix) > max_suffix: + # ' (' = 2, '...)' = 4, total overhead = 6 + suffix = f' ({label[:max_suffix - 6]}...)' + token_name += suffix + return token_name + TOKEN_MODE_TO_SCOPE = { 'read': ['osf.full_read', 'osf.users.email_read', 'osf.users.profile_read'], 'readwrite': ['osf.full_read', 'osf.full_write', 'osf.users.email_read', 'osf.users.profile_read'], @@ -175,9 +189,7 @@ def create_delegation_token(user, role: str, mode: str, label: str = '') -> Dict ) scopes.append(scope) - token_name = f'Workflow delegation: {role}' - if label: - token_name += f' ({label})' + token_name = build_token_name(role, label) token = ApiOAuth2PersonalToken( owner=user, diff --git a/addons/workflow/views.py b/addons/workflow/views.py index 0501e9b6973..8fbb8065582 100644 --- a/addons/workflow/views.py +++ b/addons/workflow/views.py @@ -246,15 +246,14 @@ def _get_engine_or_404(engine_id: str, user) -> WorkflowEngine: has_institution_access = _user_has_engine_admin_access(user, engine) if not has_institution_access: - accessible_nodes = AbstractNode.objects.filter(_contributors=user, is_deleted=False) + accessible_nodes = AbstractNode.objects.get_nodes_for_user(user, include_mapcore_groups=True) has_template_access = WorkflowTemplate.objects.filter( node__in=accessible_nodes, definition__engine=engine, ).exists() if not has_template_access: has_template_access = WorkflowActivation.objects.filter( - node___contributors=user, - node__is_deleted=False, + node__in=accessible_nodes, template__definition__engine=engine, ).exists() @@ -293,12 +292,11 @@ def _get_template_or_404(template_id: str, user) -> WorkflowTemplate: data={'message': 'Workflow template not found.'}, ) - has_direct_access = template.node.contributors.filter(id=user.id).exists() + has_direct_access = template.node.is_contributor_or_group_member(user) if not has_direct_access: has_activation_access = WorkflowActivation.objects.filter( template=template, - node___contributors=user, - node__is_deleted=False, + node__in=AbstractNode.objects.get_nodes_for_user(user, include_mapcore_groups=True), ).exists() if not has_activation_access and not _user_can_access_template_via_visibility(user, template): raise HTTPError( diff --git a/api/nodes/utils.py b/api/nodes/utils.py index d11349229ec..4985e60e90e 100644 --- a/api/nodes/utils.py +++ b/api/nodes/utils.py @@ -12,6 +12,7 @@ from addons.wiki.models import NodeSettings as WikiNodeSettings from osf.models import AbstractNode, Preprint, Guid, NodeRelation, Contributor from osf.models.node import NodeGroupObjectPermission +from osf.models.mapcore_node_group import MapCoreNodeGroup from osf.utils import permissions from api.base.exceptions import ServiceUnavailableError @@ -98,7 +99,23 @@ def optimize_node_queryset(self, queryset): read_permission = Permission.objects.get(codename=permissions.READ_NODE) contrib = Contributor.objects.filter(user=auth.user, node=OuterRef('pk')) user_group = OSFUserGroup.objects.filter(osfuser_id=auth.user.id if auth.user else None, group_id=OuterRef('group_id')) - node_group = NodeGroupObjectPermission.objects.annotate(user_group=Subquery(user_group.values_list('group_id')[:1])).filter(user_group__isnull=False, content_object_id=OuterRef('pk')) + # mAP core group membership grants node permissions through the node's + # permission auth groups without an osfuser_groups row, so it needs its + # own membership path here (mirrors AbstractNode.get_permissions). + mapcore_node_group = MapCoreNodeGroup.objects.filter( + is_deleted=False, + group_id=OuterRef('group_id'), + mapcore_group__mapcore_user_groups__user_id=auth.user.id if auth.user else None, + mapcore_group__mapcore_user_groups__is_deleted=False, + node__addons_groups_node_settings__is_deleted=False, + ) + node_group = NodeGroupObjectPermission.objects.annotate( + user_group=Subquery(user_group.values_list('group_id')[:1]), + mapcore_member=Exists(mapcore_node_group), + ).filter( + Q(user_group__isnull=False) | Q(mapcore_member=True), + content_object_id=OuterRef('pk'), + ) # user_is_contrib means user is a traditional contributor, while has_read/write/admin are permissions the user has either through group membership or contributorship return queryset.prefetch_related('root').prefetch_related('subjects').annotate( user_is_contrib=Exists(contrib), diff --git a/api_tests/users/views/test_user_nodes_mapcore_group_permissions.py b/api_tests/users/views/test_user_nodes_mapcore_group_permissions.py new file mode 100644 index 00000000000..e6327d4fa8b --- /dev/null +++ b/api_tests/users/views/test_user_nodes_mapcore_group_permissions.py @@ -0,0 +1,58 @@ +import pytest + +from api.base.settings.defaults import API_BASE +from framework.auth.core import Auth +from osf.models.mapcore_group import MapCoreGroup +from osf.models.mapcore_node_group import MapCoreNodeGroup +from osf.models.mapcore_user_group import MapCoreUserGroup +from osf_tests.factories import ( + AuthUserFactory, + ProjectFactory, +) +from osf.utils import permissions + + +@pytest.mark.django_db +class TestUserNodesMapCoreGroupPermissions: + """current_user_permissions on the nodes list must reflect mAP core + group membership, matching the node detail endpoint.""" + + @pytest.fixture() + def group_member(self): + return AuthUserFactory() + + @pytest.fixture() + def project(self, group_member): + creator = AuthUserFactory() + project = ProjectFactory(creator=creator, is_public=False) + project.add_addon('groups', auth=Auth(creator)) + project.save() + + mapcore_group = MapCoreGroup.objects.create(_id='test-mapcore-group') + MapCoreNodeGroup.objects.create( + node=project, + group=project.get_group(permissions.WRITE), + mapcore_group=mapcore_group, + creator=creator, + ) + MapCoreUserGroup.objects.create( + user=group_member, + mapcore_group=mapcore_group, + ) + return project + + def test_list_permissions_match_detail(self, app, group_member, project): + list_url = '/{}users/me/nodes/'.format(API_BASE) + res = app.get(list_url, auth=group_member.auth) + assert res.status_code == 200 + nodes = {n['id']: n for n in res.json['data']} + assert project._id in nodes + + detail_url = '/{}nodes/{}/'.format(API_BASE, project._id) + detail = app.get(detail_url, auth=group_member.auth) + assert detail.status_code == 200 + detail_perms = detail.json['data']['attributes']['current_user_permissions'] + assert permissions.WRITE in detail_perms + + list_perms = nodes[project._id]['attributes']['current_user_permissions'] + assert list_perms == detail_perms