From 2c170c354c8c5fbe2d3d064e7e25f1e1abebaca0 Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Wed, 11 Nov 2026 21:29:16 +0530 Subject: [PATCH 1/5] OpenConceptLab/ocl_online#488 | Access-control leftovers: viewable counts, follows, pins, hierarchy URLs, events, GraphQL search --- core/collections/models.py | 14 +-- core/collections/serializers.py | 3 +- core/collections/views.py | 2 +- core/common/mixins.py | 6 -- core/common/models.py | 11 +-- core/common/views.py | 7 +- core/concepts/models.py | 26 ++++-- core/concepts/serializers.py | 75 ++++++++++------ core/events/models.py | 71 +++++++++++---- core/events/views.py | 9 +- core/graphql/queries.py | 29 ++++-- .../tests/test_concepts_from_source.py | 41 +++++++++ .../integration_tests/tests_access_control.py | 72 ++++++++++++++- .../tests_cross_repo_access.py | 87 ++++++++++++++++++ core/integration_tests/tests_events.py | 28 +++--- core/integration_tests/tests_sources.py | 3 +- core/integration_tests/tests_users.py | 88 +++++++++++++++++-- core/map_projects/views.py | 7 ++ core/pins/permissions.py | 3 +- core/pins/tests.py | 9 +- core/pins/views.py | 10 ++- core/sources/models.py | 20 ++++- core/sources/views.py | 2 +- core/users/documents.py | 1 + core/users/models.py | 6 +- core/users/serializers.py | 66 +++++++++++--- core/users/views.py | 5 ++ 27 files changed, 563 insertions(+), 138 deletions(-) diff --git a/core/collections/models.py b/core/collections/models.py index fab4fcc12..9ee469c03 100644 --- a/core/collections/models.py +++ b/core/collections/models.py @@ -1475,11 +1475,15 @@ def _get_resolved_valueset_versions(self, valueset_version_cache=None): versions.append(valueset_version_cache[cache_key]) return versions - def get_resolved_repo_versions_serialized( - self, system_version_cache=None, valueset_version_cache=None, expansion_cache=None): - """Serialize repository versions resolved from this reference's system and valuesets.""" - system_versions = self._get_resolved_system_versions(system_version_cache, expansion_cache) - valueset_versions = self._get_resolved_valueset_versions(valueset_version_cache) + def get_resolved_repo_versions_serialized( # pylint: disable=too-many-arguments + self, system_version_cache=None, valueset_version_cache=None, expansion_cache=None, user=None): + """Serialize repository versions resolved from this reference's system and valuesets that `user` can view.""" + system_versions = [ + version for version in self._get_resolved_system_versions(system_version_cache, expansion_cache) + if version.has_view_access(user)] + valueset_versions = [ + version for version in self._get_resolved_valueset_versions(valueset_version_cache) + if version.has_view_access(user)] data = [] if system_versions: from core.sources.serializers import SourceVersionListSerializer diff --git a/core/collections/serializers.py b/core/collections/serializers.py index f246034a7..41d3077b8 100644 --- a/core/collections/serializers.py +++ b/core/collections/serializers.py @@ -609,7 +609,8 @@ def get_resolved_repo_versions(self, obj): if not self.include_resolved_repo_versions: return None return obj.get_resolved_repo_versions_serialized( - self.resolved_system_version_cache, self.resolved_valueset_version_cache, self.expansion_cache + self.resolved_system_version_cache, self.resolved_valueset_version_cache, self.expansion_cache, + get(self, 'context.request.user') ) diff --git a/core/collections/views.py b/core/collections/views.py index fe8ea36dc..b0958a0e6 100644 --- a/core/collections/views.py +++ b/core/collections/views.py @@ -346,7 +346,7 @@ def get_serializer_class(self): def get(self, request, *args, **kwargs): reference = self.get_queryset() - data = reference.get_resolved_repo_versions_serialized() + data = reference.get_resolved_repo_versions_serialized(user=request.user) return Response(data) diff --git a/core/common/mixins.py b/core/common/mixins.py index 6f10a0f37..a6ba6fbf3 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -232,12 +232,6 @@ def _list(self, request): # pylint:disable=too-many-locals,too-many-branches,to if not data: search_string = query_params.get('type', None) search_term = query_params.get('q', None) - if is_csv: - pattern = search_term - if pattern: - query_params._mutable = True # pylint: disable=protected-access - query_params['q'] = "*" + search_term + "*" - if is_csv and not search_string: return self.get_csv(request) diff --git a/core/common/models.py b/core/common/models.py index f4189b19c..c8eea9f7b 100644 --- a/core/common/models.py +++ b/core/common/models.py @@ -22,7 +22,7 @@ from django_elasticsearch_dsl.signals import RealTimeSignalProcessor from elasticsearch import ApiError, TransportError from elasticsearch.helpers import BulkIndexError, parallel_bulk, streaming_bulk -from pydash import get, compact +from pydash import get from core.celery import app as celery_app from core.common.tasks import update_collection_active_concepts_count, update_collection_active_mappings_count, \ @@ -1212,10 +1212,6 @@ def seed_references(self): def should_auto_expand(self): return True - @property - def identity_uris(self): - return compact([self.uri, self.canonical_url]) - @classmethod def persist_new(cls, obj, created_by, **kwargs): errors = {} @@ -1717,9 +1713,8 @@ def has_export(self): def can_view_all_content(self, user): if get(user, 'is_anonymous'): return False - return get( - user, 'is_staff' - ) or self.public_can_view or self.user_id == user.id or self.organization.members.filter(id=user.id).exists() + return bool(get(user, 'is_staff') or self.public_can_view or (user and (self.user_id == user.id or ( + self.organization_id and self.organization.members.filter(id=user.id).exists())))) @classmethod def resolve_expression_to_version(cls, expression, namespace=None): diff --git a/core/common/views.py b/core/common/views.py index aef46291a..65258a40e 100644 --- a/core/common/views.py +++ b/core/common/views.py @@ -207,6 +207,8 @@ def filter_queryset(self, queryset=None): return _queryset def filter_queryset_by_owner(self, queryset): + if self.user_is_self: + return queryset.filter(user=self.request.user) if 'user' in self.kwargs: return queryset.filter(user__username=self.kwargs['user']) if 'org' in self.kwargs: @@ -816,8 +818,9 @@ def __apply_common_search_filters(self, ignore_retired_filter=False, force=False if ((self.is_concept_container_document_model() or self.is_repo_document_model()) and self.request.query_params.get('allVersions', None) in TRUTHY): default_filters.pop('version', None) - if self.is_user_document() and self.should_include_inactive(): - default_filters.pop('is_active', None) + if self.is_user_document() and not self.should_include_inactive(): + # excludes rather than requires, so a profile indexed without the field yet still shows + results = results.exclude('term', is_active=False) updated_by = self.request.query_params.get(UPDATED_BY_USERNAME_PARAM, None) if updated_by: diff --git a/core/concepts/models.py b/core/concepts/models.py index cd7deffdd..3b0764a65 100644 --- a/core/concepts/models.py +++ b/core/concepts/models.py @@ -8,7 +8,7 @@ from pydash import get, compact, has from core.common.checksums import ChecksumModel -from core.common.constants import ISO_639_1, LATEST, HEAD, ALL +from core.common.constants import ISO_639_1, LATEST, HEAD, ALL, ACCESS_TYPE_NONE from core.common.db_functions import ImmutableUnaccent from core.common.mixins import SourceChildMixin from core.common.models import VersionedModel, ConceptContainerModel @@ -1348,7 +1348,7 @@ def update_mappings(self): # Updates mappings where mapping.to_concept or mapping.from_concepts matches concept's mnemonic and parent # only mappings whose last saver can view this concept's repo; others keep the external code, as on save from core.mappings.models import Mapping - parent_uris = self.parent.identity_uris + parent_uris = self.parent.get_mapping_link_uris() can_view = self.parent.get_view_access_criteria('updated_by') Mapping.objects.filter( @@ -1382,6 +1382,20 @@ def index_mapped_codes(cls, queryset): from core.concepts.documents import ConceptDocument return cls.batch_index_partial_by_ids(queryset, ConceptDocument, cls.get_mapped_codes_actions, refresh=False) + @staticmethod + def get_viewable(queryset, user): + """The concepts of `queryset` in repos `user` (None or anonymous for the public) can view.""" + if get(user, 'is_staff'): + return queryset + if not get(user, 'is_authenticated'): + return queryset.exclude(public_access=ACCESS_TYPE_NONE) + return Concept.apply_user_criteria(queryset, user) + + def get_viewable_hierarchy_concept_urls(self, relation, user): + """parent_concept_urls or child_concept_urls (`relation`), of concepts in repos `user` can view.""" + return list(self.get_viewable( + self.get_hierarchy_related_queryset(relation), user).values_list('uri', flat=True)) + @property def parent_concept_urls(self): return self.get_hierarchy_concept_urls('parent_concepts') @@ -1456,12 +1470,14 @@ def parent_concepts_count(self): def parent_concept_queryset(self): return self.get_hierarchy_related_queryset('parent_concepts') - def get_hierarchy_path(self): + def get_hierarchy_path(self, user=None): + """The URLs from the top of the hierarchy down to this concept's parent, through parents in repos `user` + (None for the public) can view.""" result = [] - parent_concept = self.parent_concepts.first() + parent_concept = self.get_viewable(self.parent_concepts.all(), user).first() while parent_concept is not None: result.append(drop_version(parent_concept.uri)) - parent_concept = parent_concept.parent_concepts.first() + parent_concept = self.get_viewable(parent_concept.parent_concepts.all(), user).first() result.reverse() return result diff --git a/core/concepts/serializers.py b/core/concepts/serializers.py index 777747178..78929fb99 100644 --- a/core/concepts/serializers.py +++ b/core/concepts/serializers.py @@ -30,12 +30,21 @@ def get_collection_uri_from_view(view, request): def get_viewable_concepts(queryset, request): """Concepts from repos the requester can view.""" - user = get(request, 'user') - if get(user, 'is_staff'): - return queryset - if not get(user, 'is_authenticated'): - return queryset.exclude(public_access=ACCESS_TYPE_NONE) - return Concept.apply_user_criteria(queryset, user) + return Concept.get_viewable(queryset, get(request, 'user')) + + +class ViewableHierarchyUrlsField(ListField): + """ + A concept's parent or child concept URLs (`relation`), of concepts in repos the requester can view: the context's + `user`, else its request's user. + """ + def __init__(self, relation, **kwargs): + self.relation = relation + super().__init__(**kwargs) + + def get_attribute(self, instance): + user = self.context['user'] if 'user' in self.context else get(self.context, 'request.user') + return instance.get_viewable_hierarchy_concept_urls(self.relation, user) class LocalizedNameSerializer(ModelSerializer): @@ -153,8 +162,9 @@ class ConceptAbstractSerializer(SourceVersionFieldMixin, AbstractResourceSeriali parent_concepts = SerializerMethodField() child_concepts = SerializerMethodField() hierarchy_path = SerializerMethodField() - parent_concept_urls = ListField(allow_null=True, required=False, allow_empty=True) - child_concept_urls = ListField(read_only=True) + parent_concept_urls = ViewableHierarchyUrlsField( + 'parent_concepts', allow_null=True, required=False, allow_empty=True) + child_concept_urls = ViewableHierarchyUrlsField('child_concepts', read_only=True) summary = SerializerMethodField() references = SerializerMethodField() checksums = SerializerMethodField() @@ -325,12 +335,12 @@ def get_parent_concepts(self, obj): def get_hierarchy_path(self, obj): if self.include_hierarchy_path: - return obj.get_hierarchy_path() + return obj.get_hierarchy_path(get(self, 'context.request.user')) return None def get_summary(self, obj): if self.include_summary: - return ConceptSummarySerializer(obj).data + return ConceptSummarySerializer(obj, context=self.context).data return None @@ -430,13 +440,20 @@ class ConceptSummarySerializer(ModelSerializer): names = SerializerMethodField() descriptions = SerializerMethodField() versions = SerializerMethodField() - parents = IntegerField(source='parent_concepts_count', read_only=True) - children = IntegerField(source='children_concepts_count', read_only=True) + parents = SerializerMethodField() + children = SerializerMethodField() class Meta: model = Concept fields = ('children', 'parents', 'names', 'descriptions', 'versions', 'id', 'uuid', 'versioned_object_id') + def get_parents(self, obj): + """Only parents in repos the requester can view, as the lists show.""" + return Concept.get_viewable(obj.parent_concept_queryset(), get(self, 'context.request.user')).count() + + def get_children(self, obj): + return Concept.get_viewable(obj.child_concept_queryset(), get(self, 'context.request.user')).count() + @staticmethod def get_names(obj): return obj.names.count() @@ -613,20 +630,22 @@ def get_previous_version_url(self, obj): return obj.prev_version_uri @staticmethod - def get_parent_concept_urls(obj): - uris = set(c.uri for c in obj.parent_concepts.all()) + def get_shared_hierarchy_urls(obj, relation): + """An export is served to anyone who can view its repo version, so it links only concepts of the same source + and public ones. Reads the export's prefetched relations.""" + concepts = list(getattr(obj, relation).all()) versioned = getattr(obj, 'versioned_object', None) if versioned: - uris.update(c.uri for c in versioned.parent_concepts.all()) - return list(uris) + concepts += getattr(versioned, relation).all() + return list({ + concept.uri for concept in concepts + if concept.parent_id == obj.parent_id or concept.public_access != ACCESS_TYPE_NONE}) - @staticmethod - def get_child_concept_urls(obj): - uris = set(c.uri for c in obj.child_concepts.all()) - versioned = getattr(obj, 'versioned_object', None) - if versioned: - uris.update(c.uri for c in versioned.child_concepts.all()) - return list(uris) + def get_parent_concept_urls(self, obj): + return self.get_shared_hierarchy_urls(obj, 'parent_concepts') + + def get_child_concept_urls(self, obj): + return self.get_shared_hierarchy_urls(obj, 'child_concepts') class ConceptVersionDetailSerializer(CollectionVersionsMixin, SourceVersionFieldMixin, ModelSerializer): @@ -651,8 +670,8 @@ class ConceptVersionDetailSerializer(CollectionVersionsMixin, SourceVersionField update_comment = CharField(source='comment', required=False, allow_null=True, allow_blank=True) parent_concepts = SerializerMethodField() child_concepts = SerializerMethodField() - parent_concept_urls = ListField(read_only=True) - child_concept_urls = ListField(read_only=True) + parent_concept_urls = ViewableHierarchyUrlsField('parent_concepts', read_only=True) + child_concept_urls = ViewableHierarchyUrlsField('child_concepts', read_only=True) source_versions = ListField(read_only=True) collection_versions = SerializerMethodField() references = SerializerMethodField() @@ -751,7 +770,7 @@ class ConceptHierarchySerializer(ModelSerializer): uuid = CharField(source='id') id = EncodedDecodedCharField(source='mnemonic') url = CharField(source='uri') - children = ListField(source='child_concept_urls') + children = ViewableHierarchyUrlsField('child_concepts') name = CharField(source='display_name') class Meta: @@ -785,7 +804,7 @@ def __init__(self, *args, **kwargs): def get_children(self, obj): if self.include_child_concepts: - return obj.child_concept_urls + return obj.get_viewable_hierarchy_concept_urls('child_concepts', get(self, 'context.request.user')) return None @@ -816,5 +835,5 @@ def __init__(self, *args, **kwargs): def get_parents(self, obj): if self.include_parent_concepts: - return obj.parent_concept_urls + return obj.get_viewable_hierarchy_concept_urls('parent_concepts', get(self, 'context.request.user')) return None diff --git a/core/events/models.py b/core/events/models.py index b7ad856e3..dbdea90f4 100644 --- a/core/events/models.py +++ b/core/events/models.py @@ -90,30 +90,71 @@ def object(self): return UserProfile.objects.filter(uri=self.object_url).first() return None - @property - def referenced_object(self): # pylint: disable=too-many-return-statements - if not self.referenced_object_url: + @staticmethod + def get_model_for_url(url): # pylint: disable=too-many-return-statements + """The model of the object an event's URL names.""" + if not url: return None - if '/mappings/' in self.referenced_object_url: + if '/mappings/' in url: from core.mappings.models import Mapping - return Mapping.objects.filter(uri=self.referenced_object_url).first() - if '/concepts/' in self.referenced_object_url: + return Mapping + if '/concepts/' in url: from core.concepts.models import Concept - return Concept.objects.filter(uri=self.referenced_object_url).first() - if '/sources/' in self.referenced_object_url: + return Concept + if '/sources/' in url: from core.sources.models import Source - return Source.objects.filter(uri=self.referenced_object_url).first() - if '/collections/' in self.referenced_object_url: + return Source + if '/collections/' in url: from core.collections.models import Collection - return Collection.objects.filter(uri=self.referenced_object_url).first() - if '/orgs/' in self.referenced_object_url: + return Collection + if '/orgs/' in url: from core.orgs.models import Organization - return Organization.objects.filter(uri=self.referenced_object_url).first() - if '/users/' in self.referenced_object_url: + return Organization + if '/users/' in url: from core.users.models import UserProfile - return UserProfile.objects.filter(uri=self.referenced_object_url).first() + return UserProfile return None + @property + def referenced_object(self): + model = self.get_model_for_url(self.referenced_object_url) + return model.objects.filter(uri=self.referenced_object_url).first() if model else None + + @classmethod + def get_hidden_urls(cls, urls, user): + """The URLs among `urls` of objects that exist and `user` can't view now. A user can always be viewed.""" + from core.users.models import UserProfile + urls_by_model = {} + for url in urls: + model = cls.get_model_for_url(url) + if model and model is not UserProfile: + urls_by_model.setdefault(model, set()).add(url) + hidden = set() + for model, model_urls in urls_by_model.items(): + for obj in model.objects.filter(uri__in=model_urls): + if not UserProfile.can_user_view(user, obj): + hidden.add(obj.uri) + return hidden + + @classmethod + def get_visible_to(cls, queryset, user): + """ + The events of `queryset` without those of an org or repo `user` can't view now, such as one made private after + the event: its stored URLs would still name it. Staff get them all. + """ + if getattr(user, 'is_staff', False): + return queryset + is_combined = bool(queryset.query.combinator) + pairs = queryset.order_by().values_list('object_url', 'referenced_object_url') + if not is_combined: # a union's rows are distinct already + pairs = pairs.distinct() + hidden = cls.get_hidden_urls({url for pair in pairs for url in pair}, user) + if not hidden: + return queryset + if is_combined: # a union can't be filtered + queryset = cls.objects.filter(id__in=queryset.order_by().values('id')) + return queryset.exclude(object_url__in=hidden).exclude(referenced_object_url__in=hidden) + @property def referenced_object_repr(self): return self.get_object_repr(self.referenced_object or self._referenced_object) diff --git a/core/events/views.py b/core/events/views.py index 94ae20801..30464898f 100644 --- a/core/events/views.py +++ b/core/events/views.py @@ -35,6 +35,9 @@ def get_owner(self): return self.owner def get_queryset(self): + return Event.get_visible_to(self.get_events(), self.request.user) + + def get_events(self): owner = self.get_owner() return self.apply_permissions(owner, owner.events) @@ -52,12 +55,12 @@ def get(self, request, *args, **kwargs): class UserEventsView(EventsView): """Events of a User""" - def get_queryset(self): + def get_events(self): user = self.get_owner() scopes = compact(self.request.query_params.get('scopes', '').split(',')) queryset = Event.objects.none() if 'self' in scopes or not scopes: - queryset = super().get_queryset() + queryset = super().get_events() include_private = self.should_include_private_events(user) if 'all' in scopes: queryset = queryset.union(Event.get_user_all_events(user, include_private)) @@ -77,7 +80,7 @@ def get(self, request, *args, **kwargs): return response return super().get(request, *args, **kwargs) - def get_queryset(self): + def get_events(self): from core.users.models import UserProfile user = UserProfile.objects.filter(username=settings.HIGHLIGHTED_EVENTS_FROM_USERNAME).first() events = Event.objects.none() diff --git a/core/graphql/queries.py b/core/graphql/queries.py index 3f1e9a0ea..f3ec19058 100644 --- a/core/graphql/queries.py +++ b/core/graphql/queries.py @@ -6,7 +6,7 @@ import strawberry from asgiref.sync import sync_to_async -from django.db.models import Case, IntegerField, Prefetch, Q, When +from django.db.models import Case, F, IntegerField, Prefetch, Q, When from django.utils import timezone from elasticsearch import ConnectionError as ESConnectionError, TransportError from elasticsearch_dsl import Q as ES_Q @@ -369,12 +369,14 @@ def concept_ids_from_es( try: search = ConceptDocument.search() - if source_version: - search = search.filter('term', source=source_version.mnemonic.lower()) - if source_version.is_head: - search = search.filter('term', is_latest_version=True) - else: - search = search.filter('term', source_version=source_version.version) + if source_version: # this owner's source by its exact names, as a same-named one of another owner may exist + search = search.filter('term', **{'owner_text.keyword': source_version.parent_resource}) + search = search.filter('term', owner_type='Organization' if source_version.organization_id else 'User') + search = search.filter('term', **{'source_text.keyword': source_version.mnemonic}) + if source_version and not source_version.is_head: + search = search.filter('term', source_version=source_version.version) + else: # one hit per concept, its latest version (versioned_object_ids maps it to the concept) + search = search.filter('term', is_latest_version=True) search = search.filter('term', retired=False) visible_repo_criteria = get_visible_repo_criteria(user) if visible_repo_criteria: @@ -405,6 +407,12 @@ def concept_ids_from_es( return None +def versioned_object_ids(concept_ids: Sequence[int]) -> list[int]: + """The concepts (HEAD rows) of these concept versions, in the same order.""" + heads = dict(Concept.objects.filter(id__in=concept_ids).values_list('id', 'versioned_object_id')) + return list(dict.fromkeys(heads.get(concept_id) or concept_id for concept_id in concept_ids)) + + def fallback_db_search(base_qs, query: str): trimmed = query.strip() if not trimmed: @@ -458,6 +466,8 @@ async def concepts_for_query( # pylint: disable=too-many-arguments else: return [], total else: + if not source_version or source_version.is_head: # the hits are latest versions; HEAD rows are listed + concept_ids = await sync_to_async(versioned_object_ids)(concept_ids) ordering = Case( *[When(id=pk, then=pos) for pos, pk in enumerate(concept_ids)], output_field=IntegerField() @@ -507,9 +517,10 @@ async def concepts( # pylint: disable=too-many-arguments,too-many-locals base_qs = build_base_queryset(source_version) mapping_prefetch = build_mapping_prefetch(source_version) else: - # Global search across all repositories the user can view + # Global search across the HEAD of every repository the user can view, a row per concept source_version = None - base_qs = get_viewable_concepts(Concept.objects.filter(is_active=True, retired=False), info.context) + base_qs = get_viewable_concepts( + Concept.objects.filter(is_active=True, retired=False, id=F('versioned_object_id')), info.context) mapping_prefetch = build_global_mapping_prefetch(user) if concept_ids_param: diff --git a/core/graphql/tests/test_concepts_from_source.py b/core/graphql/tests/test_concepts_from_source.py index 23c69df0c..a9b948b33 100644 --- a/core/graphql/tests/test_concepts_from_source.py +++ b/core/graphql/tests/test_concepts_from_source.py @@ -3,6 +3,7 @@ import uuid from unittest import mock +import factory from django.test import override_settings, tag from rest_framework.authtoken.models import Token @@ -474,6 +475,10 @@ def test_fetch_concepts_global_search(self): } } """ + # global search reads latest versions, which the API gives the concept's names too + ConceptNameFactory( + concept=self.concept1.get_latest_version(), name='Hypertension', locale='en', locale_preferred=True) + ConceptDocument().update(Concept.objects.filter(versioned_object_id=self.concept1.id), refresh=True) status, data = self._execute(query, {'query': 'hyper'}) self.assertEqual(status, 200) payload = data['concepts'] @@ -661,3 +666,39 @@ def test_query_search_lists_and_counts_only_viewable_concepts(self, _fallback_mo _, data = self._execute( query, {'org': self.organization.mnemonic, 'source': self.source.mnemonic, 'text': token}) self.assertEqual(data['concepts']['totalCount'], 1) + + @mock.patch('core.graphql.queries.fallback_db_search', side_effect=AssertionError('expected an ES search')) + def test_query_search_lists_each_concept_once_and_only_from_the_named_source(self, _fallback_mock): + self.patch_concept_es_mapping_for_ci() + token = f'zyq{uuid.uuid4().hex[:10]}' + # the same source name in another org, and concepts with their versions, as the API creates them + other_source = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='GQLOTHER'), mnemonic=self.source.mnemonic, version=HEAD) + concepts = [] + for source, mnemonic in [(self.source, 'gql-versioned'), (other_source, 'gql-other')]: + concept = Concept.persist_new({ + **factory.build(dict, FACTORY_CLASS=ConceptFactory), 'mnemonic': mnemonic, 'parent': source, + 'names': [ConceptNameFactory.build(name=f'{token} name', locale='en', locale_preferred=True)] + }, self.audit_user) + self.assertEqual(concept.errors, {}) + self.assertNotEqual(concept.get_latest_version().id, concept.id) + concepts.append(concept) + ConceptDocument().update( + Concept.objects.filter(versioned_object_id__in=[concept.id for concept in concepts]), refresh=True) + query = """ + query ($org: String, $source: String, $text: String!) { + concepts(org: $org, source: $source, query: $text) { + totalCount + results { conceptId } + } + } + """ + + for variables, concept_ids in [ + ({'text': token}, ['gql-other', 'gql-versioned']), + ({'org': self.organization.mnemonic, 'source': self.source.mnemonic, 'text': token}, + ['gql-versioned'])]: + _, data = self._execute(query, variables) + results = data['concepts']['results'] + self.assertEqual(sorted(result['conceptId'] for result in results), concept_ids, variables) + self.assertEqual(data['concepts']['totalCount'], len(concept_ids), variables) diff --git a/core/integration_tests/tests_access_control.py b/core/integration_tests/tests_access_control.py index 2a79fc56a..3607d5248 100644 --- a/core/integration_tests/tests_access_control.py +++ b/core/integration_tests/tests_access_control.py @@ -722,10 +722,30 @@ def test_task_list(self): def test_map_project_list(self): MapProjectFactory(user=self.owner, organization=None, public_access=ACCESS_TYPE_NONE, name='csv-project') - url = self.owner.uri + 'map-projects/?csv=true&verbose=true' + MapProjectFactory(user=self.owner, organization=None, public_access=ACCESS_TYPE_VIEW, name='csv-public-project') + MapProjectFactory( + user=self.outsider, organization=None, public_access=ACCESS_TYPE_VIEW, name='csv-other-project') + + for url in [self.owner.uri + 'map-projects/?csv=true&verbose=true', self.owner.uri + 'map-projects/?csv=true', + '/user/map-projects/?csv=true']: + self.assertEqual(self.export(url, self.owner, 'name'), ['csv-project', 'csv-public-project'], url) + for url in [self.owner.uri + 'map-projects/?csv=true&verbose=true', self.owner.uri + 'map-projects/?csv=true']: + self.assertEqual(self.export(url, None, 'name'), ['csv-public-project'], url) + self.assertNotIn('matches', self.export_columns(self.owner.uri + 'map-projects/?csv=true', self.owner)) + + def test_search_without_type(self): + private_org = OrganizationFactory(public_access=ACCESS_TYPE_NONE) + + mnemonics = self.export(f'/orgs/?csv=true&q={self.org.mnemonic}', None, 'mnemonic') + + self.assertIn(self.org.mnemonic, mnemonics) + self.assertNotIn(private_org.mnemonic, mnemonics) - self.assertEqual(self.export(url, self.owner, 'name'), ['csv-project']) - self.assertEqual(self.export(url, None, 'name'), []) + def export_columns(self, url, user): + response = self.request('get', url, user) + with zipfile.ZipFile(io.BytesIO(self.exports[response.data['url'].removeprefix('https://exports.test/')])) \ + as zip_file, zip_file.open(zip_file.namelist()[0]) as csv_file: + return csv.DictReader(io.TextIOWrapper(csv_file, 'utf-8-sig')).fieldnames def test_repo_concepts(self): source = UserSourceFactory(user=self.owner, public_access=ACCESS_TYPE_NONE) @@ -1150,6 +1170,27 @@ def test_private_collection_references_are_visible_to_members_only(self): self.assertEqual(self.request('get', url, self.outsider).status_code, 403) self.assertEqual(self.request('get', url, self.member).status_code, 200) + def test_resolved_repo_versions_are_those_the_requester_can_view(self): + private_source = UserSourceFactory(user=self.owner, public_access=ACCESS_TYPE_NONE) + public_source = OrganizationSourceFactory(organization=self.org, public_access=ACCESS_TYPE_VIEW) + for source in [private_source, public_source]: + CollectionReference( + expression=f'{source.uri}concepts/foo/', collection=self.collection, system=source.uri, code='foo' + ).save() + url = self.collection.uri + 'HEAD/references/?includeResolvedRepoVersions=true' + + def get_resolved_urls(user): + response = self.request('get', url, user) + self.assertEqual(response.status_code, 200) + return sorted( + version['version_url'] + for reference in response.data for version in reference['resolved_repo_versions']) + + for user in [None, self.outsider, self.member]: + self.assertEqual(get_resolved_urls(user), [public_source.uri], user) + for user in [self.owner, self.staff]: + self.assertEqual(get_resolved_urls(user), sorted([private_source.uri, public_source.uri]), user) + class SourceContentAccessTest(AccessTestMixin, OCLAPITestCase): """ @@ -1238,6 +1279,31 @@ def test_private_concept_names_and_descriptions_are_visible_to_members_only(self self.assertEqual(self.request('get', url, self.member).status_code, 200, url) +class CanViewAllContentTest(OCLTestCase): + """A collection reference reads all of a repo's content only for those who can view it.""" + def setUp(self): + super().setUp() + self.staff = UserProfile.objects.get(username='ocladmin') + self.org = OrganizationFactory() + self.member = UserProfileFactory(organizations=[self.org]) + self.owner = UserProfileFactory() + self.outsider = UserProfileFactory() + + def test_private_repos(self): + user_source = UserSourceFactory(user=self.owner, public_access=ACCESS_TYPE_NONE) + org_source = OrganizationSourceFactory(organization=self.org, public_access=ACCESS_TYPE_NONE) + + for source, viewers in [(user_source, [self.owner, self.staff]), (org_source, [self.member, self.staff])]: + for user in [self.owner, self.member, self.outsider, self.staff]: + self.assertEqual(source.can_view_all_content(user), user in viewers, (source.uri, user.username)) + self.assertFalse(source.can_view_all_content(None)) # no user: public content only + + def test_public_repo(self): + source = UserSourceFactory(user=self.owner, public_access=ACCESS_TYPE_VIEW) + + self.assertTrue(source.can_view_all_content(self.outsider)) + + class HasOwnershipTest(OCLTestCase): """HasOwnership answers for users and orgs only. Anything else is refused, except to staff.""" @staticmethod diff --git a/core/integration_tests/tests_cross_repo_access.py b/core/integration_tests/tests_cross_repo_access.py index cb6de1ca2..1d15cdacf 100644 --- a/core/integration_tests/tests_cross_repo_access.py +++ b/core/integration_tests/tests_cross_repo_access.py @@ -8,11 +8,13 @@ from core.common.constants import ACCESS_TYPE_NONE from core.common.tests import OCLAPITestCase from core.concepts.models import Concept +from core.concepts.serializers import ConceptVersionExportSerializer from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory from core.mappings.tests.factories import MappingFactory from core.orgs.tests.factories import OrganizationFactory from core.sources.models import Source from core.sources.tests.factories import OrganizationSourceFactory, UserSourceFactory +from core.url_registry.factories import GlobalURLRegistryFactory from core.users.models import UserProfile from core.users.tests.factories import UserProfileFactory @@ -222,6 +224,63 @@ def test_parents_and_children_lists_hide_private_concepts(self): self.assertEqual(sorted(concept['id'] for concept in response.data), expected, (user, url)) + def test_hierarchy_urls_paths_and_counts_hide_private_concepts(self): + child = self.persist_child('urlchild', [self.private_concept.uri, self.public_concept.uri]) + for mnemonic, parent_uri in [('privchild', self.public_concept.uri), ('privgrandchild', child.uri)]: + concept = Concept.persist_new({ + **factory.build(dict, FACTORY_CLASS=ConceptFactory), 'mnemonic': mnemonic, + 'parent': self.private_source, 'public_access': ACCESS_TYPE_NONE, + 'names': [ConceptNameFactory.build(locale='en', locale_preferred=True)], + 'parent_concept_urls': [parent_uri] + }, self.member) + self.assertEqual(concept.errors, {}) + private_child_uri = self.private_source.uri + 'concepts/privchild/' + private_grandchild_uri = self.private_source.uri + 'concepts/privgrandchild/' + + def get(url, headers): + response = self.client.get(url, **headers) + self.assertEqual(response.status_code, 200, url) + return response.data + + for user, viewable in [(None, False), (self.outsider, False), (self.member, True), (self.admin, True)]: + headers = {'HTTP_AUTHORIZATION': 'Token ' + user.get_token()} if user else {} + children = sorted([child.uri, *([private_child_uri] if viewable else [])]) + parents = sorted([self.public_concept.uri, *([self.private_concept.uri] if viewable else [])]) + + data = get(self.public_concept.uri + '?includeChildConceptURLs=true&includeSummary=true', headers) + self.assertEqual(sorted(data['child_concept_urls']), children, user) + self.assertEqual(data['summary']['children'], len(children), user) + self.assertEqual(get(self.public_concept.uri + 'summary/', headers)['children'], len(children), user) + data = get(child.uri + '?includeParentConceptURLs=true&includeHierarchyPath=true&includeSummary=true', + headers) + self.assertEqual(sorted(data['parent_concept_urls']), parents, user) + self.assertEqual(data['summary']['parents'], len(parents), user) + # the path goes up through a parent the requester can view + self.assertIn(data['hierarchy_path'], [[uri] for uri in parents], user) + data = get(self.public_concept.uri + 'children/?includeChildConcepts=true', headers) + self.assertEqual( + next(concept for concept in data if concept['id'] == 'urlchild')['children'], + [private_grandchild_uri] if viewable else [], user) + data = get(self.public_source.uri + 'hierarchy/', headers) + self.assertEqual( + sorted(next(concept for concept in data['children'] if concept['id'] == 'public')['children']), + children, user) + + + def test_exports_link_only_concepts_every_viewer_of_the_export_can_view(self): + sibling = ConceptFactory(parent=self.public_source, mnemonic='sibling', public_access=ACCESS_TYPE_NONE) + child = self.persist_child('exportchild', [self.private_concept.uri, self.public_concept.uri]) + child.parent_concepts.add(sibling) # a concept of the same source, as private as its source may be + + def linked(concept, field): # the concepts named, as the export names the linked rows (HEAD or versions) + urls = ConceptVersionExportSerializer(concept).data[field] + return {Concept.objects.get(uri=uri).mnemonic for uri in urls} + + for concept in [child, child.get_latest_version()]: + self.assertEqual(linked(concept, 'parent_concept_urls'), {'public', 'sibling'}) + self.assertEqual(linked(self.public_concept.get_latest_version(), 'child_concept_urls'), {'exportchild'}) + + class MappingTargetAccessTest(CrossRepoAccessBaseTest): def test_outsider_mapping_to_private_concept_is_external(self): from_concept = ConceptFactory(parent=self.outsider_source, mnemonic='from') @@ -329,6 +388,34 @@ def test_source_created_later_is_linked_only_to_mappings_whose_saver_can_view_it member_mapping.refresh_from_db() self.assertEqual(member_mapping.to_source_id, new_source.id) + def test_new_source_does_not_take_over_mappings_to_a_canonical_url_another_source_answers_for(self): + canonical_url = 'http://example.org/CodeSystem/claimed' + self.public_source.canonical_url = canonical_url + self.public_source.save() + GlobalURLRegistryFactory(url=canonical_url, namespace=self.public_source.organization.uri) + mapping = MappingFactory( + parent=self.outsider_source, from_concept=ConceptFactory(parent=self.outsider_source), to_concept=None, + to_source_url=canonical_url, to_concept_code='claimed', updated_by=self.outsider) + self.assertIsNone(mapping.to_source_id) + + errors = Source.persist_new( + UserSourceFactory.build(user=self.outsider, mnemonic='Claimer', canonical_url=canonical_url), + self.outsider, parent_resource=self.outsider) + self.assertEqual(errors, {}) + claimer = Source.objects.get(mnemonic='Claimer', version='HEAD') + ConceptFactory(parent=claimer, mnemonic='claimed').update_mappings() + + mapping.refresh_from_db() + self.assertIsNone(mapping.to_source_id) + self.assertIsNone(mapping.to_concept_id) + + self.public_source.update_mappings() + concept = ConceptFactory(parent=self.public_source, mnemonic='claimed') + concept.update_mappings() + mapping.refresh_from_db() + self.assertEqual(mapping.to_source_id, self.public_source.id) + self.assertEqual(mapping.to_concept_id, concept.id) + def test_edit_by_user_who_cannot_view_target_keeps_link(self): from_concept = ConceptFactory(parent=self.outsider_source, mnemonic='from') response = self.client.post( diff --git a/core/integration_tests/tests_events.py b/core/integration_tests/tests_events.py index d10e0a875..c8eff369a 100644 --- a/core/integration_tests/tests_events.py +++ b/core/integration_tests/tests_events.py @@ -278,32 +278,32 @@ def get_followed_urls(user=None): for user in [member, staff]: self.assertEqual(get_followed_urls(user), sorted([private_org.url, private_source.url, public_org.url])) - def test_get_events_show_objects_only_to_requesters_who_can_view_them_now(self): + def test_get_events_of_objects_made_private_only_to_requesters_who_can_view_them_now(self): org = OrganizationFactory(mnemonic='turned-private-org') source = OrganizationSourceFactory(organization=org, mnemonic='turned-private') member = UserProfileFactory(username='turned-member') org.members.add(member) + staff = UserProfileFactory(username='turned-staff', is_staff=True) follower = UserProfileFactory(username='turned-follower') follower.follow(source) - source.public_access = 'None' - source.save() + follower.follow(org) + for repo in [source, org]: + repo.public_access = 'None' + repo.save() - def get_events(user=None, params=''): + def get_followed_urls(user=None, params=''): headers = {'HTTP_AUTHORIZATION': f'Token {user.get_token()}'} if user else {} response = self.client.get(f'/users/turned-follower/events/{params}', **headers) self.assertEqual(response.status_code, 200) - return response.data + return sorted(event['referenced_object']['url'] for event in response.data) - # the event was public when recorded; the source it shows is live, so it's shown only to those who can view it + # the events were public when recorded; their stored URLs would still name the org and source for user in [None, follower]: - events = get_events(user) - self.assertEqual([event['referenced_object'] for event in events], [None], user) - self.assertEqual([event['description'] for event in events], ['Followed'], user) - # no CSV export, which would skip these checks - self.assertEqual(get_events(user, '?csv=true'), events, user) - events = get_events(member) - self.assertEqual([event['referenced_object']['url'] for event in events], [source.url]) - self.assertNotEqual(events[0]['description'], 'Followed') + for params in ['', '?scopes=self,following', '?scopes=all', '?csv=true']: + self.assertEqual(get_followed_urls(user, params), [], (user, params)) + for user in [member, staff]: + self.assertEqual(get_followed_urls(user), sorted([org.url, source.url]), user) + self.assertEqual(self.client.get('/orgs/turned-private-org/events/').data, []) def test_get_orgs_events(self): # pylint:disable=too-many-statements,too-many-locals alfred = UserProfileFactory(username='alfred', first_name='Alfred', last_name='Pennyworth') diff --git a/core/integration_tests/tests_sources.py b/core/integration_tests/tests_sources.py index 21d4ae445..891fd5f95 100644 --- a/core/integration_tests/tests_sources.py +++ b/core/integration_tests/tests_sources.py @@ -2215,7 +2215,8 @@ def test_get_200(self, hierarchy_mock): self.assertEqual(response.status_code, 200) self.assertEqual(response.data, 'hierarchy-response') - hierarchy_mock.assert_called_once_with(offset=100, limit=1000) + hierarchy_mock.assert_called_once_with(offset=100, limit=1000, user=ANY) + self.assertTrue(hierarchy_mock.call_args.kwargs['user'].is_anonymous) class SourceMappingsIndexViewTest(OCLAPITestCase): diff --git a/core/integration_tests/tests_users.py b/core/integration_tests/tests_users.py index 95416ba49..2cb4e25bc 100644 --- a/core/integration_tests/tests_users.py +++ b/core/integration_tests/tests_users.py @@ -6,12 +6,13 @@ from rest_framework.authtoken.models import Token from rest_framework.exceptions import ErrorDetail +from core.collections.tests.factories import UserCollectionFactory from core.common.constants import ACCESS_TYPE_NONE, ACCESS_TYPE_VIEW, RETIRED_ACCESS_TYPE_EDIT, ROLE_ADMIN, \ ROLE_MEMBER from core.common.tests import OCLAPITestCase, PREVIEW_GROUP_NAME from core.orgs.documents import OrganizationDocument from core.orgs.tests.factories import OrganizationFactory -from core.sources.tests.factories import OrganizationSourceFactory +from core.sources.tests.factories import OrganizationSourceFactory, UserSourceFactory from core.users.constants import VERIFY_EMAIL_MESSAGE, VERIFICATION_TOKEN_MISMATCH, MAPPER_USE_PERMISSION from core.users.documents import UserProfileDocument from core.users.models import UserProfile @@ -600,6 +601,23 @@ def test_get_verbose_plan_fields_only_for_self_and_staff(self): self.assertEqual(users['planfieldsother']['auth_groups'], [PREVIEW_GROUP_NAME]) self.assertIn(MAPPER_USE_PERMISSION, users['planfieldsother']['permissions']) + def test_get_verbose_extras_need_sign_in(self): + UserProfileFactory(username='verboseextras', extras={'xdept': 'xcardiology'}) + url = '/users/?verbose=true' + + response = self.client.get(url, format='json') + + self.assertEqual(response.status_code, 200) + self.assertEqual(sorted(user['username'] for user in response.data), ['ocladmin', 'verboseextras']) + for user in response.data: + self.assertNotIn('extras', user) + + response = self.client.get(url, HTTP_AUTHORIZATION='Token ' + UserProfileFactory().get_token(), format='json') + + self.assertEqual(response.status_code, 200) + users = {user['username']: user for user in response.data} + self.assertEqual(users['verboseextras']['extras'], {'xdept': 'xcardiology'}) + @patch('core.common.mixins.ListWithHeadersMixin.get_csv') def test_get_csv_is_not_exported(self, get_csv_mock): for query in ['csv=true', 'csv=1&q=ocl']: @@ -626,9 +644,20 @@ def test_get_200_with_inactive_user(self): self.assertEqual(len(response.data), 1) self.assertEqual(response.data[0]['username'], 'ocladmin') + # deactivated accounts are listed for staff only + for user in [None, UserProfileFactory(username='inactive-list-outsider')]: + headers = {'HTTP_AUTHORIZATION': 'Token ' + user.get_token()} if user else {} + for url in ['/users/?includeInactive=true', '/users/?includeInactive=true&q=inactive', + '/users/?q=inactive']: + response = self.client.get(url, format='json', **headers) + self.assertEqual(response.status_code, 200) + self.assertNotIn('inactive', [user['username'] for user in response.data], url) + UserProfile.objects.filter(username='inactive-list-outsider').delete() + staff = {'HTTP_AUTHORIZATION': 'Token ' + self.superuser.get_token()} + response = self.client.get( '/users/?includeInactive=true', - format='json' + format='json', **staff ) self.assertEqual(response.status_code, 200) @@ -637,7 +666,7 @@ def test_get_200_with_inactive_user(self): response = self.client.get( '/users/?includeInactive=true&q=', - format='json' + format='json', **staff ) self.assertEqual(response.status_code, 200) @@ -646,7 +675,7 @@ def test_get_200_with_inactive_user(self): response = self.client.get( '/users/?includeInactive=true&q=inactive', - format='json' + format='json', **staff ) self.assertEqual(response.status_code, 200) @@ -767,6 +796,33 @@ def test_get_200(self): self.assertEqual(response.data['name'], self.user.name) self.assertEqual(response.data['url'], self.user.uri) + def test_get_counts_only_what_the_requester_can_view(self): + private_org = OrganizationFactory(public_access=ACCESS_TYPE_NONE, created_by=self.user) + public_org = OrganizationFactory(created_by=self.user) + self.user.organizations.add(private_org, public_org) + org_member = UserProfileFactory(organizations=[private_org]) + UserSourceFactory(user=self.user, public_access=ACCESS_TYPE_VIEW) + UserSourceFactory(user=self.user, public_access=ACCESS_TYPE_NONE) + UserCollectionFactory(user=self.user, public_access=ACCESS_TYPE_NONE) + self.user.pins.create(resource=public_org) + fields = ('orgs', 'owned_orgs', 'sources', 'collections', 'bookmarks') + + def get_user(requester, query=''): + response = self.client.get( + f'/users/{self.user.username}/{query}', HTTP_AUTHORIZATION='Token ' + requester.get_token()) + self.assertEqual(response.status_code, 200) + return response.data + + for requester, counts, organizations in [ + (self.user, (2, 2, 2, 1, 1), 2), (self.superuser, (2, 2, 2, 1, 1), 2), + (org_member, (2, 2, 1, 0, None), 2), (UserProfileFactory(), (1, 1, 1, 0, None), 1)]: + data = get_user(requester) + self.assertEqual(tuple(data.get(field) for field in fields), counts, requester.username) + summary = get_user(requester, '?summary=true') + self.assertEqual(summary['organizations'], organizations, requester.username) + self.assertEqual((summary['sources'], summary['collections']), (1, 0), requester.username) + self.assertEqual(summary.get('bookmarks'), counts[-1], requester.username) + def test_get_200_with_subscribed_orgs(self): response = self.client.get( f'/users/{self.user.username}/?includeSubscribedOrgs=false', @@ -1394,7 +1450,7 @@ def test_get(self): self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 0) - def test_get_only_follows_the_requester_can_view(self): + def test_get_only_follows_the_requester_can_view(self): # pylint: disable=too-many-locals private_org = OrganizationFactory(mnemonic='private-followed', public_access=ACCESS_TYPE_NONE) private_source = OrganizationSourceFactory(organization=private_org, public_access=ACCESS_TYPE_NONE) public_org = OrganizationFactory(mnemonic='public-followed') @@ -1415,7 +1471,7 @@ def get_followed_urls(url, user=None): follows = next(user for user in follows if user['username'] == member.username)['following'] elif 'includeFollowing' in url: follows = follows['following'] - return sorted(follow['object']['url'] for follow in follows) + return sorted(follow['object']['url'] if follow['object'] else '' for follow in follows) following_url = f'/users/{member.username}/following/' detail_url = f'/users/{member.username}/?includeFollowing=true' @@ -1427,11 +1483,27 @@ def get_followed_urls(url, user=None): self.assertEqual(get_followed_urls(url, member), all_urls) self.assertEqual(get_followed_urls(url, staff), all_urls) - # the user themselves too, once they lose access + # the user keeps their follows of what they lost access to, without the object, so they can unfollow it private_org.members.remove(member) for url in [following_url, detail_url, list_url]: - self.assertEqual(get_followed_urls(url, member), [public_org.url]) + self.assertEqual(get_followed_urls(url, member), ['', '', public_org.url]) + self.assertEqual(get_followed_urls(url, outsider), [public_org.url]) self.assertEqual(get_followed_urls(url, staff), all_urls) + lost_follow = member.following.get(following_id=private_source.id) + response = self.client.delete( + f'{following_url}{lost_follow.id}/', HTTP_AUTHORIZATION='Token ' + member.get_token()) + self.assertEqual(response.status_code, 204) + self.assertFalse(member.following.filter(id=lost_follow.id).exists()) + + def test_get_search_lists_follows_not_users(self): + follower = UserProfileFactory(username='search-follower') + followed = UserProfileFactory(username='search-followed') + follower.follow(followed) + + response = self.client.get(f'/users/{follower.username}/following/?q=search') + + self.assertEqual(response.status_code, 200) + self.assertEqual([follow['object']['url'] for follow in response.data], [followed.url]) def test_post(self): follower = UserProfileFactory(username='follower') diff --git a/core/map_projects/views.py b/core/map_projects/views.py index 4937bab67..e9b27d1d8 100644 --- a/core/map_projects/views.py +++ b/core/map_projects/views.py @@ -82,6 +82,13 @@ def filter_queryset_by_public_access(self, queryset): Q(name__icontains=search_query) | Q(id_text__icontains=search_query)) return super().filter_queryset_by_public_access(queryset) + @staticmethod + def get_csv_rows(queryset): + """A row per listed project, read again by id as the list may be a union; content columns are left out.""" + return MapProject.objects.filter(id__in=[project.id for project in queryset]).order_by('id').values( + 'id', 'name', 'public_access', 'is_active', 'created_at', 'updated_at', 'created_by__username', + 'updated_by__username', 'organization__mnemonic', 'user__username') + @swagger_auto_schema(manual_parameters=[q_param]) def get(self, request, *args, **kwargs): return self.list(request, *args, **kwargs) diff --git a/core/pins/permissions.py b/core/pins/permissions.py index a6db981f4..d6c32f6aa 100644 --- a/core/pins/permissions.py +++ b/core/pins/permissions.py @@ -5,7 +5,8 @@ class CanEditPins(BasePermission): """ - Anyone can read the pins they can view (Pin.get_visible_to). A user's pins can be changed by that user, an + An org's pins can be read by anyone who can view them (Pin.get_visible_to), a user's by that user and staff + (PinBaseView). A user's pins can be changed by that user, an org's pins by its members, and any pin by staff. Checked against the user or org the pins belong to. """ def has_object_permission(self, request, view, obj): diff --git a/core/pins/tests.py b/core/pins/tests.py index 8c16d152e..f8aadd319 100644 --- a/core/pins/tests.py +++ b/core/pins/tests.py @@ -249,14 +249,13 @@ def get_pin_uris(url, token=None): self.assertEqual(response.status_code, 200) return sorted(pin['resource_uri'] for pin in response.data) - for token in [None, UserProfileFactory().get_token()]: - self.assertEqual(get_pin_uris(owner.uri + 'pins/', token), [public_source.uri]) + for token, status_code in [(None, 401), (UserProfileFactory().get_token(), 403)]: self.assertEqual(get_pin_uris(public_org.uri + 'pins/', token), [public_source.uri]) self.assertEqual(get_pin_uris(private_org.uri + 'pins/', token), []) + # a user's pins are for that user and staff only headers = {'HTTP_AUTHORIZATION': 'Token ' + token} if token else {} - response = self.client.get( - owner.uri + f'pins/{private_user_pin.id}/', format='json', **headers) - self.assertEqual(response.status_code, 404) + for url in [owner.uri + 'pins/', owner.uri + f'pins/{private_user_pin.id}/']: + self.assertEqual(self.client.get(url, format='json', **headers).status_code, status_code) for token in [owner.get_token(), admin.get_token()]: self.assertEqual( diff --git a/core/pins/views.py b/core/pins/views.py index 620da2a8d..963e070c4 100644 --- a/core/pins/views.py +++ b/core/pins/views.py @@ -3,7 +3,7 @@ from rest_framework import status from rest_framework.generics import ListAPIView, \ RetrieveUpdateDestroyAPIView -from rest_framework.permissions import IsAuthenticatedOrReadOnly +from rest_framework.permissions import IsAuthenticatedOrReadOnly, SAFE_METHODS from rest_framework.response import Response from core.common.constants import MAX_PINS_ALLOWED, INCLUDE_CREATOR_PINS @@ -55,7 +55,15 @@ def get_parent_filter(self): return {'organization__mnemonic': self.kwargs['org']} return None + def can_read_user_pins(self): + """A user's pins are read by that user and staff only.""" + user = self.request.user + return self.kwargs.get('user_is_self') or user.is_staff or ( + user.is_authenticated and user.username == self.kwargs.get('user')) + def get_queryset(self): + if self.get_parent_type() == 'user' and self.request.method in SAFE_METHODS and not self.can_read_user_pins(): + self.permission_denied(self.request) filters = self.get_parent_filter() criteria = Q(**filters) if self.should_include_creator_pins() and 'org' not in self.kwargs: diff --git a/core/sources/models.py b/core/sources/models.py index 38ea9163b..e254b8f4b 100644 --- a/core/sources/models.py +++ b/core/sources/models.py @@ -240,13 +240,23 @@ def is_validation_necessary(self): return self.custom_validation_schema is not None and self.active_concepts + def get_mapping_link_uris(self): + """The URLs a stored mapping names this source by to be linked to it: its own, and its canonical URL only while + that resolves to this source, as on a mapping's save, so a new source can't take over another's mappings.""" + uris = [self.uri] + if self.canonical_url: + resolved, _ = Source.resolve_reference_expression(self.canonical_url, None, HEAD) + if resolved.id and resolved.versioned_object_url == self.versioned_object_url: + uris.append(self.canonical_url) + return uris + def update_mappings(self): # Updates mappings where mapping.to_source_url or mapping.from_source_url matches source url or canonical url from core.mappings.models import Mapping from core.mappings.documents import MappingDocument # only mappings whose last saver can view this repo; others keep the external URL, as on save - uris = self.identity_uris + uris = self.get_mapping_link_uris() can_view = self.get_view_access_criteria('updated_by') to_queryset = Mapping.objects.filter(to_source_url__in=uris) @@ -401,12 +411,14 @@ def clean_filters(self): def get_parentless_concepts(self): return self.concepts.filter(parent_concepts__isnull=True, id=F('versioned_object_id')) - def hierarchy(self, offset=0, limit=100): + def hierarchy(self, offset=0, limit=100, user=None): + """The parentless concepts, each with its children's URLs in repos `user` (None for the public) can view.""" from core.concepts.serializers import ConceptHierarchySerializer hierarchy_root = None if offset == 0: hierarchy_root = self.hierarchy_root + context = {'user': user} parent_less_children = self.get_parentless_concepts() if hierarchy_root: parent_less_children = parent_less_children.exclude(mnemonic=hierarchy_root.mnemonic) @@ -419,10 +431,10 @@ def hierarchy(self, offset=0, limit=100): children = [] if parent_less_children.exists(): - children = ConceptHierarchySerializer(parent_less_children, many=True).data + children = ConceptHierarchySerializer(parent_less_children, many=True, context=context).data if hierarchy_root: - children.append({**ConceptHierarchySerializer(hierarchy_root).data, 'root': True}) + children.append({**ConceptHierarchySerializer(hierarchy_root, context=context).data, 'root': True}) return { 'id': self.mnemonic, diff --git a/core/sources/views.py b/core/sources/views.py index eb476310e..17a7fe440 100644 --- a/core/sources/views.py +++ b/core/sources/views.py @@ -649,7 +649,7 @@ def retrieve(self, request, *args, **kwargs): limit = int(params.get('limit')) if 'offset' in params: offset = int(params.get('offset')) - return Response(instance.hierarchy(offset=offset, limit=limit)) + return Response(instance.hierarchy(offset=offset, limit=limit, user=request.user)) class SourceSummaryView(SummaryMixin, SourceBaseView, RetrieveAPIView): diff --git a/core/users/documents.py b/core/users/documents.py index f60457cd5..0ca485ce4 100644 --- a/core/users/documents.py +++ b/core/users/documents.py @@ -30,6 +30,7 @@ class Django: fields = [ 'is_superuser', 'is_staff', + 'is_active', ] @staticmethod diff --git a/core/users/models.py b/core/users/models.py index 21f339a90..bbd8e6728 100644 --- a/core/users/models.py +++ b/core/users/models.py @@ -343,10 +343,10 @@ def can_user_view(user, obj): return bool(repo and hasattr(repo, 'has_view_access') and repo.has_view_access(user)) def get_following_visible_to(self, viewer): - """This user's follows, without those of objects `viewer` can't view (the user too, once they lose access). - Staff get them all.""" + """This user's follows, without those of objects `viewer` can't view. The user and staff get them all, so the + user can unfollow what they lost access to (FollowingSerializer then shows no object).""" follows = self.following.prefetch_related('following') - if getattr(viewer, 'is_staff', False): + if getattr(viewer, 'is_staff', False) or getattr(viewer, 'id', None) == self.id: return follows hidden_ids = [follow.id for follow in follows if not self.can_user_view(viewer, follow.following)] return follows.exclude(id__in=hidden_ids) if hidden_ids else follows diff --git a/core/users/serializers.py b/core/users/serializers.py index d55f91f44..d8be7ae8a 100644 --- a/core/users/serializers.py +++ b/core/users/serializers.py @@ -18,7 +18,8 @@ TRUTHY = get_truthy_values() PRIVATE_USER_FIELDS = ( - 'email', 'last_login', 'is_staff', 'is_superuser', 'auth_groups', 'permissions', 'capabilities', 'pins') + 'email', 'last_login', 'is_staff', 'is_superuser', 'auth_groups', 'permissions', 'capabilities', 'pins', + 'bookmarks') def can_view_private_user_fields(request, user): @@ -30,15 +31,44 @@ def can_view_private_user_fields(request, user): class PrivateUserFieldsMixin: - """Email, account flags, plan fields and pins are only for the user themselves and staff.""" + """Email, account flags, plan fields, pins and bookmarks are only for the user themselves and staff. + Extras need sign-in, as on /users//extras/.""" def to_representation(self, instance): data = super().to_representation(instance) - if not can_view_private_user_fields(get(self, 'context.request'), instance): + request = get(self, 'context.request') + if not can_view_private_user_fields(request, instance): for field in PRIVATE_USER_FIELDS: data.pop(field, None) + if not get(request, 'user.is_authenticated'): + data.pop('extras', None) return data +class ViewableCountsMixin: + """A user's org and repo counts: all of them for the user and staff, else those the requester can view.""" + def shows_all_counts(self, obj): + return can_view_private_user_fields(get(self, 'context.request'), obj) + + def get_viewable_orgs(self, obj): + orgs = obj.organizations.all() + if self.shows_all_counts(obj): + return orgs + from core.orgs.models import Organization + return Organization.get_visible_to(get(self, 'context.request.user'), orgs) + + def get_orgs(self, obj): + return self.get_viewable_orgs(obj).count() + + def get_owned_orgs(self, obj): + return self.get_viewable_orgs(obj).filter(created_by=obj).count() + + def get_sources(self, obj): + return obj.all_sources_count if self.shows_all_counts(obj) else obj.public_sources + + def get_collections(self, obj): + return obj.all_collections_count if self.shows_all_counts(obj) else obj.public_collections + + class OrganizationRoleMixin: """On an org's member list, each member's role in that org.""" def to_representation(self, instance): @@ -59,10 +89,11 @@ class Meta: ) -class UserSummarySerializer(OrganizationRoleMixin, PrivateUserFieldsMixin, serializers.ModelSerializer): +class UserSummarySerializer( + OrganizationRoleMixin, PrivateUserFieldsMixin, ViewableCountsMixin, serializers.ModelSerializer): sources = IntegerField(source='public_sources') collections = IntegerField(source='public_collections') - organizations = IntegerField(source='orgs_count') + organizations = serializers.SerializerMethodField() bookmarks = IntegerField(source='bookmarks_count') class Meta: @@ -72,6 +103,9 @@ class Meta: 'is_superuser', 'is_staff', 'first_name', 'last_name', 'status', 'bookmarks' ) + def get_organizations(self, obj): + return self.get_orgs(obj) + class UserCreateSerializer(serializers.ModelSerializer): type = serializers.CharField(source='resource_type', read_only=True) @@ -178,13 +212,16 @@ def get_object(obj): class FollowingSerializer(AbstractFollowerSerializer): - @staticmethod - def get_object(obj): + def get_object(self, obj): + """The followed object, or None when the requester can't view it now (the follower's own list keeps it).""" following = obj.following + if not following or not UserProfile.can_user_view(get(self, 'context.request.user'), following): + return None return following.get_brief_serializer()(following).data -class UserDetailSerializer(OrganizationRoleMixin, PrivateUserFieldsMixin, AbstractResourceSerializer): +class UserDetailSerializer( + OrganizationRoleMixin, PrivateUserFieldsMixin, ViewableCountsMixin, AbstractResourceSerializer): type = serializers.CharField(source='resource_type', read_only=True) uuid = serializers.CharField(source='id', read_only=True) username = serializers.CharField(required=False, validators=[ @@ -200,11 +237,11 @@ class UserDetailSerializer(OrganizationRoleMixin, PrivateUserFieldsMixin, Abstra location = serializers.CharField(required=False, allow_blank=True, allow_null=True) bio = serializers.CharField(required=False, allow_blank=True, allow_null=True) preferred_locale = serializers.CharField(required=False) - orgs = serializers.IntegerField(read_only=True, source='orgs_count') - owned_orgs = serializers.IntegerField(read_only=True, source='owned_orgs_count') - sources = serializers.IntegerField(read_only=True, source='all_sources_count') + orgs = serializers.SerializerMethodField() + owned_orgs = serializers.SerializerMethodField() + sources = serializers.SerializerMethodField() bookmarks = serializers.IntegerField(read_only=True, source='bookmarks_count') - collections = serializers.IntegerField(read_only=True, source='all_collections_count') + collections = serializers.SerializerMethodField() created_on = serializers.DateTimeField(source='created_at', read_only=True) updated_on = serializers.DateTimeField(source='updated_at', read_only=True) created_by = serializers.CharField(read_only=True) @@ -293,8 +330,9 @@ def get_pins(self, obj): return None def get_following(self, obj): - """Only follows of objects the requester can view.""" - return FollowingSerializer(obj.get_following_visible_to(get(self, 'context.request.user')), many=True).data + """Follows of objects the requester can view; all of them for the user and staff.""" + return FollowingSerializer( + obj.get_following_visible_to(get(self, 'context.request.user')), many=True, context=self.context).data def validate_username(self, value): from core.services.auth.core import AuthService diff --git a/core/users/views.py b/core/users/views.py index ff85f8c20..9bcca24e4 100644 --- a/core/users/views.py +++ b/core/users/views.py @@ -209,6 +209,10 @@ class UserBaseView(BaseAPIView): serializer_class = UserDetailSerializer staff_only_filters = ('is_superuser', 'is_staff', 'is_admin') + def should_include_inactive(self): + """Deactivated accounts are listed for staff only.""" + return self.request.user.is_staff and super().should_include_inactive() + def get_faceted_filters(self, split=False, params=None, additional_fields=None, repo_default_filters=None): filters = super().get_faceted_filters(split, params, additional_fields, repo_default_filters) if not self.request.user.is_staff: @@ -601,6 +605,7 @@ def delete(self, request, *args, **kwargs): class AbstractFollowerFollowedView(UserBaseView): permission_classes = (AllowAny, ) default_qs_sort_attr = '-follow_date' + is_searchable = False # a search would list users, not this user's follows def get_object(self, queryset=None): if self.user_is_self: From da19f1ab5c98e751b64356ffc5a68b57418e7bd8 Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Wed, 11 Nov 2026 22:12:03 +0530 Subject: [PATCH 2/5] OpenConceptLab/ocl_online#488 | Review fixes: per-requester hierarchy params skip the list cache, event visibility filters in the database by URL position, summary counts, verbose map-project CSV, registry in canonical mapping test --- core/common/mixins.py | 11 +- core/events/models.py | 111 +++++++++++------- .../integration_tests/tests_access_control.py | 2 + core/integration_tests/tests_concepts.py | 5 +- core/integration_tests/tests_events.py | 38 ++++++ core/integration_tests/tests_mappings.py | 3 + core/integration_tests/tests_users.py | 2 +- core/map_projects/views.py | 10 +- core/users/serializers.py | 4 +- 9 files changed, 132 insertions(+), 54 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index a6ba6fbf3..78275e3e2 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -27,7 +27,8 @@ LIST_DEFAULT_LIMIT, HTTP_COMPRESS_HEADER, CSV_DEFAULT_LIMIT, FACETS_ONLY, INCLUDE_RETIRED_PARAM, \ SEARCH_STATS_ONLY, INCLUDE_SEARCH_STATS, UPDATED_BY_USERNAME_PARAM, CHECKSUM_STANDARD_HEADER, \ CHECKSUM_SMART_HEADER, SEARCH_LATEST_REPO_VERSION, SAME_STANDARD_CHECKSUM_ERROR, ACCESS_TYPE_VIEW, \ - INCLUDE_PARENT_CONCEPTS, INCLUDE_CHILD_CONCEPTS + INCLUDE_PARENT_CONCEPTS, INCLUDE_CHILD_CONCEPTS, INCLUDE_PARENT_CONCEPT_URLS, INCLUDE_CHILD_CONCEPT_URLS, \ + INCLUDE_HIERARCHY_PATH, INCLUDE_SUMMARY from core.common.permissions import HasPrivateAccess, HasOwnership, CanViewConceptDictionary, \ CanViewConceptDictionaryVersion, CanEditConceptDictionary from .checksums import ChecksumModel @@ -40,6 +41,9 @@ logger = logging.getLogger('oclapi') TRUTHY = get_truthy_values() +PER_REQUESTER_LIST_PARAMS = ( + INCLUDE_PARENT_CONCEPTS, INCLUDE_CHILD_CONCEPTS, INCLUDE_PARENT_CONCEPT_URLS, INCLUDE_CHILD_CONCEPT_URLS, + INCLUDE_HIERARCHY_PATH, INCLUDE_SUMMARY) MAX_UNPAGINATED_RESULTS = 1000 # `Compress` lists; above this, users.list_unpaginated (ocl_online#230) @@ -185,12 +189,11 @@ def __get_cached_data_if_any(self, request): def __can_cache(self): # facets/search-stats are header driven and change the response shape, but are not part of the cache key; - # included parent/child concepts are filtered per requester, so they can't be shared + # hierarchy concepts, URLs, paths and summary counts are filtered per requester, so they can't be shared params = self.request.query_params return (self.is_repo_version_children_request_without_any_search() and not self.should_include_facets() and not self.should_include_search_stats() and - params.get(INCLUDE_PARENT_CONCEPTS) not in TRUTHY and - params.get(INCLUDE_CHILD_CONCEPTS) not in TRUTHY and + all(params.get(param) not in TRUTHY for param in PER_REQUESTER_LIST_PARAMS) and get(self, 'parent_resource.is_latest_version', False)) def list(self, request, *args, **kwargs): diff --git a/core/events/models.py b/core/events/models.py index dbdea90f4..fa778536b 100644 --- a/core/events/models.py +++ b/core/events/models.py @@ -1,6 +1,19 @@ from django.db import models +from django.db.models.functions import Coalesce from pydash import has, compact +from core.common.constants import ACCESS_TYPE_VIEW, RETIRED_ACCESS_TYPE_EDIT, HEAD + + +def get_repo_head_url(field): + """The URL of the repo HEAD an event URL is in, read by position, or '' when it names no repo.""" + return Coalesce( + models.Func( + models.F(field), function='SUBSTRING', + template="%(function)s(%(expressions)s FROM '^(/(?:orgs|users)/[^/]+/(?:sources|collections)/[^/]+/)')", + output_field=models.TextField()), + models.Value(''), output_field=models.TextField()) + class Event(models.Model): object_url = models.CharField(max_length=255) @@ -92,49 +105,68 @@ def object(self): @staticmethod def get_model_for_url(url): # pylint: disable=too-many-return-statements - """The model of the object an event's URL names.""" - if not url: + """ + The model of the object an event's URL names, read by position: + ///[//[/]][//...] + """ + parts = [part for part in (url or '').split('/') if part] + if len(parts) < 2 or parts[0] not in ('orgs', 'users'): return None - if '/mappings/' in url: - from core.mappings.models import Mapping - return Mapping - if '/concepts/' in url: + if len(parts) == 2: + if parts[0] == 'orgs': + from core.orgs.models import Organization + return Organization + from core.users.models import UserProfile + return UserProfile + if len(parts) < 4 or parts[2] not in ('sources', 'collections'): + return None + content_type = parts[4] if len(parts) >= 6 else None + if content_type not in ('concepts', 'mappings') and len(parts) >= 7: + content_type = parts[5] + if content_type == 'concepts': from core.concepts.models import Concept return Concept - if '/sources/' in url: + if content_type == 'mappings': + from core.mappings.models import Mapping + return Mapping + if len(parts) > 5: + return None + if parts[2] == 'sources': from core.sources.models import Source return Source - if '/collections/' in url: - from core.collections.models import Collection - return Collection - if '/orgs/' in url: - from core.orgs.models import Organization - return Organization - if '/users/' in url: - from core.users.models import UserProfile - return UserProfile - return None + from core.collections.models import Collection + return Collection @property def referenced_object(self): model = self.get_model_for_url(self.referenced_object_url) return model.objects.filter(uri=self.referenced_object_url).first() if model else None - @classmethod - def get_hidden_urls(cls, urls, user): - """The URLs among `urls` of objects that exist and `user` can't view now. A user can always be viewed.""" - from core.users.models import UserProfile - urls_by_model = {} - for url in urls: - model = cls.get_model_for_url(url) - if model and model is not UserProfile: - urls_by_model.setdefault(model, set()).add(url) - hidden = set() - for model, model_urls in urls_by_model.items(): - for obj in model.objects.filter(uri__in=model_urls): - if not UserProfile.can_user_view(user, obj): - hidden.add(obj.uri) - return hidden + @staticmethod + def get_hidden_criteria(field, user): + """ + A query on an event URL field for the orgs, repos, repo versions and repo content `user` can't view now, as + UserProfile.can_user_view decides; built from subqueries so the database applies it with the page. + """ + from core.collections.models import Collection + from core.orgs.models import Organization + from core.sources.models import Source + is_authenticated = bool(user and user.is_authenticated) + + orgs = Organization.objects.exclude(id__in=Organization.get_public().values('id')) + if is_authenticated: + orgs = orgs.exclude(members__id=user.id) + criteria = models.Q(**{f'{field}__in': orgs.filter(uri__isnull=False).values('uri')}) + + for model in (Source, Collection): + repos = model.objects.exclude(public_access__in=[ACCESS_TYPE_VIEW, RETIRED_ACCESS_TYPE_EDIT]) + if is_authenticated: + repos = repos.exclude(user_id=user.id).exclude(organization__members__id=user.id) + repos = repos.filter(uri__isnull=False) + # a private HEAD hides its versions and content too; a private version only itself + criteria |= models.Q(**{f'{field}_repo__in': repos.filter(version=HEAD).values('uri')}) + criteria |= models.Q(**{f'{field}__in': repos.exclude(version=HEAD).values('uri')}) + return criteria @classmethod def get_visible_to(cls, queryset, user): @@ -144,16 +176,13 @@ def get_visible_to(cls, queryset, user): """ if getattr(user, 'is_staff', False): return queryset - is_combined = bool(queryset.query.combinator) - pairs = queryset.order_by().values_list('object_url', 'referenced_object_url') - if not is_combined: # a union's rows are distinct already - pairs = pairs.distinct() - hidden = cls.get_hidden_urls({url for pair in pairs for url in pair}, user) - if not hidden: - return queryset - if is_combined: # a union can't be filtered + if queryset.query.combinator: # a union can't be filtered queryset = cls.objects.filter(id__in=queryset.order_by().values('id')) - return queryset.exclude(object_url__in=hidden).exclude(referenced_object_url__in=hidden) + return queryset.annotate( + object_url_repo=get_repo_head_url('object_url'), + referenced_object_url_repo=get_repo_head_url('referenced_object_url') + ).exclude( + cls.get_hidden_criteria('object_url', user) | cls.get_hidden_criteria('referenced_object_url', user)) @property def referenced_object_repr(self): diff --git a/core/integration_tests/tests_access_control.py b/core/integration_tests/tests_access_control.py index 3607d5248..37e327b2a 100644 --- a/core/integration_tests/tests_access_control.py +++ b/core/integration_tests/tests_access_control.py @@ -732,6 +732,8 @@ def test_map_project_list(self): for url in [self.owner.uri + 'map-projects/?csv=true&verbose=true', self.owner.uri + 'map-projects/?csv=true']: self.assertEqual(self.export(url, None, 'name'), ['csv-public-project'], url) self.assertNotIn('matches', self.export_columns(self.owner.uri + 'map-projects/?csv=true', self.owner)) + verbose_url = self.owner.uri + 'map-projects/?csv=true&verbose=true' + self.assertIn('matches', self.export_columns(verbose_url, self.owner)) def test_search_without_type(self): private_org = OrganizationFactory(public_access=ACCESS_TYPE_NONE) diff --git a/core/integration_tests/tests_concepts.py b/core/integration_tests/tests_concepts.py index da9a38241..9b4597af6 100644 --- a/core/integration_tests/tests_concepts.py +++ b/core/integration_tests/tests_concepts.py @@ -3412,7 +3412,8 @@ def test_repo_version_concepts_are_served_from_cache(self): def test_repo_version_concepts_with_hierarchy_includes_are_not_cached(self): body_key, _ = self.source_v1.get_concepts_cache_keys() - for param in ['includeParentConcepts', 'includeChildConcepts']: + for param in ['includeParentConcepts', 'includeChildConcepts', 'includeParentConceptURLs', + 'includeChildConceptURLs', 'includeHierarchyPath', 'includeSummary']: key = f'{body_key}?{param}=true' url = f'/orgs/{self.org}/sources/MySource/v1/concepts/?{param}=true' @@ -3422,7 +3423,7 @@ def test_repo_version_concepts_with_hierarchy_includes_are_not_cached(self): self.assertEqual(response.data[0]['id'], 'MyConcept1') self.assertIsNone(cache.get(key)) - # the included concepts are filtered per requester, so a cached response is never served + # hierarchy fields are filtered per requester, so a cached response is never served cache.set(key, [{'id': 'FromCache'}]) response = self.client.get(url) diff --git a/core/integration_tests/tests_events.py b/core/integration_tests/tests_events.py index c8eff369a..ec26f69de 100644 --- a/core/integration_tests/tests_events.py +++ b/core/integration_tests/tests_events.py @@ -1,4 +1,7 @@ +from core.collections.tests.factories import OrganizationCollectionFactory from core.common.tests import OCLAPITestCase +from core.concepts.tests.factories import ConceptFactory +from core.events.models import Event from core.events.tests.factories import EventFactory from core.orgs.tests.factories import OrganizationFactory from core.sources.tests.factories import OrganizationSourceFactory @@ -305,6 +308,41 @@ def get_followed_urls(user=None, params=''): self.assertEqual(get_followed_urls(user), sorted([org.url, source.url]), user) self.assertEqual(self.client.get('/orgs/turned-private-org/events/').data, []) + def test_get_events_hide_private_repo_versions_and_content_by_url_position(self): + org = OrganizationFactory(mnemonic='by-position') + member = UserProfileFactory(username='by-position-member') + org.members.add(member) + source = OrganizationSourceFactory(organization=org, mnemonic='mappings') + version = OrganizationSourceFactory(organization=org, mnemonic='mappings', version='v1') + collection = OrganizationCollectionFactory(organization=org, mnemonic='concepts') + concept = ConceptFactory(parent=source) + follower = UserProfileFactory(username='by-position-follower') + follower.follow(collection) + follower.follow(concept) + released = EventFactory(object_url=follower.url, referenced_object_url=version.uri, event_type='Released') + public_event = EventFactory(object_url=follower.url, referenced_object_url=org.url) + for repo in [source, collection]: + repo.public_access = 'None' + repo.save() + + def get_event_ids(user=None): + headers = {'HTTP_AUTHORIZATION': f'Token {user.get_token()}'} if user else {} + response = self.client.get('/users/by-position-follower/events/?scopes=self', **headers) + self.assertEqual(response.status_code, 200) + return sorted(event['id'] for event in response.data) + + all_ids = sorted(follower.events.values_list('id', flat=True)) + hidden_ids = [released.id, *follower.events.filter(event_type=Event.FOLLOWED).values_list('id', flat=True)] + self.assertEqual(len(hidden_ids), 3) + self.assertIn(public_event.id, all_ids) + for user in [None, follower]: + self.assertEqual(get_event_ids(user), sorted(set(all_ids) - set(hidden_ids)), user) + self.assertEqual(get_event_ids(member), all_ids) + + # the check is part of the events query, so building it reads nothing + with self.assertNumQueries(0): + Event.get_visible_to(follower.events, follower) + def test_get_orgs_events(self): # pylint:disable=too-many-statements,too-many-locals alfred = UserProfileFactory(username='alfred', first_name='Alfred', last_name='Pennyworth') bruce = UserProfileFactory(username='bruce', first_name='Bruce', last_name='Wayne') diff --git a/core/integration_tests/tests_mappings.py b/core/integration_tests/tests_mappings.py index 04d9bb849..9244100a4 100644 --- a/core/integration_tests/tests_mappings.py +++ b/core/integration_tests/tests_mappings.py @@ -13,6 +13,7 @@ from core.mappings.tests.factories import MappingFactory from core.orgs.tests.factories import OrganizationFactory from core.sources.tests.factories import UserSourceFactory, OrganizationSourceFactory +from core.url_registry.factories import GlobalURLRegistryFactory from core.users.models import UserProfile from core.users.tests.factories import UserProfileFactory @@ -498,6 +499,7 @@ def test_post_using_canonical_url_201(self): # pylint: disable=too-many-stateme self.assertIsNone(mapping.to_source_id) to_source = OrganizationSourceFactory(canonical_url="http://who.int/ICPC-2") + GlobalURLRegistryFactory(url=to_source.canonical_url, namespace=to_source.organization.uri) to_source.update_mappings() mapping.refresh_from_db() @@ -517,6 +519,7 @@ def test_post_using_canonical_url_201(self): # pylint: disable=too-many-stateme self.assertEqual(mapping.from_source, None) from_source = OrganizationSourceFactory(canonical_url="http://loinc.org") + GlobalURLRegistryFactory(url=from_source.canonical_url, namespace=from_source.organization.uri) from_source.update_mappings() mapping.refresh_from_db() diff --git a/core/integration_tests/tests_users.py b/core/integration_tests/tests_users.py index 2cb4e25bc..6c80980d4 100644 --- a/core/integration_tests/tests_users.py +++ b/core/integration_tests/tests_users.py @@ -820,7 +820,7 @@ def get_user(requester, query=''): self.assertEqual(tuple(data.get(field) for field in fields), counts, requester.username) summary = get_user(requester, '?summary=true') self.assertEqual(summary['organizations'], organizations, requester.username) - self.assertEqual((summary['sources'], summary['collections']), (1, 0), requester.username) + self.assertEqual((summary['sources'], summary['collections']), counts[2:4], requester.username) self.assertEqual(summary.get('bookmarks'), counts[-1], requester.username) def test_get_200_with_subscribed_orgs(self): diff --git a/core/map_projects/views.py b/core/map_projects/views.py index e9b27d1d8..0f2e6686b 100644 --- a/core/map_projects/views.py +++ b/core/map_projects/views.py @@ -82,10 +82,12 @@ def filter_queryset_by_public_access(self, queryset): Q(name__icontains=search_query) | Q(id_text__icontains=search_query)) return super().filter_queryset_by_public_access(queryset) - @staticmethod - def get_csv_rows(queryset): - """A row per listed project, read again by id as the list may be a union; content columns are left out.""" - return MapProject.objects.filter(id__in=[project.id for project in queryset]).order_by('id').values( + def get_csv_rows(self, queryset): + """A row per listed project, read again by id as the list may be a union; verbose keeps every column.""" + rows = MapProject.objects.filter(id__in=[project.id for project in queryset]).order_by('id') + if self.is_verbose(): + return rows.values() + return rows.values( 'id', 'name', 'public_access', 'is_active', 'created_at', 'updated_at', 'created_by__username', 'updated_by__username', 'organization__mnemonic', 'user__username') diff --git a/core/users/serializers.py b/core/users/serializers.py index d8be7ae8a..876e6c15b 100644 --- a/core/users/serializers.py +++ b/core/users/serializers.py @@ -91,8 +91,8 @@ class Meta: class UserSummarySerializer( OrganizationRoleMixin, PrivateUserFieldsMixin, ViewableCountsMixin, serializers.ModelSerializer): - sources = IntegerField(source='public_sources') - collections = IntegerField(source='public_collections') + sources = serializers.SerializerMethodField() + collections = serializers.SerializerMethodField() organizations = serializers.SerializerMethodField() bookmarks = IntegerField(source='bookmarks_count') From dea0ffc0409ae89c8c62433048501bca485b391e Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Thu, 12 Nov 2026 07:43:41 +0530 Subject: [PATCH 3/5] OpenConceptLab/ocl_online#488 | Adversarial review fixes: can_view_all_content follows the HEAD, CSV exports apply q, event filter reads public access as is_public_access does --- core/common/mixins.py | 10 ++++++---- core/common/models.py | 2 +- core/events/models.py | 7 +++++-- core/integration_tests/tests_access_control.py | 15 ++++++++++++++- 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index 78275e3e2..158ee8c77 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -235,7 +235,8 @@ def _list(self, request): # pylint:disable=too-many-locals,too-many-branches,to if not data: search_string = query_params.get('type', None) search_term = query_params.get('q', None) - if is_csv and not search_string: + is_csv_search = is_csv and bool(search_string or search_term) + if is_csv and not is_csv_search: return self.get_csv(request) if self.only_facets(): @@ -253,9 +254,10 @@ def _list(self, request): # pylint:disable=too-many-locals,too-many-branches,to if self.object_list is None: self.object_list = self.filter_queryset() - if is_csv and search_string: - klass = type(self.object_list[0]) - queryset = klass.objects.filter(id__in=self.get_object_ids()) + if is_csv_search: # the search's hits, read again within the view's queryset so its access checks apply + listed = self.get_queryset() + queryset = listed.model.objects.filter( + id__in=list(self.get_object_ids())).filter(id__in=listed.order_by().values('id')) return self.get_csv(request, queryset) # Skip pagination if compressed results are requested diff --git a/core/common/models.py b/core/common/models.py index c8eea9f7b..c2351ea4b 100644 --- a/core/common/models.py +++ b/core/common/models.py @@ -1713,7 +1713,7 @@ def has_export(self): def can_view_all_content(self, user): if get(user, 'is_anonymous'): return False - return bool(get(user, 'is_staff') or self.public_can_view or (user and (self.user_id == user.id or ( + return bool(get(user, 'is_staff') or self.is_publicly_viewable or (user and (self.user_id == user.id or ( self.organization_id and self.organization.members.filter(id=user.id).exists())))) @classmethod diff --git a/core/events/models.py b/core/events/models.py index fa778536b..426585d71 100644 --- a/core/events/models.py +++ b/core/events/models.py @@ -153,13 +153,16 @@ def get_hidden_criteria(field, user): from core.sources.models import Source is_authenticated = bool(user and user.is_authenticated) - orgs = Organization.objects.exclude(id__in=Organization.get_public().values('id')) + # not public as BaseModel.is_public_access reads it: View, or the retired Edit, in any case + is_private = ~models.Q(public_access__iexact=ACCESS_TYPE_VIEW) & ~models.Q( + public_access__iexact=RETIRED_ACCESS_TYPE_EDIT) + orgs = Organization.objects.filter(is_private) if is_authenticated: orgs = orgs.exclude(members__id=user.id) criteria = models.Q(**{f'{field}__in': orgs.filter(uri__isnull=False).values('uri')}) for model in (Source, Collection): - repos = model.objects.exclude(public_access__in=[ACCESS_TYPE_VIEW, RETIRED_ACCESS_TYPE_EDIT]) + repos = model.objects.filter(is_private) if is_authenticated: repos = repos.exclude(user_id=user.id).exclude(organization__members__id=user.id) repos = repos.filter(uri__isnull=False) diff --git a/core/integration_tests/tests_access_control.py b/core/integration_tests/tests_access_control.py index 37e327b2a..209e1009d 100644 --- a/core/integration_tests/tests_access_control.py +++ b/core/integration_tests/tests_access_control.py @@ -736,12 +736,15 @@ def test_map_project_list(self): self.assertIn('matches', self.export_columns(verbose_url, self.owner)) def test_search_without_type(self): - private_org = OrganizationFactory(public_access=ACCESS_TYPE_NONE) + private_org = OrganizationFactory(public_access=ACCESS_TYPE_NONE, mnemonic=self.org.mnemonic + '-private') + unmatched_org = OrganizationFactory(public_access=ACCESS_TYPE_VIEW, mnemonic='csv-unmatched') mnemonics = self.export(f'/orgs/?csv=true&q={self.org.mnemonic}', None, 'mnemonic') self.assertIn(self.org.mnemonic, mnemonics) self.assertNotIn(private_org.mnemonic, mnemonics) + # the export is the search's, not the whole list + self.assertNotIn(unmatched_org.mnemonic, mnemonics) def export_columns(self, url, user): response = self.request('get', url, user) @@ -1305,6 +1308,16 @@ def test_public_repo(self): self.assertTrue(source.can_view_all_content(self.outsider)) + def test_public_version_of_a_private_head(self): + head = OrganizationSourceFactory(organization=self.org, public_access=ACCESS_TYPE_NONE) + version = OrganizationSourceFactory( + organization=self.org, mnemonic=head.mnemonic, version='v1', public_access=ACCESS_TYPE_VIEW) + + for user in [self.member, self.outsider, self.staff]: + self.assertEqual( + version.can_view_all_content(user), version.has_view_access(user), user.username) + self.assertFalse(version.can_view_all_content(self.outsider)) + class HasOwnershipTest(OCLTestCase): """HasOwnership answers for users and orgs only. Anything else is refused, except to staff.""" From cd45cec0445a2c46be1a8ab65d96130813465704 Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Thu, 12 Nov 2026 08:07:18 +0530 Subject: [PATCH 4/5] OpenConceptLab/ocl_online#488 | CSV q without type searches only Elasticsearch-searched lists, from the hits as before; ES tag on the org search export test --- core/common/mixins.py | 13 +++++++------ core/integration_tests/tests_access_control.py | 12 ++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index 158ee8c77..0ba3221df 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -235,7 +235,8 @@ def _list(self, request): # pylint:disable=too-many-locals,too-many-branches,to if not data: search_string = query_params.get('type', None) search_term = query_params.get('q', None) - is_csv_search = is_csv and bool(search_string or search_term) + # q alone takes the search path only where it's an Elasticsearch search; other lists apply it themselves + is_csv_search = is_csv and bool(search_string or (search_term and get(self, 'is_searchable', False))) if is_csv and not is_csv_search: return self.get_csv(request) @@ -254,11 +255,11 @@ def _list(self, request): # pylint:disable=too-many-locals,too-many-branches,to if self.object_list is None: self.object_list = self.filter_queryset() - if is_csv_search: # the search's hits, read again within the view's queryset so its access checks apply - listed = self.get_queryset() - queryset = listed.model.objects.filter( - id__in=list(self.get_object_ids())).filter(id__in=listed.order_by().values('id')) - return self.get_csv(request, queryset) + if is_csv_search: # the search's hits; none gives an empty export where the document's model is known + ids = list(self.get_object_ids()) + klass = get(self, 'document_model.Django.model') if not ids else None + klass = klass or type(self.object_list[0]) + return self.get_csv(request, klass.objects.filter(id__in=ids)) # Skip pagination if compressed results are requested compress = self.should_compress() diff --git a/core/integration_tests/tests_access_control.py b/core/integration_tests/tests_access_control.py index 209e1009d..52c892d99 100644 --- a/core/integration_tests/tests_access_control.py +++ b/core/integration_tests/tests_access_control.py @@ -720,6 +720,17 @@ def test_task_list(self): self.assertEqual(self.export(url, self.owner, 'name'), ['csv-owner-task']) self.assertEqual(self.export(url, self.outsider, 'name'), []) + def test_task_list_search_without_type(self): + # a list that applies q itself exports all its matches, not a search's first 100 + for index in range(101): + Task.objects.create(name=f'csv-import-{index}', created_by=self.owner) + Task.objects.create(name='csv-other', created_by=self.owner) + + names = self.export(self.owner.uri + 'tasks/?csv=true&q=csv-import', self.owner, 'name') + + self.assertEqual(len(names), 101) + self.assertNotIn('csv-other', names) + def test_map_project_list(self): MapProjectFactory(user=self.owner, organization=None, public_access=ACCESS_TYPE_NONE, name='csv-project') MapProjectFactory(user=self.owner, organization=None, public_access=ACCESS_TYPE_VIEW, name='csv-public-project') @@ -735,6 +746,7 @@ def test_map_project_list(self): verbose_url = self.owner.uri + 'map-projects/?csv=true&verbose=true' self.assertIn('matches', self.export_columns(verbose_url, self.owner)) + @tag('es') def test_search_without_type(self): private_org = OrganizationFactory(public_access=ACCESS_TYPE_NONE, mnemonic=self.org.mnemonic + '-private') unmatched_org = OrganizationFactory(public_access=ACCESS_TYPE_VIEW, mnemonic='csv-unmatched') From 870dc9ff16218303a878de7a58cf191498aa40ee Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Thu, 12 Nov 2026 09:15:03 +0530 Subject: [PATCH 5/5] OpenConceptLab/ocl_online#488 | Org search export test indexes its orgs before searching Co-Authored-By: Claude Opus 5.5 --- core/integration_tests/tests_access_control.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/integration_tests/tests_access_control.py b/core/integration_tests/tests_access_control.py index 52c892d99..d1242e25b 100644 --- a/core/integration_tests/tests_access_control.py +++ b/core/integration_tests/tests_access_control.py @@ -15,6 +15,7 @@ from core.concepts.models import Concept from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory, ConceptDescriptionFactory from core.map_projects.tests.factories import MapProjectFactory, AutomatchRunFactory +from core.orgs.documents import OrganizationDocument from core.orgs.models import Organization from core.orgs.tests.factories import OrganizationFactory from core.pins.models import Pin @@ -750,6 +751,7 @@ def test_map_project_list(self): def test_search_without_type(self): private_org = OrganizationFactory(public_access=ACCESS_TYPE_NONE, mnemonic=self.org.mnemonic + '-private') unmatched_org = OrganizationFactory(public_access=ACCESS_TYPE_VIEW, mnemonic='csv-unmatched') + OrganizationDocument().update([self.org, private_org, unmatched_org], refresh=True) mnemonics = self.export(f'/orgs/?csv=true&q={self.org.mnemonic}', None, 'mnemonic')